Re: commons-dbcp git commit: [DBCP-517] Make defensive copies of char[] passwords.

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: commons-dbcp git commit: [DBCP-517] Make defensive copies of char[] passwords.

Oliver Heger-3
+1, Thank you.

I had planned to create a patch for this issue, but did not find the
time so far.

BTW, I have quite often the need to create defensive copies of arrays or
collections in some variants (e.g. null safe, with wrapping to an
unmodifiable collection, etc.). Could this be an addition to [lang]? A
new utility class like DefensiveCopyUtils?

Oliver

Am 25.07.2018 um 00:34 schrieb [hidden email]:

> Repository: commons-dbcp
> Updated Branches:
>   refs/heads/master 70822f11d -> d7969ac93
>
>
> [DBCP-517] Make defensive copies of char[] passwords.
>
> Project: http://git-wip-us.apache.org/repos/asf/commons-dbcp/repo
> Commit: http://git-wip-us.apache.org/repos/asf/commons-dbcp/commit/d7969ac9
> Tree: http://git-wip-us.apache.org/repos/asf/commons-dbcp/tree/d7969ac9
> Diff: http://git-wip-us.apache.org/repos/asf/commons-dbcp/diff/d7969ac9
>
> Branch: refs/heads/master
> Commit: d7969ac934e752e7a7b258fa5a5af9a563c40a13
> Parents: 70822f1
> Author: Gary Gregory <[hidden email]>
> Authored: Tue Jul 24 16:34:43 2018 -0600
> Committer: Gary Gregory <[hidden email]>
> Committed: Tue Jul 24 16:34:43 2018 -0600
>
> ----------------------------------------------------------------------
>  src/changes/changes.xml                                 |  5 ++++-
>  src/main/java/org/apache/commons/dbcp2/Utils.java       | 12 ++++++++++++
>  .../commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java    |  4 ++--
>  .../dbcp2/datasources/CPDSConnectionFactory.java        | 11 ++++++++++-
>  .../dbcp2/cpdsadapter/TestDriverAdapterCPDS.java        |  9 +++++++++
>  .../dbcp2/datasources/TestCPDSConnectionFactory.java    | 10 ++++++++++
>  6 files changed, 47 insertions(+), 4 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/changes/changes.xml
> ----------------------------------------------------------------------
> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> index c924411..8f1de55 100644
> --- a/src/changes/changes.xml
> +++ b/src/changes/changes.xml
> @@ -61,9 +61,12 @@ The <action> type attribute can be add,update,fix,remove.
>  
>    <body>
>      <release version="2.6.0" date="2018-MM-DD" description="This is a minor release, including bug fixes and enhancements.">
> -      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Gary Gregory">
> +      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Tom Jenkinson, Gary Gregory">
>          Allow DBCP to register with a TransactionSynchronizationRegistry for XA cases.
>        </action>
> +      <action dev="ggregory" type="update" issue="DBCP-517" due-to="Gary Gregory">
> +        Make defensive copies of char[] passwords.
> +      </action>
>      </release>
>      <release version="2.5.0" date="2018-07-15" description="This is a minor release, including bug fixes and enhancements.">
>        <action dev="ggregory" type="update" issue="DBCP-505" due-to="Gary Gregory">
>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/Utils.java
> ----------------------------------------------------------------------
> diff --git a/src/main/java/org/apache/commons/dbcp2/Utils.java b/src/main/java/org/apache/commons/dbcp2/Utils.java
> index 8e798c4..244b51b 100644
> --- a/src/main/java/org/apache/commons/dbcp2/Utils.java
> +++ b/src/main/java/org/apache/commons/dbcp2/Utils.java
> @@ -72,6 +72,17 @@ public final class Utils {
>      }
>  
>      /**
> +     * Clones the given char[] if not null.
> +     *
> +     * @param value
> +     *            may be null.
> +     * @return a cloned char[] or null.
> +     */
> +    public static char[] clone(final char[] value) {
> +        return value == null ? null : value.clone();
> +    }
> +
> +    /**
>       * Closes the ResultSet (which may be null).
>       *
>       * @param resultSet
> @@ -169,4 +180,5 @@ public final class Utils {
>      public static String toString(final char[] value) {
>          return value == null ? null : String.valueOf(value);
>      }
> +
>  }
>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> ----------------------------------------------------------------------
> diff --git a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> index bbc8831..0844c9b 100644
> --- a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> +++ b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> @@ -423,8 +423,8 @@ public class DriverAdapterCPDS implements ConnectionPoolDataSource, Referenceabl
>       */
>      public void setPassword(final char[] userPassword) {
>          assertInitializationAllowed();
> -        this.userPassword = userPassword;
> -        update(connectionProperties, KEY_PASSWORD, Utils.toString(userPassword));
> +        this.userPassword = Utils.clone(userPassword);
> +        update(connectionProperties, KEY_PASSWORD, Utils.toString(this.userPassword));
>      }
>  
>      /**
>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> ----------------------------------------------------------------------
> diff --git a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> index f0ae74d..c0ee90b 100644
> --- a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> +++ b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> @@ -122,6 +122,15 @@ class CPDSConnectionFactory
>      }
>  
>      /**
> +     * (Testing API) Gets the value of password for the default user.
> +     *
> +     * @return value of password.
> +     */
> +    char[] getPasswordCharArray() {
> +        return userPassword;
> +    }
> +    
> +    /**
>       * Returns the object pool used to pool connections created by this factory.
>       *
>       * @return ObjectPool managing pooled connections
> @@ -335,7 +344,7 @@ class CPDSConnectionFactory
>       *            new password
>       */
>      public synchronized void setPassword(final char[] userPassword) {
> -        this.userPassword = userPassword;
> +        this.userPassword =  Utils.clone(userPassword);
>      }
>  
>      /**
>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> ----------------------------------------------------------------------
> diff --git a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> index 0669b1f..7bae26e 100644
> --- a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> +++ b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> @@ -208,6 +208,15 @@ public class TestDriverAdapterCPDS {
>      }
>  
>      @Test
> +    public void testSetPasswordThenModCharArray() {
> +        char[] pwd = {'a' };
> +        pcds.setPassword(pwd);
> +        assertEquals("a", pcds.getPassword());
> +        pwd[0] = 'b';
> +        assertEquals("a", pcds.getPassword());
> +    }
> +
> +    @Test
>      public void testSetPasswordNullWithConnectionProperties() throws Exception {
>          pcds.setConnectionProperties(new Properties());
>          pcds.setPassword("Secret");
>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> ----------------------------------------------------------------------
> diff --git a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> index 3f9c157..86c0dfe 100644
> --- a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> +++ b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> @@ -143,6 +143,16 @@ public class TestCPDSConnectionFactory {
>          assertEquals(0, pool.getNumIdle());
>      }
>  
> +    @Test
> +    public void testSetPasswordThenModCharArray() {
> +        final CPDSConnectionFactory factory = new CPDSConnectionFactory(cpds, null, -1, false, "userName", "password");
> +        char[] pwd = {'a' };
> +        factory.setPassword(pwd);
> +        assertEquals("a", String.valueOf(factory.getPasswordCharArray()));
> +        pwd[0] = 'b';
> +        assertEquals("a", String.valueOf(factory.getPasswordCharArray()));
> +    }
> +
>      /**
>       * JIRA: DBCP-442
>       */
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: commons-dbcp git commit: [DBCP-517] Make defensive copies of char[] passwords.

