Ensure authz role for API key is named after owner role #59041
Conversation
albertzaharovits
added
>non-issue
:Security/Authorization
|
Pinging @elastic/es-security (:Security/Authorization) |
| @@ -86,7 +81,7 @@ public IndicesAccessControl authorize(String action, Set<String> requestedIndice | |||
| @Override | |||
| public Automaton allowedActionsMatcher(String index) { | |||
| final Automaton allowedMatcher = super.allowedActionsMatcher(index); | |||
| final Automaton limitedByMatcher = super.allowedActionsMatcher(index); | |||
| final Automaton limitedByMatcher = limitedBy.allowedActionsMatcher(index); | |||
There was a problem hiding this comment.
Yes. At first sight testing for it was not trivial. The bug doesn't seem important though. I'll see what I can do.
There was a problem hiding this comment.
I've pushed a simple unit test 9738963, it is trivial after all.
The bug is not consequential. It could manifest when resizing and when adding aliases, when using keys with role descriptors, where it fails to allow the operation, although it should.
...n/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java
Show resolved
Hide resolved
|
Per #58928 (comment), @albertzaharovits and I had a chat about this offline and I want to summarise why I think this change makes sense. When we started thinking about API Keys, we planned to store them with a single list of privileges. That is, this API Key can do these things (e.g. write to a specific index). We intended that the create API Key endpoint would enforce that the privileges specified for the API Key would be a subset of the calling user's privileges. However, there are edge cases that mean that we cannot universally determine whether an API Key is a subset of a user's privileges (or alternatively, perfectly calculate the intersection of the caller's privileges with the specified privileges). So, instead the implement requires a runtime check that against the API key's privileges and the caller's privileges. That design choice matters here because it has changed the way we think about and describe API Keys. We no longer think about API Keys as a thing that has its own privilege set, so much as a thing that has its owner's privilege set, but with an optional restriction (this is reflected in the API, where it is possible to create an API key that has no restrictions and simply inherits its owner's privileges). The choice here, to swap the enclosing order in the |
| @@ -65,6 +83,17 @@ public static void assertThrowsAuthorizationException(LuceneTestCase.ThrowingRun | |||
| assertAuthorizationException(securityException, messageMatcher); | |||
| } | |||
|
|
|||
| public static Authentication createApiKeyAuthentication(ApiKeyService apiKeyService, Authentication authentication, | |||
There was a problem hiding this comment.
Something doesn't feel quite right about this.
Let's chat, but I'd prefer a public static method on the ApiKeyService (like createKeyForTesting) rather than making existing methods public.
There was a problem hiding this comment.
Yeah, I've been ignorant to break the ApiKeyService abstraction for test purposes.
I've created a public static inner class ApiKeyServiceTests.Utils that translates an authentication for a user to an authentication with an API Key.
| authenticatedBy = new RealmRef(ApiKeyService.API_KEY_REALM_NAME, ApiKeyService.API_KEY_REALM_TYPE, nodeName); | ||
| writeAuthToContext(new Authentication(user, authenticatedBy, null, Version.CURRENT, | ||
| Authentication.AuthenticationType.API_KEY, authResult.getMetadata())); | ||
| final Authentication authentication = apiKeyService.createApiKeyAuthentication(authResult, nodeName); |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine update branch |
The composite role that is used for authz, following the authn with an API key, is an intersection of the privileges from the owner role and the key privileges defined when the key has been created. This change ensures that the `#names` property of such a role equals the `#names` property of the key owner role, thereby rectifying the value for the `user.roles` audit event field.
The composite role that is used for authz, following the authn with an API key, is an intersection of the privileges from the owner role and the key privileges defined when the key has been created.
This change ensures that the
#namesproperty of such a role equals thenamesof the owner role.Relates #58928 (comment)