Skip to content

Commit

Permalink
Fix: removed log stack trace on authentication failure
Browse files Browse the repository at this point in the history 
Removed logging of auth events in bolt
  • Loading branch information
@fickludd
fickludd committed Sep 20, 2016
1 parent c287d3f commit 2a1f833
Show file tree
Hide file tree
Showing 9 changed files with 179 additions and 59 deletions.
6 changes: 3 additions & 3 deletions community/bolt/src/main/java/org/neo4j/bolt/BoltKernelExtension.java
View file
Expand Up @@ -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(),
Expand Down Expand Up @@ -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 );
}
}
8 changes: 1 addition & 7 deletions community/bolt/src/main/java/org/neo4j/bolt/security/auth/BasicAuthentication.java
View file
Expand Up @@ -29,24 +29,19 @@
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.
*/
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
Expand Down Expand Up @@ -76,7 +71,6 @@ private AuthenticationResult doAuthenticate( Map<String,Object> 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 );
}

Expand Down
31 changes: 31 additions & 0 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltConnectionAuthFatality.java
View file
@@ -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 <http://www.gnu.org/licenses/>.
*/
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 );
}
}
31 changes: 31 additions & 0 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltProtocolBreachFatality.java
View file
@@ -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 <http://www.gnu.org/licenses/>.
*/
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 );
}
}
22 changes: 11 additions & 11 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/BoltStateMachine.java
View file
Expand Up @@ -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() );
}
}
},
Expand Down Expand Up @@ -551,14 +551,14 @@ public State init( BoltStateMachine machine, String userAgent, Map<String, Objec
{
String msg = "INIT cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

public State ackFailure( BoltStateMachine machine ) throws BoltConnectionFatality
{
String msg = "ACK_FAILURE cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

public State interrupt( BoltStateMachine machine ) throws BoltConnectionFatality
Expand All @@ -567,36 +567,36 @@ public State interrupt( BoltStateMachine machine ) throws BoltConnectionFatality
// a RESET message.
String msg = "RESET cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

public State reset( BoltStateMachine machine ) throws BoltConnectionFatality
{
String msg = "RESET cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

public State run( BoltStateMachine machine, String statement, Map<String, Object> params ) throws
BoltConnectionFatality
{
String msg = "RUN cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

public State discardAll( BoltStateMachine machine ) throws BoltConnectionFatality
{
String msg = "DISCARD_ALL cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

public State pullAll( BoltStateMachine machine ) throws BoltConnectionFatality
{
String msg = "PULL_ALL cannot be handled by a session in the " + name() + " state.";
fail( machine, new Neo4jError( Status.Request.Invalid, msg ) );
throw new BoltConnectionFatality( msg );
throw new BoltProtocolBreachFatality( msg );
}

State resetMachine( BoltStateMachine machine ) throws BoltConnectionFatality
Expand Down
20 changes: 15 additions & 5 deletions community/bolt/src/main/java/org/neo4j/bolt/v1/runtime/concurrent/RunnableBoltWorker.java
View file
Expand Up @@ -19,7 +19,9 @@
*/
package org.neo4j.bolt.v1.runtime.concurrent;

import org.neo4j.bolt.v1.runtime.BoltConnectionAuthFatality;
import org.neo4j.bolt.v1.runtime.BoltConnectionFatality;
import org.neo4j.bolt.v1.runtime.BoltProtocolBreachFatality;
import org.neo4j.bolt.v1.runtime.BoltStateMachine;
import org.neo4j.bolt.v1.runtime.BoltWorker;
import org.neo4j.bolt.v1.runtime.Job;
Expand Down Expand Up @@ -94,12 +96,21 @@ public void run()
}
}
}
catch ( Throwable e )
catch ( BoltConnectionAuthFatality e )
{
// this is logged in the SecurityLog
}
catch ( BoltProtocolBreachFatality e )
{
log.error( "Bolt protocol breach in session '" + machine.key() + "'" );
}
catch ( Throwable t )
{
log.error( "Worker for session '" + machine.key() + "' crashed: " + e.getMessage(), e );
userLog.error( "Fatal, worker for session '" + machine.key() + "' crashed. Please" +
" contact your support representative if you are unable to resolve this.", e );

" contact your support representative if you are unable to resolve this.", t );
}
finally
{
// Attempt to close the session, as an effort to release locks and other resources held by the session
machine.close();
}
Expand All @@ -118,7 +129,6 @@ private void execute( Job job ) throws BoltConnectionFatality
{
if ( job == SHUTDOWN )
{
machine.close();
keepRunning = false;
}
else
Expand Down
29 changes: 10 additions & 19 deletions community/bolt/src/test/java/org/neo4j/bolt/security/auth/BasicAuthenticationTest.java
View file
Expand Up @@ -28,8 +28,6 @@
import org.neo4j.kernel.api.exceptions.Status;
import org.neo4j.kernel.api.security.AuthenticationResult;
import org.neo4j.kernel.api.security.exception.InvalidAuthTokenException;
import org.neo4j.logging.Log;
import org.neo4j.logging.LogProvider;
import org.neo4j.server.security.auth.BasicAuthManager;
import org.neo4j.server.security.auth.BasicAuthSubject;
import org.neo4j.server.security.auth.PasswordPolicy;
Expand All @@ -40,7 +38,6 @@
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.anyMap;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.neo4j.helpers.collection.MapUtil.map;

Expand All @@ -55,7 +52,7 @@ public void shouldNotDoAnythingOnSuccess() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.SUCCESS );

Expand All @@ -71,10 +68,7 @@ public void shouldThrowAndLogOnFailure() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
Log log = mock( Log.class );
LogProvider logProvider = mock( LogProvider.class );
when( logProvider.getLog( BasicAuthentication.class ) ).thenReturn( log );
BasicAuthentication authentication = new BasicAuthentication( manager, logProvider );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.FAILURE );

Expand All @@ -85,9 +79,6 @@ public void shouldThrowAndLogOnFailure() throws Exception

// When
authentication.authenticate( map( "scheme", "basic", "principal", "bob", "credentials", "secret" ) );

//Then
verify( log ).warn( "Failed authentication attempt for 'bob')" );
}

