RCS 2.0 - add remote indices privileges to role models #90614

n1v0lg · 2022-10-03T14:36:30Z

This PR updates role-related data models to allow specifying remote
index privileges.

The change is only concerned with allowing end-users to specify a
remote cluster target via the API (i.e., not files), and to exclude
remote index privileges from local authorization. Fetching the relevant
remote index privileges and serializing them for cross cluster requests
will come in a separate PR.

The changes in this PR support specifying remote_indices in the Roles
API:

POST /_security/role/remote-search
{
  "indices": [ // unchanged
    {
      "names": ["*index*"],
      "privileges": ["read"] 
    }
  ],
  "remote_indices": [
    {
      "names": ["*index*"],
      "privileges": ["read"],
      "clusters": ["remote-a", "remote-b-*"] 
    }
  ]
}

A key decision in this PR is separating RemoteIndicesPermission
entirely from IndicesPermission within Role and the implementing
classes. The primary reason for this is that RemoteIndicesPermission
and IndicesPermission represent fairly different concepts, in terms
of business logic. This approach also makes it easier to ensure that
remote indices permissions do not impact local authorization, since
remote index permissions are handled as a separate category altogether.
The alternative would have been extending IndicesPermission with
remote clusters but that would require to "ad-hoc" exclude them from
performing local authorization actions and make this change more
invasive and complex.

To keep the scope of the PR down, I will address below items in follow up PRs:

Validation logic to verify that a given remote cluster is configured
Support remote_indices for (QC-side, normal) API keys (see my inline comment in ApiKeyService)
Support remote_indices for build-in roles and users
Support file-based definitions of roles with remote indices
Show remote_indices in GetUserPrivileges response
(Potentially) gracefully handle (or fail) HasPrivileges

n1v0lg · 2022-10-18T08:29:58Z

@justincr-elastic regarding:

Should all of the modified main classes check the feature flag? For example, PutRoleRequest, RoleDescriptor, IndicesPermissions, Role. LimitedRole, etc.

I don't think so. We want to enforce the feature flag "at the boundaries" such that end-users can't specify remote indices via our APIs w/o the feature flag (or set RCS2.0-specific settings etc.). So long as we enforce the feature flag correctly at the boundaries, this guards internal classes such as IndicesPermissions and allows us to limit the number of feature flag checks and the complexity of the overall change.

In this PR, the logic in RoleDescriptor.parse enforces that remote_indices can only be specified if the feature flag is enabled. parse is in turn used under the hood by e.g., PutRoleRequestBuilder which protects the Put Role API, and likewise for API keys. With this check, a RoleDescriptor will never have remote indices if the the feature flag is false, and likewise Role, IndicesPermissions, etc won't either.

The other place it's critical to enforce the feature flag in this PR is inside SecuritySystemIndices to only apply the index mapping if the flag is set.

I've also added stop gap measure for built-in roles and file-based roles.

n1v0lg · 2022-10-18T11:40:23Z

@elasticsearchmachine run elasticsearch-ci/release-tests

justincr-elastic · 2022-10-18T16:41:09Z

I think we should prevent roles with remote indices to be created/updated in a mixed version cluster.

Do you mean mixed QC vs FC, or multiple mixed FC versions? Doing that in a QC connected to two mixed FCs is complicated.

QC: 8.LATEST
FC1: 8.LATEST (RCS 2.0 depends on QC cluster.remote.FC1.authorization present or absent)
FC2: 7.17.0 (RCS 1.0 only)

The QC decides to use RCS 2.0 on a per-FC basis. We would need to check the QC setting, and finish two transport handshakes to each FC. At that point, we have enough info to allow a remote index privilege for FC1, and prevent for FC2. And that assumes no changes to the QC setting or FC1 version later on.

n1v0lg · 2022-10-19T08:00:42Z

@justincr-elastic

Do you mean mixed QC vs FC, or multiple mixed FC versions? Doing that in a QC connected to two mixed FCs is complicated.

The issue @ywangd raised is a querying cluster with mixed-version nodes. For example, suppose you have node A and B where A is on 8.6 and B is on 8.5. Node A saves a role descriptor with remote index privileges in the security index. Later, node B attempts to read it and fails because it can't deserialize remote_indices. I've already addressed this by checking the min version and throwing if it's below 8.6 here.

jakelandis

I didn't quite all the through the review (I should be able to finish tomorrow) but looking good so far. A couple minor comments.

jakelandis · 2022-10-19T20:38:04Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java

    }

-    private static RoleDescriptor.IndicesPrivileges parseIndex(String roleName, XContentParser parser, boolean allow2xFormat)
+    private static IndicesPrivileges parseIndex(final String roleName, final XContentParser parser, final boolean allow2xFormat)


Not so much related to this PR, but .... do we need to preserve allow2xFormat in these method signatures ? It is not very descriptive to what this does or how/why you would set it true/false. For this PR is it possible to just hard code true/false and omit from the method signatures ? (if not that's fine , just trying to prevent carrying forward unnecessary cruft if this is indeed cruft)

Unfortunately no clean way to omit it for parseIndex but I did omit it for parseRemoteIndex as per @ywangd's suggestion in a separate comment.

Just for context, allow2xFormat is there because we used a different format for field-level security definitions in the past. Most callers of the parse method do not allow legacy format, but FileRolesStore does since we don't want to fail parsing roles defined in files, even when legacy.

We really should remove it and there is an open issue for it #30104 (and 2 old PRs ...)
But it does not have to be part of this PR. Something for tech debt week? :)

Yup, I still haven't signed up for any issues other than test failures for tech debt week. I'll pick this one up (and create a Jira issue if needed)

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java

jakelandis · 2022-10-19T21:09:52Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

