Skip to content

Implement lookup of permissions for API keys #35970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 6, 2018

Conversation

jaymode
Copy link
Member

@jaymode jaymode commented Nov 27, 2018

This change implements a lookup of permissions for API keys when a
request moves to authorization. In order to support this, the
authentication of an API key will attach values as metadata on the
authentication result. The values attached will include the source
of the role descriptors. The authentication service will then copy
this metadata to the authentication object and set the authentication
type to API_KEY. The authorization service will use the authentication
type to make a decision on how the roles should be obtained.

This change implements a lookup of permissions for API keys when a
request moves to authorization. In order to support this, the
authentication of an API key will attach values as metadata on the
authentication result. The values attached will include the source
of the role descriptors. The authentication service will then copy
this metadata to the authentication object and set the authentication
type to API_KEY. The authorization service will use the authentication
type to make a decision on how the roles should be obtained.
@jaymode jaymode added >non-issue WIP :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Nov 27, 2018
@jaymode jaymode requested review from bizybot and tvernum November 27, 2018 20:54
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jaymode
Copy link
Member Author

jaymode commented Nov 27, 2018

@tvernum @bizybot this is an incomplete alternative for #35546. Can you please take a look and let me know if this approach is better?

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I much prefer this approach to the one that created named roles.

@jaymode jaymode removed the WIP label Nov 30, 2018
@jaymode
Copy link
Member Author

jaymode commented Nov 30, 2018

@tvernum @bizybot I think this is at a point where it is ready for reviews.

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

public enum AuthenticationType {
REALM,
API_KEY,
TOKEN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding ANONYMOUS as an option here?
I think the current behaviour will treat it as REALM which isn't quite right.
Happy to see it in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it is. I think there can be some other values as well. Will address in a followup

Copy link
Contributor

@bizybot bizybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, had a comment on the caching of role which if desirable we can do it in next PRs. Thank you.


rolesStore.buildRoleFromDescriptors(roleDescriptorList, fieldPermissionsCache, ActionListener.wrap(role -> {
threadContext.putTransient(API_KEY_ID_KEY, apiKeyId);
threadContext.putTransient(API_KEY_ROLE_KEY, role);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this as caching the built role similar to what we do in CompositeRolesStore (roleCache where we cache the role against the set of role names). But here we are keeping it in the ThreadContext as transient, I was wondering if it would be helpful to or desirable to have a cache similar to what we have in CompositeRolesStore?
We can enhance this if required in next PRs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, this is not equivalent to caching and caching is superior so we'll want to do that.

@jaymode jaymode merged commit 476879a into elastic:security_api_keys Dec 6, 2018
@jaymode jaymode deleted the api_key_authz_take2 branch December 6, 2018 15:58
jaymode added a commit to jaymode/elasticsearch that referenced this pull request Dec 6, 2018
This change builds upon the work done in elastic#35970 and adds appropriate
types for anonymous and internal authentication to the
`AuthenticationType` enum.
jaymode added a commit that referenced this pull request Dec 13, 2018
This change builds upon the work done in #35970 and adds appropriate
types for anonymous and internal authentication to the
`AuthenticationType` enum.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants