REST API changes for manage-own-api-key privilege by bizybot · Pull Request #44936 · elastic/elasticsearch

bizybot · 2019-07-29T05:28:14Z

This commit adds a flag that can be set to true if the
API key request (Get or Invalidate) is for the API keys owned
by the currently authenticated user only.
These only interface changes and once the actual cluster privilege
manage_own_api_key is done, we will have another PR to make the
interface work.

The Get API behavior would be:

when owner is set to true
GET /_security/api_key?id=abcd&owner=true
the Rest controller will take care of setting realm_name and username to the
values for the authenticated user and only return results if it finds one owned by
the currently authenticated user.
when owner is set to false (default)
GET /_security/api_key?id=abcd
the Rest controller will assume realm_name and username to be unspecified
meaning it will try to search for the API key across users and realms.
This will fail if the user has only manage_own_api_key privilege.

Similarly, for Delete API key behavior:

when owner is set to true
DELETE /_security/api_key
```
{
  "id" : "VuaCfGcBCdbkQm-e5aOx",
  "owner": "true"
}
```
the Rest controller will take care of setting realm_name and username to the values
for the authenticated user and only invalidate key if it finds one owned by
the currently authenticated user.
when my_api_keys_only is set to false (default)
DELETE /_security/api_key
```
{
  "id" : "VuaCfGcBCdbkQm-e5aOx",
  "owner": "false"
}
```
the Rest controller will assume realm_name and username to be unspecified meaning it will
try to search for the API key across users and realms. This will fail if the user has only
manage_own_api_key privilege.

TODO:

HLRC changes - these will be done in a separate PR
Actual enforcement of my_api_keys_only in a separate PR

Relates #40031

This commit adds a flag that can be set to `true` if the API key request (Get or Invalidate) is for the API keys owned by the current authenticated user only. The Get API behavior would be: - when `my_api_keys_only` is set to `true` `GET /_security/api_key?id=abcd&my_api_keys_only=true` the Rest controller will take care of setting `realm_name` and `username` to the values for authenticated user and only return results if it finds one owned by the currently authenticated user. - when `my_api_keys_only` is set to `false` (default) `GET /_security/api_key?id=abcd` the Rest controller will assume `realm_name` and `username` to be unspecified meaning it will try to search for the API key across users and realms. This will fail if the user has only `manage_own_api_key` privilege. Similarly, for Delete API key behavior: - when `my_api_keys_only` is set to `true` `DELETE /_security/api_key` ``` { "id" : "VuaCfGcBCdbkQm-e5aOx", "my_api_keys_only": "true" } ``` the Rest controller will take care of setting `realm_name` and `username` to the values for authenticated user and only invalidate key if it finds one owned by the currently authenticated user. - when `my_api_keys_only` is set to `false` (default) `DELETE /_security/api_key` ``` { "id" : "VuaCfGcBCdbkQm-e5aOx", "my_api_keys_only": "true" } ``` the Rest controller will assume `realm_name` and `username` to be unspecified meaning it will try to search for the API key across users and realms. This will fail if the user has only `manage_own_api_key` privilege.

elasticmachine · 2019-07-29T05:28:16Z

Pinging @elastic/es-security

albertzaharovits

LGTM save for the lack of version guards.

...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequest.java

...core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java

bizybot · 2019-07-30T06:01:39Z

Connection to 127.0.0.1 closed by remote host.
@elasticmachine run elasticsearch-ci/1 elasticsearch-ci/packaging-sample

bizybot · 2019-07-30T06:02:09Z

@elasticmachine run elasticsearch-ci/packaging-sample

tvernum · 2019-08-01T06:03:31Z

Did you consider alternatives for the parameter name? I don't love it (it's a bit verbose), but I don't want to go back over old ground if you've worked through a bunch of options already.

I would think that owned=true would be fine.

bizybot · 2019-08-01T06:41:54Z

Did you consider alternatives for the parameter name? I don't love it (it's a bit verbose), but I don't want to go back over old ground if you've worked through a bunch of options already.

I would think that owned=true would be fine.

I did consider but went with verbose as the intent was clear with the name, I agree it is too verbose now that I look at it. I like owned but by whom is not clear. Is it okay as the documentation for the API would have that information or do we want to be little more specific like owned_by_me instead? Thank you for your comments.

