Skip to content

Commit

Permalink
Improved way of removing password from query log.
Browse files Browse the repository at this point in the history
  • Loading branch information
@eebus @OliviaYtterbrink
eebus authored and OliviaYtterbrink committed Mar 13, 2017
1 parent b841821 commit a48d939
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 34 deletions.
71 changes: 38 additions & 33 deletions ...e/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java
View file
Expand Up @@ -139,6 +139,11 @@ static class QueryLogger implements QueryExecutionMonitor
private final long thresholdMillis;
private final boolean logQueryParameters;

private static final Pattern PASSWORD_PATTERN = Pattern.compile(
"(?:(?i)call)\\s+dbms(?:\\.security)?\\.change(?:User)?Password\\(" +
"(?:\\s*(?:'(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*,)?" +
"\\s*('(?:(?<=\\\\)'|[^'])*'|\"(?:(?<=\\\\)\"|[^\"])*\"|\\$\\w*|\\{\\w*\\})\\s*\\)" );

QueryLogger( Clock clock, Log log, long thresholdMillis, boolean logQueryParameters )
{
this.clock = clock;
Expand Down Expand Up @@ -176,41 +181,30 @@ private String logEntry( long time, ExecutingQuery query )
String metaData = mapAsString( query.metaData() );

String password = "";
boolean haveParams = false;
if ( queryText.contains( ".changePassword(" ) )

Matcher matcher = PASSWORD_PATTERN.matcher( queryText );

while ( matcher.find() )
{
Pattern pattern = Pattern.compile( "\\.changePassword\\((.*)\\)" );
Matcher matcher = pattern.matcher( queryText );
if ( matcher.find() )
password = matcher.group( 1 ).trim();
if ( password.charAt( 0 ) == '$' )
{
password = matcher.group( 1 );
if ( password.charAt( 0 ) != '$' )
{
queryText = queryText.replace( password, "******" );
}
else
{
haveParams = true;
}

password = password.substring( 1 );
}
else if ( password.charAt( 0 ) == '{' )
{
password = password.substring( 1, password.length() - 1 );
}
else
{
queryText = queryText.replace( password, "******" );
password = "";
}
}

if ( logQueryParameters )
{
String params = mapAsString( query.queryParameters() );
if ( haveParams )
{
password = password.replace( "$", "" );

Pattern pattern = Pattern.compile( password + ".*('.*')" );
Matcher matcher = pattern.matcher( params );
if ( matcher.find() )
{
password = matcher.group( 1 );
params = params.replace( password, "******" );
}
}
String params = mapAsString( query.queryParameters(), password );
return format( "%d ms: %s - %s - %s - %s", time, sourceString, queryText, params, metaData );
}
else
Expand All @@ -219,8 +213,12 @@ private String logEntry( long time, ExecutingQuery query )
}
}

@SuppressWarnings( "unchecked" )
private static String mapAsString( Map<String,Object> params )
{
return mapAsString( params, "" );
}

