Updatable API keys - noop check #88346

n1v0lg · 2022-07-07T13:41:04Z

This PR adds a noop check for API key updates. If we detect a noop
update, i.e., an update that does not result in any changes to the
existing doc, we skip the index step and return updated = false in
the response.

This PR also extends test coverage around various corner cases.

n1v0lg · 2022-07-11T09:35:13Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

-        for (final String clusterPrivilege : clusterPrivileges) {
-            putRoleRequest.cluster(clusterPrivilege);
-        }
+        putRoleRequest.cluster(clusterPrivileges);


Fix test to use all cluster privs, not just last.

n1v0lg · 2022-07-11T10:18:50Z

@elasticmachine run elasticsearch-ci/part-2-fips plz. Unrelated failure.

ywangd

I have a few comments.

I appreciate your exhaustive work for the isNoop method.

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

ywangd · 2022-07-12T05:05:23Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

+        assert userRoles != null;
+        final List<RoleDescriptor> currentLimitedByRoleDescriptorRoles = parseRoleDescriptorsBytes(


Nit: The name currentLimitedByRoleDescriptorRoles is a bit awkward. It should just be currentLimitedByRoleDescriptors. Also, userRoles really should be userRoleDescriptors since the code has distinct concepts for role, role name and role descriptor.

Oh whoops currentLimitedByRoleDescriptorRoles is definitely a copy&mishap. Thanks for catching 👍

Also, userRoles really should be userRoleDescriptors since the code has distinct concepts for role, role name and role descriptor.

I'm good with that, I went with userRoles because that's what it was called before the work on updatable API keys elsewhere in this class.

I went with userRoles because that's what it was called before the work on updatable API keys elsewhere in this class.

I figured. I wasn't a fan of this name in other places as well. But we never bothered to change them ...

ywangd · 2022-07-12T05:11:04Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

-        return builder.endObject();
+        return new ApiKeyDocBuilderWithNoopFlag(
+            builder.endObject(),
+            isNoop(currentApiKeyDoc, targetDocVersion, authentication, request, userRoles)
+        );


This works. But I think it can be simplified a bit if:

Perform isNoop check in the beginning of the method. So we can avoid creating and preparing the builder all together if no change.

Remove the ApiKeyDocBuilderWithNoopFlag record type and just return null if there is no change or the actual builder if there are any changes.

Performing the noop check at the beginning of the method is how I had this structured initially, so I'm happy to switch it back to that. My thinking around always building was that it makes the method more generic and gives it simpler semantics: it creates the object and tells the caller if it's a noop -- then it's up to the caller to decide what to do with it.

This is just to give context. I'm good with your suggestion 👍

Thanks for the context. It helps. I think it will at least partially ease your concern if we rename this method to maybeBuildUpdatedDocument to indicate caller needs to perform null check.

ywangd · 2022-07-12T05:34:21Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

+                // Role descriptor corresponding to SecuritySettingsSource.TEST_ROLE_YML to ensure that these stay the same and
+                // don't cause an update
+                Set.of(
+                    new RoleDescriptor(
+                        TEST_ROLE,
+                        new String[] { "ALL" },
+                        new RoleDescriptor.IndicesPrivileges[] {
+                            RoleDescriptor.IndicesPrivileges.builder()
+                                .indices("*")
+                                .allowRestrictedIndices(true)
+                                .privileges("ALL")
+                                .build() },
+                        null
+                    )
+                ),


For this assumption to uphold for future changes, it would be better if we check that it indeed results into a noop update before this check. Otherwise, any future change to the test role will essentially make this test not doing anything.

ywangd · 2022-07-12T05:57:42Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

+        final List<RoleDescriptor> currentLimitedByRoleDescriptorRoles = parseRoleDescriptorsBytes(
+            request.getId(),
+            apiKeyDoc.limitedByRoleDescriptorsBytes,
+            RoleReference.ApiKeyRoleType.LIMITED_BY


This might be OK though the behaviour is not perfect.

The method parseRoleDescriptorsBytes replaces old superuser role definition (7.x) with the new one (8.0). This means it can return a role descriptor that is different from what is actually stored. If we only test against the returned role descriptor, we may miss an update because the equality does not apply to the actual stored descriptor.

In practice this can only happen in a mix version cluster (8.4 + 7.17). If update skips for a superuser limitedByRoleDescriptor. It may not be too big of a deal especially since the legacy descriptor will be handled correctly when loaded (at authentication time).

Alternatively, we can check whether the current doc has version 7.x and the returned a descriptor is a superuser. If answers to both are true, it is not a noop (because we will be updating the legacy superuser descriptor to the new one). This would also means after update, the key will lose the legacy superuser behaviour even when it is used on a 7.17 node. But that is probably OK (and even desired due to consistency). But this is still not always correct because if the key is updated for a second time, the check would suggest an update is needed but it is not true (because it was updated already in the first update).

It is definitely an edge case. I don't really have a strong opinion. But at least it worths some code level comment. What do you think?

Discussed this on Slack:

For this PR, we will simply document the behavior as a comment. We will tweak the behavior in a follow up PR to call parseRoleDescriptorsBytes such that it does not replace superuser role descriptors, effectively allowing legacy descriptors to be updated to 8.x descriptors.

n1v0lg · 2022-07-12T09:51:31Z

@elasticmachine run elasticsearch-ci/part-2

ywangd

LGTM

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

* upstream/master: (38 commits) Simplify map copying (elastic#88432) Make DiffableUtils.diff implementation agnostic (elastic#88403) Ingest: Start separating Metadata from IngestSourceAndMetadata (elastic#88401) Move runtime fields base scripts out of scripting fields api package. (elastic#88488) Enable TRACE Logging for test and increase timeout (elastic#88477) Mute ReactiveStorageIT#testScaleDuringSplitOrClone (elastic#88480) Track the count of failed invocations since last successful policy snapshot (elastic#88398) Avoid noisy exceptions on data nodes when aborting snapshots (elastic#88476) Fix ReactiveStorageDeciderServiceTests testNodeSizeForDataBelowLowWatermark (elastic#88452) INFO logging of snapshot restore and completion (elastic#88257) unmute test (elastic#88454) Updatable API keys - noop check (elastic#88346) Corrected an incomplete sentence. (elastic#86542) Use consistent shard map type in IndexService (elastic#88465) Stop registering TestGeoShapeFieldMapperPlugin in ESIntegTestCase (elastic#88460) TSDB: RollupShardIndexer logging improvements (elastic#88416) Audit API key ID when create or grant API keys (elastic#88456) Bound random negative size test in SearchSourceBuilderTests#testNegativeSizeErrors (elastic#88457) Updatable API keys - logging audit trail event (elastic#88276) Polish reworked LoggedExec task (elastic#88424) ... # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/RollupShardIndexer.java

n1v0lg added 30 commits June 21, 2022 11:56

Service and request draft

3bba543

Update response

3a709a9

Get api key docs for name

fc53cf3

More

74d264c

More

0bc2ec5

Check expiration

648ae52

Integ test

2d43dda

More

4384e56

Assert role descriptors as expected

3d73b99

Clean up role descriptor checks

ff54fec

Test not found

e484923

WIP expected metadata

a9c8caa

Fix metadata

d638a45

Nit

0c819e4

Fix setting metadata

abe677b

Nit

3c7b2c1

WIP use old role descriptors

29f0320

Clean up

eef2b38

More todos

e5b8650

One more

30cfc39

Other users API key not found

b14271a

Nit

cd5d58f

Tweaks

eeca38a

Null bytes

7c3d3f0

Other

f6d1783

Not found test clean up

0e44586

Tweaks

3c83743

Inactive tests

b88cdc8

Expiration test

68eaa91

Friendlier exception message

36bb431

n1v0lg added 3 commits July 11, 2022 10:56

Merge branch 'master' into update-api-keys-noop-check

4a02111

Clean up tests

4944be6

Unused import

370de6e

n1v0lg added :Security/Security Security issues without another label and removed :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jul 11, 2022

Tweak area

d1c400c

n1v0lg commented Jul 11, 2022

View reviewed changes

Typos

534f8e5

n1v0lg marked this pull request as ready for review July 11, 2022 11:20

n1v0lg requested a review from ywangd July 11, 2022 11:21

ywangd reviewed Jul 12, 2022

View reviewed changes

n1v0lg added 8 commits July 12, 2022 10:03

Address comments

679edc7

Fix test

4134ea0

Consistently use roleDescriptors

958f7b9

Add comment

4f7eb54

Add comment

a4d9705

Merge branch 'master' into update-api-keys-noop-check

c30c26a

Tweak comment

cc7f5a9

Nit

cd730c0

n1v0lg requested a review from ywangd July 12, 2022 09:51

Merge branch 'master' into update-api-keys-noop-check

173acd7

ywangd approved these changes Jul 12, 2022

View reviewed changes

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java Outdated Show resolved Hide resolved

maybe

7d320b1

n1v0lg merged commit 28048a5 into elastic:master Jul 12, 2022

n1v0lg deleted the update-api-keys-noop-check branch July 12, 2022 13:34

ywangd mentioned this pull request Jul 14, 2022

Updatable API keys - auto-update legacy superuser RDs #88514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updatable API keys - noop check #88346

Updatable API keys - noop check #88346

n1v0lg commented Jul 7, 2022 •

edited

Loading

n1v0lg Jul 11, 2022

n1v0lg commented Jul 11, 2022

ywangd left a comment

ywangd Jul 12, 2022

n1v0lg Jul 12, 2022

ywangd Jul 12, 2022

ywangd Jul 12, 2022

n1v0lg Jul 12, 2022

ywangd Jul 12, 2022

ywangd Jul 12, 2022

ywangd Jul 12, 2022 •

edited

Loading

n1v0lg Jul 12, 2022

n1v0lg commented Jul 12, 2022

ywangd left a comment

		assert userRoles != null;
		final List<RoleDescriptor> currentLimitedByRoleDescriptorRoles = parseRoleDescriptorsBytes(

Updatable API keys - noop check #88346

Updatable API keys - noop check #88346

Conversation

n1v0lg commented Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

n1v0lg commented Jul 11, 2022

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg commented Jul 12, 2022

ywangd left a comment

Choose a reason for hiding this comment

n1v0lg commented Jul 7, 2022 •

edited

Loading

ywangd Jul 12, 2022 •

edited

Loading