Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57431/#review168833 --- Ship it! Ship It! - Ken Howe On March 13, 2017, 6:05 p.m., Kevin Duling wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57431/ > --- > > (Updated March 13, 2017, 6:05 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund. > > > Repository: geode > > > Description > --- > > GEODE-2633: When turning on fine logging, GEODE logs the keystore password in > clear text > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java > 742e7f3e93e595844675d0789b995e4ceb4431ac > > geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java > 419f3f976159e601c95a2042bafd96cc9fe9465f > > > Diff: https://reviews.apache.org/r/57431/diff/1/ > > > Testing > --- > > precheckin completed > > > Thanks, > > Kevin Duling > >
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57431/#review168808 --- Ship it! Ship It! - Jinmei Liao On March 13, 2017, 6:05 p.m., Kevin Duling wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57431/ > --- > > (Updated March 13, 2017, 6:05 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund. > > > Repository: geode > > > Description > --- > > GEODE-2633: When turning on fine logging, GEODE logs the keystore password in > clear text > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java > 742e7f3e93e595844675d0789b995e4ceb4431ac > > geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java > 419f3f976159e601c95a2042bafd96cc9fe9465f > > > Diff: https://reviews.apache.org/r/57431/diff/1/ > > > Testing > --- > > precheckin completed > > > Thanks, > > Kevin Duling > >
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
> On March 8, 2017, 1:16 p.m., Jinmei Liao wrote:
> > Any test changes? We probably can create a integrated/dunit test that would
> > start a server with those ssl properties (including passwords) turned on,
> > and have debug level truned on, and security truned on as well, and have
> > gfsh connect to it using username and password, and see if any of the
> > password show up in the logs.
>
> Kevin Duling wrote:
> `ArgumentRedactorJUnitTest` already has 85% method coverage and 95% line
> coverage of `ArgumentRedactor`. `SocketCreator.printConfig()` is a void
> method which only writes out to the log file. I could add a specific test
> for the expected ssl property, but it's already covered by the comparison
> `compareKey.toLowerCase().contains("password");`
>
> Jinmei Liao wrote:
> ArugmentRedactor is defintely doing the right thing since we have test
> coverage on that. It's the SocketCreator that is the problem here.
>
> Kevin Duling wrote:
> I disagree. ArgumentRedactor just has to populate the StringBuilder with
> the correct string. The only additional piece is calling the logger to write
> the file, which is Log4J, not SocketCreator. I'm not clear on what
> additional testing would prove.
>
> `RestSecurityWithSSLTest` is probably a good test to work from as it is
> an entry point for this code.
>
> Jared Stewart wrote:
> I think the goal of a test for SocketCreator would be that if someone
> reverts the changes to SocketCreator made by this commit, there should be a
> test that fails.
>
> Jinmei Liao wrote:
> My point as well. We should have a test that would fail without your
> changes in SocketCreator, and then passes when you put in the fix.
>
> Kirk Lund wrote:
> We really need to move away from writing lots of additional DUnit tests.
> Instead, write a true unit test if you're going to write further tests. I'm
> available to pair on this to help come up with a meaningful unit test that is
> NOT a DUnit test.
I've opened https://issues.apache.org/jira/browse/GEODE-2650 for a more generic
test to scan for clear-text passwords.
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57431/#review168333
---
On March 8, 2017, 12:58 p.m., Kevin Duling wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57431/
> ---
>
> (Updated March 8, 2017, 12:58 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> ---
>
> GEODE-2633: When turning on fine logging, GEODE logs the keystore password in
> clear text
>
>
> Diffs
> -
>
> geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> 742e7f3e93e595844675d0789b995e4ceb4431ac
>
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
> 419f3f976159e601c95a2042bafd96cc9fe9465f
>
>
> Diff: https://reviews.apache.org/r/57431/diff/1/
>
>
> Testing
> ---
>
> precheckin running
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57431/ --- (Updated March 13, 2017, 11:05 a.m.) Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund. Repository: geode Description --- GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text Diffs - geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java 742e7f3e93e595844675d0789b995e4ceb4431ac geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java 419f3f976159e601c95a2042bafd96cc9fe9465f Diff: https://reviews.apache.org/r/57431/diff/1/ Testing (updated) --- precheckin completed Thanks, Kevin Duling
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
> On March 8, 2017, 9:16 p.m., Jinmei Liao wrote:
> > Any test changes? We probably can create a integrated/dunit test that would
> > start a server with those ssl properties (including passwords) turned on,
> > and have debug level truned on, and security truned on as well, and have
> > gfsh connect to it using username and password, and see if any of the
> > password show up in the logs.
>
> Kevin Duling wrote:
> `ArgumentRedactorJUnitTest` already has 85% method coverage and 95% line
> coverage of `ArgumentRedactor`. `SocketCreator.printConfig()` is a void
> method which only writes out to the log file. I could add a specific test
> for the expected ssl property, but it's already covered by the comparison
> `compareKey.toLowerCase().contains("password");`
>
> Jinmei Liao wrote:
> ArugmentRedactor is defintely doing the right thing since we have test
> coverage on that. It's the SocketCreator that is the problem here.
>
> Kevin Duling wrote:
> I disagree. ArgumentRedactor just has to populate the StringBuilder with
> the correct string. The only additional piece is calling the logger to write
> the file, which is Log4J, not SocketCreator. I'm not clear on what
> additional testing would prove.
>
> `RestSecurityWithSSLTest` is probably a good test to work from as it is
> an entry point for this code.
>
> Jared Stewart wrote:
> I think the goal of a test for SocketCreator would be that if someone
> reverts the changes to SocketCreator made by this commit, there should be a
> test that fails.
>
> Jinmei Liao wrote:
> My point as well. We should have a test that would fail without your
> changes in SocketCreator, and then passes when you put in the fix.
We really need to move away from writing lots of additional DUnit tests.
Instead, write a true unit test if you're going to write further tests. I'm
available to pair on this to help come up with a meaningful unit test that is
NOT a DUnit test.
- Kirk
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57431/#review168333
---
On March 8, 2017, 8:58 p.m., Kevin Duling wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57431/
> ---
>
> (Updated March 8, 2017, 8:58 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> ---
>
> GEODE-2633: When turning on fine logging, GEODE logs the keystore password in
> clear text
>
>
> Diffs
> -
>
> geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> 742e7f3e93e595844675d0789b995e4ceb4431ac
>
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
> 419f3f976159e601c95a2042bafd96cc9fe9465f
>
>
> Diff: https://reviews.apache.org/r/57431/diff/1/
>
>
> Testing
> ---
>
> precheckin running
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57431/#review168481 --- Ship it! Ship It! - Kirk Lund On March 8, 2017, 8:58 p.m., Kevin Duling wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57431/ > --- > > (Updated March 8, 2017, 8:58 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund. > > > Repository: geode > > > Description > --- > > GEODE-2633: When turning on fine logging, GEODE logs the keystore password in > clear text > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java > 742e7f3e93e595844675d0789b995e4ceb4431ac > > geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java > 419f3f976159e601c95a2042bafd96cc9fe9465f > > > Diff: https://reviews.apache.org/r/57431/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Kevin Duling > >
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
> On March 8, 2017, 9:16 p.m., Jinmei Liao wrote:
> > Any test changes? We probably can create a integrated/dunit test that would
> > start a server with those ssl properties (including passwords) turned on,
> > and have debug level truned on, and security truned on as well, and have
> > gfsh connect to it using username and password, and see if any of the
> > password show up in the logs.
>
> Kevin Duling wrote:
> `ArgumentRedactorJUnitTest` already has 85% method coverage and 95% line
> coverage of `ArgumentRedactor`. `SocketCreator.printConfig()` is a void
> method which only writes out to the log file. I could add a specific test
> for the expected ssl property, but it's already covered by the comparison
> `compareKey.toLowerCase().contains("password");`
>
> Jinmei Liao wrote:
> ArugmentRedactor is defintely doing the right thing since we have test
> coverage on that. It's the SocketCreator that is the problem here.
>
> Kevin Duling wrote:
> I disagree. ArgumentRedactor just has to populate the StringBuilder with
> the correct string. The only additional piece is calling the logger to write
> the file, which is Log4J, not SocketCreator. I'm not clear on what
> additional testing would prove.
>
> `RestSecurityWithSSLTest` is probably a good test to work from as it is
> an entry point for this code.
>
> Jared Stewart wrote:
> I think the goal of a test for SocketCreator would be that if someone
> reverts the changes to SocketCreator made by this commit, there should be a
> test that fails.
My point as well. We should have a test that would fail without your changes in
SocketCreator, and then passes when you put in the fix.
- Jinmei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57431/#review168333
---
On March 8, 2017, 8:58 p.m., Kevin Duling wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57431/
> ---
>
> (Updated March 8, 2017, 8:58 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> ---
>
> GEODE-2633: When turning on fine logging, GEODE logs the keystore password in
> clear text
>
>
> Diffs
> -
>
> geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> 742e7f3e93e595844675d0789b995e4ceb4431ac
>
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
> 419f3f976159e601c95a2042bafd96cc9fe9465f
>
>
> Diff: https://reviews.apache.org/r/57431/diff/1/
>
>
> Testing
> ---
>
> precheckin running
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
> On March 8, 2017, 1:16 p.m., Jinmei Liao wrote:
> > Any test changes? We probably can create a integrated/dunit test that would
> > start a server with those ssl properties (including passwords) turned on,
> > and have debug level truned on, and security truned on as well, and have
> > gfsh connect to it using username and password, and see if any of the
> > password show up in the logs.
>
> Kevin Duling wrote:
> `ArgumentRedactorJUnitTest` already has 85% method coverage and 95% line
> coverage of `ArgumentRedactor`. `SocketCreator.printConfig()` is a void
> method which only writes out to the log file. I could add a specific test
> for the expected ssl property, but it's already covered by the comparison
> `compareKey.toLowerCase().contains("password");`
>
> Jinmei Liao wrote:
> ArugmentRedactor is defintely doing the right thing since we have test
> coverage on that. It's the SocketCreator that is the problem here.
I disagree. ArgumentRedactor just has to populate the StringBuilder with the
correct string. The only additional piece is calling the logger to write the
file, which is Log4J, not SocketCreator. I'm not clear on what additional
testing would prove.
`RestSecurityWithSSLTest` is probably a good test to work from as it is an
entry point for this code.
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57431/#review168333
---
On March 8, 2017, 12:58 p.m., Kevin Duling wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57431/
> ---
>
> (Updated March 8, 2017, 12:58 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> ---
>
> GEODE-2633: When turning on fine logging, GEODE logs the keystore password in
> clear text
>
>
> Diffs
> -
>
> geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> 742e7f3e93e595844675d0789b995e4ceb4431ac
>
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
> 419f3f976159e601c95a2042bafd96cc9fe9465f
>
>
> Diff: https://reviews.apache.org/r/57431/diff/1/
>
>
> Testing
> ---
>
> precheckin running
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
> On March 8, 2017, 9:16 p.m., Jinmei Liao wrote:
> > Any test changes? We probably can create a integrated/dunit test that would
> > start a server with those ssl properties (including passwords) turned on,
> > and have debug level truned on, and security truned on as well, and have
> > gfsh connect to it using username and password, and see if any of the
> > password show up in the logs.
>
> Kevin Duling wrote:
> `ArgumentRedactorJUnitTest` already has 85% method coverage and 95% line
> coverage of `ArgumentRedactor`. `SocketCreator.printConfig()` is a void
> method which only writes out to the log file. I could add a specific test
> for the expected ssl property, but it's already covered by the comparison
> `compareKey.toLowerCase().contains("password");`
ArugmentRedactor is defintely doing the right thing since we have test coverage
on that. It's the SocketCreator that is the problem here.
- Jinmei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57431/#review168333
---
On March 8, 2017, 8:58 p.m., Kevin Duling wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57431/
> ---
>
> (Updated March 8, 2017, 8:58 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> ---
>
> GEODE-2633: When turning on fine logging, GEODE logs the keystore password in
> clear text
>
>
> Diffs
> -
>
> geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> 742e7f3e93e595844675d0789b995e4ceb4431ac
>
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
> 419f3f976159e601c95a2042bafd96cc9fe9465f
>
>
> Diff: https://reviews.apache.org/r/57431/diff/1/
>
>
> Testing
> ---
>
> precheckin running
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
> On March 8, 2017, 1:16 p.m., Jinmei Liao wrote:
> > Any test changes? We probably can create a integrated/dunit test that would
> > start a server with those ssl properties (including passwords) turned on,
> > and have debug level truned on, and security truned on as well, and have
> > gfsh connect to it using username and password, and see if any of the
> > password show up in the logs.
`ArgumentRedactorJUnitTest` already has 85% method coverage and 95% line
coverage of `ArgumentRedactor`. `SocketCreator.printConfig()` is a void method
which only writes out to the log file. I could add a specific test for the
expected ssl property, but it's already covered by the comparison
`compareKey.toLowerCase().contains("password");`
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57431/#review168333
---
On March 8, 2017, 12:58 p.m., Kevin Duling wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57431/
> ---
>
> (Updated March 8, 2017, 12:58 p.m.)
>
>
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> ---
>
> GEODE-2633: When turning on fine logging, GEODE logs the keystore password in
> clear text
>
>
> Diffs
> -
>
> geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
> 742e7f3e93e595844675d0789b995e4ceb4431ac
>
> geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
> 419f3f976159e601c95a2042bafd96cc9fe9465f
>
>
> Diff: https://reviews.apache.org/r/57431/diff/1/
>
>
> Testing
> ---
>
> precheckin running
>
>
> Thanks,
>
> Kevin Duling
>
>
Re: Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57431/#review168333 --- Any test changes? We probably can create a integrated/dunit test that would start a server with those ssl properties (including passwords) turned on, and have debug level truned on, and security truned on as well, and have gfsh connect to it using username and password, and see if any of the password show up in the logs. - Jinmei Liao On March 8, 2017, 8:58 p.m., Kevin Duling wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57431/ > --- > > (Updated March 8, 2017, 8:58 p.m.) > > > Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund. > > > Repository: geode > > > Description > --- > > GEODE-2633: When turning on fine logging, GEODE logs the keystore password in > clear text > > > Diffs > - > > geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java > 742e7f3e93e595844675d0789b995e4ceb4431ac > > geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java > 419f3f976159e601c95a2042bafd96cc9fe9465f > > > Diff: https://reviews.apache.org/r/57431/diff/1/ > > > Testing > --- > > precheckin running > > > Thanks, > > Kevin Duling > >
Review Request 57431: GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57431/ --- Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Kirk Lund. Repository: geode Description --- GEODE-2633: When turning on fine logging, GEODE logs the keystore password in clear text Diffs - geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java 742e7f3e93e595844675d0789b995e4ceb4431ac geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java 419f3f976159e601c95a2042bafd96cc9fe9465f Diff: https://reviews.apache.org/r/57431/diff/1/ Testing --- precheckin running Thanks, Kevin Duling