private static String mapAsString( Map<String, Object> params, String obfuscate )
{
if ( params == null )
{
Expand All @@ -232,11 +230,18 @@ private static String mapAsString( Map<String,Object> params )
for ( Map.Entry<String,Object> entry : params.entrySet() )
{
builder
.append( sep )
.append( entry.getKey() )
.append( ": " )
.append( valueToString( entry.getValue() ) );
.append( sep )
.append( entry.getKey() )
.append( ": " );

if ( entry.getKey().equals( obfuscate ) )
{
builder.append( "******" );
}
else
{
builder.append( valueToString( entry.getValue() ) );
}
sep = ", ";
}
builder.append( "}" );
Expand Down
103 changes: 102 additions & 1 deletion enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java
View file
Expand Up @@ -55,6 +55,7 @@
import org.neo4j.kernel.impl.factory.GraphDatabaseFacade;
import org.neo4j.logging.AssertableLogProvider;
import org.neo4j.server.security.enterprise.auth.EmbeddedInteraction;
import org.neo4j.server.security.enterprise.auth.EnterpriseAuthAndUserManager;
import org.neo4j.test.TestEnterpriseGraphDatabaseFactory;
import org.neo4j.test.rule.TestDirectory;
import org.neo4j.test.rule.fs.DefaultFileSystemRule;
Expand Down Expand Up @@ -359,6 +360,38 @@ public void shouldNotLogPassword() throws Exception
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

@Test
public void shouldNotLogChangeUserPassword() 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 );
EnterpriseSecurityContext neo = authManager.login( AuthToken.newBasicAuthToken( "neo4j", "neo4j" ) );

String query = "CALL dbms.security.changeUserPassword('neo4j', 'abc123')";
try ( InternalTransaction tx = database
.beginTransaction( KernelTransaction.Type.explicit, neo ) )
{
Result res = database.execute( tx, query, Collections.emptyMap() );
res.close();
tx.success();
}
finally
{
database.shutdown();
}

List<String> logLines = readAllLines( logFilename );
assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ),
containsString( "CALL dbms.security.changeUserPassword('neo4j', ******)") ) ;
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

@Test
public void shouldNotLogPasswordEvenIfPasswordIsSilly() throws Exception
{
Expand All @@ -371,7 +404,7 @@ public void shouldNotLogPasswordEvenIfPasswordIsSilly() throws Exception
EnterpriseAuthManager authManager = database.getDependencyResolver().resolveDependency( EnterpriseAuthManager.class );
EnterpriseSecurityContext neo = authManager.login( AuthToken.newBasicAuthToken( "neo4j", "neo4j" ) );

String query = "CALL dbms.security.changePassword('.changePassword(silly)')";
String query = "CALL dbms.security.changePassword('.changePassword(\\'si\"lly\\')')";
try ( InternalTransaction tx = database
.beginTransaction( KernelTransaction.Type.explicit, neo ) )
{
Expand All @@ -391,6 +424,42 @@ public void shouldNotLogPasswordEvenIfPasswordIsSilly() throws Exception
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

@Test
public void shouldNotLogPasswordEvenIfYouDoTwoThingsAtTheSameTime() 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','.changePassword(silly)') "+
"CALL dbms.security.changeUserPassword('smith','other$silly') RETURN 1";
try ( InternalTransaction tx = database
.beginTransaction( KernelTransaction.Type.explicit, neo ) )
{
Result res = database.execute( tx, query, Collections.emptyMap() );
res.close();
tx.success();
}
finally
{
database.shutdown();
}

List<String> logLines = readAllLines( logFilename );
assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ),
containsString( "CALL dbms.security.changeUserPassword('neo4j',******) CALL dbms.security.changeUserPassword('smith',******)") ) ;
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

@Test
public void shouldNotLogPasswordInParams() throws Exception
{
Expand Down Expand Up @@ -423,6 +492,38 @@ public void shouldNotLogPasswordInParams() throws Exception
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

@Test
public void shouldNotLogPasswordInDeprecatedParams() 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 );
EnterpriseSecurityContext neo = authManager.login( AuthToken.newBasicAuthToken( "neo4j", "neo4j" ) );

String query = "CALL dbms.changePassword({password})";
try ( InternalTransaction tx = database
.beginTransaction( KernelTransaction.Type.explicit, neo ) )
{
Result res = database.execute( tx, query, Collections.singletonMap( "password", "abc123" ) );
res.close();
tx.success();
}
finally
{
database.shutdown();
}

List<String> logLines = readAllLines( logFilename );
assertEquals( 1, logLines.size() );
assertThat( logLines.get( 0 ),
containsString( "{password: ******}"));
assertThat( logLines.get( 0 ), containsString( neo.subject().username() ) );
}

private void executeQueryAndShutdown( GraphDatabaseService database )
{
executeQueryAndShutdown( database, QUERY, Collections.emptyMap() );
Expand Down

0 comments on commit a48d939

Please sign in to comment.