Respect runas realm for ApiKey security operations by ywangd · Pull Request #52178 · elastic/elasticsearch

ywangd · 2020-02-11T02:02:46Z

When user A run as user B and perform any API key related operations,
user B's realm should always be used to associate with the API key.
Currently user A's realm is used when getting or invalidating API keys
and owner=true. The PR is to fix this bug.

resolves: #51975

elasticmachine · 2020-02-11T02:02:49Z

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

ywangd · 2020-02-11T02:05:37Z

Does this change need to be documented somewhere similar to what we do for breaking changes?

tvernum · 2020-02-11T10:25:03Z

This looks good to me, but per your comment on the issue, it looks like there's additional changes needed.

Also, given we consider this a bug, I'd like a test for the actual bug. The fact that you can make the change and not need to fix any tests in the API permissions area, shows that we've got a testing gap there.

I think we want to duplicate ApiKeyIntegTests.testGetApiKeysOwnedByCurrentAuthenticatedUser and ApiKeyIntegTests.testInvalidateApiKeysOwnedByCurrentAuthenticatedUser with a run-as user. You'll need to do some refactoring on createApiKeys to support a run-as user there too, but I think it's all obvious enough.

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

ywangd · 2020-02-12T02:56:15Z

Added tests for both get api key and invalidate api key with runas user. Thanks

…apikey-ops

albertzaharovits

The overall layout is correct, but there is a small inverted logic of authenticated - lookup that is wrong.

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

albertzaharovits · 2020-02-13T10:02:39Z

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

+            "es-security-runas-user", userWithManageOwnApiKeyRole));
+
+        PlainActionFuture<GetApiKeyResponse> listener = new PlainActionFuture<>();
+        client.execute(GetApiKeyAction.INSTANCE, GetApiKeyRequest.forOwnedApiKeys(), listener);


add a test when the owned flag is not set, but the username and realm are.

I was meant do test without the "owner" flag since the updated logic won't get executed with its presense. Somehow I ended up forgetting it. Will update. Thanks

Add tests for get and invalidate API key without "owner=true"

albertzaharovits · 2020-02-13T10:06:12Z

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

+            "es-security-runas-user", userWithManageOwnApiKeyRole));
+
+        PlainActionFuture<InvalidateApiKeyResponse> listener = new PlainActionFuture<>();
+        client.execute(InvalidateApiKeyAction.INSTANCE, InvalidateApiKeyRequest.forOwnedApiKeys(), listener);


same as above, we should also test without the owner flag toggled but with the concrete username and realm set.

I would also add a negative test, but this is usually better to do in unit tests, but I sometimes sneak in a negative test in integ tests as well.

Added negative tests for calling get and invalidate API key with mismatching username and/or realm name.

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

…security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java Co-Authored-By: Albert Zaharovits <albert.zaharovits@gmail.com>

…apikey-ops

ywangd · 2020-02-13T12:14:35Z

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

+    private void createUserWithRunAsRole() throws ExecutionException, InterruptedException {
+        final PutUserRequest putUserRequest = new PutUserRequest();
+        putUserRequest.username("user_with_run_as_role");
+        putUserRequest.roles("run_as_role");
+        putUserRequest.passwordHash(SecuritySettingsSource.TEST_PASSWORD_HASHED.toCharArray());
+        PlainActionFuture<PutUserResponse> listener = new PlainActionFuture<>();
+        final Client client = client().filterWithHeader(Map.of("Authorization",
+            UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER,
+                SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)));
+        client.execute(PutUserAction.INSTANCE, putUserRequest, listener);
+        final PutUserResponse putUserResponse = listener.get();
+        assertTrue(putUserResponse.created());
+    }


In order to have negative tests for realm name mismatch, user_with_run_as_role needs to be created in a different realm other than file (which is handled by configureUsers()). This new helper method creates the user in the native realm.

Can we add that explanation to the code?

ywangd · 2020-02-13T12:17:35Z

There were some sloppiness in the last change. Thanks @albertzaharovits for catching them. It is now updated accordingly. Thanks!

albertzaharovits

LGTM 👍

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

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

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

…apikey-ops

tvernum · 2020-02-21T01:04:11Z

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

+        int noOfApiKeys, TimeValue expiration, String... clusterPrivileges) {
+        final Map<String, String> headers = Map.of("Authorization",
+            UsernamePasswordToken.basicAuthHeaderValue(runAsUser, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING),
+            "es-security-runas-user", user);


These seem backwards to me. You authenticate as runAsUser but run-as user.
I assume the parameters are just strangely named, but it probably makes sense to have authenticatingUser and owningUser or something like that.

Good point. It does read awkward. Updated.

tvernum · 2020-02-21T01:05:38Z

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

+    private void createUserWithRunAsRole() throws ExecutionException, InterruptedException {
+        final PutUserRequest putUserRequest = new PutUserRequest();
+        putUserRequest.username("user_with_run_as_role");
+        putUserRequest.roles("run_as_role");
+        putUserRequest.passwordHash(SecuritySettingsSource.TEST_PASSWORD_HASHED.toCharArray());
+        PlainActionFuture<PutUserResponse> listener = new PlainActionFuture<>();
+        final Client client = client().filterWithHeader(Map.of("Authorization",
+            UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER,
+                SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)));
+        client.execute(PutUserAction.INSTANCE, putUserRequest, listener);
+        final PutUserResponse putUserResponse = listener.get();
+        assertTrue(putUserResponse.created());
+    }


