Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix whitelisting of non-existing ks/table #14

Merged
merged 5 commits into from
Sep 26, 2018
Merged

Fix whitelisting of non-existing ks/table #14

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a2c633
Fix role based whitelist for non-existing ks/table
eperott Sep 17, 2018
04426e4
Cleanup and add note to CHANGES
eperott Sep 25, 2018
aa60b79
Resolve invalid table
eperott Sep 25, 2018
24e7f7a
Remove unused import
eperott Sep 25, 2018
2d04ced
Handle RuntimeExceptions
eperott Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changes

## Version 0.22.0
* Fix role based whitelist for non-existing ks/table (Ericsson/ecaudit#10)

## Version 0.21.0
* Public release on Maven Central

Expand Down
100 changes: 85 additions & 15 deletions src/main/java/com/ericsson/bss/cassandra/ecaudit/AuditEntryBuilderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//**********************************************************************
package com.ericsson.bss.cassandra.ecaudit;

import java.util.List;
import java.util.Set;

import org.apache.cassandra.auth.DataResource;
Expand All @@ -23,32 +24,42 @@
import org.apache.cassandra.auth.RoleResource;
import org.apache.cassandra.cql3.CFName;
import org.apache.cassandra.cql3.CQLStatement;
import org.apache.cassandra.cql3.ColumnCondition;
import org.apache.cassandra.cql3.ColumnIdentifier;
import org.apache.cassandra.cql3.QueryProcessor;
import org.apache.cassandra.cql3.statements.AlterRoleStatement;
import org.apache.cassandra.cql3.statements.AuthenticationStatement;
import org.apache.cassandra.cql3.statements.CFStatement;
import org.apache.cassandra.cql3.statements.CreateRoleStatement;
import org.apache.cassandra.cql3.statements.DropRoleStatement;
import org.apache.cassandra.cql3.statements.ListPermissionsStatement;
import org.apache.cassandra.cql3.statements.ListRolesStatement;
import org.apache.cassandra.cql3.statements.ModificationStatement;
import org.apache.cassandra.cql3.statements.ParsedStatement;
import org.apache.cassandra.cql3.statements.PermissionsManagementStatement;
import org.apache.cassandra.cql3.statements.RoleManagementStatement;
import org.apache.cassandra.cql3.statements.SchemaAlteringStatement;
import org.apache.cassandra.cql3.statements.SelectStatement;
import org.apache.cassandra.cql3.statements.TruncateStatement;
import org.apache.cassandra.cql3.statements.UseStatement;
import org.apache.cassandra.db.KeyspaceNotDefinedException;
import org.apache.cassandra.service.ClientState;
import org.apache.cassandra.utils.Pair;
import org.apache.commons.lang3.reflect.FieldUtils;

import com.ericsson.bss.cassandra.ecaudit.entry.AuditEntry;
import com.ericsson.bss.cassandra.ecaudit.entry.AuditEntry.Builder;
import com.ericsson.bss.cassandra.ecaudit.facade.CassandraAuditException;
import com.google.common.collect.ImmutableSet;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class AuditEntryBuilderFactory
{
// This list of predefines and immutable permission sets are injected to the audit entries
// created in this factory. They should stay immutable since the AduitVO type and its builder
public static final Logger LOG = LoggerFactory.getLogger(AuditEntryBuilderFactory.class);

// This list of predefined and immutable permission sets are injected to the audit entries
// created in this factory. They should stay immutable since the {@link AuditEntry} type and its builder
// doesn't make copies of the sets.
public static final Set<Permission> ALL_PERMISSIONS = Permission.ALL; // NOSONAR
public static final Set<Permission> SELECT_PERMISSIONS = ImmutableSet.of(Permission.SELECT);
Expand All @@ -62,20 +73,49 @@ public class AuditEntryBuilderFactory

public Builder createEntryBuilder(String operation, ClientState state)
{
CQLStatement statement;
try
{
statement = QueryProcessor.getStatement(operation, state).statement;
CQLStatement statement = QueryProcessor.getStatement(operation, state).statement;
return createEntryBuilder(statement);
}
catch (KeyspaceNotDefinedException e)
{
LOG.trace("Failed to prepare statement for non-existing resource - trying unprepared", e);
ParsedStatement parsedStatement = getParsedStatement(operation, state);
return createEntryBuilder(parsedStatement);
}
catch (RuntimeException e)
{
// This is typically the result of a query towards a non-existing resource
return AuditEntry.newBuilder()
.permissions(ALL_PERMISSIONS)
.resource(DataResource.root());
LOG.trace("Failed to parse or prepare statement - assuming default permissions and resources", e);
return createDefaultEntryBuilder();
}
}

return createEntryBuilder(statement);
private ParsedStatement getParsedStatement(String operation, ClientState state)
{
ParsedStatement parsedStatement = QueryProcessor.parseStatement(operation);

// Set keyspace for statement that require login
if (parsedStatement instanceof CFStatement) {
((CFStatement) parsedStatement).prepareKeyspace(state);
}
return parsedStatement;
}

private Builder createEntryBuilder(ParsedStatement parsedStatement)
{

if (parsedStatement instanceof SelectStatement.RawStatement)
{
return createSelectEntryBuilder((SelectStatement.RawStatement) parsedStatement);
}
if (parsedStatement instanceof ModificationStatement.Parsed)
{
return createModificationEntryBuilder((ModificationStatement.Parsed) parsedStatement);
}

LOG.trace("Unable to determine statement type for parsing - assuming default permissions and resources");
return createDefaultEntryBuilder();
}

public Builder createEntryBuilder(CQLStatement statement) // NOSONAR
Expand Down Expand Up @@ -133,9 +173,8 @@ public Builder createEntryBuilder(CQLStatement statement) // NOSONAR
return createSchemaAlteringEntryBuilder((SchemaAlteringStatement) statement);
}

return AuditEntry.newBuilder()
.permissions(ALL_PERMISSIONS)
.resource(DataResource.root());
LOG.trace("Unrecognized statement type - assuming default permissions and resources");
return createDefaultEntryBuilder();
}