garydgregory
I'd we do not already have that, the name does not need "defensive".
Commons Collections would be where to put collections related code.

Gary

On Wed, Jul 25, 2018, 14:00 Oliver Heger <[hidden email]>
wrote:

> +1, Thank you.
>
> I had planned to create a patch for this issue, but did not find the
> time so far.
>
> BTW, I have quite often the need to create defensive copies of arrays or
> collections in some variants (e.g. null safe, with wrapping to an
> unmodifiable collection, etc.). Could this be an addition to [lang]? A
> new utility class like DefensiveCopyUtils?
>
> Oliver
>
> Am 25.07.2018 um 00:34 schrieb [hidden email]:
> > Repository: commons-dbcp
> > Updated Branches:
> >   refs/heads/master 70822f11d -> d7969ac93
> >
> >
> > [DBCP-517] Make defensive copies of char[] passwords.
> >
> > Project: http://git-wip-us.apache.org/repos/asf/commons-dbcp/repo
> > Commit:
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/commit/d7969ac9
> > Tree: http://git-wip-us.apache.org/repos/asf/commons-dbcp/tree/d7969ac9
> > Diff: http://git-wip-us.apache.org/repos/asf/commons-dbcp/diff/d7969ac9
> >
> > Branch: refs/heads/master
> > Commit: d7969ac934e752e7a7b258fa5a5af9a563c40a13
> > Parents: 70822f1
> > Author: Gary Gregory <[hidden email]>
> > Authored: Tue Jul 24 16:34:43 2018 -0600
> > Committer: Gary Gregory <[hidden email]>
> > Committed: Tue Jul 24 16:34:43 2018 -0600
> >
> > ----------------------------------------------------------------------
> >  src/changes/changes.xml                                 |  5 ++++-
> >  src/main/java/org/apache/commons/dbcp2/Utils.java       | 12
> ++++++++++++
> >  .../commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java    |  4 ++--
> >  .../dbcp2/datasources/CPDSConnectionFactory.java        | 11 ++++++++++-
> >  .../dbcp2/cpdsadapter/TestDriverAdapterCPDS.java        |  9 +++++++++
> >  .../dbcp2/datasources/TestCPDSConnectionFactory.java    | 10 ++++++++++
> >  6 files changed, 47 insertions(+), 4 deletions(-)
> > ----------------------------------------------------------------------
> >
> >
> >
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/changes/changes.xml
> > ----------------------------------------------------------------------
> > diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> > index c924411..8f1de55 100644
> > --- a/src/changes/changes.xml
> > +++ b/src/changes/changes.xml
> > @@ -61,9 +61,12 @@ The <action> type attribute can be
> add,update,fix,remove.
> >
> >    <body>
> >      <release version="2.6.0" date="2018-MM-DD" description="This is a
> minor release, including bug fixes and enhancements.">
> > -      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Gary
> Gregory">
> > +      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Tom
> Jenkinson, Gary Gregory">
> >          Allow DBCP to register with a
> TransactionSynchronizationRegistry for XA cases.
> >        </action>
> > +      <action dev="ggregory" type="update" issue="DBCP-517"
> due-to="Gary Gregory">
> > +        Make defensive copies of char[] passwords.
> > +      </action>
> >      </release>
> >      <release version="2.5.0" date="2018-07-15" description="This is a
> minor release, including bug fixes and enhancements.">
> >        <action dev="ggregory" type="update" issue="DBCP-505"
> due-to="Gary Gregory">
> >
> >
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/Utils.java
> > ----------------------------------------------------------------------
> > diff --git a/src/main/java/org/apache/commons/dbcp2/Utils.java
> b/src/main/java/org/apache/commons/dbcp2/Utils.java
> > index 8e798c4..244b51b 100644
> > --- a/src/main/java/org/apache/commons/dbcp2/Utils.java
> > +++ b/src/main/java/org/apache/commons/dbcp2/Utils.java
> > @@ -72,6 +72,17 @@ public final class Utils {
> >      }
> >
> >      /**
> > +     * Clones the given char[] if not null.
> > +     *
> > +     * @param value
> > +     *            may be null.
> > +     * @return a cloned char[] or null.
> > +     */
> > +    public static char[] clone(final char[] value) {
> > +        return value == null ? null : value.clone();
> > +    }
> > +
> > +    /**
> >       * Closes the ResultSet (which may be null).
> >       *
> >       * @param resultSet
> > @@ -169,4 +180,5 @@ public final class Utils {
> >      public static String toString(final char[] value) {
> >          return value == null ? null : String.valueOf(value);
> >      }
> > +
> >  }
> >
> >
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> > ----------------------------------------------------------------------
> > diff --git
> a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> > index bbc8831..0844c9b 100644
> > ---
> a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> > +++
> b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> > @@ -423,8 +423,8 @@ public class DriverAdapterCPDS implements
> ConnectionPoolDataSource, Referenceabl
> >       */
> >      public void setPassword(final char[] userPassword) {
> >          assertInitializationAllowed();
> > -        this.userPassword = userPassword;
> > -        update(connectionProperties, KEY_PASSWORD,
> Utils.toString(userPassword));
> > +        this.userPassword = Utils.clone(userPassword);
> > +        update(connectionProperties, KEY_PASSWORD,
> Utils.toString(this.userPassword));
> >      }
> >
> >      /**
> >
> >
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> > ----------------------------------------------------------------------
> > diff --git
> a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> > index f0ae74d..c0ee90b 100644
> > ---
> a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> > +++
> b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> > @@ -122,6 +122,15 @@ class CPDSConnectionFactory
> >      }
> >
> >      /**
> > +     * (Testing API) Gets the value of password for the default user.
> > +     *
> > +     * @return value of password.
> > +     */
> > +    char[] getPasswordCharArray() {
> > +        return userPassword;
> > +    }
> > +
> > +    /**
> >       * Returns the object pool used to pool connections created by this
> factory.
> >       *
> >       * @return ObjectPool managing pooled connections
> > @@ -335,7 +344,7 @@ class CPDSConnectionFactory
> >       *            new password
> >       */
> >      public synchronized void setPassword(final char[] userPassword) {
> > -        this.userPassword = userPassword;
> > +        this.userPassword =  Utils.clone(userPassword);
> >      }
> >
> >      /**
> >
> >
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> > ----------------------------------------------------------------------
> > diff --git
> a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> > index 0669b1f..7bae26e 100644
> > ---
> a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> > +++
> b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> > @@ -208,6 +208,15 @@ public class TestDriverAdapterCPDS {
> >      }
> >
> >      @Test
> > +    public void testSetPasswordThenModCharArray() {
> > +        char[] pwd = {'a' };
> > +        pcds.setPassword(pwd);
> > +        assertEquals("a", pcds.getPassword());
> > +        pwd[0] = 'b';
> > +        assertEquals("a", pcds.getPassword());
> > +    }
> > +
> > +    @Test
> >      public void testSetPasswordNullWithConnectionProperties() throws
> Exception {
> >          pcds.setConnectionProperties(new Properties());
> >          pcds.setPassword("Secret");
> >
> >
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> > ----------------------------------------------------------------------
> > diff --git
> a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> > index 3f9c157..86c0dfe 100644
> > ---
> a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> > +++
> b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> > @@ -143,6 +143,16 @@ public class TestCPDSConnectionFactory {
> >          assertEquals(0, pool.getNumIdle());
> >      }
> >
> > +    @Test
> > +    public void testSetPasswordThenModCharArray() {
> > +        final CPDSConnectionFactory factory = new
> CPDSConnectionFactory(cpds, null, -1, false, "userName", "password");
> > +        char[] pwd = {'a' };
> > +        factory.setPassword(pwd);
> > +        assertEquals("a",
> String.valueOf(factory.getPasswordCharArray()));
> > +        pwd[0] = 'b';
> > +        assertEquals("a",
> String.valueOf(factory.getPasswordCharArray()));
> > +    }
> > +
> >      /**
> >       * JIRA: DBCP-442
> >       */
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: commons-dbcp git commit: [DBCP-517] Make defensive copies of char[] passwords.

