Create a role per api key #35546
Create a role per api key #35546
Conversation
This commit changes how we handle roles for api keys. Previously, the role descriptors were stored inside of the api key document as a dynamic field. Storing roles this way meant that a new method for role resolution and caching would be needed, which further complicates this feature. Instead we can take the descriptors, merge them into a single descriptor, and create a specific role for the api key. This allows us to reuse the existing framework and APIs to view the permissions that a api key grants.
jaymode
added
>non-issue
:Security/Authentication
|
Pinging @elastic/es-security |
| /** | ||
| * Combines the provided role descriptors into a put role request with the specified name | ||
| */ | ||
| private PutRoleRequest mergeRoleDescriptors(String name, List<RoleDescriptor> descriptors) { |
There was a problem hiding this comment.
I guess we will test creation of role after merging in integTest or do we want to add a unit test for this?
|
run gradle build tests |
| final List<RoleDescriptor.ApplicationResourcePrivileges> applicationResourcePrivileges = new ArrayList<>(); | ||
| final List<ConditionalClusterPrivilege> conditionalClusterPrivileges = new ArrayList<>(); | ||
| for (RoleDescriptor descriptor : descriptors) { | ||
| indicesPrivileges.addAll(Arrays.asList(descriptor.getIndicesPrivileges())); |
There was a problem hiding this comment.
Hi @jaymode, I think I missed this and while working on subset code I realized whether we should be merging the indices privileges if the index names match? (Something on the lines that we do with MergeableIndicesPrivileges)
There was a problem hiding this comment.
I'm on the fence about whether this is the right direction to go in.
I see the value in making everything the same, but at the same time, these aren't just "roles", they permission sets that are represented as a role, and I think we're blurring the line here.
I just feel a bit uneasy about it.
| throw new IllegalStateException("index request cannot be null"); | ||
| } | ||
|
|
||
| client.execute(PutRoleAction.INSTANCE, putRoleRequest, ActionListener.wrap(response -> |
There was a problem hiding this comment.
Why isn't this execute performed withOrigin?
We shouldn't assume/require that all users who can create API keys can Put Roles.
| clusterPrivileges.addAll(Arrays.asList(descriptor.getClusterPrivileges())); | ||
| runAsPrivileges.addAll(Arrays.asList(descriptor.getRunAs())); | ||
| applicationResourcePrivileges.addAll(Arrays.asList(descriptor.getApplicationPrivileges())); | ||
| conditionalClusterPrivileges.addAll(Arrays.asList(descriptor.getConditionalClusterPrivileges())); |
There was a problem hiding this comment.
This could create a role descriptor that cannot be converted to XContent.
If two role descriptors have
global: { application: { manage: { applications: [ "app1" ] } } }global: { application: { manage: { applications: [ "app2 "] } } }
then the merged role descriptor cannot produce valid XContent.
The GetUserPrivileges action had this problem and had to use a different output format to accommodate the potential for multivariate fields.
There was a problem hiding this comment.
This is a good point and would break the get api once implemented.
|
@tvernum do you have any thoughts or suggestions about an alternative? |
|
Sorry for not replying earlier. I'll review the alternative proposed in #35970. My other thought on how to do this would be:
|
|
Closing this in favor of #35970 |
This commit changes how we handle roles for api keys. Previously, the
role descriptors were stored inside of the api key document as a
dynamic field. Storing roles this way meant that a new method for role
resolution and caching would be needed, which further complicates this
feature. Instead we can take the descriptors, merge them into a single
descriptor, and create a specific role for the api key. This allows us
to reuse the existing framework and APIs to view the permissions that
a api key grants.