Skip to content

Commit

Permalink
Added more tests and made regex clearer.
Browse files Browse the repository at this point in the history
  • Loading branch information
@eebus
eebus committed Mar 14, 2017
1 parent a48d939 commit 3c26fe6
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 11 deletions.
28 changes: 17 additions & 11 deletions ...e/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java
View file
Expand Up @@ -23,7 +23,11 @@
import java.io.File; import java.io.File;
import java.io.OutputStream; import java.io.OutputStream;
import java.time.Clock; import java.time.Clock;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map; import java.util.Map;
import java.util.Set;
import java.util.regex.Matcher; import java.util.regex.Matcher;
import java.util.regex.Pattern; import java.util.regex.Pattern;


Expand Down Expand Up @@ -140,9 +144,12 @@ static class QueryLogger implements QueryExecutionMonitor
private final boolean logQueryParameters; private final boolean logQueryParameters;


private static final Pattern PASSWORD_PATTERN = Pattern.compile( private static final Pattern PASSWORD_PATTERN = Pattern.compile(
// call signature
"(?:(?i)call)\\s+dbms(?:\\.security)?\\.change(?:User)?Password\\(" + "(?:(?i)call)\\s+dbms(?:\\.security)?\\.change(?:User)?Password\\(" +
"(?:\\s*(?:'(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*,)?" + // optional username parameter, in single, double quotes, or parametrized
"\\s*('(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*\\)" ); "(?:\\s*(?:'(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*,)?" +
// password parameter, in single, double quotes, or parametrized
"\\s*('(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*\\)" );


QueryLogger( Clock clock, Log log, long thresholdMillis, boolean logQueryParameters ) QueryLogger( Clock clock, Log log, long thresholdMillis, boolean logQueryParameters )
{ {
Expand Down Expand Up @@ -180,20 +187,19 @@ private String logEntry( long time, ExecutingQuery query )
String queryText = query.queryText(); String queryText = query.queryText();
String metaData = mapAsString( query.metaData() ); String metaData = mapAsString( query.metaData() );


String password = ""; Set<String> passwordParams = new HashSet<>();

Matcher matcher = PASSWORD_PATTERN.matcher( queryText ); Matcher matcher = PASSWORD_PATTERN.matcher( queryText );


while ( matcher.find() ) while ( matcher.find() )
{ {
password = matcher.group( 1 ).trim(); String password = matcher.group( 1 ).trim();
if ( password.charAt( 0 ) == '$' ) if ( password.charAt( 0 ) == '$' )
{ {
password = password.substring( 1 ); passwordParams.add( password.substring( 1 ) );
} }
else if ( password.charAt( 0 ) == '{' ) else if ( password.charAt( 0 ) == '{' )
{ {
password = password.substring( 1, password.length() - 1 ); passwordParams.add( password.substring( 1, password.length() - 1 ) );
} }
else else
{ {
Expand All @@ -204,7 +210,7 @@ else if ( password.charAt( 0 ) == '{' )


if ( logQueryParameters ) if ( logQueryParameters )
{ {
String params = mapAsString( query.queryParameters(), password ); String params = mapAsString( query.queryParameters(), passwordParams );
return format( "%d ms: %s - %s - %s - %s", time, sourceString, queryText, params, metaData ); return format( "%d ms: %s - %s - %s - %s", time, sourceString, queryText, params, metaData );
} }
else else
Expand All @@ -215,10 +221,10 @@ else if ( password.charAt( 0 ) == '{' )


private static String mapAsString( Map<String,Object> params ) private static String mapAsString( Map<String,Object> params )
{ {
return mapAsString( params, "" ); return mapAsString( params, Collections.emptySet() );
} }


private static String mapAsString( Map<String, Object> params, String obfuscate ) private static String mapAsString( Map<String, Object> params, Collection<String> obfuscate )
{ {
if ( params == null ) if ( params == null )
{ {
Expand All @@ -234,7 +240,7 @@ private static String mapAsString( Map<String, Object> params, String obfuscate
.append( entry.getKey() ) .append( entry.getKey() )
.append( ": " ); .append( ": " );


if ( entry.getKey().equals( obfuscate ) ) if ( obfuscate.contains( entry.getKey() ) )
{ {
builder.append( "******" ); builder.append( "******" );
} }
Expand Down
48 changes: 48 additions & 0 deletions enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java
View file
Expand Up @@ -357,6 +357,7 @@ public void shouldNotLogPassword() throws Exception
assertEquals( 1, logLines.size() ); assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ), assertThat( logLines.get( 0 ),
containsString( "CALL dbms.security.changePassword(******)") ) ; containsString( "CALL dbms.security.changePassword(******)") ) ;
assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
} }


Expand Down Expand Up @@ -389,6 +390,7 @@ public void shouldNotLogChangeUserPassword() throws Exception
assertEquals( 1, logLines.size() ); assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ), assertThat( logLines.get( 0 ),
containsString( "CALL dbms.security.changeUserPassword('neo4j', ******)") ) ; containsString( "CALL dbms.security.changeUserPassword('neo4j', ******)") ) ;
assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
} }


Expand Down Expand Up @@ -421,6 +423,7 @@ public void shouldNotLogPasswordEvenIfPasswordIsSilly() throws Exception
assertEquals( 1, logLines.size() ); assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ), assertThat( logLines.get( 0 ),
containsString( "CALL dbms.security.changePassword(******)") ) ; containsString( "CALL dbms.security.changePassword(******)") ) ;
assertThat( logLines.get( 0 ),not( containsString( ".changePassword(\\'si\"lly\\" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
} }


Expand Down Expand Up @@ -457,6 +460,49 @@ public void shouldNotLogPasswordEvenIfYouDoTwoThingsAtTheSameTime() throws Excep
assertEquals( 1, logLines.size() ); assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ), assertThat( logLines.get( 0 ),
containsString( "CALL dbms.security.changeUserPassword('neo4j',******) CALL dbms.security.changeUserPassword('smith',******)") ) ; containsString( "CALL dbms.security.changeUserPassword('neo4j',******) CALL dbms.security.changeUserPassword('smith',******)") ) ;
assertThat( logLines.get( 0 ),not( containsString( "other$silly" ) ) );
assertThat( logLines.get( 0 ),not( containsString( ".changePassword(silly)" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

@Test
public void shouldNotLogPasswordEvenIfYouDoTwoThingsAtTheSameTimeWithSeveralParms() throws Exception
{
GraphDatabaseFacade database = (GraphDatabaseFacade) databaseBuilder
.setConfig( GraphDatabaseSettings.log_queries, Settings.TRUE )
.setConfig( GraphDatabaseSettings.logs_directory, logsDirectory.getPath() )
.setConfig( GraphDatabaseSettings.auth_enabled, Settings.TRUE )
.newGraphDatabase();

EnterpriseAuthManager authManager = database.getDependencyResolver().resolveDependency( EnterpriseAuthManager.class );
EnterpriseAuthAndUserManager userManager= database.getDependencyResolver().resolveDependency(
EnterpriseAuthAndUserManager.class );
userManager.getUserManager().newUser( "smith", "himitsu", false );
EnterpriseSecurityContext neo = authManager.login( AuthToken.newBasicAuthToken( "neo4j", "neo4j" ) );

String query = "CALL dbms.security.changeUserPassword('neo4j',$first) "+
"CALL dbms.security.changeUserPassword('smith',$second) RETURN 1";
try ( InternalTransaction tx = database
.beginTransaction( KernelTransaction.Type.explicit, neo ) )
{
Map<String,Object> params = new HashMap( );
params.put("first",".changePassword(silly)");
params.put("second",".other$silly");
Result res = database.execute( tx, query, params );
res.close();
tx.success();
}
finally
{
database.shutdown();
}

List<String> logLines = readAllLines( logFilename );
assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ),
containsString( "{first: ******, second: ******}"));
assertThat( logLines.get( 0 ),not( containsString( "other$silly" ) ) );
assertThat( logLines.get( 0 ),not( containsString( ".changePassword(silly)" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
} }


Expand Down Expand Up @@ -489,6 +535,7 @@ public void shouldNotLogPasswordInParams() throws Exception
assertEquals( 1, logLines.size() ); assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ), assertThat( logLines.get( 0 ),
containsString( "{password: ******}")); containsString( "{password: ******}"));
assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
} }


Expand Down Expand Up @@ -521,6 +568,7 @@ public void shouldNotLogPasswordInDeprecatedParams() throws Exception
assertEquals( 1, logLines.size() ); assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ), assertThat( logLines.get( 0 ),
containsString( "{password: ******}")); containsString( "{password: ******}"));
assertThat( logLines.get( 0 ),not( containsString( "abc123" ) ) );
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) ); assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
} }


Expand Down

0 comments on commit 3c26fe6

Please sign in to comment.