Skip to content

Commit

Permalink
Improved naming and testing.
Browse files Browse the repository at this point in the history 
Also now supports only wildcard character '*' and not all of regex in the procedure lists.
  • Loading branch information
@eebus
eebus committed Feb 28, 2017
1 parent bbe9432 commit e0f8707
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 26 deletions.
10 changes: 7 additions & 3 deletions community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java
View file
Expand Up @@ -515,18 +515,22 @@ public enum LabelIndex
pathSetting( "unsupported.dbms.security.auth_store.location", NO_DEFAULT );

@Description( "A list of procedures (comma separated) that are allowed full access to the database. " +
"The list may contain both fully-qualified procedure names, and partial names with the wildcard '*'. " +
"Note that this enables these procedures to bypass security. Use with caution." )
public static final Setting<String> procedure_unrestricted =
setting( "dbms.security.procedures.unrestricted", Settings.STRING, "" );


@Description( "Whether or not to release the exclusive schema lock is while building uniqueness constraints index" )
@Internal
public static final Setting<Boolean> release_schema_lock_while_building_constraint = setting(
"unsupported.dbms.schema.release_lock_while_building_constraint", BOOLEAN, FALSE );

@Description( "A list of procedures (comma separated) that are to be loaded." +
" If this setting is left empty all procedures will be loaded." )
public static final Setting<String> procedure_white_list =

@Description( "A list of procedures (comma separated) that are to be loaded. " +
"The list may contain both fully-qualified procedure names, and partial names with the wildcard '*'. " +
"If this setting is left empty no procedures will be loaded." )
public static final Setting<String> procedure_whitelist =
setting( "dbms.security.procedures.white_list", Settings.STRING, "*" );
// Bolt Settings

Expand Down
5 changes: 3 additions & 2 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ProcedureConfig.java
View file
Expand Up @@ -70,7 +70,7 @@ public ProcedureConfig( Config config )
parseMatchers( GraphDatabaseSettings.procedure_unrestricted.name(), config, PROCEDURE_DELIMITER,
ProcedureConfig::compilePattern );
this.whiteList =
parseMatchers( GraphDatabaseSettings.procedure_white_list.name(), config, PROCEDURE_DELIMITER,
parseMatchers( GraphDatabaseSettings.procedure_whitelist.name(), config, PROCEDURE_DELIMITER,
ProcedureConfig::compilePattern );
}

Expand Down Expand Up @@ -109,13 +109,14 @@ boolean fullAccessFor( String procedureName )
return accessPatterns.stream().anyMatch( pattern -> pattern.matcher( procedureName ).matches() );
}

boolean whiteListed( String procedureName )
boolean isWhitelisted( String procedureName )
{
return whiteList.stream().anyMatch( pattern -> pattern.matcher( procedureName ).matches() );
}

private static Pattern compilePattern( String procedure )
{
procedure = procedure.replaceAll( "([\\[\\]\\\\?()\\^${}+|\\-&])", "\\\\$1" );
return Pattern.compile( procedure.trim().replaceAll( "\\.", "\\\\." ).replaceAll( "\\*", ".*" ) );
}

