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. dbcp-SharedPoolWithPasswordChange-20070309.patch
        2 kB
        Michael T. Dean
      3. changePassword.patch
        26 kB
        Phil Steitz
      4. ASF.LICENSE.NOT.GRANTED--dbcp-SharedPoolWithPasswordChange.patch
        2 kB
        Michael T. Dean

        Activity

          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