Add `manage_own_api_key` cluster privilege #45696

bizybot · 2019-08-19T12:47:00Z

This commit adds manage_own_api_key cluster privilege which
only allows api key cluster actions on API keys owned by the
currently authenticated user.

Relates: #40031

Currently, cluster permission checks whether a cluster action is permitted and optionally in the context of a request. There are scenarios where we would want to check whether the cluster action is permitted, optionally in the context of a request and current authentication. For example, management of API keys is only restricted to the API keys owned by the current user. In this case, along with the cluster action and API key request, the check needs to perform whether the currently authenticated user is indeed allowed to operate only on owned API keys. With this commit, we are introducing one more context of the current authentication that can be considered during permission evaluation. Relates: elastic#40031

…based-permission-check

elasticmachine · 2019-08-19T12:47:04Z

Pinging @elastic/es-security

The permission checks that are dependent on actions and optionally on request and/or on authentication, now have a way to specify the predicates. By default the implementation will tests all the predicates to be successful for the operation to be allowed. In case customization is required one has option to implement `PermissionCheck`. - Adds a permission check predicate interface that also allows implementers to specify behavior for `implies`.

This commit adds `manage_own_api_key` cluster privilege which only allows api key cluster actions on API keys owned by the current authenticated user.

albertzaharovits

I left two questions.

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

...security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java

…based-permission-check

albertzaharovits

It is still not clear to me what API-Key-privileges do I have if I authenticate with an API Key. Do I inherit the API-Key privileges from the user that originally created the first API key that created my API Key, etc... ? Or I don't have any API Key privilege.

For example, a user with manage_api_key create a key. Does that key grant manage_api_key as well? I think it shouldn't but the code looks like it does.
The same for manage_own_api_key.

I

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

...k/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

…o-cp

.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ClusterPermission.java

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

bizybot · 2019-08-22T15:31:04Z

@albertzaharovits
Just to help the discussion and for my sanity:

User ▼ API Key Action ►	Create API key	Invalidate API key	GET API key
User with `all` or `manage_security` or `manage_api_key` privilege	Allow	Allow	Allow
User authenticated using API key with `all` or `manage_security` or `manage_api_key` privilege	Allowed but resultant API keys are no-op	Allow	Allow
User with `manage_own_api_key` privilege	Allow	Allowed only on owned API keys	Allowed only on owned API keys
User authenticated using API key with `manage_own_api_key` privilege	Allowed but resultant API keys are no-op	Allowed only to invalidate self	Allowed only to retrieve self
User with no privilege related to API keys	Deny	Deny	Deny
User authenticated using API keys with no privilege related to API keys	Deny	Deny	Allowed only to retrieve self

albertzaharovits

I left one suggestion and voiced my preference about not using the owner flag in the authz process.
LGTM

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

albertzaharovits · 2019-08-22T21:39:22Z

Thank you for the tabled explanation @bizybot !
Nitpicking: An API key with manage_security can always create more API keys, because it can create other users (assuming the native realm is enabled) and create APi keys for those.

I agree we should explicitly deny authorization for API keys with manage_api_key (I won't worry about BWC because I consider this a bug) and this might require building a new NamedPrivilege implementation 🌞

…security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java Co-Authored-By: Albert Zaharovits <[email protected]>

tvernum

I think we're very close on this.

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

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

...k/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

tvernum · 2019-08-23T01:38:57Z

...security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java

+                realm = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM);
+            } else {
+                realm = authentication.getAuthenticatedBy().getName();
+            }


The same logic is in 2 transport actions. That seems like a strong enough argument to move it to a static method on the service.

- separate tests to test one thing - common code to extract realm name move to ApiKeyService - final and move variables to where it is being used - test API keys with no cluster privilege for an API key

tvernum

LGTM, subject to 1 minor change.

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java

tvernum · 2019-08-23T06:11:02Z

@bizybot It looks like you requested another approval after I had already commented.

bizybot · 2019-08-23T06:13:24Z

@bizybot It looks like you requested another approval after I had already commented.

Oh, I looked at one of the cached pages in the browser and thought I missed on refresh.

Yogesh Gaikwad added 2 commits August 12, 2019 13:59

Merge branch 'manage-own-api-key-privilege' into moap-authentication-…

60f436b

…based-permission-check

bizybot added >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.4.0 labels Aug 19, 2019

Yogesh Gaikwad added 10 commits August 20, 2019 13:36

remove unwanted code

8896dcd

remove unwanted code change, raise a separate pr for it

4976d78

fix tests

54490a7

precommit error

9033996

oh precommit, ran build on another branch, fixed it

541cfad

Add manage_own_api_key cluster privilege

aa623c3

This commit adds `manage_own_api_key` cluster privilege which only allows api key cluster actions on API keys owned by the current authenticated user.

add tests

efc2c2b

