Fix owner user realm check for API key authentication #84325

ywangd · 2022-02-24T05:09:52Z

API Key can run-as since #79809. There are places in the code where we
assume API key cannot run-as. Most of them are corrected in #81564.
But there are still a few things got missed. This PR fixes the methods
for checking owner user realm for API key.

This means, when API Keys "running-as" (impersonating other users),
we do not expose the authenticating key ID and name to the end-user
such as the Authenticate API and the SetSecurityUseringest processor.
Only the effective user is revealed, just like in the regular case of a realm
user run as. For audit logging, the key's ID and name are not exposed
either. But this is mainly because there are no existing fields suitable for
these information. We do intend to add them later (#84394) because
auditing logging is to consumed by system admin instead of end-users.

Note the resource sharing check (canAccessResourcesOf) also needs to be
fixed, this will be handled by #84277

API Key can run-as since elastic#79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in elastic#81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by elastic#84277

elasticmachine · 2022-02-24T05:09:56Z

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

ywangd · 2022-02-24T05:12:09Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java

-        if (isAuthenticatedWithApiKey()) {
+        if (isApiKey()) {
            this.assertApiKeyMetadata();
            final String apiKeyId = (String) this.metadata.get(AuthenticationField.API_KEY_ID_KEY);
            final String apiKeyName = (String) this.metadata.get(AuthenticationField.API_KEY_NAME_KEY);


This change means we are not going to show API key ID and name in the authentication API response if the request is made by an API key run-as some other user. I think this makes sense. Ideally once we figure out what the effective user is, it should behave mostly the same as the user authentication itself. Also, it is plausible for certain setup, the underlying API key is not mean to reveal to the end-user.

ywangd · 2022-02-24T05:13:24Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java

-        if (isAuthenticatedWithServiceAccount()) {
+        if (isServiceAccount()) {
            final String tokenName = (String) getMetadata().get(ServiceAccountSettings.TOKEN_NAME_FIELD);
            assert tokenName != null : "token name cannot be null";
            final String tokenSource = (String) getMetadata().get(ServiceAccountSettings.TOKEN_SOURCE_FIELD);


Similar to the above: If the service account run-as another user, the service token name and type won't show up in the authentication response. This in theory could be a "breaking change" except we don't have any service account that has "run-as" privilege. So the change here is safe.

ywangd · 2022-02-24T05:15:43Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

+                        // API key can run-as, when that happens, the following field will be _es_api_key,
+                        // not the API key owner user's realm.
                        .with(PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authentication.getAuthenticatedBy().getName());
+                    // TODO: API key can run-as which means we could use extra fields


We don't have an existing field that can be used for logging API related information when an API key performs run-as. I think this should be tackled in a separate PR. The information is currently lacking, but not wrong, which makes sense to add more in future. Besides, we need more time to decide about exactly what fields these new info should take.

Do you mind raising a GH issue for it?

I have also realized that we audit the service token identifier in the case of it running-as some other user, but the format is probably not consistent.
Would it be more appropriate to also leave that as a TODO (also a GH issue)?

Yes we should discuss both issues and I raised #84394

Btw, also added a TODO.

albertzaharovits

LGTM

I would polish a bit the PR message, eg:
"
For API Keys "running-as" (impersonating other users), do not expose (in the_security/_authenticate, security APIs, audit logging, and the Set Security User ingest processor) the authenticating key id and name. Hence only the effective user is revealed, just like in the regular case of a user run as.
"

ywangd · 2022-02-27T23:54:43Z

I would polish a bit the PR message, eg: " For API Keys "running-as" (impersonating other users), do not expose (in the_security/_authenticate, security APIs, audit logging, and the Set Security User ingest processor) the authenticating key id and name. Hence only the effective user is revealed, just like in the regular case of a user run as. "

I updated the PR description based on your suggestion. Thanks!

…sumption

API Key can run-as since elastic#79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in elastic#81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. This means, when API Keys "running-as" (impersonating other users), we do not expose the authenticating key ID and name to the end-user such as the Authenticate API and the SetSecurityUseringest processor. Only the effective user is revealed, just like in the regular case of a realm user run as. For audit logging, the key's ID and name are not exposed either. But this is mainly because there are no existing fields suitable for these information. We do intend to add them later (elastic#84394) because auditing logging is to consumed by system admin instead of end-users. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by elastic#84277

* Fix owner user realm check for API key authentication (#84325) API Key can run-as since #79809. There are places in the code where we assume API key cannot run-as. Most of them are corrected in #81564. But there are still a few things got missed. This PR fixes the methods for checking owner user realm for API key. This means, when API Keys "running-as" (impersonating other users), we do not expose the authenticating key ID and name to the end-user such as the Authenticate API and the SetSecurityUseringest processor. Only the effective user is revealed, just like in the regular case of a realm user run as. For audit logging, the key's ID and name are not exposed either. But this is mainly because there are no existing fields suitable for these information. We do intend to add them later (#84394) because auditing logging is to consumed by system admin instead of end-users. Note the resource sharing check (canAccessResourcesOf) also needs to be fixed, this will be handled by #84277 * fix test

ywangd added >non-issue :Security/Security Security issues without another label v8.1.0 v8.2.0 labels Feb 24, 2022

ywangd requested a review from albertzaharovits February 24, 2022 05:09

elasticmachine added the Team:Security Meta label for security team label Feb 24, 2022

ywangd commented Feb 24, 2022

View reviewed changes

albertzaharovits approved these changes Feb 27, 2022

View reviewed changes

ywangd mentioned this pull request Feb 27, 2022

Improve auditing for run-as #84394

Open

ywangd added the auto-backport-and-merge label Feb 27, 2022

ywangd added 2 commits February 28, 2022 11:09

Merge remote-tracking branch 'origin/master' into more-api-key-run-as…

2ab2f32

…sumption

address feedback

9ea1a20

ywangd merged commit ba958e2 into elastic:master Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix owner user realm check for API key authentication #84325

Fix owner user realm check for API key authentication #84325

ywangd commented Feb 24, 2022 •

edited

Loading

elasticmachine commented Feb 24, 2022

ywangd Feb 24, 2022

ywangd Feb 24, 2022

ywangd Feb 24, 2022

albertzaharovits Feb 27, 2022

albertzaharovits Feb 27, 2022

ywangd Feb 27, 2022

ywangd Feb 28, 2022

albertzaharovits left a comment

ywangd commented Feb 27, 2022

Fix owner user realm check for API key authentication #84325

Fix owner user realm check for API key authentication #84325

Conversation

ywangd commented Feb 24, 2022 • edited Loading

elasticmachine commented Feb 24, 2022

ywangd Feb 24, 2022

Choose a reason for hiding this comment

ywangd Feb 24, 2022

Choose a reason for hiding this comment

ywangd Feb 24, 2022

Choose a reason for hiding this comment

albertzaharovits Feb 27, 2022

Choose a reason for hiding this comment

albertzaharovits Feb 27, 2022

Choose a reason for hiding this comment

ywangd Feb 27, 2022

Choose a reason for hiding this comment

ywangd Feb 28, 2022

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

ywangd commented Feb 27, 2022

ywangd commented Feb 24, 2022 •

edited

Loading