Expand Down
7 changes: 4 additions & 3 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureCompiler.java
View file
Expand Up @@ -179,13 +179,14 @@ List<CallableProcedure> compileProcedure( Class<?> procDefinition, Optional<Stri
String definedName = method.getAnnotation( Procedure.class ).name();
QualifiedName procName = extractName( procDefinition, method, valueName, definedName );

if ( !(fullAccess || config.whiteListed( procName.toString() )) )
if ( fullAccess || config.isWhitelisted( procName.toString() ) )
{
log.warn( String.format( "The procedure '%s' is not white listed.", procName.toString() ) );
out.add( compileProcedure( procDefinition, constructor, method, warning, fullAccess, procName ) );
}
else
{
out.add( compileProcedure( procDefinition, constructor, method, warning, fullAccess, procName ) );
log.warn( String.format( "The procedure '%s' is not on the whitelist and won't be loaded.",
procName.toString() ) );
}
}
out.sort( Comparator.comparing( a -> a.signature().name().toString() ) );
Expand Down
31 changes: 25 additions & 6 deletions community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureTest.java
View file
Expand Up @@ -54,7 +54,7 @@
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.neo4j.graphdb.factory.GraphDatabaseSettings.procedure_white_list;
import static org.neo4j.graphdb.factory.GraphDatabaseSettings.procedure_whitelist;
import static org.neo4j.helpers.collection.Iterators.asList;
import static org.neo4j.helpers.collection.MapUtil.genericMap;
import static org.neo4j.kernel.api.proc.ProcedureSignature.procedureSignature;
Expand Down Expand Up @@ -314,7 +314,7 @@ public void shouldLoadWhiteListedProcedure() throws Throwable
{
// Given
ProcedureConfig config = new ProcedureConfig( Config.defaults().with(
genericMap( procedure_white_list.name(), "org.neo4j.kernel.impl.proc.listCoolPeople") ) );
genericMap( procedure_whitelist.name(), "org.neo4j.kernel.impl.proc.listCoolPeople") ) );

Log log = mock(Log.class);
ReflectiveProcedureCompiler procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components,
Expand All @@ -335,7 +335,7 @@ public void shouldNotLoadNoneWhiteListedProcedure() throws Throwable
{
// Given
ProcedureConfig config = new ProcedureConfig( Config.defaults().with(
genericMap( procedure_white_list.name(), "org.neo4j.kernel.impl.proc.NOTlistCoolPeople") ) );
genericMap( procedure_whitelist.name(), "org.neo4j.kernel.impl.proc.NOTlistCoolPeople") ) );

Log log = mock(Log.class);
ReflectiveProcedureCompiler procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components,
Expand All @@ -346,16 +346,16 @@ public void shouldNotLoadNoneWhiteListedProcedure() throws Throwable
procedureCompiler.compileProcedure( SingleReadOnlyProcedure.class, Optional.empty(), false );
// Then
verify( log )
.warn( "The procedure 'org.neo4j.kernel.impl.proc.listCoolPeople' is not white listed." );
.warn( "The procedure 'org.neo4j.kernel.impl.proc.listCoolPeople' is not on the whitelist and won't be loaded." );
assertThat( proc.isEmpty(), is(true) );
}

@Test
public void shouldLoadFullAccessProcedure() throws Throwable
public void shouldIgnoreWhiteListingIfFullAccess() throws Throwable
{
// Given
ProcedureConfig config = new ProcedureConfig( Config.defaults().with(
genericMap( procedure_white_list.name(), "org.neo4j.kernel.impl.proc.NOTlistCoolPeople") ) );
genericMap( procedure_whitelist.name(), "empty") ) );
Log log = mock(Log.class);
ReflectiveProcedureCompiler procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components,
components, log, config );
Expand All @@ -368,6 +368,25 @@ public void shouldLoadFullAccessProcedure() throws Throwable
assertEquals( result.next()[0], "Bonnie" );
}

@Test
public void shouldNotLoadAnyProcedureIfConfigIsEmpty() throws Throwable
{
// Given
ProcedureConfig config = new ProcedureConfig( Config.defaults().with(
genericMap( procedure_whitelist.name(), "") ) );
Log log = mock(Log.class);
ReflectiveProcedureCompiler procedureCompiler = new ReflectiveProcedureCompiler( new TypeMappers(), components,
components, log, config );

// When
List<CallableProcedure> proc =
procedureCompiler.compileProcedure( SingleReadOnlyProcedure.class, Optional.empty(), false );
// Then
verify( log )
.warn( "The procedure 'org.neo4j.kernel.impl.proc.listCoolPeople' is not on the whitelist and won't be loaded." );
assertThat( proc.isEmpty(), is(true) );
}

