-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Make reserved built-in roles queryable #117581
Make reserved built-in roles queryable #117581
Conversation
this.reservedRolesSupplier = CachedSupplier.wrap(() -> { | ||
final Collection<RoleDescriptor> roleDescriptors = ReservedRolesStore.roleDescriptors(); | ||
return new QueryableBuiltInRoles( | ||
roleDescriptors.stream().collect(Collectors.toMap(RoleDescriptor::getName, QueryableBuiltInRolesUtils::calculateHash)), |
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.
Calculating hash per role, so we can optimize the update and only update the roles whose definition have actually changed.
final String roleDigest = roles.rolesDigest().get(role.getName()); | ||
if (indexedRolesDigests == null || indexedRolesDigests.containsKey(role.getName()) == false) { | ||
rolesToUpsert.add(role); | ||
} else if (indexedRolesDigests.get(role.getName()).equals(roleDigest) == 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.
We want to update only the roles whose definition (digest) changed. On average this will be one or two roles per release.
…able-built-in-roles
…able-built-in-roles
…able-built-in-roles
|
||
@Override | ||
public void addListener(QueryableBuiltInRoles.Listener listener) { | ||
// no-op: reserved roles are static and do not change |
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 reserved roles do not change dynamically.
@@ -189,9 +190,9 @@ public RoleDescriptor( | |||
this.indicesPrivileges = indicesPrivileges != null ? indicesPrivileges : IndicesPrivileges.NONE; | |||
this.applicationPrivileges = applicationPrivileges != null ? applicationPrivileges : ApplicationResourcePrivileges.NONE; | |||
this.runAs = runAs != null ? runAs : Strings.EMPTY_ARRAY; | |||
this.metadata = metadata != null ? Collections.unmodifiableMap(metadata) : Collections.emptyMap(); | |||
this.metadata = metadata != null ? Collections.unmodifiableMap(new TreeMap<>(metadata)) : Collections.emptyMap(); |
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.
Using ordered TreeMap
here to produce consistent hash.
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.
Good catch! Just in case you've considered the same thing: instead of imposing this constraint here, I wonder if it makes sense to confine it to QueryableBuiltInRolesUtils
by copying the role descriptor. That would avoid data copying on each role descriptor creation. Just a thought though, good to leave as is.
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 was thinking of a similar(ish) approach: serialize to JSON, flatten all properties, sort them and then hash strings. This way I would avoid imposing any order in the constructor or during parsing of roles, but rather during hashing. Also, we would avoid copying role descriptors (not really true as we would copy a json map) and sorting would be confined in QueryableBuiltInRolesUtils
. LMWYT
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 could look something like this:
public static String calculateHash(final RoleDescriptor roleDescriptor) {
final MessageDigest hash = MessageDigests.sha256();
try (XContentBuilder jsonBuilder = XContentFactory.jsonBuilder()) {
roleDescriptor.toXContent(jsonBuilder, EMPTY_PARAMS);
final Map<String, Object> flattenMap = Maps.flatten(
XContentHelper.convertToMap(BytesReference.bytes(jsonBuilder), /*ordered*/ true, XContentType.JSON).v2(),
true, // flattenArrays
true // ordered
);
hash.update(flattenMap.toString().getBytes(StandardCharsets.UTF_8));
} catch (IOException e) {
throw new IllegalStateException("failed to compute role digest of [" + roleDescriptor.getName() +"] role", e);
}
// HEX vs Base64 encoding is a trade-off between readability and space efficiency
// opting for Base64 here to reduce the size of the cluster state
return Base64.getEncoder().encodeToString(hash.digest());
}
Edit: I pushed the change 70181d6. I think this is a better approach as it should be future proof in case we add new map properties.
...y/src/main/java/org/elasticsearch/xpack/security/authz/store/QueryableBuiltInRolesStore.java
Outdated
Show resolved
Hide resolved
...urity/src/main/java/org/elasticsearch/xpack/security/support/FeatureNotEnabledException.java
Show resolved
Hide resolved
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; | ||
import org.elasticsearch.xpack.security.authz.store.FileRolesStore; | ||
|
||
public interface QueryableBuiltInRolesProviderFactory { |
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.
Need to go with the factory approach in order to be able to inject different QueryableBuiltInRoles.Provider
implementations.
@@ -52,7 +50,7 @@ protected void doExecute(Task task, final GetRolesRequest request, final ActionL | |||
} | |||
|
|||
final Set<String> rolesToSearchFor = new HashSet<>(); | |||
final List<RoleDescriptor> reservedRoles = new ArrayList<>(); |
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.
Using a set here, since list caused duplicate results to be returned. I would propose we keep this workaround, until we enable this feature by default. Later we can remove the logic of combining static and native roles here, and always return roles from the .security index.
Makes total sense to have one. I'll prefer to add it in a followup PR, as this PR is already getting too big to backport. |
…able-built-in-roles-tracked
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.
LGTM -- great work on this. Few more comments/suggestions but no need for a re-review.
...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java
Outdated
Show resolved
Hide resolved
...urity/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesUtils.java
Show resolved
Hide resolved
...y-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryableReservedRolesIT.java
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesUtilsTests.java
Show resolved
Hide resolved
💔 Backport failed
You can use sqren/backport to manually backport by running |
This PR makes reserved built-in roles queryable via Query Role API by indexing them into the
.security
index.Currently, the built-in roles were only available via Get Role API.
The built-in roles are synced into the
.security
index on cluster recovery. The.security
index will be created (if it's not existing) before built-in roles are synced. In order to avoid concurrent updates, the built-in roles will only be synced by a master node.Once the built-in roles are synced, the information about indexed roles is kept in the cluster state as part of the
.security
index's metadata. The map containing role names and their digests is persisted as part ofqueryable_built_in_roles_digest
property:Important: The reserved roles stored in the
.security
index are only intended to be used for querying and retrieving. The role resolution and mapping during authentication will remain the same and give a priority to static/file role definitions. This is ensured by the order in which role providers (built-in, file and native) are invoked. It’s important to note this because there can be a short period of time where we have a temporary inconsistency between actual built-in role definitions and what is stored in the.security
index.Note: The functionality is temporarily hidden behind the
es.queryable_built_in_roles_enabled
system property. By default, the flag is disabled and will become enabled in a followup PR. The reason for this is to keep this PR as small as possible and to avoid the need to adjust a large number of tests that don't expect.security
index to exist.Testing:
To run and test locally execute
./gradlew run -Dtests.jvm.argline="-Des.queryable_built_in_roles_enabled=true"
.To query all reserved built-in roles execute: