From 2a1f833d2b412b5f2abab4316c424d5e09983343 Mon Sep 17 00:00:00 2001 From: fickludd Date: Tue, 20 Sep 2016 10:30:18 +0200 Subject: [PATCH] Fix: removed log stack trace on authentication failure Removed logging of auth events in bolt --- .../org/neo4j/bolt/BoltKernelExtension.java | 6 +- .../security/auth/BasicAuthentication.java | 8 +-- .../runtime/BoltConnectionAuthFatality.java | 31 +++++++++ .../runtime/BoltProtocolBreachFatality.java | 31 +++++++++ .../bolt/v1/runtime/BoltStateMachine.java | 22 +++--- .../concurrent/RunnableBoltWorker.java | 20 ++++-- .../auth/BasicAuthenticationTest.java | 29 +++----- .../concurrent/RunnableBoltWorkerTest.java | 68 ++++++++++++++++++- .../v1/runtime/integration/SessionRule.java | 23 +++---- 9 files changed, 179 insertions(+), 59 deletions(-) create mode 100644 community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltConnectionAuthFatality.java create mode 100644 community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltProtocolBreachFatality.java diff --git a/community/bolt/src/main/java/org/neo4j/bolt/BoltKernelExtension.java b/community/bolt/src/main/java/org/neo4j/bolt/BoltKernelExtension.java index 0b5085b6d83dd..6b9811a1f2113 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/BoltKernelExtension.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/BoltKernelExtension.java @@ -153,7 +153,7 @@ public Lifecycle newInstance( KernelContext context, Dependencies dependencies ) Netty4LogBridge.setLogProvider( logService.getInternalLogProvider() ); - Authentication authentication = authentication( dependencies.config(), dependencies.authManager(), logService ); + Authentication authentication = authentication( dependencies.authManager() ); BoltFactory boltConnectionManagerFactory = life.add( new LifecycleManagedBoltFactory( api, dependencies.usageData(), logService, dependencies.txBridge(), @@ -274,8 +274,8 @@ private KeyStoreInformation createKeyStore( Configuration config, Log log, Adver return new KeyStoreFactory().createKeyStore( privateKeyPath, certificatePath ); } - private Authentication authentication( Config config, AuthManager authManager, LogService logService ) + private Authentication authentication( AuthManager authManager ) { - return new BasicAuthentication( authManager, logService.getInternalLogProvider() ); + return new BasicAuthentication( authManager ); } } diff --git a/community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java b/community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java index 1209d2626356b..6b2576e5068dc 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java @@ -29,11 +29,8 @@ import org.neo4j.kernel.api.security.AuthToken; import org.neo4j.kernel.api.exceptions.InvalidArgumentsException; import org.neo4j.kernel.api.security.exception.InvalidAuthTokenException; -import org.neo4j.logging.Log; -import org.neo4j.logging.LogProvider; import static org.neo4j.kernel.api.security.AuthToken.NEW_CREDENTIALS; -import static org.neo4j.kernel.api.security.AuthToken.PRINCIPAL; /** * Performs basic authentication with user name and password. @@ -41,12 +38,10 @@ public class BasicAuthentication implements Authentication { private final AuthManager authManager; - private final Log log; - public BasicAuthentication( AuthManager authManager, LogProvider logProvider ) + public BasicAuthentication( AuthManager authManager ) { this.authManager = authManager; - this.log = logProvider.getLog( getClass() ); } @Override @@ -76,7 +71,6 @@ private AuthenticationResult doAuthenticate( Map authToken ) thro case TOO_MANY_ATTEMPTS: throw new AuthenticationException( Status.Security.AuthenticationRateLimit ); default: - log.warn( "Failed authentication attempt for '%s'", AuthToken.safeCast( PRINCIPAL, authToken ) ); throw new AuthenticationException( Status.Security.Unauthorized ); } diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltConnectionAuthFatality.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltConnectionAuthFatality.java new file mode 100644 index 0000000000000..d9d4d57bf70fe --- /dev/null +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltConnectionAuthFatality.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.bolt.v1.runtime; + +/** + * Indicates that bolt connection has been fatally misused and therefore the server should close the connection. + */ +public class BoltConnectionAuthFatality extends BoltConnectionFatality +{ + public BoltConnectionAuthFatality( String message ) + { + super( message ); + } +} diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltProtocolBreachFatality.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltProtocolBreachFatality.java new file mode 100644 index 0000000000000..fcb8107674460 --- /dev/null +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltProtocolBreachFatality.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.bolt.v1.runtime; + +/** + * Indicates that bolt connection has been fatally misused and therefore the server should close the connection. + */ +public class BoltProtocolBreachFatality extends BoltConnectionFatality +{ + public BoltProtocolBreachFatality( String message ) + { + super( message ); + } +} diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java index deb0e1ff60ebd..4a79ae470dfbb 100644 --- a/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java +++ b/community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java @@ -344,12 +344,12 @@ public State init( BoltStateMachine machine, String userAgent, catch ( AuthenticationException e ) { fail( machine, new Neo4jError( e.status(), e.getMessage() ) ); - throw new BoltConnectionFatality( e.getMessage() ); + throw new BoltConnectionAuthFatality( e.getMessage() ); } - catch ( Throwable e ) + catch ( Throwable t ) { - fail( machine, new Neo4jError( Status.General.UnknownError, e.getMessage() ) ); - throw new BoltConnectionFatality( e.getMessage() ); + fail( machine, new Neo4jError( Status.General.UnknownError, t.getMessage() ) ); + throw new BoltConnectionFatality( t.getMessage() ); } } }, @@ -551,14 +551,14 @@ public State init( BoltStateMachine machine, String userAgent, Map params ) throws @@ -582,21 +582,21 @@ public State run( BoltStateMachine machine, String statement, Map s.run( "Hello, world!", null, null ) ); worker.enqueue( RunnableBoltWorker.SHUTDOWN ); @@ -49,7 +77,6 @@ public void shouldExecuteWorkWhenRun() throws Throwable public void errorThrownDuringExecutionShouldCauseSessionClose() throws Throwable { // Given - BoltStateMachine machine = mock( BoltStateMachine.class ); RunnableBoltWorker worker = new RunnableBoltWorker( machine, NullLogService.getInstance() ); worker.enqueue( s -> { throw new RuntimeException( "It didn't work out." ); @@ -61,4 +88,41 @@ public void errorThrownDuringExecutionShouldCauseSessionClose() throws Throwable // Then verify( machine ).close(); } + + @Test + public void authExceptionShouldNotBeLoggedHere() throws Throwable + { + // Given + RunnableBoltWorker worker = new RunnableBoltWorker( machine, logService ); + worker.enqueue( s -> { + throw new BoltConnectionAuthFatality( "fatality" ); + } ); + + // When + worker.run(); + + // Then + verify( machine ).close(); + internalLog.assertNone( inLog( RunnableBoltWorker.class ).any() ); + userLog.assertNone( inLog( RunnableBoltWorker.class ).any() ); + } + + @Test + public void protocolBreachesShouldBeLoggedWithoutStackTraces() throws Throwable + { + // Given + RunnableBoltWorker worker = new RunnableBoltWorker( machine, logService ); + worker.enqueue( s -> { + throw new BoltProtocolBreachFatality( "protocol breach fatality" ); + } ); + + // When + worker.run(); + + // Then + verify( machine ).close(); + internalLog.assertExactly( inLog( RunnableBoltWorker.class ).error( "Bolt protocol breach in session " + + "'test-session'" ) ); + userLog.assertNone( inLog( RunnableBoltWorker.class ).any() ); + } } diff --git a/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/integration/SessionRule.java b/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/integration/SessionRule.java index 61cdb2752ec86..d377f461088f3 100644 --- a/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/integration/SessionRule.java +++ b/community/bolt/src/test/java/org/neo4j/bolt/v1/runtime/integration/SessionRule.java @@ -36,15 +36,12 @@ import org.neo4j.bolt.security.auth.BasicAuthentication; import org.neo4j.bolt.v1.runtime.BoltStateMachine; import org.neo4j.bolt.v1.runtime.LifecycleManagedBoltFactory; -import org.neo4j.bolt.v1.transport.integration.Neo4jWithSocket; import org.neo4j.graphdb.DependencyResolver; import org.neo4j.graphdb.config.Setting; import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.kernel.api.bolt.BoltConnectionTracker; import org.neo4j.kernel.api.security.AuthManager; -import org.neo4j.kernel.configuration.Config; import org.neo4j.kernel.impl.core.ThreadToStatementContextBridge; -import org.neo4j.kernel.impl.logging.LogService; import org.neo4j.kernel.impl.logging.NullLogService; import org.neo4j.kernel.impl.transaction.log.TransactionIdStore; import org.neo4j.kernel.internal.GraphDatabaseAPI; @@ -70,13 +67,15 @@ public void evaluate() throws Throwable config.put( GraphDatabaseSettings.auth_enabled, Boolean.toString( authEnabled ) ); gdb = (GraphDatabaseAPI) new TestGraphDatabaseFactory().newImpermanentDatabase( config ); DependencyResolver resolver = gdb.getDependencyResolver(); - LogService logService = NullLogService.getInstance(); - - Authentication authentication = authentication( resolver.resolveDependency( Config.class ), - resolver.resolveDependency( AuthManager.class ), logService ); - boltFactory = new LifecycleManagedBoltFactory( gdb, new UsageData( null ), logService, - resolver.resolveDependency( ThreadToStatementContextBridge.class ), - authentication, BoltConnectionTracker.NOOP ); + Authentication authentication = authentication( resolver.resolveDependency( AuthManager.class ) ); + boltFactory = new LifecycleManagedBoltFactory( + gdb, + new UsageData( null ), + NullLogService.getInstance(), + resolver.resolveDependency( ThreadToStatementContextBridge.class ), + authentication, + BoltConnectionTracker.NOOP + ); boltFactory.start(); try { @@ -99,9 +98,9 @@ public void evaluate() throws Throwable }; } - private Authentication authentication( Config config, AuthManager authManager, LogService logService ) + private Authentication authentication( AuthManager authManager ) { - return new BasicAuthentication( authManager, logService.getInternalLogProvider() ); + return new BasicAuthentication( authManager ); } BoltStateMachine newMachine( String connectionDescriptor )