Commons Dbcp
  1. Commons Dbcp
  2. DBCP-8

[dbcp][PATCH] Handle changed passwords in SharedPoolDataSource

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      Problem Summary:
      Currently, DBCP does not provide support for dealing with password changes when
      using InstanceKeyDataSources. This means that when using a concrete
      implementation of InstanceKeyDataSource, such as SharedPoolDataSource or
      PerUserPoolDataSource, if a user changes his/her password, the entire connection
      pool must be restarted for DBCP to recognize the new password. In the event the
      connection pool is being managed by a container, such as Tomcat, it requires
      restarting the container. Users have previously requested an ability to handle
      these situations (see
      http://mail-archives.eu.apache.org/mod_mbox/jakarta-commons-user/200405.mbox/%3C40B5F9DC.1080101@pandora.be%3E
      and
      http://mail-archives.eu.apache.org/mod_mbox/jakarta-commons-user/200405.mbox/%3c40ACA3DE.8080302@pandora.be%3e
      for examples).

      Proposed Patch:

      The attached patch provides support for using the SharedPoolDataSource in
      situations where user passwords may be changed. Support is provided by changing
      UserPassKey and SharedPoolDataSource to use the concatenation of the username
      and password as the key. In this way, after a user changes password, a
      getConnection(String, String) method call will cause the pool to create a new
      Connection using the new username and password. The Connection that was created
      using the old username and password remains in the pool, where--assuming the
      pool is set up to remove idle objects--it will be collected by the idle object
      eviction thread or eventually (once the pool is full) be discarded according to
      the LRU algorithm provided by the pool.

      Other Solutions Considered:

      In making this patch, I considered several other possible algorithms but chose
      the implemented algorithm as the best combination of safety and ease of use.
      And-as a bonus-it is a very unobtrusive solution.

      • The "public void close(String name)" method recommended by Dirk Verbeeck in
        the first link above is relatively complex to implement in the case of a
        SharedPoolDataSource (but would be much easier for a PerUserPoolDataSource, as
        he suggested). In the case of a SharedPoolDataSource, it's possible that
        multiple Connections exist for the specified user, in which case
        SharedPoolDataSource would have to provide logic to deal with the cases where
        one or more existing connections for the user are checked out at the time the
        method is called--not to mention the logic required to find all existing
        connections and close them without needlessly opening new connections (since all
        users share the same pool in a SharedPoolDataSource instead of having a pool per
        user as in PerUserPoolDataSource).
      • Adding a method "public void getConnection(String username, String password,
        boolean closeAndReconnectOnPasswordMismatch)" has similar problems with the
        possibility of the existence of multiple open connections for the given
        username. If only the current connection is closed, we may be leaving
        connections that are associated with the old password in the pool; therefore,
        the application would need to use the closeAndReconnect functionality in
        most-if not all-cases where a connection is required. If all connections are
        closed, we have the same situation described above.

      Furthermore, neither of the above methods is a part of the DataSource interface;
      and, therefore, both would require the application to downcast to the
      appropriate DBCP type to make the method call. For all practical purposes, this
      negates the benefits of using an interface in the first place.

      Additionally, adding new methods as above requires the application to know when
      the user has changed his/her password, and provides a potential failure mode
      when the user changes his/her password through an external application (i.e.
      directly on the database or using some other application). Although it is
      possible for the application to catch the plain-vanilla SQLException thrown by
      the getConnection(String, String) method of InstanceKeyDataSource and parse the
      exception message looking for the expected text ("Given password did not match
      password used to create the PooledConnection."), doing so is not at all an
      elegant solution. Even if a new PasswordChangedException (which extends
      SQLException) were created, the application would then need to downcast the
      DataSource to make the appropriate call, so the previously mentioned problems
      with the solution apply.

      Also, both of the new methods allow a Denial-of-Service-type attack against the
      connection pool wherein a user may prevent the connection pool from reusing
      connections by forcing it to close connections and attempt a reconnect when
      given an incorrect password. Depending on the application design, this weakness
      could be exploited from a login page, knowing only valid usernames. The
      proposed solution does not suffer from this problem as the existing connections
      are kept, so a user requesting a connection with a bad password would simply
      cause DBCP to attempt to make a connection that the database would refuse
      because of an invalid password.

      • Checking the given password in the getPooledConnectionAndInfo( ) method and,
        if different from the originally given password, attempting to connect to the
        database with the new username/password combination and changing the password
        associated with the UserPassKey after a successful connection would also work,
        but requires duplicating the password check done by the InstanceKeyDataSource in
        subclasses wanting to support use after password changes (or removing the check
        from InstanceKeyDataSource and requiring subclasses to handle the situation
        appropriately). Furthermore, since database behavior regarding usability of
        Connections after a password change is database-dependent, we cannot be sure
        that the existing connections-which were created with the old password-are
        still valid, so this approach would require users to run a validation query.
        Therefore, it seems more reasonable to simply leave the existing connections and
        let them be evicted or thrown away when the pool becomes full.

      Design Decisions for the Attached Patch:

      The first required change was to modify the hashCode() method in the UserPassKey
      class to create a hash based upon both the username and password. This was done
      simply by concatenating username and password and returning the hashCode of the
      combined String. If the username is null, 0 is returned. If the password is
      null but the username is not, the String "null" will be concatenated to the
      username and the hashCode of the combined String will be returned. Although it
      would have been prettier to only append the password if it is not null, the
      additional logic and extra processing time required for the change did not seem
      worthwhile.

      Next, the getUserPassKey(String username, String password) method was modified
      to ensure the username and password were used as the key for the Map. In the
      event that username and password are both null, the key will be "nullnull".
      Similarly, the String "null" will be used for the part associated with username
      or password if one is null. This means that we will never use a null value for
      a key; however, the effect is the same--since the hashCode() of the String
      "nullnull" will always be the same, we'll have only one entry for the
      UserPassKey having null username and password. We only create a new Map entry
      when either username or password is different (as opposed to before, where we
      only create a new entry when username is different).

      Although using the password in the key may be considered to have security
      implications (as a hash of username and password could be considered a password
      equivalent), the data will only be available to the application--which, itself,
      is handling the password. Furthermore, since the UserPassKey is storing the
      unencoded password, and since the UserPassKey's toString() method returns the
      unencoded username and password, the proposed patch does not seem to negatively
      impact security of the class.

      The patch also modifies testIncorrectPassword() in TestSharedPoolDataSource.
      After the patch is applied, the SharedPoolDataSource will attempt to connect
      with the new (incorrect) password. The DataSource throws a SQLNestedException
      ("Could not retrieve connection info from pool") chained to a SQLException ("x
      is not the correct password for u1. The correct password is p1").

      While the existing patch only applies to the SharedPoolDataSource, I would be
      happy to provide a similar patch for the PerUserPoolDataSource (for the reasons
      given above, I feel this approach is also the best approach for the
      PerUserPoolDataSource). If interested, let me know and I'll file a patch on
      another bug report.

      1. passwordChange2.patch
        44 kB
        Phil Steitz
      2. changePassword.patch
        26 kB
        Phil Steitz
      3. dbcp-SharedPoolWithPasswordChange-20070309.patch
        2 kB
        Michael T. Dean
      4. ASF.LICENSE.NOT.GRANTED--dbcp-SharedPoolWithPasswordChange.patch
        2 kB
        Michael T. Dean

        Activity

        Hide
        Michael T. Dean added a comment -

        Created an attachment (id=14741)
        Proposed Patch

        Show
        Michael T. Dean added a comment - Created an attachment (id=14741) Proposed Patch
        Hide
        Michael T. Dean added a comment -

        Updated patch that applies cleanly to current SVN trunk

        Show
        Michael T. Dean added a comment - Updated patch that applies cleanly to current SVN trunk
        Hide
        Mark Thomas added a comment -

        Thanks for the patch. It has been applied to trunk.

        Show
        Mark Thomas added a comment - Thanks for the patch. It has been applied to trunk.
        Hide
        Mark Thomas added a comment -

        Re-opening as the fix needs to be extended to the PerUserPoolDataSource

        Show
        Mark Thomas added a comment - Re-opening as the fix needs to be extended to the PerUserPoolDataSource
        Hide
        Mark Thomas added a comment -

        Fixed in trunk and will be in 1.3 onwards.

        Show
        Mark Thomas added a comment - Fixed in trunk and will be in 1.3 onwards.
        Hide
        Phil Steitz added a comment -

        I am concerned that the implemented solution creates a security problem, or at least a misleading security contract. If the use case we are trying to satisfy is database passwords being changed, it seems to me that we really do need to implement a close() method as Dirk suggested. Under the current implementation, getConnection(username, password) will continue to work with the "old" password, returning pooled connections created using the old password if these still exist in the pool.

        Show
        Phil Steitz added a comment - I am concerned that the implemented solution creates a security problem, or at least a misleading security contract. If the use case we are trying to satisfy is database passwords being changed, it seems to me that we really do need to implement a close() method as Dirk suggested. Under the current implementation, getConnection(username, password) will continue to work with the "old" password, returning pooled connections created using the old password if these still exist in the pool.
        Hide
        Sebb added a comment -

        Seems to me that the only way that the code can know that the password has been changed is when an application calls getConnection() using the new password.

        So if the login succeeds, then this becomes the new password for the user, and any connections in the pool which have a different password for that user can be dropped, and any connections that are returned to the pool subsequently can be dropped if they don't match the new password.

        I've not looked at the code in detail - and it seems a bit too simple - so I may have overlooked something here.

        Show
        Sebb added a comment - Seems to me that the only way that the code can know that the password has been changed is when an application calls getConnection() using the new password. So if the login succeeds, then this becomes the new password for the user, and any connections in the pool which have a different password for that user can be dropped, and any connections that are returned to the pool subsequently can be dropped if they don't match the new password. I've not looked at the code in detail - and it seems a bit too simple - so I may have overlooked something here.
        Hide
        Phil Steitz added a comment -

        You are describing one reasonable scenario. The code does not do this or anything similar. So if we keep it as is, we at least need to document the behavior.

        Show
        Phil Steitz added a comment - You are describing one reasonable scenario. The code does not do this or anything similar. So if we keep it as is, we at least need to document the behavior.
        Hide
        Phil Steitz added a comment -

        To qualify my first statement above, after the patch the code is no less secure than before - i.e., before the patch, after a password change, the old password would continue to work as long as there were connections available in the pool (and they were not killed on the server side). After the patch, the changed password will work as well.

        Show
        Phil Steitz added a comment - To qualify my first statement above, after the patch the code is no less secure than before - i.e., before the patch, after a password change, the old password would continue to work as long as there were connections available in the pool (and they were not killed on the server side). After the patch, the changed password will work as well.
        Hide
        Sebb added a comment -

        Sorry, I see my previous comment was ambiguous.

        What I meant was that the code could be AMENDED to keep track of the current user/password combination, and use that to invalidate connections which were set up using a different password.

        As far as I can tell, that would prevent the old password from being used again when getting a new connection from the pool.

        It would not stop an active connection from being used, but IMO that's really up to the database engine whether or not a password change has any effect on an open connection. Indeed, the database might insist all connections are closed before permitting a password change.

        With my suggestion, there's no need to change to the API, the user just needs to provide the new password, which is what they would do anyway.

        If the user provides a wrong new password, then the database connection will fail, and in that case obviously the current user/password mapping should not be changed.

        Won't that work?

        Show
        Sebb added a comment - Sorry, I see my previous comment was ambiguous. What I meant was that the code could be AMENDED to keep track of the current user/password combination, and use that to invalidate connections which were set up using a different password. As far as I can tell, that would prevent the old password from being used again when getting a new connection from the pool. It would not stop an active connection from being used, but IMO that's really up to the database engine whether or not a password change has any effect on an open connection. Indeed, the database might insist all connections are closed before permitting a password change. With my suggestion, there's no need to change to the API, the user just needs to provide the new password, which is what they would do anyway. If the user provides a wrong new password, then the database connection will fail, and in that case obviously the current user/password mapping should not be changed. Won't that work?
        Hide
        Phil Steitz added a comment -

        I agree that the code could be modified to do what you are suggesting, but it will be tricky to do so, especially for SharedPoolDataSource.

        Here is another problem with the patch. While unlikely, it is possible that username+password combinations can collide. This would not result in credential sharing, but it would result in getConnection failures, as the password comparison in InstanceKeyDatasource.getConnection would fail. Note that other than in this case, the check will now never fail.

        Here is one idea, which I have not fully thought through, but which may work:

        Modify the following code that validates the password in InstanceKeyDataSource.getConnection

        if (!(null == password ? null == info.getPassword() 
                        : password.equals(info.getPassword()))) {
                    closeDueToException(info);
                    throw new SQLException("Given password did not match password used"
                                           + " to create the PooledConnection.");
         }
        

        to call testCPDS(username, password) before throwing. If it succeeds, that means the password has been changed, so at this point, you go into a loop, calling getPooledConnectionAndInfo(username, password) up to maxActive times until you get an info object with the new password, calling closeDueToException on the "bad" ones. This should clear any idle connections with the old password and will also take care of the ones that are out as they would meet a similar fate when they come back and are subsequently checked out.

        Unfortunately, closeDueToExcption really just returns the connection to the pool, so to actually make this work, we would need to add the ability to mark a PooledConnectionAndInfo object as bad and modify CPDSConnectionFactory.passivate to inspect this property and destroy the object if it is marked as bad.

        I apologize for not digging deeply enough into this when the patch was originally applied.

        Show
        Phil Steitz added a comment - I agree that the code could be modified to do what you are suggesting, but it will be tricky to do so, especially for SharedPoolDataSource. Here is another problem with the patch. While unlikely, it is possible that username+password combinations can collide. This would not result in credential sharing, but it would result in getConnection failures, as the password comparison in InstanceKeyDatasource.getConnection would fail. Note that other than in this case, the check will now never fail. Here is one idea, which I have not fully thought through, but which may work: Modify the following code that validates the password in InstanceKeyDataSource.getConnection if (!( null == password ? null == info.getPassword() : password.equals(info.getPassword()))) { closeDueToException(info); throw new SQLException( "Given password did not match password used" + " to create the PooledConnection." ); } to call testCPDS(username, password) before throwing. If it succeeds, that means the password has been changed, so at this point, you go into a loop, calling getPooledConnectionAndInfo(username, password) up to maxActive times until you get an info object with the new password, calling closeDueToException on the "bad" ones. This should clear any idle connections with the old password and will also take care of the ones that are out as they would meet a similar fate when they come back and are subsequently checked out. Unfortunately, closeDueToExcption really just returns the connection to the pool, so to actually make this work, we would need to add the ability to mark a PooledConnectionAndInfo object as bad and modify CPDSConnectionFactory.passivate to inspect this property and destroy the object if it is marked as bad. I apologize for not digging deeply enough into this when the patch was originally applied.
        Hide
        Sebb added a comment -

        The same key is generated for both ("foo","bar") and ("foob","ar") in .PerUserPoolDataSource.getPoolKey(String username, String password).

        So the code in InstanceKeyDataSource.getConnection is required in order to ensure that the key of username+password only matches the correct username - it's not just needed to check that the correct password has been used.

        Show
        Sebb added a comment - The same key is generated for both ("foo","bar") and ("foob","ar") in .PerUserPoolDataSource.getPoolKey(String username, String password). So the code in InstanceKeyDataSource.getConnection is required in order to ensure that the key of username+password only matches the correct username - it's not just needed to check that the correct password has been used.
        Hide
        Phil Steitz added a comment -

        Here is a patch that implements changed password functionality in SharedPoolDataSource, PerUserPoolDataSource with the
        following semantics for both datasources:

        When a getConnection(username, password) request is processed, an attempt is made to borrow a connection from the backing pool. If successful, a PooledConnectionAndInfo instance is provided by the backing pool. The password field is then compared to the actual parameter provided to getConnection and if it matches, the PooledConnection that it contains is returned to the client. If the password does not match, an attempt is made to connect to the database using the parameters supplied by the client (using testCPDS). If this succeeds, the password must have changed since the PooledConnectionAndInfo instance was created. In this case, the PooledConnectionAndInfo instance is destroyed and removed from the backing pool, and the pool associated with <username> is cleared (using a newly added invalidate method and a reference that it maintains to its parent factoryl). The getConnection implementation (in InstanceKeyDataSource) then proceeds to retry up to 10 times (number could be set lower, as retries should not in general be necessary, but threads returning old connections could force retries), until it gets a PooledConnectionAndInfo with the correct password or encounters a pool error (e.g. NoSuchElementException).

        Summary of changes

        InstanceKeyDataSource:

        • modified getConnection(username, password) to implement the algorithm above.

        PooledConnectionAndInfo:

        • added a "factory" field to hold a reference to the factory that created the instance. This is typed as ConnectionEventListener, which is perhaps misleading, but this is the only interface or parent that CPDSConnectionFactory, KeyedCPDSConnectionFactory share.
        • added a setFactoryPassword method to support resetting the password used by the factory when creating connections. This is a no-op if the factory is KeyedCPDSConnectionFactory (SharedPoolDataSource), but is needed for CPDSConnectionFactory (PerUsserPoolDataSpource).
        • added an invalidate method. The implementation of this method uses the factory reference to get a reference to the backing pool and then invokes the pool's invalidate method. This ensures that the PooledConnection is destroyed and pool counters are appropriately updated.

        CPDSConnectionFactory:

        • added setPassword method to reset the password used for a user pool (PerUserPoolDataSource)
        • passed reference to itself in PooledConnectionAndInfo constructor in makeObject

        KeyedCPDSConnectionFactory:

        • passed reference to itself in PooledConnectionAndInfo constructor in makeObject

        PoolKey:

        • revert to using only the username in the key

        PerUserPoolDataSource:

        • add logic to check to see if getPooledConnectionAndInfo borrowObject failure is due to factory makeObject failure as a result of password change. In this case, check to see if the password provided in the call works to establish a connection and, if so, reset the factory password, clear the pool and retry.
        • changed getPoolKey to only use the username (reverting prior change)

        UserKey:

        • changed equals and hashCode to identify keys with equal user names. The password field is retained, as it is needed by the KeyedCPDSConnectionFactory.

        TestPerUserPoolDataSource, TestSharedPoolDataSource

        • modified test cases to ensure that after password changes, old passwords do not continue to work in getConnection and idle instances with old passwords are removed from the pool

        Comments / suggestions for improvement / arguments for no change welcome. If we do go down this path, the pool fields (and constructor arguments) in CPDSConnectionFactory and KeyedCPDSConnectionFactory should be changed to GenericObjectPool, GenericKeyedObjectPool, respectively. The patch casts to the concrete implementations because invalidation requires the clear methods which are sadly missing from the interfaces. This would not be an incompatible change as these classes have package scope.

        Side note: the last observation above (package scope) means that we can take care of the protected fields in these classes that should be private

        Show
        Phil Steitz added a comment - Here is a patch that implements changed password functionality in SharedPoolDataSource, PerUserPoolDataSource with the following semantics for both datasources: When a getConnection(username, password) request is processed, an attempt is made to borrow a connection from the backing pool. If successful, a PooledConnectionAndInfo instance is provided by the backing pool. The password field is then compared to the actual parameter provided to getConnection and if it matches, the PooledConnection that it contains is returned to the client. If the password does not match, an attempt is made to connect to the database using the parameters supplied by the client (using testCPDS). If this succeeds, the password must have changed since the PooledConnectionAndInfo instance was created. In this case, the PooledConnectionAndInfo instance is destroyed and removed from the backing pool, and the pool associated with <username> is cleared (using a newly added invalidate method and a reference that it maintains to its parent factoryl). The getConnection implementation (in InstanceKeyDataSource) then proceeds to retry up to 10 times (number could be set lower, as retries should not in general be necessary, but threads returning old connections could force retries), until it gets a PooledConnectionAndInfo with the correct password or encounters a pool error (e.g. NoSuchElementException). Summary of changes InstanceKeyDataSource: modified getConnection(username, password) to implement the algorithm above. PooledConnectionAndInfo: added a "factory" field to hold a reference to the factory that created the instance. This is typed as ConnectionEventListener, which is perhaps misleading, but this is the only interface or parent that CPDSConnectionFactory, KeyedCPDSConnectionFactory share. added a setFactoryPassword method to support resetting the password used by the factory when creating connections. This is a no-op if the factory is KeyedCPDSConnectionFactory (SharedPoolDataSource), but is needed for CPDSConnectionFactory (PerUsserPoolDataSpource). added an invalidate method. The implementation of this method uses the factory reference to get a reference to the backing pool and then invokes the pool's invalidate method. This ensures that the PooledConnection is destroyed and pool counters are appropriately updated. CPDSConnectionFactory: added setPassword method to reset the password used for a user pool (PerUserPoolDataSource) passed reference to itself in PooledConnectionAndInfo constructor in makeObject KeyedCPDSConnectionFactory: passed reference to itself in PooledConnectionAndInfo constructor in makeObject PoolKey: revert to using only the username in the key PerUserPoolDataSource: add logic to check to see if getPooledConnectionAndInfo borrowObject failure is due to factory makeObject failure as a result of password change. In this case, check to see if the password provided in the call works to establish a connection and, if so, reset the factory password, clear the pool and retry. changed getPoolKey to only use the username (reverting prior change) UserKey: changed equals and hashCode to identify keys with equal user names. The password field is retained, as it is needed by the KeyedCPDSConnectionFactory. TestPerUserPoolDataSource, TestSharedPoolDataSource modified test cases to ensure that after password changes, old passwords do not continue to work in getConnection and idle instances with old passwords are removed from the pool Comments / suggestions for improvement / arguments for no change welcome. If we do go down this path, the pool fields (and constructor arguments) in CPDSConnectionFactory and KeyedCPDSConnectionFactory should be changed to GenericObjectPool, GenericKeyedObjectPool, respectively. The patch casts to the concrete implementations because invalidation requires the clear methods which are sadly missing from the interfaces. This would not be an incompatible change as these classes have package scope. Side note: the last observation above (package scope) means that we can take care of the protected fields in these classes that should be private
        Hide
        Sebb added a comment -

        What about connections which are returned to the pool?
        If these have the old password, I assume this will be detected when the pool item is next considered for use.
        However, this may be much later - would it not be better to drop them before returning to the pool? Or is that too much work?

        Some minor points:

        The PooledConnectionAndInfo ctor Javadoc says:

        The factory must be an instance of either
        CPDSConnectionFactory (used by PerUserPoolDataSource) or
        KeyedCPDSConnectionFactory (used by SharedPoolDataSource).

        However, it does not check this. Perhaps it should, rather than waiting until invalidate() causes a ClassCastException

        ==

        In the code

        if (factory != null) {
          if (factory instanceof CPDSConnectionFactory) {
        

        the null check is redundant as null can be used in the instanceof check without a problem.

        Show
        Sebb added a comment - What about connections which are returned to the pool? If these have the old password, I assume this will be detected when the pool item is next considered for use. However, this may be much later - would it not be better to drop them before returning to the pool? Or is that too much work? Some minor points: The PooledConnectionAndInfo ctor Javadoc says: The factory must be an instance of either CPDSConnectionFactory (used by PerUserPoolDataSource) or KeyedCPDSConnectionFactory (used by SharedPoolDataSource). However, it does not check this. Perhaps it should, rather than waiting until invalidate() causes a ClassCastException == In the code if (factory != null ) { if (factory instanceof CPDSConnectionFactory) { the null check is redundant as null can be used in the instanceof check without a problem.
        Hide
        Phil Steitz added a comment -

        Good points. Detecting and killing "old password" connections when they are returned would be possible for PerUserPoolDataSource, since the CPDSConnectionFactory stores the (single user) password. The only way that I can think of to do this for SharedPoolDataSource would be to attempt a database authenication each time a connection is returned, which is obviously not a good idea. I will look more fully into what would be required to do this for PerUserPoolDataSource.

        Agree on the null check and I guess the PooledConnectionAndInfo constructor should check and throw.

        Show
        Phil Steitz added a comment - Good points. Detecting and killing "old password" connections when they are returned would be possible for PerUserPoolDataSource, since the CPDSConnectionFactory stores the (single user) password. The only way that I can think of to do this for SharedPoolDataSource would be to attempt a database authenication each time a connection is returned, which is obviously not a good idea. I will look more fully into what would be required to do this for PerUserPoolDataSource. Agree on the null check and I guess the PooledConnectionAndInfo constructor should check and throw.
        Hide
        Phil Steitz added a comment -

        I had forgotten that both CPDSConnectionFactory and KeyedCPDSConnectionFactory maintain maps holding (weak) references to each of the PooledConnectionAndInfo instances managed by their pools - including the wrappers for the PooledConnections that are checked out. So the invalidate method in the patch could, in addition to clearing the idle instance pool for the affected user, somehow mark the PooledConnectionAndInfo instances corresponding to "old password" connections checked out and then the factory could invalidate them when it gets a connectionClosed event on a marked connection. The factories could even be configured to close the connections before they are returned, if that is the desired behavior (I would not recommend that by default, but it could be a config option).

        Another problem that I have been worried a little about is the "denial of service" issue raised above. Both the original patch and the alternative suffer from the problem that if bad passwords supplied in getConnection(username, password) are common (for previously authenticated usernames), this may strain database resources, as the testCPDS check requires a database connection. Note that the only net new load is the case where connections have been successfully established for a given username and then bad passwords are supplied frequently for the same username. Random bad pairs would have the same load effect as they do in 1.2.2. If we want to be conservative here, we might make the changed password functionality configurable, with default turned off.

        Show
        Phil Steitz added a comment - I had forgotten that both CPDSConnectionFactory and KeyedCPDSConnectionFactory maintain maps holding (weak) references to each of the PooledConnectionAndInfo instances managed by their pools - including the wrappers for the PooledConnections that are checked out. So the invalidate method in the patch could, in addition to clearing the idle instance pool for the affected user, somehow mark the PooledConnectionAndInfo instances corresponding to "old password" connections checked out and then the factory could invalidate them when it gets a connectionClosed event on a marked connection. The factories could even be configured to close the connections before they are returned, if that is the desired behavior (I would not recommend that by default, but it could be a config option). Another problem that I have been worried a little about is the "denial of service" issue raised above. Both the original patch and the alternative suffer from the problem that if bad passwords supplied in getConnection(username, password) are common (for previously authenticated usernames), this may strain database resources, as the testCPDS check requires a database connection. Note that the only net new load is the case where connections have been successfully established for a given username and then bad passwords are supplied frequently for the same username. Random bad pairs would have the same load effect as they do in 1.2.2. If we want to be conservative here, we might make the changed password functionality configurable, with default turned off.
        Hide
        Sebb added a comment -

        Minor issue: setPassword() does not need to be synch., because the variable is volatile.

        On further reflection, I'm not sure I like the proposed patch - for example I'm not sure that PooledConnectionAndInfo should have a reference to the factory used to create it.

        However I don't (yet) know the DBCP design well enough to know if there is any other way to solve the problem.

        Show
        Sebb added a comment - Minor issue: setPassword() does not need to be synch., because the variable is volatile. On further reflection, I'm not sure I like the proposed patch - for example I'm not sure that PooledConnectionAndInfo should have a reference to the factory used to create it. However I don't (yet) know the DBCP design well enough to know if there is any other way to solve the problem.
        Hide
        Phil Steitz added a comment -

        I think I was planning to clean up the access on _password, making it private, so access could be syched.

        Agree that it is a little smelly and inefficient to have PooledConnectionAndInfo instances hold factory references. For SharedPoolDataSource, the datasource itself could hold a reference to the factory. PerUserPoolDataSoures would have to maintain a map of such references. I will look into alternatives. Suggestions welcome.

        Show
        Phil Steitz added a comment - I think I was planning to clean up the access on _password, making it private, so access could be syched. Agree that it is a little smelly and inefficient to have PooledConnectionAndInfo instances hold factory references. For SharedPoolDataSource, the datasource itself could hold a reference to the factory. PerUserPoolDataSoures would have to maintain a map of such references. I will look into alternatives. Suggestions welcome.
        Hide
        Sebb added a comment -

        Seems to me that PerUserPoolDataSource could store the factory in the map, instead of the pool - or failing that a wrapper object that contained both.
        This would allow PUPDS to track the password.

        BTW, it seems to me that CPDSConnectionFactory really ought not to have any way of changing the CPDS or Pool once the factory has been created.
        Not sure it makes sense for either to be changed on the fly - I suspect it would seriously mess things up.

        Show
        Sebb added a comment - Seems to me that PerUserPoolDataSource could store the factory in the map, instead of the pool - or failing that a wrapper object that contained both. This would allow PUPDS to track the password. BTW, it seems to me that CPDSConnectionFactory really ought not to have any way of changing the CPDS or Pool once the factory has been created. Not sure it makes sense for either to be changed on the fly - I suspect it would seriously mess things up.
        Hide
        Phil Steitz added a comment -

        Here is an updated patch that accomplishes what changePassword.patch did without adding a factory reference to PooledConnectionAndInfo. Instead, a new interface is added - PooledConnectionManager - including methods to invalidate PooledConnections and this interface is implemented by the connection factories. PerUserPoolDataSource is modified to replace its pools map with a "managers" map holding references to source factories rather than host pools. Otherwise changes are similar to changePassword.patch. The invalidate method is a useful enhancement which could potentially be exposed by the datasources, allowing users to selectively "realy close" pooled connections without messing up the pool or its counters.

        This patch also reduces mutability of the factories.

        For SharedPooleDataSource / KeyedCPDSConnectionFactory, connections returning after password reset are still not closed immediately. To implement this would require more work.

        Show
        Phil Steitz added a comment - Here is an updated patch that accomplishes what changePassword.patch did without adding a factory reference to PooledConnectionAndInfo. Instead, a new interface is added - PooledConnectionManager - including methods to invalidate PooledConnections and this interface is implemented by the connection factories. PerUserPoolDataSource is modified to replace its pools map with a "managers" map holding references to source factories rather than host pools. Otherwise changes are similar to changePassword.patch. The invalidate method is a useful enhancement which could potentially be exposed by the datasources, allowing users to selectively "realy close" pooled connections without messing up the pool or its counters. This patch also reduces mutability of the factories. For SharedPooleDataSource / KeyedCPDSConnectionFactory, connections returning after password reset are still not closed immediately. To implement this would require more work.
        Hide
        Sebb added a comment -

        Latest patch looks good, except I'm not sure that CPDSConnectionFactory should allow the username to be changed, only the password.

        It's a pity that CPDSConnectionFactory is not quite immutable, but I suspect it might be rather complicated to update the stored factory when the password changes.

        Show
        Sebb added a comment - Latest patch looks good, except I'm not sure that CPDSConnectionFactory should allow the username to be changed, only the password. It's a pity that CPDSConnectionFactory is not quite immutable, but I suspect it might be rather complicated to update the stored factory when the password changes.
        Hide
        Sebb added a comment -

        Just noticed a minor issue in the existing testChangePassword() methods:

        In the following sample:

        try {
            ds.getConnection("foo", "bar").close(); // old password
            fail("Should have generated SQLException"); 
        } catch etc
        

        the .close() should be removed - the test is supposed to be checking getConnection(), not close().
        In theory the get could succeed and the close() fail.

        Need to fix these at some point.

        Show
        Sebb added a comment - Just noticed a minor issue in the existing testChangePassword() methods: In the following sample: try { ds.getConnection( "foo" , "bar" ).close(); // old password fail( "Should have generated SQLException" ); } catch etc the .close() should be removed - the test is supposed to be checking getConnection(), not close(). In theory the get could succeed and the close() fail. Need to fix these at some point.
        Hide
        Phil Steitz added a comment -

        Committed passwordChange2.patch with one change in r907288. Per Sebb's comment, the only mutability needed in CPDSConnectionFactory is the password, so PooledConnectionManager exposes only setPassword, rather than setUserPassKey.

        Dropped extraneous close in the changePassword tests in r907290.

        Show
        Phil Steitz added a comment - Committed passwordChange2.patch with one change in r907288. Per Sebb's comment, the only mutability needed in CPDSConnectionFactory is the password, so PooledConnectionManager exposes only setPassword, rather than setUserPassKey. Dropped extraneous close in the changePassword tests in r907290.

          People

          • Assignee:
            Mark Thomas
            Reporter:
            Michael T. Dean
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development