Skip to content

Commit

Permalink
Fix user secret filter of Operator
Browse files Browse the repository at this point in the history 
Operator should be able to drop granted access to a secret key. On the
other hand, Operator must declare all keys if they are not granted by
users.
  • Loading branch information
@frsyuki
frsyuki committed Dec 26, 2016
1 parent 7accda5 commit 584e75a
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 45 deletions.
50 changes: 33 additions & 17 deletions digdag-core/src/main/java/io/digdag/core/agent/DefaultSecretProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,25 @@
import io.digdag.spi.SecretStore;

import java.util.List;
import java.util.function.Predicate;

import static java.util.Locale.ENGLISH;

class DefaultSecretProvider
implements SecretProvider
{
static interface OperatorSecretFilter
{
boolean test(String key, boolean userGranted);
}

private final SecretAccessContext context;
private final SecretAccessPolicy secretAccessPolicy;
private final Config grants;
private final Predicate<String> operatorSecretFilter;
private final OperatorSecretFilter operatorSecretFilter;
private final SecretStore secretStore;

DefaultSecretProvider(
SecretAccessContext context, SecretAccessPolicy secretAccessPolicy, Config grants, Predicate<String> operatorSecretFilter, SecretStore secretStore)
SecretAccessContext context, SecretAccessPolicy secretAccessPolicy, Config grants, OperatorSecretFilter operatorSecretFilter, SecretStore secretStore)
{
this.context = context;
this.secretAccessPolicy = secretAccessPolicy;
Expand All @@ -46,22 +52,20 @@ public Optional<String> getSecretOptional(String key)
Preconditions.checkArgument(key.indexOf('*') == -1, errorMessage);

//// Secret access control:
// 1. Reject if the operator doesn't need the access to the secret (operatorSecretFilter).
// 2. Allow if users explicitly grant access using _secret directive (Config grants).
// 3. Allow if system access policy (SecretAccessPolicy) allows.
// 4. Reject.
// 1. If users explicitly grant access using _secret directive in workflow definition (Config grants):
// 1-a. Allow if the operator wants to use it (operatorSecretFilter)
// 1-b. Reject.
// 2. If the system access policy file (SecretAccessPolicy) allows:
// 2-a. Allow if the operator wants to use it (operatorSecretFilter)
// 2-b. Reject.
// 3. Otherwise, reject.

List<String> segments = Splitter.on('.').splitToList(key);
segments.forEach(segment -> Preconditions.checkArgument(!Strings.isNullOrEmpty(segment)));

//// Step 1.
// Operator can drop access privilege to a key because it knows whether the secret is necessary or not.
if (!operatorSecretFilter.test(key)) {
throw new SecretAccessFilteredException(key, "Unexpected access to a secret key: '" + key + "'");
}

//// Step 2.
// If the key falls under the scope of an explicit grant, then fetch the secret identified by remounting the key path into the grant path.
//// Case 1.
// If the key falls under the scope of a grant explicitly written by users,
// then fetch the secret identified by remounting the key path into the grant path.
JsonNode scope = grants.getInternalObjectNode();
int i = 0;

Expand Down Expand Up @@ -89,20 +93,32 @@ else if (node.isTextual()) {
.from(base)
.append(remainder)
.join(Joiner.on('.'));
if (!operatorSecretFilter.test(remounted, true)) {
throw new SecretAccessFilteredException(key, String.format(ENGLISH,
"Unexpected access to a secret key '%s' aliased from '%s'", key, remounted));
}
return fetchSecret(remounted);
}
else if (node.isBoolean() && node.asBoolean()) {
// Reached a grant leaf.
if (!operatorSecretFilter.test(key, true)) {
throw new SecretAccessFilteredException(key, String.format(ENGLISH,
"Unexpected access to a secret key '%s'", key));
}
return fetchSecret(key);
}
else {
throw new AssertionError();
}
}

//// Step 3.
// No explicit grant. Check key against system acl to see if access is granted by default.
//// Case 2.
// No explicit grant by users. Check key against system acl to see if access is granted by default.
if (secretAccessPolicy.isSecretAccessible(context, key)) {
if (!operatorSecretFilter.test(key, false)) {
throw new SecretAccessFilteredException(key, String.format(ENGLISH,
"Undeclared access to a secret key '%s'. OperatorFactory must declare this key in getSecretAccessList method.", key));
}
return fetchSecret(key);
}

