Add a new API to update RCS specific API keys by ywangd · Pull Request #96085 · elastic/elasticsearch

ywangd · 2023-05-15T03:09:40Z

This PR adds a new endpoint to update RCS specific API keys. It works similarly to the existing UpdateApiKey API except:

It requires manage_security permission
It does not permit empty request body because it does not use limited-by model
It takes the simplified "access" payload as the CreateCrossClusterApiKey API

Relates: #95714

ywangd · 2023-05-16T02:05:11Z

.../org/elasticsearch/xpack/security/action/apikey/TransportUpdateCrossClusterApiKeyAction.java

+public final class TransportUpdateCrossClusterApiKeyAction extends TransportBaseUpdateApiKeyAction<
+    UpdateCrossClusterApiKeyRequest,
+    UpdateApiKeyResponse> {


It is technically possible to have a bulk version of this transport action and use it for the single update REST action as well. But doing so could make auditing message look a bit weird because we differentiate beteween single and bulk update in audit events for REST API keys. So I chose to have a dedicate single update transport action and leave out bulk update transport action since we don't need it for now.

ywangd · 2023-05-16T02:05:17Z

.../org/elasticsearch/xpack/security/action/apikey/TransportUpdateCrossClusterApiKeyAction.java

+            new BaseBulkUpdateApiKeyRequest(List.of(request.getId()), request.getRoleDescriptors(), request.getMetadata()) {
+                @Override
+                public ApiKey.Type getType() {
+                    return ApiKey.Type.CROSS_CLUSTER;
+                }
+            },


Use anonymous class here to have one less class file.

ywangd · 2023-05-16T02:10:50Z

...nalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java

+        // Cross-cluster API keys can be created by an API key as long as it has manage_security
+        final boolean createWithUser = randomBoolean();
+        if (createWithUser) {
+            client().execute(CreateCrossClusterApiKeyAction.INSTANCE, request, future);
+        } else {
+            final CreateApiKeyResponse createAdminKeyResponse = new CreateApiKeyRequestBuilder(client()).setName("admin-key")


Adding a randomisation here which should have been part of #95714.

elasticsearchmachine · 2023-05-16T02:15:10Z

Pinging @elastic/es-security (Team:Security)

n1v0lg

I went through the production code (still need to review tests) and I'm on board with the changes, except one behavior which we have to tweak:

Currently, if an API key creates a cross cluster API key, it becomes impossible to update it. The API key can't update it because we prevent this in the ApiKeyService. The owner of the original API key can't update it either because the realm doesn't match anymore (it's set to _es_api_key on the cross cluster key on creation).

I think the simplest way to address this is to simply skip supporting derived cross cluster API keys for now (since there wasn't a strong ask for these in the first place), but wanted to also see your thoughts. Apologies for not catching this in the review of the create API! And apologies if I'm missing something and this isn't actually an issue.

n1v0lg · 2023-05-16T13:08:04Z

...vileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java

        "cluster:admin/xpack/security/api_key/bulk_update",
        "cluster:admin/xpack/security/cache/clear",
-        "cluster:admin/xpack/security/cross_cluster/api_key/create",
+        TcpTransport.isUntrustedRemoteClusterEnabled() ? "cluster:admin/xpack/security/cross_cluster/api_key/create" : null,


Good catch!

...ain/java/org/elasticsearch/xpack/security/action/apikey/TransportBaseUpdateApiKeyAction.java

n1v0lg · 2023-05-16T13:22:27Z

...rc/main/java/org/elasticsearch/xpack/security/action/apikey/TransportUpdateApiKeyAction.java

+    }
+
+    @Override
+    void resolveUserRoleDescriptors(Authentication authentication, ActionListener<Set<RoleDescriptor>> listener) {


Nit: I think we could collapse the two abstract methods into one, since they are only called together. Something like:

abstract void doExecuteUpdate( Task task, Request request, Authentication authentication, ActionListener<Response> listener );

The fact that we are resolving role descriptors is arguably an implementation detail of the regular update actions; for cross cluster, we don't really resolve.

Yeah one abstract method looks better. I have updated accordingly e62022c

ywangd · 2023-05-16T14:33:17Z

For the derived key issue, I commented on the PR of create API here #95714 (comment)

I personally think it's a separate issue since it's a known bug. Other than not updatable, it otherwise works just fine. My preference would be moving forward as is and fork a separate issue to agree on API key's identity and ownership. But please let me know if you think otherwise.

The other issue that I realised today is that we should have license check for both APIs. I'll have it as a separate PR to keep the size in control.

n1v0lg · 2023-05-16T14:43:08Z

For the derived key issue, I commented on the PR of create API here #95714 (comment)

Ah apologies, I missed this!

My preference would be moving forward as is and fork a separate issue to agree on API key's identity and ownership.

Since the API key does end up broken (in a surprising way), I think it would be good to either fix the bug or keep derived API keys out of scope. Totally fair to handle this outside of this PR though, and fork a separate issue 👍

The other issue that I realised today is that we should have license check for both APIs. I'll have it as a separate PR to keep the size in control.

Sounds good!

…i-keys

n1v0lg

LGTM! Sorry for the delay. Just a few smaller things and nits around tests.

n1v0lg · 2023-05-22T14:42:59Z

.../elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java

+        final CreateCrossClusterApiKeyRequest request = CreateCrossClusterApiKeyRequest.withNameAndAccess(
+            randomAlphaOfLengthBetween(3, 8),
+            """
+                                                                                                {


Nit: there is a bunch of extra whitespace here; lets nuke it

...curity-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java

...nalClusterTest/java/org/elasticsearch/xpack/security/authc/apikey/ApiKeySingleNodeTests.java

...elasticsearch/xpack/security/action/apikey/TransportUpdateCrossClusterApiKeyActionTests.java

n1v0lg · 2023-05-22T18:47:56Z

...elasticsearch/xpack/security/rest/action/apikey/RestUpdateCrossClusterApiKeyActionTests.java

+
+    public void testUpdateHasTypeOfCrossCluster() throws Exception {
+        final String id = randomAlphaOfLength(10);
+        final String access = randomFrom(ACCESS_CANDIDATES);


Totally optional but a randomCrossClusterApiKeyAccessField() or something like that could be a nice wrapper

Add suggested method to CreateCrossClusterApiKeyRequestTests and replace all occurences like this one with the new method.

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java

Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

…i-keys

ywangd · 2023-05-22T23:02:25Z

Thanks a lot for the thorough review and nice catches!

Enforce the same license level as the advanced remote cluster security feature for these APIs. Relates: #95714, #96085

This PR adds REST spec files and YAML tests for the new create and update cross-cluster API key APIs. Relates: elastic#95714, elastic#96085

This PR adds REST spec files and YAML tests for the new create and update cross-cluster API key APIs. Relates: #95714, #96085

This PR adds API doc pages for the new create and update cross-cluster API key APIs. Relates: elastic#95714, elastic#96085

This PR adds API doc pages for the new create and update cross-cluster API key APIs. Relates: #95714, #96085 Co-authored-by: Arianna Laudazzi <46651782+alaudazzi@users.noreply.github.com>

working version

f2a9d77

elasticsearchmachine added the v8.9.0 label May 15, 2023

More tests

cb15913

ywangd commented May 16, 2023

View reviewed changes

tweak

7801d39

ywangd commented May 16, 2023

View reviewed changes

ywangd marked this pull request as ready for review May 16, 2023 02:14

ywangd requested a review from n1v0lg May 16, 2023 02:14

ywangd added >non-issue :Security/Security Security issues without another label labels May 16, 2023

elasticsearchmachine added the Team:Security Meta label for security team label May 16, 2023

n1v0lg reviewed May 16, 2023

View reviewed changes

ywangd added 3 commits May 17, 2023 13:37

tweak abstract methods

e62022c

fix test

22d01f4

Merge remote-tracking branch 'origin/main' into rcs-update-special-ap…

d3df8c0

…i-keys

ywangd requested a review from n1v0lg May 17, 2023 11:24

n1v0lg approved these changes May 22, 2023

View reviewed changes

ywangd and others added 3 commits May 23, 2023 08:49

Apply suggestions from code review

42be9fd

Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

address feedback and tweak

45ef238

Merge remote-tracking branch 'origin/main' into rcs-update-special-ap…

c64bdd1

…i-keys

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 22, 2023

elasticsearchmachine merged commit 471b3f5 into elastic:main May 22, 2023

ywangd deleted the rcs-update-special-api-keys branch May 22, 2023 23:42

ywangd added a commit that referenced this pull request May 25, 2023

Enforce license for cross-cluster API key APIs (#96307)

870ca07

Enforce the same license level as the advanced remote cluster security feature for these APIs. Relates: #95714, #96085

ywangd mentioned this pull request May 25, 2023

REST spec and YAML tests for cross-cluster API key APIs #96339

Merged

elasticsearchmachine pushed a commit that referenced this pull request May 26, 2023

REST spec and YAML tests for cross-cluster API key APIs (#96339)

e5e2a04

This PR adds REST spec files and YAML tests for the new create and update cross-cluster API key APIs. Relates: #95714, #96085

ywangd added a commit to ywangd/elasticsearch that referenced this pull request May 30, 2023

[Doc] Add doc pages for cross-cluter API key APIs

a17113a

This PR adds API doc pages for the new create and update cross-cluster API key APIs. Relates: elastic#95714, elastic#96085

ywangd mentioned this pull request May 30, 2023

[Doc] Add doc pages for cross-cluter API key APIs #96415

Merged

Conversation

ywangd commented May 15, 2023

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!

elasticsearchmachine commented May 16, 2023

Uh oh!

n1v0lg left a comment • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented May 16, 2023

Uh oh!

n1v0lg commented May 16, 2023

Uh oh!

n1v0lg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd May 22, 2023 • 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!

ywangd commented May 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

n1v0lg left a comment •

edited

Loading

ywangd May 22, 2023 •

edited

Loading