Oliver Heger-3


Am 25.07.2018 um 23:36 schrieb Gary Gregory:
> I'd we do not already have that, the name does not need "defensive".
> Commons Collections would be where to put collections related code.

I would rather think that this copy functionality is pretty basic, so I
would put it in [lang]. I would not want to depend on [collections] just
to use one or two copy methods. And the utility class could support
non-collection types (mainly arrays) as well.

Oliver

>
> Gary
>
> On Wed, Jul 25, 2018, 14:00 Oliver Heger <[hidden email]>
> wrote:
>
>> +1, Thank you.
>>
>> I had planned to create a patch for this issue, but did not find the
>> time so far.
>>
>> BTW, I have quite often the need to create defensive copies of arrays or
>> collections in some variants (e.g. null safe, with wrapping to an
>> unmodifiable collection, etc.). Could this be an addition to [lang]? A
>> new utility class like DefensiveCopyUtils?
>>
>> Oliver
>>
>> Am 25.07.2018 um 00:34 schrieb [hidden email]:
>>> Repository: commons-dbcp
>>> Updated Branches:
>>>   refs/heads/master 70822f11d -> d7969ac93
>>>
>>>
>>> [DBCP-517] Make defensive copies of char[] passwords.
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/commons-dbcp/repo
>>> Commit:
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/commit/d7969ac9
>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-dbcp/tree/d7969ac9
>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-dbcp/diff/d7969ac9
>>>
>>> Branch: refs/heads/master
>>> Commit: d7969ac934e752e7a7b258fa5a5af9a563c40a13
>>> Parents: 70822f1
>>> Author: Gary Gregory <[hidden email]>
>>> Authored: Tue Jul 24 16:34:43 2018 -0600
>>> Committer: Gary Gregory <[hidden email]>
>>> Committed: Tue Jul 24 16:34:43 2018 -0600
>>>
>>> ----------------------------------------------------------------------
>>>  src/changes/changes.xml                                 |  5 ++++-
>>>  src/main/java/org/apache/commons/dbcp2/Utils.java       | 12
>> ++++++++++++
>>>  .../commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java    |  4 ++--
>>>  .../dbcp2/datasources/CPDSConnectionFactory.java        | 11 ++++++++++-
>>>  .../dbcp2/cpdsadapter/TestDriverAdapterCPDS.java        |  9 +++++++++
>>>  .../dbcp2/datasources/TestCPDSConnectionFactory.java    | 10 ++++++++++
>>>  6 files changed, 47 insertions(+), 4 deletions(-)
>>> ----------------------------------------------------------------------
>>>
>>>
>>>
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/changes/changes.xml
>>> ----------------------------------------------------------------------
>>> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
>>> index c924411..8f1de55 100644
>>> --- a/src/changes/changes.xml
>>> +++ b/src/changes/changes.xml
>>> @@ -61,9 +61,12 @@ The <action> type attribute can be
>> add,update,fix,remove.
>>>
>>>    <body>
>>>      <release version="2.6.0" date="2018-MM-DD" description="This is a
>> minor release, including bug fixes and enhancements.">
>>> -      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Gary
>> Gregory">
>>> +      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Tom
>> Jenkinson, Gary Gregory">
>>>          Allow DBCP to register with a
>> TransactionSynchronizationRegistry for XA cases.
>>>        </action>
>>> +      <action dev="ggregory" type="update" issue="DBCP-517"
>> due-to="Gary Gregory">
>>> +        Make defensive copies of char[] passwords.
>>> +      </action>
>>>      </release>
>>>      <release version="2.5.0" date="2018-07-15" description="This is a
>> minor release, including bug fixes and enhancements.">
>>>        <action dev="ggregory" type="update" issue="DBCP-505"
>> due-to="Gary Gregory">
>>>
>>>
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/Utils.java
>>> ----------------------------------------------------------------------
>>> diff --git a/src/main/java/org/apache/commons/dbcp2/Utils.java
>> b/src/main/java/org/apache/commons/dbcp2/Utils.java
>>> index 8e798c4..244b51b 100644
>>> --- a/src/main/java/org/apache/commons/dbcp2/Utils.java
>>> +++ b/src/main/java/org/apache/commons/dbcp2/Utils.java
>>> @@ -72,6 +72,17 @@ public final class Utils {
>>>      }
>>>
>>>      /**
>>> +     * Clones the given char[] if not null.
>>> +     *
>>> +     * @param value
>>> +     *            may be null.
>>> +     * @return a cloned char[] or null.
>>> +     */
>>> +    public static char[] clone(final char[] value) {
>>> +        return value == null ? null : value.clone();
>>> +    }
>>> +
>>> +    /**
>>>       * Closes the ResultSet (which may be null).
>>>       *
>>>       * @param resultSet
>>> @@ -169,4 +180,5 @@ public final class Utils {
>>>      public static String toString(final char[] value) {
>>>          return value == null ? null : String.valueOf(value);
>>>      }
>>> +
>>>  }
>>>
>>>
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
>>> ----------------------------------------------------------------------
>>> diff --git
>> a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
>> b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
>>> index bbc8831..0844c9b 100644
>>> ---
>> a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
>>> +++
>> b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
>>> @@ -423,8 +423,8 @@ public class DriverAdapterCPDS implements
>> ConnectionPoolDataSource, Referenceabl
>>>       */
>>>      public void setPassword(final char[] userPassword) {
>>>          assertInitializationAllowed();
>>> -        this.userPassword = userPassword;
>>> -        update(connectionProperties, KEY_PASSWORD,
>> Utils.toString(userPassword));
>>> +        this.userPassword = Utils.clone(userPassword);
>>> +        update(connectionProperties, KEY_PASSWORD,
>> Utils.toString(this.userPassword));
>>>      }
>>>
>>>      /**
>>>
>>>
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
>>> ----------------------------------------------------------------------
>>> diff --git
>> a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
>> b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
>>> index f0ae74d..c0ee90b 100644
>>> ---
>> a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
>>> +++
>> b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
>>> @@ -122,6 +122,15 @@ class CPDSConnectionFactory
>>>      }
>>>
>>>      /**
>>> +     * (Testing API) Gets the value of password for the default user.
>>> +     *
>>> +     * @return value of password.
>>> +     */
>>> +    char[] getPasswordCharArray() {
>>> +        return userPassword;
>>> +    }
>>> +
>>> +    /**
>>>       * Returns the object pool used to pool connections created by this
>> factory.
>>>       *
>>>       * @return ObjectPool managing pooled connections
>>> @@ -335,7 +344,7 @@ class CPDSConnectionFactory
>>>       *            new password
>>>       */
>>>      public synchronized void setPassword(final char[] userPassword) {
>>> -        this.userPassword = userPassword;
>>> +        this.userPassword =  Utils.clone(userPassword);
>>>      }
>>>
>>>      /**
>>>
>>>
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
>>> ----------------------------------------------------------------------
>>> diff --git
>> a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
>> b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
>>> index 0669b1f..7bae26e 100644
>>> ---
>> a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
>>> +++
>> b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
>>> @@ -208,6 +208,15 @@ public class TestDriverAdapterCPDS {
>>>      }
>>>
>>>      @Test
>>> +    public void testSetPasswordThenModCharArray() {
>>> +        char[] pwd = {'a' };
>>> +        pcds.setPassword(pwd);
>>> +        assertEquals("a", pcds.getPassword());
>>> +        pwd[0] = 'b';
>>> +        assertEquals("a", pcds.getPassword());
>>> +    }
>>> +
>>> +    @Test
>>>      public void testSetPasswordNullWithConnectionProperties() throws
>> Exception {
>>>          pcds.setConnectionProperties(new Properties());
>>>          pcds.setPassword("Secret");
>>>
>>>
>> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
>>> ----------------------------------------------------------------------
>>> diff --git
>> a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
>> b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
>>> index 3f9c157..86c0dfe 100644
>>> ---
>> a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
>>> +++
>> b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
>>> @@ -143,6 +143,16 @@ public class TestCPDSConnectionFactory {
>>>          assertEquals(0, pool.getNumIdle());
>>>      }
>>>
>>> +    @Test
>>> +    public void testSetPasswordThenModCharArray() {
>>> +        final CPDSConnectionFactory factory = new
>> CPDSConnectionFactory(cpds, null, -1, false, "userName", "password");
>>> +        char[] pwd = {'a' };
>>> +        factory.setPassword(pwd);
>>> +        assertEquals("a",
>> String.valueOf(factory.getPasswordCharArray()));
>>> +        pwd[0] = 'b';
>>> +        assertEquals("a",
>> String.valueOf(factory.getPasswordCharArray()));
>>> +    }
>>> +
>>>      /**
>>>       * JIRA: DBCP-442
>>>       */
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: commons-dbcp git commit: [DBCP-517] Make defensive copies of char[] passwords.

