[jira] Commented: (DBCP-8) [dbcp][PATCH] Handle changed passwords in SharedPoolDataSource

["Sebb (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/DBCP-8?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12828070#action_12828070 ] 

Sebb commented on DBCP-8:
-------------------------

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.

> [dbcp][PATCH] Handle changed passwords in SharedPoolDataSource
> --------------------------------------------------------------
>
>                 Key: DBCP-8
>                 URL: https://issues.apache.org/jira/browse/DBCP-8
>             Project: Commons Dbcp
>          Issue Type: Bug
>         Environment: Operating System: other
> Platform: Other
>            Reporter: Michael T. Dean
>            Assignee: Mark Thomas
>             Fix For: 1.3
>
>         Attachments: changePassword.patch, dbcp-SharedPoolWithPasswordChange-20070309.patch, dbcp-SharedPoolWithPasswordChange.patch, passwordChange2.patch
>
>
> 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.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.