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