fix tests

7db504c

change manage own api cluster privielge

adc6d69

bizybot force-pushed the moap-mo-cp branch from 147c661 to adc6d69 Compare August 20, 2019 08:04

Yogesh Gaikwad added 2 commits August 20, 2019 18:18

precommit error

2497c6f

no need to override

6bd259a

bizybot requested review from albertzaharovits and tvernum August 20, 2019 13:11

albertzaharovits reviewed Aug 21, 2019

View reviewed changes

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java Show resolved Hide resolved

...security/src/main/java/org/elasticsearch/xpack/security/action/TransportGetApiKeyAction.java Outdated Show resolved Hide resolved

bizybot requested a review from albertzaharovits August 21, 2019 09:29

Yogesh Gaikwad added 5 commits August 21, 2019 20:10

Merge branch 'manage-own-api-key-privilege' into moap-authentication-…

66fc5b3

…based-permission-check

address review comment

508a718

add a base class for action based permission check to enforce checks

29cefd2

protected instead of public

0eba55f

Merge branch 'manage-own-api-key-privilege' into moap-authentication-…

de88e11

…based-permission-check

albertzaharovits reviewed Aug 21, 2019

View reviewed changes

make the condition explicit

a94fa92

bizybot requested a review from albertzaharovits August 21, 2019 22:10

Merge branch 'moap-authentication-based-permission-check' into moap-m…

d5a295c

…o-cp

tvernum reviewed Aug 22, 2019

View reviewed changes

address review comments

73498ea

bizybot requested a review from tvernum August 22, 2019 03:18

Merge branch 'manage-own-api-key-privilege' into moap-mo-cp

2f7933c

bizybot changed the base branch from moap-authentication-based-permission-check to manage-own-api-key-privilege August 22, 2019 07:33

Yogesh Gaikwad added 4 commits August 22, 2019 22:17

test name and test correctness

5c9c422

Merge branch 'manage-own-api-key-privilege' into moap-mo-cp

186e599

remove extra blank lines

45b0192

check owner flag in manage own cluster privilege, add tests

dc512c2

albertzaharovits approved these changes Aug 22, 2019

View reviewed changes

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java Outdated Show resolved Hide resolved

bizybot and others added 3 commits August 23, 2019 08:05

Update x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/…

d740034

…security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java Co-Authored-By: Albert Zaharovits <[email protected]>

fix compilation error

7a9cfb7

Merge branch 'manage-own-api-key-privilege' into moap-mo-cp

569cae6

tvernum requested changes Aug 23, 2019

View reviewed changes

Yogesh Gaikwad added 2 commits August 23, 2019 12:25

address review comments

1a2a371

- separate tests to test one thing - common code to extract realm name move to ApiKeyService - final and move variables to where it is being used - test API keys with no cluster privilege for an API key

static method instead of instance method

43cf4e3

tvernum approved these changes Aug 23, 2019

View reviewed changes

...a/org/elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilege.java Outdated Show resolved Hide resolved

bizybot requested a review from tvernum August 23, 2019 02:43

add unsupported class name to exception

1e59c70

tvernum approved these changes Aug 23, 2019

View reviewed changes

bizybot merged commit ed2062f into elastic:manage-own-api-key-privilege Aug 23, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `manage_own_api_key` cluster privilege #45696

Add `manage_own_api_key` cluster privilege #45696

bizybot commented Aug 19, 2019 •

edited

Loading

elasticmachine commented Aug 19, 2019

albertzaharovits left a comment

albertzaharovits left a comment

bizybot commented Aug 22, 2019 •

edited

Loading

albertzaharovits left a comment

albertzaharovits commented Aug 22, 2019 •

edited

Loading

tvernum left a comment

tvernum Aug 23, 2019 •

edited

Loading

tvernum left a comment

tvernum commented Aug 23, 2019

bizybot commented Aug 23, 2019

Add manage_own_api_key cluster privilege #45696

Add manage_own_api_key cluster privilege #45696

Conversation

bizybot commented Aug 19, 2019 • edited Loading

elasticmachine commented Aug 19, 2019

albertzaharovits left a comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

bizybot commented Aug 22, 2019 • edited Loading

albertzaharovits left a comment

Choose a reason for hiding this comment

albertzaharovits commented Aug 22, 2019 • edited Loading

tvernum left a comment

Choose a reason for hiding this comment

tvernum Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

tvernum commented Aug 23, 2019

bizybot commented Aug 23, 2019

Add `manage_own_api_key` cluster privilege #45696

Add `manage_own_api_key` cluster privilege #45696

bizybot commented Aug 19, 2019 •

edited

Loading

bizybot commented Aug 22, 2019 •

edited

Loading

albertzaharovits commented Aug 22, 2019 •

edited

Loading

tvernum Aug 23, 2019 •

edited

Loading