You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kevin Duling <kd...@pivotal.io> on 2017/03/08 20:58:47 UTC
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
Re: Review Request 57431: GEODE-2633: When turning on fine logging,
GEODE logs the keystore password in clear text
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
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
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
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
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
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
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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
Posted by Jinmei Liao <ji...@pivotal.io>.
> 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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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
Posted by Kevin Duling <kd...@pivotal.io>.
> 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
Posted by Kirk Lund <ki...@gmail.com>.
> 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
Posted by Jinmei Liao <ji...@pivotal.io>.
> 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
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
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
>
>