diff --git a/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java b/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java index e918b685de75d..b9351b5bfad86 100644 --- a/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java +++ b/enterprise/query-logging/src/main/java/org/neo4j/kernel/impl/query/QueryLoggerKernelExtension.java @@ -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; @@ -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 @@ -219,8 +213,12 @@ private String logEntry( long time, ExecutingQuery query ) } } - @SuppressWarnings( "unchecked" ) private static String mapAsString( Map params ) + { + return mapAsString( params, "" ); + } + + private static String mapAsString( Map params, String obfuscate ) { if ( params == null ) { @@ -232,11 +230,18 @@ private static String mapAsString( Map params ) for ( Map.Entry 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( "}" ); diff --git a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java index 5edf594fa0c71..3387fef0343b5 100644 --- a/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java +++ b/enterprise/query-logging/src/test/java/org/neo4j/kernel/impl/query/QueryLoggerIT.java @@ -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; @@ -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 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 { @@ -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 ) ) { @@ -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 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 { @@ -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 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() );