garydgregory
On Thu, Jul 26, 2018 at 1:31 PM Oliver Heger <[hidden email]>
wrote:

>
>
> Am 25.07.2018 um 23:36 schrieb Gary Gregory:
> > I'd we do not already have that, the name does not need "defensive".
> > Commons Collections would be where to put collections related code.
>
> I would rather think that this copy functionality is pretty basic, so I
> would put it in [lang]. I would not want to depend on [collections] just
> to use one or two copy methods. And the utility class could support
> non-collection types (mainly arrays) as well.
>

Hi Oliver,

For a long time we've had ArrayUtils in [lang] CollectionUtils in
[collections].

I do not think that it matters whether the code is "basic" or not; the
domain of these operations is for Collection instances and [lang] does not
concern itself with Collections, Commons Collections does.

It so happens that Java makes the distinction between primitive and Objects
which is why we have arrays like int[] and Collections like List<Integer>.

These new methods belong in these two classes or in new classes but in
their respective components, for example CopyArrays in [lang] and
CopyCollection in [collections].

The theme for both components is well established so I am -1 to put
Collection code in [lang].

Gary



>
> Oliver
>
> >
> > Gary
> >
> > On Wed, Jul 25, 2018, 14:00 Oliver Heger <[hidden email]>
> > wrote:
> >
> >> +1, Thank you.
> >>
> >> I had planned to create a patch for this issue, but did not find the
> >> time so far.
> >>
> >> BTW, I have quite often the need to create defensive copies of arrays or
> >> collections in some variants (e.g. null safe, with wrapping to an
> >> unmodifiable collection, etc.). Could this be an addition to [lang]? A
> >> new utility class like DefensiveCopyUtils?
> >>
> >> Oliver
> >>
> >> Am 25.07.2018 um 00:34 schrieb [hidden email]:
> >>> Repository: commons-dbcp
> >>> Updated Branches:
> >>>   refs/heads/master 70822f11d -> d7969ac93
> >>>
> >>>
> >>> [DBCP-517] Make defensive copies of char[] passwords.
> >>>
> >>> Project: http://git-wip-us.apache.org/repos/asf/commons-dbcp/repo
> >>> Commit:
> >> http://git-wip-us.apache.org/repos/asf/commons-dbcp/commit/d7969ac9
> >>> Tree:
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/tree/d7969ac9
> >>> Diff:
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/diff/d7969ac9
> >>>
> >>> Branch: refs/heads/master
> >>> Commit: d7969ac934e752e7a7b258fa5a5af9a563c40a13
> >>> Parents: 70822f1
> >>> Author: Gary Gregory <[hidden email]>
> >>> Authored: Tue Jul 24 16:34:43 2018 -0600
> >>> Committer: Gary Gregory <[hidden email]>
> >>> Committed: Tue Jul 24 16:34:43 2018 -0600
> >>>
> >>> ----------------------------------------------------------------------
> >>>  src/changes/changes.xml                                 |  5 ++++-
> >>>  src/main/java/org/apache/commons/dbcp2/Utils.java       | 12
> >> ++++++++++++
> >>>  .../commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java    |  4 ++--
> >>>  .../dbcp2/datasources/CPDSConnectionFactory.java        | 11
> ++++++++++-
> >>>  .../dbcp2/cpdsadapter/TestDriverAdapterCPDS.java        |  9 +++++++++
> >>>  .../dbcp2/datasources/TestCPDSConnectionFactory.java    | 10
> ++++++++++
> >>>  6 files changed, 47 insertions(+), 4 deletions(-)
> >>> ----------------------------------------------------------------------
> >>>
> >>>
> >>>
> >>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/changes/changes.xml
> >>> ----------------------------------------------------------------------
> >>> diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> >>> index c924411..8f1de55 100644
> >>> --- a/src/changes/changes.xml
> >>> +++ b/src/changes/changes.xml
> >>> @@ -61,9 +61,12 @@ The <action> type attribute can be
> >> add,update,fix,remove.
> >>>
> >>>    <body>
> >>>      <release version="2.6.0" date="2018-MM-DD" description="This is a
> >> minor release, including bug fixes and enhancements.">
> >>> -      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Gary
> >> Gregory">
> >>> +      <action dev="ggregory" type="add" issue="DBCP-514" due-to="Tom
> >> Jenkinson, Gary Gregory">
> >>>          Allow DBCP to register with a
> >> TransactionSynchronizationRegistry for XA cases.
> >>>        </action>
> >>> +      <action dev="ggregory" type="update" issue="DBCP-517"
> >> due-to="Gary Gregory">
> >>> +        Make defensive copies of char[] passwords.
> >>> +      </action>
> >>>      </release>
> >>>      <release version="2.5.0" date="2018-07-15" description="This is a
> >> minor release, including bug fixes and enhancements.">
> >>>        <action dev="ggregory" type="update" issue="DBCP-505"
> >> due-to="Gary Gregory">
> >>>
> >>>
> >>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/Utils.java
> >>> ----------------------------------------------------------------------
> >>> diff --git a/src/main/java/org/apache/commons/dbcp2/Utils.java
> >> b/src/main/java/org/apache/commons/dbcp2/Utils.java
> >>> index 8e798c4..244b51b 100644
> >>> --- a/src/main/java/org/apache/commons/dbcp2/Utils.java
> >>> +++ b/src/main/java/org/apache/commons/dbcp2/Utils.java
> >>> @@ -72,6 +72,17 @@ public final class Utils {
> >>>      }
> >>>
> >>>      /**
> >>> +     * Clones the given char[] if not null.
> >>> +     *
> >>> +     * @param value
> >>> +     *            may be null.
> >>> +     * @return a cloned char[] or null.
> >>> +     */
> >>> +    public static char[] clone(final char[] value) {
> >>> +        return value == null ? null : value.clone();
> >>> +    }
> >>> +
> >>> +    /**
> >>>       * Closes the ResultSet (which may be null).
> >>>       *
> >>>       * @param resultSet
> >>> @@ -169,4 +180,5 @@ public final class Utils {
> >>>      public static String toString(final char[] value) {
> >>>          return value == null ? null : String.valueOf(value);
> >>>      }
> >>> +
> >>>  }
> >>>
> >>>
> >>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> >>> ----------------------------------------------------------------------
> >>> diff --git
> >>
> a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> >>
> b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> >>> index bbc8831..0844c9b 100644
> >>> ---
> >>
> a/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> >>> +++
> >>
> b/src/main/java/org/apache/commons/dbcp2/cpdsadapter/DriverAdapterCPDS.java
> >>> @@ -423,8 +423,8 @@ public class DriverAdapterCPDS implements
> >> ConnectionPoolDataSource, Referenceabl
> >>>       */
> >>>      public void setPassword(final char[] userPassword) {
> >>>          assertInitializationAllowed();
> >>> -        this.userPassword = userPassword;
> >>> -        update(connectionProperties, KEY_PASSWORD,
> >> Utils.toString(userPassword));
> >>> +        this.userPassword = Utils.clone(userPassword);
> >>> +        update(connectionProperties, KEY_PASSWORD,
> >> Utils.toString(this.userPassword));
> >>>      }
> >>>
> >>>      /**
> >>>
> >>>
> >>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> >>> ----------------------------------------------------------------------
> >>> diff --git
> >>
> a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> >>
> b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> >>> index f0ae74d..c0ee90b 100644
> >>> ---
> >>
> a/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> >>> +++
> >>
> b/src/main/java/org/apache/commons/dbcp2/datasources/CPDSConnectionFactory.java
> >>> @@ -122,6 +122,15 @@ class CPDSConnectionFactory
> >>>      }
> >>>
> >>>      /**
> >>> +     * (Testing API) Gets the value of password for the default user.
> >>> +     *
> >>> +     * @return value of password.
> >>> +     */
> >>> +    char[] getPasswordCharArray() {
> >>> +        return userPassword;
> >>> +    }
> >>> +
> >>> +    /**
> >>>       * Returns the object pool used to pool connections created by
> this
> >> factory.
> >>>       *
> >>>       * @return ObjectPool managing pooled connections
> >>> @@ -335,7 +344,7 @@ class CPDSConnectionFactory
> >>>       *            new password
> >>>       */
> >>>      public synchronized void setPassword(final char[] userPassword) {
> >>> -        this.userPassword = userPassword;
> >>> +        this.userPassword =  Utils.clone(userPassword);
> >>>      }
> >>>
> >>>      /**
> >>>
> >>>
> >>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> >>> ----------------------------------------------------------------------
> >>> diff --git
> >>
> a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> >>
> b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> >>> index 0669b1f..7bae26e 100644
> >>> ---
> >>
> a/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> >>> +++
> >>
> b/src/test/java/org/apache/commons/dbcp2/cpdsadapter/TestDriverAdapterCPDS.java
> >>> @@ -208,6 +208,15 @@ public class TestDriverAdapterCPDS {
> >>>      }
> >>>
> >>>      @Test
> >>> +    public void testSetPasswordThenModCharArray() {
> >>> +        char[] pwd = {'a' };
> >>> +        pcds.setPassword(pwd);
> >>> +        assertEquals("a", pcds.getPassword());
> >>> +        pwd[0] = 'b';
> >>> +        assertEquals("a", pcds.getPassword());
> >>> +    }
> >>> +
> >>> +    @Test
> >>>      public void testSetPasswordNullWithConnectionProperties() throws
> >> Exception {
> >>>          pcds.setConnectionProperties(new Properties());
> >>>          pcds.setPassword("Secret");
> >>>
> >>>
> >>
> http://git-wip-us.apache.org/repos/asf/commons-dbcp/blob/d7969ac9/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> >>> ----------------------------------------------------------------------
> >>> diff --git
> >>
> a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> >>
> b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> >>> index 3f9c157..86c0dfe 100644
> >>> ---
> >>
> a/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> >>> +++
> >>
> b/src/test/java/org/apache/commons/dbcp2/datasources/TestCPDSConnectionFactory.java
> >>> @@ -143,6 +143,16 @@ public class TestCPDSConnectionFactory {
> >>>          assertEquals(0, pool.getNumIdle());
> >>>      }
> >>>
> >>> +    @Test
> >>> +    public void testSetPasswordThenModCharArray() {
> >>> +        final CPDSConnectionFactory factory = new
> >> CPDSConnectionFactory(cpds, null, -1, false, "userName", "password");
> >>> +        char[] pwd = {'a' };
> >>> +        factory.setPassword(pwd);
> >>> +        assertEquals("a",
> >> String.valueOf(factory.getPasswordCharArray()));
> >>> +        pwd[0] = 'b';
> >>> +        assertEquals("a",
> >> String.valueOf(factory.getPasswordCharArray()));
> >>> +    }
> >>> +
> >>>      /**
> >>>       * JIRA: DBCP-442
> >>>       */
> >>>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [hidden email]
> >> For additional commands, e-mail: [hidden email]
> >>
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>