tvernum · 2019-08-01T07:33:02Z

I'm personally not that worried about whether the meaning of the parameter is 100% obvious from its name - we have docs.
I'm OK with owned_by_me but we could do own=true or owner=true if they seemed clear enough to you.

tvernum

I didn't get to finish my review, but I left some comments...

...n/core/src/test/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequestTests.java

…security/action/GetApiKeyRequestTests.java Co-Authored-By: Tim Vernum <tim@adjective.org>

bizybot · 2019-08-02T01:10:05Z

could not download artifacts
@elasticmachine run elasticsearch-ci/packaging-sample

...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequest.java

...core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java

...src/test/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequestTests.java

...ava/org/elasticsearch/xpack/security/rest/action/apikey/RestInvalidateApiKeyActionTests.java

.../test/java/org/elasticsearch/xpack/security/rest/action/apikey/RestGetApiKeyActionTests.java

bizybot · 2019-08-06T14:10:27Z

@elasticmachine run elasticsearch-ci/1

codebrain · 2019-10-25T01:09:09Z

@bizybot - PR looks good, we had to patch the REST API specification to add the "owner" flag. I will open an issue for this.

codebrain · 2019-10-25T01:12:48Z

#48499

bizybot added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.4.0 labels Jul 29, 2019

bizybot requested review from albertzaharovits and tvernum July 29, 2019 05:28

albertzaharovits approved these changes Jul 29, 2019

View reviewed changes

...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequest.java Outdated Show resolved Hide resolved

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

Yogesh Gaikwad added 2 commits July 30, 2019 12:42

address review comment, add versioning to newly added flag

d866404

Merge branch 'manage-own-api-key-privilege' into moacp-api-changes

a7cc6fd

Yogesh Gaikwad added 2 commits August 1, 2019 18:00

Merge branch 'manage-own-api-key-privilege' into moacp-api-changes

9baf648

address review comments

88ec2ef

tvernum reviewed Aug 1, 2019

View reviewed changes

bizybot and others added 5 commits August 2, 2019 07:49

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

f598ab2

…security/action/GetApiKeyRequestTests.java Co-Authored-By: Tim Vernum <tim@adjective.org>

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

f04365d

…security/action/GetApiKeyRequestTests.java Co-Authored-By: Tim Vernum <tim@adjective.org>

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

ec151b6

…security/action/GetApiKeyRequestTests.java Co-Authored-By: Tim Vernum <tim@adjective.org>

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

4f4a9fc

…security/action/GetApiKeyRequestTests.java Co-Authored-By: Tim Vernum <tim@adjective.org>

import VersionUtils

5402514

tvernum self-requested a review August 6, 2019 07:16

tvernum requested changes Aug 6, 2019

View reviewed changes

Yogesh Gaikwad added 3 commits August 6, 2019 19:28

Merge branch 'manage-own-api-key-privilege' into moacp-api-changes

0ce5316

address review comments

6ed0281

remove unused import

481f561

bizybot requested a review from tvernum August 6, 2019 12:52

tvernum approved these changes Aug 7, 2019

View reviewed changes

bizybot merged commit 522f005 into elastic:manage-own-api-key-privilege Aug 7, 2019

colings86 added the >enhancement label Aug 30, 2019

codebrain mentioned this pull request Oct 14, 2019

7.4 meta ticket elastic/elasticsearch-net#4133

Closed

56 tasks

This was referenced Oct 23, 2019

7.4.1 meta ticket elastic/elasticsearch-net#4174

Closed

Security API changes to add owner flag elastic/elasticsearch-net#4178

Merged

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

Conversation

bizybot commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 29, 2019

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bizybot commented Jul 30, 2019

Uh oh!

bizybot commented Jul 30, 2019

Uh oh!

tvernum commented Aug 1, 2019

Uh oh!

bizybot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvernum commented Aug 1, 2019

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bizybot commented Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bizybot commented Aug 6, 2019

Uh oh!

codebrain commented Oct 25, 2019

Uh oh!

codebrain commented Oct 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bizybot commented Jul 29, 2019 •

edited

Loading

bizybot commented Aug 1, 2019 •

edited

Loading