Fix user secret filter of Operator #439
Conversation
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.
frsyuki
force-pushed
the
user-secret-fix
branch
from
584e75a
to
7e25ff6
Compare
| if (!operatorSecretFilter.test(key, 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()) { |
There was a problem hiding this comment.
We don't take care of Integer?
There was a problem hiding this comment.
I think we don't have to because previous code also doesn't.
|
|
||
| class DefaultSecretProvider | ||
| implements SecretProvider | ||
| { | ||
| static interface OperatorSecretFilter |
| // 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) |
There was a problem hiding this comment.
1-a and 2-a is actually different in terms of existence of user grant. Could you add a bit detailed explanations?
| private static class OperatorSecretFilter | ||
| implements Predicate<String> | ||
| private static class AgentOperatorSecretFilter | ||
| implements DefaultSecretProvider.OperatorSecretFilter |
There was a problem hiding this comment.
Does this class need to have allowUserSecretAccess method? It seems there is no actual usage of this class without calling the method. If we can remove the method, this class can be simpler like this:
private static class AgentOperatorSecretFilter
implements DefaultSecretProvider.OperatorSecretFilter
{
private final Set<String> predeclaredSecretKeys;
private final Predicate<String> dynamicFilter;
AgentOperatorSecretFilter(OperatorFactory factory, Operator operator)
{
this.predeclaredSecretKeys = ImmutableSet.copyOf(
factory.getSecretAccessList().getSecretKeys());
this.dynamicFilter = (key) -> operator.testUserSecretAccess(key);
}
@Override
public boolean test(String key, boolean userGranted)
{
if (userGranted || predeclaredSecretKeys.contains(key)) {
return dynamicFilter.test(key);
}
return false;
}
}
There was a problem hiding this comment.
it is not possible because an Operator instance is not available until OperatorManager calls OperatorFactory.newOperator(OperatorContext) but a DefaultSecretProvider.OperatorSecretFilter instance is required by OperatorFactory.newOperator(OperatorContext) because OperatorContext contains a SecretProvider instance that contains OperatorSecretFilter.
There was a problem hiding this comment.
Ah, I see. I overlooked it...
There was a problem hiding this comment.
I updated the comment at cc0ddd0.
|
LGTM 👍 But maybe we'd better get feedbacks from @danielnorberg if possible |
frsyuki
force-pushed
the
user-secret-fix
branch
from
cc0ddd0
to
e6e4365
Compare
|
👍 |
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.