@Test
Expand All @@ -96,7 +87,7 @@ public void shouldIndicateThatCredentialsExpired() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.PASSWORD_CHANGE_REQUIRED );

Expand All @@ -114,7 +105,7 @@ public void shouldFailWhenTooManyAttempts() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.TOO_MANY_ATTEMPTS );

Expand All @@ -133,7 +124,7 @@ public void shouldBeAbleToUpdateCredentials() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.SUCCESS );

Expand All @@ -150,7 +141,7 @@ public void shouldBeAbleToUpdateExpiredCredentials() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.PASSWORD_CHANGE_REQUIRED );

Expand All @@ -167,7 +158,7 @@ public void shouldNotBeAbleToUpdateCredentialsIfOldCredentialsAreInvalid() throw
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenReturn( authSubject );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.FAILURE );

Expand All @@ -187,7 +178,7 @@ public void shouldThrowWithNoScheme() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenThrow( new InvalidAuthTokenException( "foo" ) );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.SUCCESS );

Expand All @@ -205,7 +196,7 @@ public void shouldFailOnUnknownScheme() throws Exception
// Given
BasicAuthManager manager = mock( BasicAuthManager.class );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( manager.login( anyMap() ) ).thenThrow( new InvalidAuthTokenException( "foo" ) );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.SUCCESS );

Expand All @@ -224,7 +215,7 @@ public void shouldFailOnMalformedToken() throws Exception
BasicAuthManager manager = new BasicAuthManager( mock( UserRepository.class), mock( PasswordPolicy.class ),
FakeClock.systemUTC() );
BasicAuthSubject authSubject = mock( BasicAuthSubject.class );
BasicAuthentication authentication = new BasicAuthentication( manager, mock( LogProvider.class ) );
BasicAuthentication authentication = new BasicAuthentication( manager );
when( authSubject.getAuthenticationResult() ).thenReturn( AuthenticationResult.SUCCESS );

// Expect
Expand Down

0 comments on commit 2a1f833

Please sign in to comment.