Replace superuser role for API keys #81977
Conversation
This PR replace superuser role of an API key with the new limited superuser role to prevent write access to system indices. Relates: elastic#81400
ywangd
added
>breaking
WIP
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
tvernum
left a comment
tvernum
left a comment
There was a problem hiding this comment.
I'm happy with the approach.
I haven't reviewed 100% of the changes, but I can do a final pass in the morning.
| return List.of(limitedByRoleReference); | ||
| } | ||
| return List.of(new RoleReference.ApiKeyRoleReference(apiKeyId, roleDescriptorsBytes, "apikey_role"), limitedByRoleReference); | ||
| return List.of(new RoleReference.ApiKeyRoleReference(apiKeyId, roleDescriptorsBytes, false), limitedByRoleReference); |
There was a problem hiding this comment.
It's probably a nit, but I think an enum would be better than a boolean. Something like:
enum ApiKeyRoleType {
ASSIGNED_ROLES, LIMITED_BY_ROLES
}
(I'm happy with different names for them enum class, or members)
There was a problem hiding this comment.
I added the enum and also use the enum toString for the source of the roleKey to avoid having extra string constants. It might also become useful if later we decide to support layered limited-by. I think it's an overall improvement. Thanks for the suggestion.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
…ecurity/authc/ApiKeyService.java Co-authored-by: Tim Vernum <[email protected]>
|
@elasticmachine update branch |
|
CI Failure is not related and not reproducible. I raised #82647 |
|
@elasticmachine run elasticsearch-ci/part-1 |
This PR replace superuser role of an API key with the new limited superuser role to prevent write access to system indices. The replacement should only happen to the builtin superuser role. If there is a user-created role that has the exact same definition as the superuser role, it will not be replaced because we consider users are explicitly opt-in for ALL access in this scenario. Relates: elastic#81400
This PR replace superuser role of an API key with the new limited superuser role to prevent write access to system indices. The replacement should only happen to the builtin superuser role. If there is a user-created role that has the exact same definition as the superuser role, it will not be replaced because we consider users are explicitly opt-in for ALL access in this scenario. Backport: #82650
3 participants
This PR replaces superuser role of an API key with the new limited
superuser role to prevent write access to system indices.
The replacement should only happen to the builtin superuser role.
If there is a user-created role that has the exact same definition as
the superuser role, it will not be replaced because we consider users
are explicitly opt-in for ALL access in this scenario.
Since API key captures (snapshots) user roles when creating the key,
it is not always straightforward to decide whether the captured role descriptor
is indeed the builtin superuser, e.g. before #82049, the key role descriptors can
be created exactly like the builtin superuer (including the
_reservedmetadatafield). The solution is to only replace role descriptors from the limited-by roles because:
or they are empty (derived keys)
will not have be able to have role descriptors with
_reservedmetadata field. For oldkeys, we can add additional checks to drop any invalid metadata field (or throw errors).
In short, we should be able to prevent a custom role looks exactly like the builtin superuser
role.
Relates: #81400