public Builder createSelectEntryBuilder(SelectStatement statement)
Expand All @@ -145,13 +184,36 @@ public Builder createSelectEntryBuilder(SelectStatement statement)
.resource(DataResource.table(statement.keyspace(), statement.columnFamily()));
}

public Builder createSelectEntryBuilder(SelectStatement.RawStatement statement)
{
return AuditEntry.newBuilder()
.permissions(SELECT_PERMISSIONS)
.resource(DataResource.table(statement.keyspace(), statement.columnFamily()));
}

public Builder createModificationEntryBuilder(ModificationStatement statement)
{
return AuditEntry.newBuilder()
.permissions(statement.hasConditions() ? CAS_PERMISSIONS : MODIFY_PERMISSIONS)
.resource(DataResource.table(statement.keyspace(), statement.columnFamily()));
}

public Builder createModificationEntryBuilder(ModificationStatement.Parsed statement)
{
Set<Permission> permissions;
try
{
boolean hasCondition = !((List<Pair<ColumnIdentifier.Raw, ColumnCondition.Raw>>) FieldUtils.readField(statement, "conditions", true)).isEmpty();
permissions = hasCondition ? CAS_PERMISSIONS : MODIFY_PERMISSIONS;
} catch (IllegalAccessException e) {
throw new CassandraAuditException("Failed to resolve resource", e);
}

return AuditEntry.newBuilder()
.permissions(permissions)
.resource(DataResource.table(statement.keyspace(), statement.columnFamily()));
}