public static class MyOutputRecord
{
public String name;
Expand Down
80 changes: 68 additions & 12 deletions enterprise/security/src/test/java/org/neo4j/kernel/impl/proc/ProcedureConfigTest.java
View file
Expand Up @@ -26,7 +26,7 @@
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.neo4j.graphdb.factory.GraphDatabaseSettings.procedure_unrestricted;
import static org.neo4j.graphdb.factory.GraphDatabaseSettings.procedure_white_list;
import static org.neo4j.graphdb.factory.GraphDatabaseSettings.procedure_whitelist;
import static org.neo4j.helpers.collection.MapUtil.genericMap;
import static org.neo4j.kernel.impl.proc.ProcedureConfig.PROC_ALLOWED_SETTING_DEFAULT_NAME;
import static org.neo4j.kernel.impl.proc.ProcedureConfig.PROC_ALLOWED_SETTING_ROLES;
Expand Down Expand Up @@ -193,27 +193,83 @@ public void shouldAllowFullAccessWildcardProceduresNames()
public void shouldBlockWithWhiteListingForProcedures()
{
Config config = Config.defaults().with( genericMap( procedure_unrestricted.name(),
"test.procedure.name, test.procedure.name2", procedure_white_list.name(),
"test.procedure.name, test.procedure.name2", procedure_whitelist.name(),
"test.procedure.name") );
ProcedureConfig procConfig = new ProcedureConfig( config );

assertThat( procConfig.whiteListed( "xyzabc" ), equalTo( false ) );
assertThat( procConfig.whiteListed( "test.procedure.name" ), equalTo( true ) );
assertThat( procConfig.whiteListed( "test.procedure.name2" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "xyzabc" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "test.procedure.name" ), equalTo( true ) );
assertThat( procConfig.isWhitelisted( "test.procedure.name2" ), equalTo( false ) );
}

@Test
public void shouldAllowWhiteListsWildcardProceduresNames()
{
Config config = Config.defaults().with( genericMap( procedure_white_list.name(),
Config config = Config.defaults().with( genericMap( procedure_whitelist.name(),
" test.procedure.* , test.*.otherName" ));
ProcedureConfig procConfig = new ProcedureConfig( config );

assertThat( procConfig.whiteListed( "xyzabc" ), equalTo( false ) );
assertThat( procConfig.whiteListed( "test.procedure.name" ), equalTo( true ) );
assertThat( procConfig.whiteListed( "test.procedure.otherName" ), equalTo( true ) );
assertThat( procConfig.whiteListed( "test.other.otherName" ), equalTo( true ) );
assertThat( procConfig.whiteListed( "test.other.cool.otherName" ), equalTo( true ) );
assertThat( procConfig.whiteListed( "test.other.name" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "xyzabc" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "test.procedure.name" ), equalTo( true ) );
assertThat( procConfig.isWhitelisted( "test.procedure.otherName" ), equalTo( true ) );
assertThat( procConfig.isWhitelisted( "test.other.otherName" ), equalTo( true ) );
assertThat( procConfig.isWhitelisted( "test.other.cool.otherName" ), equalTo( true ) );
assertThat( procConfig.isWhitelisted( "test.other.name" ), equalTo( false ) );
}

@Test
public void shouldIgnoreOddRegex()
{
Config config = Config.defaults().with( genericMap( procedure_whitelist.name(), "[\\db^a]*" ) );
ProcedureConfig procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "123" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "b" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "a" ), equalTo( false ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "(abc)" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "(abc)" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "^$" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "^$" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "\\" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "\\" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "&&" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "&&" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "\\p{Lower}" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "a" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "\\p{Lower}" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "a+" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "aaaaaa" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "a+" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "a|b" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "a" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "b" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "|" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "a|b" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "[a-c]" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "a" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "b" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "c" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "-" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "[a-c]" ), equalTo( true ) );

config = Config.defaults().with( genericMap( procedure_whitelist.name(), "a\tb" ) );
procConfig = new ProcedureConfig( config );
assertThat( procConfig.isWhitelisted( "a b" ), equalTo( false ) );
assertThat( procConfig.isWhitelisted( "a\tb" ), equalTo( true ) );
}
}

0 comments on commit e0f8707

Please sign in to comment.