commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Phil Steitz (JIRA)" <j...@apache.org>
Subject [jira] Updated: (DBCP-8) [dbcp][PATCH] Handle changed passwords in SharedPoolDataSource
Date Thu, 21 Jan 2010 23:06:54 GMT

     [ https://issues.apache.org/jira/browse/DBCP-8?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Phil Steitz updated DBCP-8:
---------------------------

    Attachment: changePassword.patch

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 ;)



> [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
>
>
> 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.


Mime
View raw message