public Builder createTruncateEntryBuilder(TruncateStatement statement)
{
return AuditEntry.newBuilder()
Expand Down Expand Up @@ -190,10 +252,11 @@ public Builder updateBatchEntryBuilder(Builder builder, String operation, Client
}
catch (RuntimeException e)
{
LOG.trace("Failed to parse or prepare entry in batch statement - assuming default permissions and resources", e);
// This is typically the result of a query towards a non-existing resource
return builder
.permissions(CAS_PERMISSIONS)
.resource(DataResource.root());
// TODO: We should be able to fix this
// But right now we don't get here since batch statements fail pre-processing
return createDefaultEntryBuilder();
}

return updateBatchEntryBuilder(builder, (ModificationStatement) statement);
Expand Down Expand Up @@ -316,6 +379,13 @@ public Builder createSchemaAlteringEntryBuilder(SchemaAlteringStatement statemen
.resource(toDataResource(cfName));
}

private Builder createDefaultEntryBuilder()
{
return AuditEntry.newBuilder()
.permissions(ALL_PERMISSIONS)
.resource(DataResource.root());
}

private DataResource toDataResource(CFName cfName)
{
if (cfName != null && cfName.hasKeyspace())
Expand Down
55 changes: 44 additions & 11 deletions src/test/java/com/ericsson/bss/cassandra/ecaudit/integration/ITVerifyAudit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
package com.ericsson.bss.cassandra.ecaudit.integration;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
Expand All @@ -31,6 +31,7 @@
import java.util.List;
import java.util.stream.Collectors;

import com.datastax.driver.core.exceptions.InvalidQueryException;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
Expand Down Expand Up @@ -111,7 +112,7 @@ public static void beforeClass() throws Exception

session.execute(
new SimpleStatement(
"CREATE ROLE sam WITH PASSWORD = 'secret' AND LOGIN = true AND SUPERUSER = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data/system, data/system_schema, data/ecks/ectbl, connections' }"));
"CREATE ROLE sam WITH PASSWORD = 'secret' AND LOGIN = true AND SUPERUSER = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data/system, data/system_schema, data/ecks/ectbl, data/nonexistingks, data/validks/nonexistingtbl, connections' }"));
session.execute(
new SimpleStatement(
"GRANT MODIFY ON ecks.ectbl TO sam"));
Expand Down Expand Up @@ -503,21 +504,14 @@ public void testFailedStatementsAreLogged()
"CREATE TABLE ecks.ectbl (partk int PRIMARY KEY, clustk text, value text)",
"INSERT INTO invalidks.invalidtbl (partk, clustk, value) VALUES (1, 'one', 'valid')",
"SELECT * FROM invalidks.invalidtbl",
"SELECT * FROM ecks.invalidtbl",
"DELETE FROM invalidks.invalidtbl WHERE partk = 2",
"DROP KEYSPACE invalidks",
"DROP ROLE invaliduser");

for (String statement : statements)
{
try
{
session.execute(new SimpleStatement(statement));
fail("Expected statement to be rejected");
}
catch (DriverException e)
{
// Intentionally left empty
}
assertThatExceptionOfType(DriverException.class).isThrownBy(() -> session.execute(new SimpleStatement(statement)));
}

ArgumentCaptor<ILoggingEvent> loggingEventCaptor = ArgumentCaptor.forClass(ILoggingEvent.class);
Expand All @@ -531,6 +525,45 @@ public void testFailedStatementsAreLogged()
.containsAll(expectedAttemptsAndFails(statements));
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing it, but I'd like to see a test case for a failed batch statement which is not whitelisted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Known issue: #11

public void testFailedWhitelistedStatementsAreNotLogged()
{
List<String> statements = Arrays.asList(
"SELECT * FROM nonexistingks.nonexistingtbl",
"SELECT * FROM validks.nonexistingtbl",
"INSERT INTO nonexistingks.nonexistingtbl (partk, clustk, value) VALUES (1, 'one', 'valid')",
"INSERT INTO validks.nonexistingtbl (partk, clustk, value) VALUES (1, 'one', 'valid')");

try (Cluster privateCluster = cdt.createCluster("sam", "secret");
Session privateSession = privateCluster.connect())
{
for (String statement : statements)
{
assertThatExceptionOfType(InvalidQueryException.class).isThrownBy(() -> privateSession.execute(new SimpleStatement(statement)));
}
}
}

