Fix can access resource checks for API Keys with run as #84277

albertzaharovits · 2022-02-23T12:24:22Z

This fixes two things for the "can access" authz check:

API Keys running as, have access to the resources created by the effective run as user
tokens created by API Keys (with the client credentials) have access to the API Key's resources

In addition, this PR moves some of the authz plumbing code from the Async and Scroll services classes under the Security Context class (as a minor refactoring).

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

...in/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationContext.java

elasticmachine · 2022-02-27T20:46:18Z

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

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

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

ywangd

LGTM

Thanks for handling this. Backport will be a challenge. 🙏

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

ywangd · 2022-02-28T04:55:00Z

...ugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java

+        if ((false == FileRealmSettings.TYPE.equals(realmRef.getType()) && false == FileRealmSettings.TYPE.equals(otherRealmRef.getType()))
+            || (false == NativeRealmSettings.TYPE.equals(realmRef.getType())
+                && false == NativeRealmSettings.TYPE.equals(otherRealmRef.getType()))) {
+            Authentication runAs4 = original.runAs(user, otherRealmRef);
+            assertCannotAccessResources(original, runAs4);
+            assertCannotAccessResources(original.token(), runAs4);
+            assertCannotAccessResources(original, runAs4.token());
+            assertCannotAccessResources(original.token(), runAs4.token());
+        }


This if check does not seem to be useful since otherRealmRef is created as:

final RealmRef otherRealmRef = randomValueOtherThan(realmRef, () -> randomRealmRef(false));

So it is never equal to realmRef.

The if is needed, but it was wrong. I've fixed it, and left a comment.

The idea is that the realm refs are different but if they're both file or both native, then the ownership check should pass.

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/async/AsyncTaskServiceTests.java

ywangd · 2022-02-28T05:20:30Z

Btw, I think we probably can drop the >bug label since API key run-as was not yet released and token created by API key wasn't really working before 8.1. What do you think?

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 · 2022-02-28T07:30:44Z

Backport will be a challenge

I added a few helper methods to AuthenticationTests while doing my backports. There are now methods like randomRealmAuthentication etc. It also has methods like AuthenticationTests#toRunAs and AuthenticationTests#toToken to mirror the new methods added to Authentication. I added them to the Test file because I don't want to change production code for tests. I hope they could be useful for your backport as well.

albertzaharovits · 2022-02-28T13:03:17Z

Btw, I think we probably can drop the >bug label since API key run-as was not yet released and token created by API key wasn't really working before 8.1. What do you think?

Okay, I'll label this >non-issue.

This fixes two things for the "can access" authz check: * API Keys running as, have access to the resources created by the effective run as user * tokens created by API Keys (with the client credentials) have access to the API Key's resources In addition, this PR moves some of the authz plumbing code from the Async and Scroll services classes under the Security Context class (as a minor refactoring).

* Fix can access resource checks for API Keys with run as (#84277) This fixes two things for the "can access" authz check: * API Keys running as, have access to the resources created by the effective run as user * tokens created by API Keys (with the client credentials) have access to the API Key's resources In addition, this PR moves some of the authz plumbing code from the Async and Scroll services classes under the Security Context class (as a minor refactoring). * spotless * Merge fallout * AuthenticationTests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

albertzaharovits added 2 commits February 23, 2022 14:21

AsyncTask

6c86bdc

Merge branch 'master' into can-access-resource-of

9668ce9

elasticsearchmachine added the v8.2.0 label Feb 23, 2022

albertzaharovits added 3 commits February 23, 2022 14:32

spotless

357f0f3

SecuritySearchOperationListenerTests

e85fbff

Merge branch 'master' into can-access-resource-of

d5a3e98

ywangd mentioned this pull request Feb 24, 2022

Fix owner user realm check for API key authentication #84325

Merged

ywangd reviewed Feb 24, 2022

View reviewed changes

...in/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationContext.java Outdated Show resolved Hide resolved

albertzaharovits added 4 commits February 27, 2022 08:02

Merge branch 'master' into can-access-resource-of

d9e495a

Leave the logic in Authentication

0623c95

tests

a539966

Good

222348f

albertzaharovits self-assigned this Feb 27, 2022

albertzaharovits added v8.1.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Feb 27, 2022

albertzaharovits requested a review from ywangd February 27, 2022 20:46

albertzaharovits marked this pull request as ready for review February 27, 2022 20:46

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

albertzaharovits added the >bug label Feb 27, 2022

Update docs/changelog/84277.yaml

2174d74

ywangd reviewed Feb 28, 2022

View reviewed changes

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

ywangd approved these changes Feb 28, 2022

View reviewed changes

ywangd mentioned this pull request Feb 28, 2022

Fix owner user realm check for API key authentication (#84325) #84401

Merged

albertzaharovits added 2 commits February 28, 2022 14:51

Merge branch 'master' into can-access-resource-of

dec8d56

Nits

fcf3b76

albertzaharovits added >non-issue and removed >bug labels Feb 28, 2022

Delete docs/changelog/84277.yaml

6edef84

albertzaharovits added auto-backport-and-merge auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Feb 28, 2022

elasticsearchmachine merged commit 2f0733d into elastic:master Feb 28, 2022

albertzaharovits deleted the can-access-resource-of branch February 28, 2022 14:01

albertzaharovits mentioned this pull request Feb 28, 2022

Fix can access resource checks for API Keys with run as #84431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix can access resource checks for API Keys with run as #84277

Fix can access resource checks for API Keys with run as #84277

Uh oh!

albertzaharovits commented Feb 23, 2022 •

edited

Loading

Uh oh!

Uh oh!

elasticmachine commented Feb 27, 2022

Uh oh!

Uh oh!

ywangd left a comment

Uh oh!

Uh oh!

ywangd Feb 28, 2022

Uh oh!

albertzaharovits Feb 28, 2022 •

edited

Loading

Uh oh!

Uh oh!

ywangd commented Feb 28, 2022

Uh oh!

ywangd commented Feb 28, 2022

Uh oh!

albertzaharovits commented Feb 28, 2022

Uh oh!

Uh oh!

Fix can access resource checks for API Keys with run as #84277

Fix can access resource checks for API Keys with run as #84277

Uh oh!

Conversation

albertzaharovits commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Feb 27, 2022

Uh oh!

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywangd Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Feb 28, 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!

ywangd commented Feb 28, 2022

Uh oh!

ywangd commented Feb 28, 2022

Uh oh!

albertzaharovits commented Feb 28, 2022

Uh oh!

Uh oh!

albertzaharovits commented Feb 23, 2022 •

edited

Loading

albertzaharovits Feb 28, 2022 •

edited

Loading