-            innerPutRole(request, role, listener);
-        }
+        } else if (role.hasRemoteIndicesPrivileges()
+            && clusterService.state().nodes().getMinNodeVersion().before(RoleDescriptor.VERSION_REMOTE_INDICES)) {


just double checking that this in only ever called from a user's action to add/update a role, right ? (if not then we should be really careful about which workflows throw during mixed clusters)

yup this is only ever called via TransportPutRoleAction which in turn is only ever invoked through PUT role API calls

What workflow do you have in mind? Internally (ILM, ML), I don't think we have anything that mutates security entities. Externally, creating and granting API keys are probably the most used security operations by stack and solutions. API keys will be supporting remote indices, so theorectically it could be an issue. Enterprise search also manages user roles. We should communicate this clearly to other teams, but I don't think it should not affect our decision here.

ywangd

LGTM

The indices privilege collating code is quite verbose. But I think it works for now and we can come back to it later if necessary. Collating has limited value. It helps hasPrivileges but still not enough because it does not actually collate per index name. The same now goes for remote cluster names. But we can look for ways of improvements when we are at it.

ywangd · 2022-10-20T05:27:29Z

...ain/java/org/elasticsearch/xpack/core/security/authz/permission/RemoteIndicesPermission.java

+                        // Deliberately passing EMPTY here since *which* indices are restricted is determined not on the querying cluster
+                        // but rather on the fulfilling cluster
+                        new RestrictedIndices(Automatons.EMPTY),


I am a bit unsure about this. By itself, it won't fully work anyway for hasPrivileges check (assuming that is the intention). Because in IndicesPermission#checkResourcePrivileges, we also perform operations with QC's restrictedIndices. So this method also needs to be changed for hasPrivileges to work. That would be two places for special handling. But I'd prefer having special logic in one place if possible. I don't have any good suggestion right now. Just a point that we can revisit in future PRs.

Agreed that we will need to revisit this. I think when we get to implementing the pre-flight authorization check on the querying cluster it will become clearer what to do about restricted indices here. Will keep your concern in mind. Thanks for raising!

ywangd · 2022-10-20T05:37:13Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java

        private Builder(RoleDescriptor rd, @Nullable FieldPermissionsCache fieldPermissionsCache, RestrictedIndices restrictedIndices) {
+            // TODO handle this when we introduce remote index privileges for built-in users and roles. That's the only production code
+            // using this builder
+            assert false == rd.hasRemoteIndicesPrivileges();


This constructor works totally different from the other one and it always confuses me. I think it is worthwhile to split this out to be its own Builder when time comes.

I can fold this into a preamble PR for when I work on dealing with remote indices for built-in roles. I'll collect this and other RCS 2.0-tangential refactor opportunities under a new Jira issue.

...ore/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java

ywangd · 2022-10-20T08:21:57Z

...ugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorTests.java

+            IllegalArgumentException.class,
+            () -> RoleDescriptor.RemoteIndicesPrivileges.builder().indices("idx").privileges("priv").build()
+        );
+        assertThat(ex.getMessage(), containsString("[clusters] must refer to at least one cluster alias or cluster alias pattern"));


Should the error message be a bit more specific to about which clusters field this is? e.g. the [cluster] field under remote_indices must ...

Tweaked to "the [remote_indices] sub-field [clusters] must refer to at least one cluster alias or cluster alias pattern"

ywangd · 2022-10-20T08:35:16Z

x-pack/plugin/core/build.gradle

+if (BuildParams.isSnapshotBuild() == false) {
+  tasks.named("test").configure {
+    systemProperty 'es.untrusted_remote_cluster_feature_flag_registered', 'true'


I think this does not work when running tests in IDE? Is it not a problem? For user profiles, we used to configure the feature flag system property explicitly in test setup. Not advocating it as the right way to do things. But do we need a way to test in IDE? Running through Gradle for unit tests is slower.

For me this works from IntelliJ, for a test that requires the flag to be set:

:x-pack:plugin:core:test --tests "org.elasticsearch.xpack.core.security.authz.RoleDescriptorTests.testParse" -Dbuild.snapshot=false -Dlicense.key="x-pack/license-tools/src/test/resources/public.key"

ywangd · 2022-10-20T08:48:44Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

+            if (isExplicitDenial) {
+                return;
+            }
            for (final IndicesPrivileges indicesPrivilege : indicesPrivileges) {


These variable names and class name are very much confusing. No need to tackle it in this PR. But a good tech debt item.

Agreed, the plural is unfortunate. No good alternative comes to mind off the top of my head though, so future tech debt item it is.

ywangd · 2022-10-20T09:05:21Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

-            innerPutRole(request, role, listener);
-        }
+        } else if (role.hasRemoteIndicesPrivileges()
+            && clusterService.state().nodes().getMinNodeVersion().before(RoleDescriptor.VERSION_REMOTE_INDICES)) {


What workflow do you have in mind? Internally (ILM, ML), I don't think we have anything that mutates security entities. Externally, creating and granting API keys are probably the most used security operations by stack and solutions. API keys will be supporting remote indices, so theorectically it could be an issue. Enterprise search also manages user roles. We should communicate this clearly to other teams, but I don't think it should not affect our decision here.

...ity/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java

ywangd · 2022-10-20T09:28:13Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java

    }

-    private static RoleDescriptor.IndicesPrivileges parseIndex(String roleName, XContentParser parser, boolean allow2xFormat)
+    private static IndicesPrivileges parseIndex(final String roleName, final XContentParser parser, final boolean allow2xFormat)


We really should remove it and there is an open issue for it #30104 (and 2 old PRs ...)
But it does not have to be part of this PR. Something for tech debt week? :)

n1v0lg · 2022-10-20T13:29:18Z

@elasticsearchmachine run elasticsearch-ci/part-1-fips

n1v0lg · 2022-10-20T14:37:44Z

@elasticsearchmachine run elasticsearch-ci/part-1

justincr-elastic

LGTM

justincr-elastic

LGTM

This PR refactors functionality for building a role from a single role descriptor. This used to be handled by a specialized constructor of the Role.Builder class that accessed internal fields. This PR consolidates this logic into a static method that uses the builder's canonical methods instead. The method is used to construct the roles of internal users, from static role descriptors, as well as in test code. Relates: #90614 (comment)

This PR enables support for defining remote_indices for API keys in order to be able to use them to authenticate cross cluster calls. The main change is made to the role classes (SimpleRole and LimitedRole), which now support constructing RoleDescriptorsIntersection on the querying cluster side. This RoleDescriptorsIntersection is sent to the fulfilling cluster as part of the RemoteAccessAuthentication. Relates to #90614

n1v0lg added 29 commits September 28, 2022 17:53

WIP include target clusters in indices privileges

080e32c

Update security index mapping

904d574

Skip remote privs in local role building

fd5c23e

Hacky remote indices building

c3bfb72

Some clean up

ef1c9c0

Better name

31934df

WIP cleanup

f80249e

Inline for now

ac2c10a

Tweaks

0706531

More tests

de5279a

Merge branch 'main' into rcs-2-0/specify-target-clusters

7e01513

Target -> remote

0cf92eb

TODO

7fa220f

RestIT

3c8d686

Get user privileges

90b9a88

Deal with null

fc5d245

Merge branch 'main' into rcs-2-0/specify-target-clusters

5b3cc75

toString

d3f9d76

Fix test

7edc6c6

More clean up

9198cb8

More tests

4d54035

More tweaks

8f310a0

Test merging

c0973f8

Clean up indices building

22ccb13

Merge branch 'main' into rcs-2-0/specify-target-clusters

db346b7

WIP remote indices permission

ff34459

Fix format

b6dca1d

Checkstyle

72ca34f

Merge branch 'main' into rcs-2-0/specify-target-clusters

9fde05f

n1v0lg added the >non-issue label Oct 3, 2022

n1v0lg added 2 commits October 18, 2022 12:53

Review feedback

a669129

Merge branch 'main' into rcs-2-0/specify-target-clusters

f19c05a

n1v0lg added 5 commits October 18, 2022 14:54

Merge branch 'main' into rcs-2-0/specify-target-clusters

a899155

Use record instead of tuple

5eadf39

Merge branch 'main' into rcs-2-0/specify-target-clusters

faa502e

Merge branch 'main' into rcs-2-0/specify-target-clusters

f3ef216

Use remote indices version more

a78b81b

n1v0lg requested a review from justincr-elastic October 18, 2022 14:13

n1v0lg added 2 commits October 19, 2022 10:15

Re-use constant

ad32cfb

Merge branch 'main' into rcs-2-0/specify-target-clusters

25bf133

jakelandis reviewed Oct 19, 2022

View reviewed changes

Merge branch 'main' into rcs-2-0/specify-target-clusters

cfb75af

ywangd approved these changes Oct 20, 2022

View reviewed changes

Address review feedback

c3aa7ac

justincr-elastic reviewed Oct 23, 2022

View reviewed changes

justincr-elastic approved these changes Oct 23, 2022

View reviewed changes

Merge branch 'main' into rcs-2-0/specify-target-clusters

979e251

n1v0lg merged commit 0456868 into elastic:main Oct 24, 2022

n1v0lg mentioned this pull request Oct 25, 2022

Refactor building role from single role descriptor #91107

Merged

slobodanadamovic mentioned this pull request Feb 2, 2023

Support API keys as authentication subjects for RCS 2.0 #93414

Merged

n1v0lg deleted the rcs-2-0/specify-target-clusters branch August 31, 2023 09:09

RCS 2.0 - add remote indices privileges to role models #90614

RCS 2.0 - add remote indices privileges to role models #90614

Uh oh!

Conversation

n1v0lg commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1v0lg commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1v0lg commented Oct 18, 2022

Uh oh!

justincr-elastic commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1v0lg commented Oct 19, 2022

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1v0lg Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1v0lg Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1v0lg Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1v0lg commented Oct 20, 2022

Uh oh!

n1v0lg commented Oct 20, 2022

Uh oh!

justincr-elastic left a comment

Choose a reason for hiding this comment

Uh oh!

justincr-elastic left a comment

Choose a reason for hiding this comment

n1v0lg commented Oct 3, 2022 •

edited

Loading

n1v0lg commented Oct 18, 2022 •

edited

Loading

justincr-elastic commented Oct 18, 2022 •

edited

Loading

n1v0lg Oct 20, 2022 •

edited

Loading

n1v0lg Oct 20, 2022 •

edited

Loading

n1v0lg Oct 20, 2022 •

edited

Loading