[ https://issues.apache.org/jira/browse/DBCP-8?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Michael T. Dean updated DBCP-8: ------------------------------- Attachment: dbcp-SharedPoolWithPasswordChange-20070309.patch Updated patch that applies cleanly to current SVN trunk > [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 > 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/[EMAIL > PROTECTED] > and > http://mail-archives.eu.apache.org/mod_mbox/jakarta-commons-user/200405.mbox/[EMAIL > PROTECTED] > 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. --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]