Enforce API key type on authentication by ywangd · Pull Request #95908 · elastic/elasticsearch

ywangd · 2023-05-08T09:03:02Z

The PR adds enforcement for API key type at authentication time. Concretely, new cross-cluster API keys (#95714) can only be used on the dedicated remote cluster interface and the existing (rest) API key must not be used for new remote cluster communication. To make cross-cluster API keys actually usable after authentication, the PR also adds support for resolving their roles.

The PR adds support for building roles of cross-cluster API keys. These API keys are different than exsiting API keys in that they do not use the limited-by model. Their roles are only resolvable in the context of cross-cluster access.

ywangd · 2023-05-09T05:19:40Z

...lugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyAuthenticator.java

+                // TODO: emit audit event
                listener.onFailure(e);


We have an open issue (#52551) for missing audit logs when ApiKeyService fails to authenticate. It impacts here as well, i.e. when API key type does not match, the authentication terminates, but no audit event is generated. I will fix it separately when working on the plan item for audit logging. More specifically, I plan to emit an authenticationFailure event here when the authentication "terminates". But not when it "continues", which shoud be fixed separately.

ywangd · 2023-05-09T05:21:32Z

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

+        } else if (apiKeyDoc.type != credentials.expectedType) {
+            listener.onResponse(
+                AuthenticationResult.terminate(
+                    Strings.format(
+                        "authentication expected API key type of [%s], but API key [%s] has type [%s]",


Since we have found a way to have the type information carried by ApiKeyCredentials instead of Authenticator#Context, I think it is now better to perform the type check in ApiKeyService instead of ApiKeyAuthenticator. A major advantage is that the type check can happen before we validate the API key secret. It's both faster and more secure.

I think there's a trade-off here: technically this means that anyone with an API key ID (which is public-ish info that ends up all over logs, etc) but no valid credential could find out if an API key is REST or CROSS_CLUSTER simply by trying to authenticate with the given ID and a bogus secret. I don't think the API key type is very sensitive info, and I like the advantage of failing early and preventing authentication attempts, so I think I'm on board. WDYT?

This is a good point. In theory we should not reveal any information unless authentication is successful. I was thinking to limit cross-cluster API keys as much as possible on the REST interface, i.e. not letting them to trigger hash computation. But that is probably not really necessary since it is probably easier for someone to get a valid API key ID and trigger hash compuation compared to getting a API key ID specific for cross-cluster usage.

Though I did consider the type information to be public, I did suggest prefix the ID with it. However since we are not revealing it at this stage, it is probably still overall better to place the check behind hash verification. It means extra milliseconds needs to be spent on first authentication. But it is cached afterwards and should have very minor overhead.

In summary, I moved the check after hash verification.

...lasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityWithMultipleRemotesRestIT.java

elasticsearchmachine · 2023-05-09T07:00:42Z

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

n1v0lg

LGTM! This came with a v neat and small change set.

I'm pondering the following: before this new restriction, QC-side admins could use routes like GET /_security/_authenticate to get information about the API key they've been given, which could be useful for debugging. This is not possible anymore. I think that's fine, and there was never a requirement for QC admins to be able to do this. Just something that occurred to me. It's an exception to other kinds of credentials where it's possible to run _authenticate and friends.

Also, optional but I don't think we covered secondary authentication tests (sorry if did and I missed it!).

n1v0lg · 2023-05-09T09:16:40Z

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

    }

+    // Package private for testing
+    RoleReferenceIntersection buildRoleReferencesForCrossClusterApiKey() {


Nit: since this is an internal method, and separate from generic API keys now, we could return a role reference instead of an intersection.

Indeed. I was too busy at copy & paste ... Thanks!

n1v0lg · 2023-05-09T09:43:33Z

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

+        } else if (apiKeyDoc.type != credentials.expectedType) {
+            listener.onResponse(
+                AuthenticationResult.terminate(
+                    Strings.format(
+                        "authentication expected API key type of [%s], but API key [%s] has type [%s]",


I think there's a trade-off here: technically this means that anyone with an API key ID (which is public-ish info that ends up all over logs, etc) but no valid credential could find out if an API key is REST or CROSS_CLUSTER simply by trying to authenticate with the given ID and a bogus secret. I don't think the API key type is very sensitive info, and I like the advantage of failing early and preventing authentication attempts, so I think I'm on board. WDYT?

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

n1v0lg · 2023-05-09T15:11:39Z

...src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityRestIT.java

+            () -> performRequestWithRemoteSearchUser(new Request("GET", "/wrong_api_key_type:*/_search"))
+        );
+        assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(401));
+        assertThat(e.getMessage(), containsString("unable to authenticate user "));


Not for this PR but currently, and really more a musing than an actionable suggestion.

Currently, if a REST API key is configured (in place of a cross-cluster one) this is actually not that easy to detect, for a QC-side admin.

All they get are generic 401 messages on CCS (and other remote requests), without the extra context that this is due to the wrong API key type (I think).

This isn't something we introduced with this change (same problem exists for e.g., key not found) and may be kind of tricky to address. So perhaps the best way to deal with this is just having a trouble-shooting section in docs that emphasizes common causes for a 401 here...

Just jotting this down. As I said, not something to worry about now.

Errr this turns out to be a test bug. Thanks for cathing it!

With #94998, we should report underlying error in the response. The real issue here is that the remote_search user was not created on the querying cluster. It was a native user created in the beginning of testCrossClusterSearch method and I didn't copy it in the new method. So It was actually reporting 401 of the remote_search on the querying cluster. I fixed it by merging it back to testCrossClusterSearch. Now the error response contains message about mismatched API key type. Also these errors are logging on the QC side so that QC admins will be able to see them right after configuring the remote cluster because the handshake would fail for the same error. In summary, I think the error should be pretty visible on the QC side.

n1v0lg · 2023-05-09T15:22:55Z

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

            @Nullable BytesReference metadataFlattened
        ) {
            this.docType = docType;
+            this.type = type == null ? ApiKey.Type.REST : type;


Optional, but wondering if we should trace log here -- could be useful debugging context.

ywangd · 2023-05-10T01:17:36Z

before this new restriction, QC-side admins could use routes like GET /_security/_authenticate to get information about the API key they've been given, which could be useful for debugging. This is not possible anymore. I think that's fine, and there was never a requirement for QC admins to be able to do this. Just something that occurred to me. It's an exception to other kinds of credentials where it's possible to run _authenticate and friends.

This is a good point. I haven't thought about it before. It's worth a discussion. I raised a JIRA issue which we can evaluate once the main tasks are done.

I don't think we covered secondary authentication tests

You are right. I didn't have direct tests for it. It's now added. Thanks!

…ding

ywangd · 2023-05-10T02:30:25Z

@elasticmachine run elasticsearch-ci/part-2

ywangd · 2023-05-10T02:58:35Z

Failure is unrelated I raised #95970

ywangd · 2023-05-10T02:58:57Z

@elasticmachine run elasticsearch-ci/part-2

ywangd · 2023-05-10T03:29:46Z

@elasticmachine run elasticsearch-ci/part-2

Add role building logic for cross-cluster API keys

7c7278a

The PR adds support for building roles of cross-cluster API keys. These API keys are different than exsiting API keys in that they do not use the limited-by model. Their roles are only resolvable in the context of cross-cluster access.

elasticsearchmachine added the v8.9.0 label May 8, 2023

ywangd added 4 commits May 8, 2023 19:03

tweak

ecc144c

fix tests

0168adb

fix test

c222119

Enforce API key type and add/adjust tests.

68be6b4

ywangd commented May 9, 2023

View reviewed changes

fix tests and tweak

65021ad

ywangd changed the title ~~Add role building logic for cross-cluster API keys~~ Enforce API key type on authentication May 9, 2023

ywangd commented May 9, 2023

View reviewed changes

...lasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityWithMultipleRemotesRestIT.java Show resolved Hide resolved

ywangd added >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels May 9, 2023

ywangd marked this pull request as ready for review May 9, 2023 07:00

ywangd requested a review from n1v0lg May 9, 2023 07:00

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

n1v0lg approved these changes May 9, 2023

View reviewed changes

ywangd added 4 commits May 10, 2023 11:17

address feedback

fa5295e

tweak

c2305e7

Merge remote-tracking branch 'origin/main' into rcs-api-key-role-buil…

b5fccc7

…ding

tweak

1435a64

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

elasticsearchmachine merged commit 86f3716 into elastic:main May 10, 2023

ywangd deleted the rcs-api-key-role-building branch May 10, 2023 04:12

Conversation

ywangd commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

elasticsearchmachine commented May 9, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented May 10, 2023

Uh oh!

ywangd commented May 10, 2023

Uh oh!

ywangd commented May 10, 2023

Uh oh!

ywangd commented May 10, 2023

Uh oh!

ywangd commented May 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ywangd commented May 8, 2023 •

edited

Loading

n1v0lg left a comment •

edited

Loading