Can we add that explanation to the code?

…apikey-ops

tvernum

LGTM

When user A runs as user B and performs any API key related operations, user B's realm should always be used to associate with the API key. Currently user A's realm is used when getting or invalidating API keys and owner=true. The PR is to fix this bug. resolves: elastic#51975

When user A runs as user B and performs any API key related operations, user B's realm should always be used to associate with the API key. Currently user A's realm is used when getting or invalidating API keys and owner=true. The PR is to fix this bug. resolves: #51975

ywangd · 2020-02-27T23:54:41Z

Backported:

v7.7.0 Respect runas realm for ApiKey security operations (#52178) #52932

Avoid string comparison when we can use safter enums. This refactor is a follow up for #52178. Resolves: #52511

Avoid string comparison when we can use safter enums. This refactor is a follow up for elastic#52178. Resolves: elastic#52511

Avoid string comparison when we can use safer enums. This refactor is a follow up for #52178. Resolves: #52511

Initial fix for elastic#51975

9712caf

ywangd added >bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.7.0 labels Feb 11, 2020

ywangd requested review from albertzaharovits and tvernum February 11, 2020 02:02

ywangd added the WIP label Feb 11, 2020

ywangd added 2 commits February 12, 2020 11:45

Update manage own api key to always use source realm

3899fcc

Add integration tests for runas API key ops

e264109

ywangd commented Feb 12, 2020

View reviewed changes

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

ywangd removed the WIP label Feb 12, 2020

Merge remote-tracking branch 'origin/master' into es-51975-runas-for-…

455a6d6

…apikey-ops

albertzaharovits requested changes Feb 13, 2020

View reviewed changes

albertzaharovits reviewed Feb 13, 2020

View reviewed changes

.../elasticsearch/xpack/core/security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java Outdated Show resolved Hide resolved

ywangd and others added 3 commits February 13, 2020 21:25

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

f966f4d

…security/authz/privilege/ManageOwnApiKeyClusterPrivilegeTests.java Co-Authored-By: Albert Zaharovits <albert.zaharovits@gmail.com>

Address feedback

f3cd6ff

Merge remote-tracking branch 'origin/master' into es-51975-runas-for-…

584343a

…apikey-ops

ywangd commented Feb 13, 2020

View reviewed changes

ywangd requested a review from albertzaharovits February 13, 2020 12:17

albertzaharovits approved these changes Feb 19, 2020

View reviewed changes

ywangd added 2 commits February 19, 2020 23:09

Address feedback

df9cf5d

Merge remote-tracking branch 'origin/master' into es-51975-runas-for-…

b7bdd03

…apikey-ops

tvernum reviewed Feb 21, 2020

View reviewed changes

Address feedback

57b5b06

ywangd added 2 commits February 21, 2020 13:09

Merge remote-tracking branch 'origin/master' into es-51975-runas-for-…

b1bce89

…apikey-ops

Update

a05fdeb

ywangd requested a review from tvernum February 21, 2020 02:16

tvernum approved these changes Feb 27, 2020

View reviewed changes

ywangd merged commit d02e1e3 into elastic:master Feb 27, 2020

ywangd added the backport pending label Feb 27, 2020

ywangd removed the backport pending label Feb 27, 2020

ywangd mentioned this pull request Mar 25, 2020

Check authentication type using enum instead of string #54145

Merged

ywangd added a commit that referenced this pull request Mar 26, 2020

Check authentication type using enum instead of string (#54145)

e244a3b

Avoid string comparison when we can use safter enums. This refactor is a follow up for #52178. Resolves: #52511

ywangd mentioned this pull request Mar 26, 2020

Check authentication type using enum instead of string (#54145) #54246

Merged

ywangd added a commit that referenced this pull request Mar 26, 2020

Check authentication type using enum instead of string (#54145) (#54246)

1afd510

Avoid string comparison when we can use safer enums. This refactor is a follow up for #52178. Resolves: #52511

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

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

Conversation

ywangd commented Feb 11, 2020

Uh oh!

elasticmachine commented Feb 11, 2020

Uh oh!

ywangd commented Feb 11, 2020

Uh oh!

tvernum commented Feb 11, 2020

Uh oh!

Uh oh!

ywangd commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertzaharovits left a comment

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!

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 Feb 13, 2020

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

ywangd commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ywangd commented Feb 12, 2020 •

edited

Loading