Skip to content

Make reserved built-in roles queryable #117581

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

Merged

Conversation

slobodanadamovic
Copy link
Contributor

@slobodanadamovic slobodanadamovic commented Nov 26, 2024

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 of queryable_built_in_roles_digest property:

GET /_cluster/state/metadata/.security
"queryable_built_in_roles_digest": {
   "superuser": "lRRmA3kPO1/ztr3ESAlTetOuDjgUC3fKcGS3ZCqM+6k=",
    ...
}

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:

POST /_security/_query/role
{
  "query": {
    "bool": {
      "must": {
        "term": {
          "metadata._reserved": true
        }
      }
    }
  }
}

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Nov 28, 2024
this.reservedRolesSupplier = CachedSupplier.wrap(() -> {
final Collection<RoleDescriptor> roleDescriptors = ReservedRolesStore.roleDescriptors();
return new QueryableBuiltInRoles(
roleDescriptors.stream().collect(Collectors.toMap(RoleDescriptor::getName, QueryableBuiltInRolesUtils::calculateHash)),
Copy link
Contributor Author

@slobodanadamovic slobodanadamovic Nov 29, 2024

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) {
Copy link
Contributor Author

@slobodanadamovic slobodanadamovic Nov 29, 2024

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.


@Override
public void addListener(QueryableBuiltInRoles.Listener listener) {
// no-op: reserved roles are static and do not change
Copy link
Contributor Author

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();
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@slobodanadamovic slobodanadamovic Dec 10, 2024

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

Copy link
Contributor Author

@slobodanadamovic slobodanadamovic Dec 10, 2024

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.

import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
import org.elasticsearch.xpack.security.authz.store.FileRolesStore;

public interface QueryableBuiltInRolesProviderFactory {
Copy link
Contributor Author

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.

Copy link
Contributor

@n1v0lg n1v0lg left a 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.

@slobodanadamovic slobodanadamovic added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 16, 2024
@elasticsearchmachine elasticsearchmachine merged commit bf1c0fe into elastic:main Dec 16, 2024
21 checks passed
@slobodanadamovic slobodanadamovic deleted the sa-queryable-built-in-roles branch December 16, 2024 18:15
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 117581

slobodanadamovic added a commit that referenced this pull request Jan 27, 2025
Making the `es.queryable_built_in_roles_enabled` feature flag enabled by default.
This feature makes the built-in roles automatically indexed in `.security` index and available
for querying via Query Role API. The consequence of this is that `.security` index is now 
created eagerly (if it's not existing) on cluster formation.

In order to keep the scope of this PR small, the feature is disabled for some of the tests, 
because they are either non-trivial to adjust or the gain is not worthy the effort to do it now. 
The tests will be adjusted in a follow-up PR and later the flag will be removed completely.

Relates to #117581
slobodanadamovic added a commit to slobodanadamovic/elasticsearch that referenced this pull request Jan 27, 2025
Making the `es.queryable_built_in_roles_enabled` feature flag enabled by default.
This feature makes the built-in roles automatically indexed in `.security` index and available
for querying via Query Role API. The consequence of this is that `.security` index is now
created eagerly (if it's not existing) on cluster formation.

In order to keep the scope of this PR small, the feature is disabled for some of the tests,
because they are either non-trivial to adjust or the gain is not worthy the effort to do it now.
The tests will be adjusted in a follow-up PR and later the flag will be removed completely.

Relates to elastic#117581

(cherry picked from commit 52e0f21)

# Conflicts:
#	modules/dot-prefix-validation/build.gradle
#	test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java
#	x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmElasticAutoconfigIntegTests.java
elasticsearchmachine pushed a commit that referenced this pull request Jan 27, 2025
…120886)

* Enable queryable built-in roles feature by default (#120323)

Making the `es.queryable_built_in_roles_enabled` feature flag enabled by default.
This feature makes the built-in roles automatically indexed in `.security` index and available
for querying via Query Role API. The consequence of this is that `.security` index is now
created eagerly (if it's not existing) on cluster formation.

In order to keep the scope of this PR small, the feature is disabled for some of the tests,
because they are either non-trivial to adjust or the gain is not worthy the effort to do it now.
The tests will be adjusted in a follow-up PR and later the flag will be removed completely.

Relates to #117581

(cherry picked from commit 52e0f21)

# Conflicts:
#	modules/dot-prefix-validation/build.gradle
#	test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java
#	x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/ReservedRealmElasticAutoconfigIntegTests.java

* Update InternalTestCluster.java

remove line snuck after resolving merge confilcs

* Update build.gradle

fix build.gradle

* Update build.gradle

fix build.gradle by removing invalid task

* remove non-existing timeout parameter on 8.x branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC serverless-linked Added by automation, don't add manually Team:Security Meta label for security team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants