-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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.
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
WIldcard IndicesPermissions don't cover .security #36765
WIldcard IndicesPermissions don't cover .security #36765
Conversation
Pinging @elastic/es-security |
public static List<String> indexNames() { | ||
return Collections.unmodifiableList(Arrays.asList(SECURITY_INDEX_NAME, INTERNAL_SECURITY_INDEX)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are responsible for most of the changes. Security index names had to be moved from x-pack:plugin:security
to x-pack:plugin:core
. I have created org.elasticsearch.xpack.core.security.index.SystemIndicesNames
.
} | ||
return indexMatcher(indices); | ||
}; | ||
this.systemIndicesAutomaton = Automatons.patterns(SystemIndicesNames.indexNames()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not make this a static final
instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had moved it as a class member 👍
At some point I wanted to guard all Automaton
s construction with the failure handler with the verbose exception message but gave up and forgot to change this.
} | ||
} | ||
try { | ||
final Automaton exactMatchAutomaton = Automatons.patterns(exactMatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this drops the exact match optimization. Why do we do it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need it for testIndexMatch
that is needed for "has privilege". In "has privilege" the user can check an index pattern (not only a concrete index as in the authorization use case), and we test this with the automaton subset operation. However, the exact optimization only works for predicates. The code for this has been copied from TransportHasPrivilegesAction
and I agree it amounts to a great deal of complexity in this class.
Yet, I think it's the right approach to have the optimization for the "authorize" use case, and a cache-able automaton for the "has privilege" use case (even though the optimization partly aims to bypass automaton generation in the first place). "has privilege" should be rare, and we cannot avoid automatons in this case.
@@ -148,6 +187,24 @@ public Automaton allowedActionsMatcher(String index) { | |||
return automatonList.isEmpty() ? Automatons.EMPTY : Automatons.unionAndMinimize(automatonList); | |||
} | |||
|
|||
public boolean testIndexMatch(String checkIndex, String checkPrivilegeName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add javadocs explaining what this is doing including why we cache groups to automatons
@@ -40,34 +44,30 @@ | |||
* A permission that is based on privileges for index related actions executed | |||
* on specific indices | |||
*/ | |||
public final class IndicesPermission implements Iterable<IndicesPermission.Group> { | |||
public final class IndicesPermission { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groups()
is sufficient to iterate over the groups (and that is preferable because it expresses ordering that is required in tests)
|
||
public static final IndicesPermission NONE = new IndicesPermission(); | ||
private static final Automaton systemIndicesAutomaton = Automatons.patterns(SystemIndicesNames.indexNames()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check we're fine with building Automatons at class initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with it since this is not going to be a big one
|
||
private final ConcurrentMap<String, Predicate<String>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>(); | ||
private final ConcurrentMap<Group, Automaton> indexGroupAutomatonCache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed for the "has privilege" API. Otherwise we would avoid dealing with automatons in favor of dealing with predicates only (needed for the authorize use case). Since has privilege is called less frequently we built predicates lazily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid another cache. We already have caching at the automata level. Maybe we could use this type of class within the group to implement the subset type functionality:
private static class NameMatcher implements Predicate<String> {
private final Set<String> exactMatch;
private final Set<String> nonExactMatch;
private final Automaton automaton;
private final CharacterRunAutomaton patternMatcher;
private NameMatcher(Set<String> exactMatch, Set<String> nonExactMatch) {
this.exactMatch = exactMatch;
this.nonExactMatch = nonExactMatch;
this.automaton = nonExactMatch.isEmpty() ? Automatons.EMPTY : Automatons.patterns(nonExactMatch);
this.patternMatcher = Automatons.runAutomaton(automaton);
}
@Override
public boolean test(String toTest) {
return exactMatch.contains(toTest) || patternMatcher.run(toTest);
}
public boolean isSupersetOf(String value) {
if (IndicesPermission.isIndexPattern(value)) {
final Automaton valueAutomaton = Automatons.patterns(value);
if (Operations.subsetOf(valueAutomaton, automaton)) {
return true;
} else if (exactMatch.isEmpty() == false) {
// slow path
Automaton encompassingAutomaton =
Automatons.unionAndMinimize(Arrays.asList(automaton, Automatons.patterns(exactMatch)));
return Operations.subsetOf(valueAutomaton, encompassingAutomaton);
}
return false;
} else {
return test(value);
}
}
@Override
public String toString() {
return "NameMatcher for " + Stream.concat(exactMatch.stream(), nonExactMatch.stream()).collect(Collectors.joining("|"));
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with not caching the complete automaton and only building the complete one in specific circumstances. I assume has_privilege
is rare call. Alternatively, we could have a LazyInitializable
member inside the Group
(this is still caching, but not exposed). I would however put this all inside the Group
and not create another class.
What do you say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about this as much as the 2 of you, so I might be missing something, but under what circumstances would an exactMatch
index pattern, ever return true for a wildcard has-privileges pattern?
all
: index-001
would never imply read
: index-*
I think this proposed NameMatcher
can be smart enough to implement this strategy on isSupersetOf
- if
value
in a plain string returntest(value)
- else return
Operations.subsetOf(Automatons.patterns(value), this.automaton)
with whatever checks we need for restricted_indices
} else { | ||
return false; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The predicate is splayed out like this to accommodate for the logging statement.
@@ -20,7 +20,7 @@ | |||
public static final Role ROLE = Role.builder(new RoleDescriptor(ROLE_NAME, new String[] { "all" }, | |||
new RoleDescriptor.IndicesPrivileges[] { | |||
RoleDescriptor.IndicesPrivileges.builder().indices("/@&~(\\.security.*)/").privileges("all").build(), | |||
RoleDescriptor.IndicesPrivileges.builder().indices(IndexAuditTrailField.INDEX_NAME_PREFIX + "-*") | |||
RoleDescriptor.IndicesPrivileges.builder().indices(SystemIndicesNames.AUDIT_INDEX_NAME_PREFIX + "-*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify this when we remove index audit.
private static boolean testIndex(Automaton checkIndex, Automaton roleIndex) { | ||
return Operations.subsetOf(checkIndex, roleIndex); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moved to IndicesPermission
ff1de91
to
aadd062
Compare
putTransientIfNonExisting(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, indicesAccessControl); | ||
} else { | ||
throw denial(auditId, authentication, action, request, permission.names()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been moved inside IndicesPermission.Group#check
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes needed to support this feel like we are hacking this in. I'll try to think of something else; maybe @tvernum could also provide thoughts.
private final Function<String, Predicate<String>> loadingFunction; | ||
|
||
private final ConcurrentHashMap<String, Predicate<String>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>(); | ||
private static final Logger logger = LogManager.getLogger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the class to the getLogger call?
|
||
public static final IndicesPermission NONE = new IndicesPermission(); | ||
private static final Automaton systemIndicesAutomaton = Automatons.patterns(SystemIndicesNames.indexNames()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with it since this is not going to be a big one
|
||
private final ConcurrentMap<String, Predicate<String>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>(); | ||
private final ConcurrentMap<Group, Automaton> indexGroupAutomatonCache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid another cache. We already have caching at the automata level. Maybe we could use this type of class within the group to implement the subset type functionality:
private static class NameMatcher implements Predicate<String> {
private final Set<String> exactMatch;
private final Set<String> nonExactMatch;
private final Automaton automaton;
private final CharacterRunAutomaton patternMatcher;
private NameMatcher(Set<String> exactMatch, Set<String> nonExactMatch) {
this.exactMatch = exactMatch;
this.nonExactMatch = nonExactMatch;
this.automaton = nonExactMatch.isEmpty() ? Automatons.EMPTY : Automatons.patterns(nonExactMatch);
this.patternMatcher = Automatons.runAutomaton(automaton);
}
@Override
public boolean test(String toTest) {
return exactMatch.contains(toTest) || patternMatcher.run(toTest);
}
public boolean isSupersetOf(String value) {
if (IndicesPermission.isIndexPattern(value)) {
final Automaton valueAutomaton = Automatons.patterns(value);
if (Operations.subsetOf(valueAutomaton, automaton)) {
return true;
} else if (exactMatch.isEmpty() == false) {
// slow path
Automaton encompassingAutomaton =
Automatons.unionAndMinimize(Arrays.asList(automaton, Automatons.patterns(exactMatch)));
return Operations.subsetOf(valueAutomaton, encompassingAutomaton);
}
return false;
} else {
return test(value);
}
}
@Override
public String toString() {
return "NameMatcher for " + Stream.concat(exactMatch.stream(), nonExactMatch.stream()).collect(Collectors.joining("|"));
}
}
final Predicate<String> indicesPatternsPredicate = indicesPatternsPredicate(Arrays.asList(indices)); | ||
this.implicitlyAuthorizeMonitorSystemIndices = (action, index) -> { | ||
return SystemIndicesNames.indexNames().contains(index) && IndexPrivilege.MONITOR.predicate().test(action) | ||
&& indicesPatternsPredicate.test(index) && privilege.predicate().test(action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I understand correctly, we need the indicesPatternsPredicate to account for the fact that the indexNameMatcher will not grant access to the security indices if they were declared as a wildcard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
This replaces the
if (!indicesAccessControl.isGranted()) {
throw denial(auditId, authentication, action, request, permission.names());
} else if (hasSecurityIndexAccess(indicesAccessControl)
&& MONITOR_INDEX_PREDICATE.test(action) == false
&& isSuperuser(authentication.getUser()) == false) {
// only the XPackUser is allowed to work with this index, but we should allow indices monitoring actions through for debugging
// purposes. These monitor requests also sometimes resolve indices concretely and then requests them
logger.debug("user [{}] attempted to directly perform [{}] against the security index [{}]",
authentication.getUser().principal(), action, SecurityIndexManager.SECURITY_INDEX_NAME);
throw denial(auditId, authentication, action, request, permission.names());
} else {
block in AuthorizationService.java
.
if (implicitlyAuthorizeMonitorSystemIndices.test(action, index)) { | ||
// we allow indices monitoring actions through for debugging purposes. These monitor requests resolve indices concretely and | ||
// then requests them. WE SHOULD BREAK THIS BEHAVIOR | ||
logger.debug("Granted monitoring passthrough for index [{}] and action [{}]", index, action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider logging the group as well
Thanks for looking again into this @jaymode ! |
I did some brief thinking and came up with the following (unfinished) that I think feels less hacky: git diff
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
index 4936071ee84..04e4298def6 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
@@ -32,6 +32,7 @@ import java.util.SortedMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Function;
import java.util.function.Predicate;
+import java.util.stream.Stream;
import static java.util.Collections.unmodifiableMap;
import static java.util.Collections.unmodifiableSet;
@@ -54,16 +55,19 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
this.groups = groups;
loadingFunction = (action) -> {
List<String> indices = new ArrayList<>();
+ boolean allowRestricted = false;
for (Group group : groups) {
if (group.actionMatcher.test(action)) {
indices.addAll(Arrays.asList(group.indices));
+ allowRestricted |= group.allowRestricted &&
+ Stream.of(".security", ".security-6").anyMatch(group.indexNameMatcher);
}
}
- return indexMatcher(indices);
+ return indexMatcher(indices, allowRestricted);
};
}
- static Predicate<String> indexMatcher(List<String> indices) {
+ static Predicate<String> indexMatcher(List<String> indices, boolean allowRestricted) {
Set<String> exactMatch = new HashSet<>();
List<String> nonExactMatch = new ArrayList<>();
for (String indexPattern : indices) {
@@ -74,6 +78,8 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
}
}
+ // nocommit handle allowRestricted. Maybe add as exact matches and if there is a Automata minus
+ // these values from it?
if (exactMatch.isEmpty() && nonExactMatch.isEmpty()) {
return s -> false;
} else if (exactMatch.isEmpty()) {
@@ -232,6 +238,7 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
private final Predicate<String> actionMatcher;
private final String[] indices;
private final Predicate<String> indexNameMatcher;
+ private final boolean allowRestricted;
public FieldPermissions getFieldPermissions() {
return fieldPermissions;
@@ -245,9 +252,10 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
this.privilege = privilege;
this.actionMatcher = privilege.predicate();
this.indices = indices;
- this.indexNameMatcher = indexMatcher(Arrays.asList(indices));
+ this.indexNameMatcher = indexMatcher(Arrays.asList(indices), false);
this.fieldPermissions = Objects.requireNonNull(fieldPermissions);
this.query = query;
+ this.allowRestricted = false; // nocommit
}
public IndexPrivilege privilege() {
@@ -275,6 +283,10 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
boolean hasQuery() {
return query != null;
}
+
+ boolean allowsRestrictedIndexAccess() {
+ return allowRestricted;
+ }
}
private static class DocumentLevelPermissions { |
test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments.
I think you're still planning to change the Predicate
/ Automaton
setup in IndicesPermission
, so I only did a partial review there. Let me know if I'm mistaken and it's ready for full review.
@@ -600,6 +613,11 @@ public IndicesPrivileges(StreamInput in) throws IOException { | |||
this.deniedFields = in.readOptionalStringArray(); | |||
this.privileges = in.readStringArray(); | |||
this.query = in.readOptionalBytesReference(); | |||
if (in.getVersion().onOrAfter(Version.V_6_7_0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is right for the end-goal when we backport this to 6.7, but it will cause BWC to fail when it is merged to master
as 6.7 won't be including the boolean field in the stream yet.
We typically commit it as V_7_0_0
and then update it to V6_7_0
at time of backport.
// by default certain restricted indices are exempted when granting privileges, as they should generally be hidden for ordinary | ||
// users. Setting this flag eliminates this special status, and any index name pattern in the permission will cover restricted | ||
// indices as well. | ||
private boolean allow_restricted_indices = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be camelCase
not snake_case
|
||
private final ConcurrentMap<String, Predicate<String>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>(); | ||
private final ConcurrentMap<Group, Automaton> indexGroupAutomatonCache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about this as much as the 2 of you, so I might be missing something, but under what circumstances would an exactMatch
index pattern, ever return true for a wildcard has-privileges pattern?
all
: index-001
would never imply read
: index-*
I think this proposed NameMatcher
can be smart enough to implement this strategy on isSupersetOf
- if
value
in a plain string returntest(value)
- else return
Operations.subsetOf(Automatons.patterns(value), this.automaton)
with whatever checks we need for restricted_indices
RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("all").build()}, | ||
RoleDescriptor.IndicesPrivileges.builder().indices("*").privileges("all").build(), | ||
RoleDescriptor.IndicesPrivileges.builder().indices(SystemIndicesNames.indexNames().toArray(new String[0])) | ||
.privileges("all").build() }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have allowRestrictedIndices(true)
I'm going to open a separate PR for the |
I have opened #37577 which supersedes this one. |
This grants the capability to grant privileges over certain restricted indices (.security and .security-6 at the moment). It also removes the special status of the superuser role. IndicesPermission.Group is extended by adding the `allow_restricted_indices` boolean flag. By default the flag is false. When it is toggled, you acknowledge that the indices under the scope of the permission group can cover the restricted indices as well. Otherwise, by default, restricted indices are ignored when granting privileges, thus rendering them hidden for authorization purposes. This effectively adds a confirmation "check-box" for roles that might grant privileges to restricted indices. The "special status" of the superuser role has been removed and coded as any other role: ``` new RoleDescriptor("superuser", new String[] { "all" }, new RoleDescriptor.IndicesPrivileges[] { RoleDescriptor.IndicesPrivileges.builder() .indices("*") .privileges("all") .allowRestrictedIndices(true) // this ----^ .build() }, new RoleDescriptor.ApplicationResourcePrivileges[] { RoleDescriptor.ApplicationResourcePrivileges.builder() .application("*") .privileges("*") .resources("*") .build() }, null, new String[] { "*" }, MetadataUtils.DEFAULT_RESERVED_METADATA, Collections.emptyMap()); ``` In the context of the Backup .security work, this allows the creation of a "curator role" that would permit listing (get settings) for all indices (including the restricted ones). That way the curator role would be able to ist and snapshot all indices, but not read or restore any of them. Supersedes #36765 Relates #34454
This grants the capability to grant privileges over certain restricted indices (.security and .security-6 at the moment). It also removes the special status of the superuser role. IndicesPermission.Group is extended by adding the `allow_restricted_indices` boolean flag. By default the flag is false. When it is toggled, you acknowledge that the indices under the scope of the permission group can cover the restricted indices as well. Otherwise, by default, restricted indices are ignored when granting privileges, thus rendering them hidden for authorization purposes. This effectively adds a confirmation "check-box" for roles that might grant privileges to restricted indices. The "special status" of the superuser role has been removed and coded as any other role: ``` new RoleDescriptor("superuser", new String[] { "all" }, new RoleDescriptor.IndicesPrivileges[] { RoleDescriptor.IndicesPrivileges.builder() .indices("*") .privileges("all") .allowRestrictedIndices(true) // this ----^ .build() }, new RoleDescriptor.ApplicationResourcePrivileges[] { RoleDescriptor.ApplicationResourcePrivileges.builder() .application("*") .privileges("*") .resources("*") .build() }, null, new String[] { "*" }, MetadataUtils.DEFAULT_RESERVED_METADATA, Collections.emptyMap()); ``` In the context of the Backup .security work, this allows the creation of a "curator role" that would permit listing (get settings) for all indices (including the restricted ones). That way the curator role would be able to ist and snapshot all indices, but not read or restore any of them. Supersedes #36765 Relates #34454
.security
and.security-6
have to be named explicitly when definingIndicesPrivileges
.Previously, granting privileges over the
*
index pattern, would have granted privileges over.security
as well. However, even if technically a role had privileges over.security
access to this index would be further restricted to only thesuperuser
role. This PR changes this, such that when a role defines indices privileges over.security
(by listing it explicitly) it will grant access without further limitations.For example, the
superuser
role'sIndicesPrivileges
changed from:to