Expand Down
23 changes: 15 additions & 8 deletions digdag-core/src/main/java/io/digdag/core/agent/OperatorManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ protected TaskResult callExecutor(Path projectPath, String type, TaskRequest mer
Config grants = mergedRequest.getConfig().getNestedOrGetEmpty("_secrets");

// Operator can drop access to access to unnecessary secrets
OperatorSecretFilter operatorSecretFilter = new OperatorSecretFilter(factory);
AgentOperatorSecretFilter operatorSecretFilter = new AgentOperatorSecretFilter(factory);

DefaultSecretProvider secretProvider = new DefaultSecretProvider(
secretContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);
Expand All @@ -324,27 +324,34 @@ protected TaskResult callExecutor(Path projectPath, String type, TaskRequest mer
return operator.run();
}

private static class OperatorSecretFilter
implements Predicate<String>
private static class AgentOperatorSecretFilter
implements DefaultSecretProvider.OperatorSecretFilter
{
private final Set<String> predeclaredSecretKeys;
private Predicate<String> userSecretAccessFilter = (key) -> false;
private boolean userSecretAccess = false;
private Predicate<String> dynamicFilter = (key) -> true;

OperatorSecretFilter(OperatorFactory factory)
AgentOperatorSecretFilter(OperatorFactory factory)
{
this.predeclaredSecretKeys = ImmutableSet.copyOf(
factory.getSecretAccessList().getSecretKeys());
}

void allowUserSecretAccess(Operator operator)
{
this.userSecretAccessFilter = (key) -> operator.testUserSecretAccess(key);
this.userSecretAccess = true;
this.dynamicFilter = (key) -> operator.testUserSecretAccess(key);
}

@Override
public boolean test(String key)
public boolean test(String key, boolean userGranted)
{
return predeclaredSecretKeys.contains(key) || userSecretAccessFilter.test(key);
if (userGranted) {
return userSecretAccess && dynamicFilter.test(key);
}
else {
return predeclaredSecretKeys.contains(key) && dynamicFilter.test(key);
}
}
}

Expand Down
35 changes: 18 additions & 17 deletions digdag-core/src/test/java/io/digdag/core/agent/DefaultSecretProviderTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package io.digdag.core.agent;

import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.base.Optional;
import io.digdag.client.config.Config;
import io.digdag.client.config.ConfigFactory;
import io.digdag.core.config.YamlConfigLoader;
import io.digdag.core.agent.DefaultSecretProvider.OperatorSecretFilter;
import io.digdag.spi.SecretAccessContext;
import io.digdag.spi.SecretAccessDeniedException;
import io.digdag.spi.SecretAccessPolicy;
Expand All @@ -27,6 +29,7 @@
import static org.junit.Assert.fail;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
Expand All @@ -45,7 +48,7 @@ public class DefaultSecretProviderTest

@Mock SecretAccessPolicy secretAccessPolicy;
@Mock SecretStore secretStore;
@Mock Predicate<String> operatorSecretFilter;
@Mock OperatorSecretFilter operatorSecretFilter;

private final SecretAccessContext secretAccessContext = SecretAccessContext.builder()
.siteId(SITE_ID)
Expand Down Expand Up @@ -76,7 +79,6 @@ public void verifyUndeclaredAccessIsDenied()
Config grants = createConfig();

when(secretAccessPolicy.isSecretAccessible(secretAccessContext, key)).thenReturn(false);
when(operatorSecretFilter.test(key)).thenReturn(true);

DefaultSecretProvider provider = new DefaultSecretProvider(secretAccessContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);

Expand All @@ -89,7 +91,6 @@ public void verifyUndeclaredAccessIsDenied()
}

verify(secretAccessPolicy).isSecretAccessible(secretAccessContext, key);
verify(operatorSecretFilter).test(key);

verifyNoMoreInteractions(secretStore);
}
Expand All @@ -109,14 +110,14 @@ public void testDefaultAccessibleSecret()

when(secretStore.getSecret(PROJECT_ID, SecretScopes.PROJECT, key)).thenReturn(Optional.of(expectedSecret));
when(secretAccessPolicy.isSecretAccessible(secretAccessContext, key)).thenReturn(true);
when(operatorSecretFilter.test(key)).thenReturn(true);
when(operatorSecretFilter.test(key, false)).thenReturn(true);

DefaultSecretProvider provider = new DefaultSecretProvider(secretAccessContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);

String secret = provider.getSecret(key);

verify(secretAccessPolicy).isSecretAccessible(secretAccessContext, key);
verify(operatorSecretFilter).test(key);
verify(operatorSecretFilter).test(key, false);
verify(secretStore).getSecret(PROJECT_ID, SecretScopes.PROJECT, key);

