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