[
https://issues.apache.org/jira/browse/DBCP-8?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Mark Thomas reopened DBCP-8:
----------------------------
Assignee: Mark Thomas
Re-opening as the fix needs to be extended to the PerUserPoolDataSource
> [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: dbcp-SharedPoolWithPasswordChange-20070309.patch,
> dbcp-SharedPoolWithPasswordChange.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.1080...@pandora.be%3e
> and
> http://mail-archives.eu.apache.org/mod_mbox/jakarta-commons-user/200405.mbox/%3c40aca3de.8080...@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.