assertThat(secret, is(expectedSecret));
Expand All @@ -141,18 +142,18 @@ public void testUserGrantedSecret(String key, String grantsYaml, String expected

String expectedSecret = "the-secret";

Config grants = YAML_CONFIG_LOADER.loadString(grantsYaml).toConfig(CONFIG_FACTORY);

when(secretStore.getSecret(PROJECT_ID, SecretScopes.PROJECT, expectedKey)).thenReturn(Optional.of(expectedSecret));
when(secretAccessPolicy.isSecretAccessible(any(SecretAccessContext.class), anyString())).thenReturn(false);
when(operatorSecretFilter.test(key)).thenReturn(true);

Config grants = YAML_CONFIG_LOADER.loadString(grantsYaml).toConfig(CONFIG_FACTORY);
when(operatorSecretFilter.test(key, true)).thenReturn(true);

DefaultSecretProvider provider = new DefaultSecretProvider(secretAccessContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);

String secret = provider.getSecret(key);

verifyNoMoreInteractions(secretAccessPolicy);
verify(operatorSecretFilter).test(key);
verify(operatorSecretFilter).test(key, true);
verify(secretStore).getSecret(PROJECT_ID, SecretScopes.PROJECT, expectedKey);

assertThat(secret, is(expectedSecret));
Expand All @@ -170,7 +171,8 @@ public void testOperatorFilteredSecret()
String key = "foo";
Config grants = createConfig();

when(operatorSecretFilter.test(key)).thenReturn(false);
when(operatorSecretFilter.test(key, false)).thenReturn(false);
when(secretAccessPolicy.isSecretAccessible(any(SecretAccessContext.class), anyString())).thenReturn(true);

DefaultSecretProvider provider = new DefaultSecretProvider(secretAccessContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);

Expand All @@ -182,9 +184,8 @@ public void testOperatorFilteredSecret()
assertThat(e.getKey(), is(key));
}

verifyNoMoreInteractions(secretAccessPolicy);

verify(operatorSecretFilter).test(key);
verify(secretAccessPolicy).isSecretAccessible(secretAccessContext, key);
verify(operatorSecretFilter).test(key, false);
verifyNoMoreInteractions(secretStore);
}

Expand All @@ -200,13 +201,13 @@ public void verifyProjectScopePrecedence()
when(secretStore.getSecret(PROJECT_ID, SecretScopes.PROJECT, key)).thenReturn(Optional.of(projectSecret));
when(secretStore.getSecret(PROJECT_ID, SecretScopes.PROJECT_DEFAULT, key)).thenReturn(Optional.of(projectDefaultSecret));
when(secretAccessPolicy.isSecretAccessible(secretAccessContext, key)).thenReturn(true);
when(operatorSecretFilter.test(key)).thenReturn(true);
when(operatorSecretFilter.test(key, false)).thenReturn(true);

DefaultSecretProvider provider = new DefaultSecretProvider(secretAccessContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);

String secret = provider.getSecret(key);

verify(operatorSecretFilter).test(key);
verify(operatorSecretFilter).test(key, false);
verify(secretAccessPolicy).isSecretAccessible(secretAccessContext, key);
verify(secretStore).getSecret(PROJECT_ID, SecretScopes.PROJECT, key);
verify(secretStore, never()).getSecret(PROJECT_ID, key, SecretScopes.PROJECT_DEFAULT);
Expand All @@ -225,13 +226,13 @@ public void verifyProjectDefaultScopeFallback()
when(secretStore.getSecret(PROJECT_ID, SecretScopes.PROJECT, key)).thenReturn(Optional.absent());
when(secretStore.getSecret(PROJECT_ID, SecretScopes.PROJECT_DEFAULT, key)).thenReturn(Optional.of(projectDefaultSecret));
when(secretAccessPolicy.isSecretAccessible(secretAccessContext, key)).thenReturn(true);
when(operatorSecretFilter.test(key)).thenReturn(true);
when(operatorSecretFilter.test(key, false)).thenReturn(true);

DefaultSecretProvider provider = new DefaultSecretProvider(secretAccessContext, secretAccessPolicy, grants, operatorSecretFilter, secretStore);

String secret = provider.getSecret(key);

verify(operatorSecretFilter).test(key);
verify(operatorSecretFilter).test(key, false);
verify(secretAccessPolicy).isSecretAccessible(secretAccessContext, key);
verify(secretStore).getSecret(PROJECT_ID, SecretScopes.PROJECT, key);
verify(secretStore).getSecret(PROJECT_ID, SecretScopes.PROJECT_DEFAULT, key);
Expand Down
6 changes: 3 additions & 3 deletions digdag-core/src/test/java/io/digdag/core/agent/OperatorSecretFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public SecretAccessOperator(OperatorContext context)
@Override
public boolean testUserSecretAccess(String key)
{
return key.equals("user_key");
return key.equals("user_key") || key.equals("statically_declared_key");
}

@Override
Expand Down Expand Up @@ -135,7 +135,7 @@ public void verifyNonPredeclaredAccessRejected()
@Test
public void verifyUserGrantedAccessAllowed()
{
Config config = newConfig().set("get", "user_key").set("_secret", newConfig().set("user_key", true));
Config config = newConfig().set("get", "user_key").set("_secrets", newConfig().set("user_key", true));
Config stored = run("secret_access", config);
assertThat(stored.get("got", String.class), is("user_key.value"));
}
Expand All @@ -146,7 +146,7 @@ public void verifyUserGrantedButOperatorFilteredAccessRejected()
exception.expect(SecretAccessFilteredException.class);
exception.expectMessage(containsString("kkk"));

Config config = newConfig().set("get", "kkk").set("_secret", newConfig().set("kkk", true));
Config config = newConfig().set("get", "kkk").set("_secrets", newConfig().set("kkk", true));
run("secret_access", config);
}

Expand Down

0 comments on commit 584e75a

Please sign in to comment.