@Test
public void testFailedWhitelistedBatchStatementIsNotLogged()
{
List<String> statements = Arrays.asList(
"INSERT INTO nonexistingks.nonexistingtbl (partk, clustk, value) VALUES (1, 'one', 'valid')",
"INSERT INTO validks.nonexistingtbl (partk, clustk, value) VALUES (1, 'one', 'valid')");

try (Cluster privateCluster = cdt.createCluster("sam", "secret");
Session privateSession = privateCluster.connect())
{
for (String statement : statements)
{
BatchStatement batch = new BatchStatement(BatchStatement.Type.UNLOGGED);
batch.add(new SimpleStatement("INSERT INTO ecks.ectbl (partk, clustk, value) VALUES (4, '4', 'valid')"));
batch.add(new SimpleStatement(statement));
assertThatExceptionOfType(InvalidQueryException.class).isThrownBy(() -> privateSession.execute(batch));
}
}
}

@Test
public void testYamlWhitelistedUserShowConnectionAttemptsButValidStatementsAreNotLogged()
{
Expand Down
47 changes: 21 additions & 26 deletions ...test/java/com/ericsson/bss/cassandra/ecaudit/integration/ITVerifyWhitelistManagement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ public void before()
@After
public void after()
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
// LoggerContext loggerContext = (LoggerContext) LoggerFactory.getILoggerFactory();
// loggerContext.getLogger(FileAuditLogger.AUDIT_LOGGER_NAME).detachAppender(mockAuditAppender);
}
Expand Down Expand Up @@ -167,10 +168,6 @@ public void testCreateUserCannotCreateWhitelistedUser()
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data' }"));
}
finally
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
}
}

@Test
Expand All @@ -182,10 +179,6 @@ public void testAuthorizedUserCanCreateWhitelisted()
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data' }"));
}
finally
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
}
}

@Test
Expand All @@ -197,10 +190,6 @@ public void testSuperUserCanCreateWhitelistedOnData()
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data' }"));
}
finally
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
}
}

@Test
Expand All @@ -212,9 +201,27 @@ public void testSuperUserCanCreateWhitelistedOnDataPartly()
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data/ecks/ectbl' }"));
}
finally
}

@Test
public void testSuperUserCanCreateWhitelistedOnNonexistingKeyspace()
{
try (Cluster privateCluster = cdt.createCluster("super_user", "secret");
Session privateSession = privateCluster.connect())
{
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data/unknownks' }"));
}
}

@Test
public void testSuperUserCanCreateWhitelistedOnNonexistingTable()
{
try (Cluster privateCluster = cdt.createCluster("super_user", "secret");
Session privateSession = privateCluster.connect())
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'data/ecks/unknowntbl' }"));
}
}

Expand All @@ -227,10 +234,6 @@ public void testSuperUserCanCreateWhitelistedOnConnection()
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'connections' }"));
}
finally
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
}
}

@Test
Expand All @@ -242,10 +245,6 @@ public void testSuperUserCanCreateWhitelistedOnRoles()
privateSession.execute(new SimpleStatement(
"CREATE ROLE temporary_user WITH PASSWORD = 'secret' AND LOGIN = true AND OPTIONS = { 'grant_audit_whitelist_for_all' : 'roles' }"));
}
finally
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
}
}

@Test
Expand All @@ -259,9 +258,5 @@ public void testTrustedUserCanDelegateWhitelistedRole()
privateSession.execute(new SimpleStatement(
"GRANT whitelist_role TO temporary_user"));
}
finally
{
session.execute(new SimpleStatement("DROP ROLE IF EXISTS temporary_user"));
}
}
}