Skip to content
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

Support better re-use of API key role caching #53939

Closed
ywangd opened this issue Mar 23, 2020 · 3 comments · Fixed by #58156
Closed

Support better re-use of API key role caching #53939

ywangd opened this issue Mar 23, 2020 · 3 comments · Fixed by #58156
Assignees
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team

Comments

@ywangd
Copy link
Member

ywangd commented Mar 23, 2020

The caching of API key role is indexed by the role names and the key ID. Therefore, even when two keys have exactly the same set of role descriptors, their roles must be calculated separately and also counted as two independant cache item. In addition, a single key's role descriptors and and limiting role descriptors are also cached seperately. This means we need at least 2 caching slots for every API key.

When there is a large amount of API keys, we could face a role cache thrashing problem. The default cache size is 10k so that 5k keys will be able to saturate it. If any of the role descriptor includes a non-empty application privilege, it could further saturate the search thread pool, which in turns leads to an overloaded cluster.

It is reasonable to assume that most of the 5k keys share the same privilege definitions. Hence the caching problem can be improved if a Role built from the same set of role descriptors can be re-used. That is, despite different API key IDs and possibly different role descriptor names, if the content of a set of role descriptors are the same, we should be able to cach and re-use it.

@ywangd ywangd added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Mar 23, 2020
@elasticmachine
Copy link
Collaborator

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

@ywangd ywangd added :Security/Security Security issues without another label and removed :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Mar 23, 2020
@ywangd
Copy link
Member Author

ywangd commented Apr 6, 2020

Authorization is performed on each node. While we can reduce the size required by API key cache on each node by adding more nodes (with sticky sessions), the role cache on each node will likely need to cater for all keys. This is another reason that role cache could use further optimisation.

@rjernst rjernst added the Team:Security Meta label for security team label May 4, 2020
@ywangd
Copy link
Member Author

ywangd commented Jun 12, 2020

A straightforward approach (and possibly minimum effort) is to cache a API key role by ApiKeyRoleDescriptors. Note that ApiKeyRoleDescriptors has a field of apiKeyId which should be excluded from equals and hashCode for it to be reusable across different keys.

The current roleCache is keyed by RoleKey, which is not compatible with ApiKeyRoleDescriptors. A separate apiKeyRoleCache is required (along with other necessary supporting code). Caching this way is mostly helpful when large number of keys share the same role, but wil likely be worse when their roles are all distinct, in which case the keys (ApiKeyRoleDescriptors) could be more expensive than the current RoleKey object.

To ensure equality of two sets of role descriptors, it is necessary to compare every element of it to be sure. Other than comparing the ApiKeyRoleDescriptors object directly, we could also compare their in string representation. This could be faster for operations like Cache#get, but any benefit could be offset'd by having to perform ApiKeyRoleDescriptors#toString beforehand. A better way around it is make the string format available during authentication process and use it for caching purpose, and only deserialised it when it is necessary to build the Role object. This however requires changes to API key document handling during the authentication process, which will be discussed in #53940.

Using either ApiKeyRoleDescriptors or its string representation as cache key is still not idea since the key is large and could in theory be unbounded because it is user input. This concern really calls for something like API key template or cloneable API key so that the cache key could be either Tuple<TemplateId, Version> or clonedFromId. Without it, an alternative to reduce the key size is obviously hashing. We could compute a hash for ApiKeyRoleDescriptors and use the hash as the key. This would introduce additional hashing cost. But if we could settle with something simple, e.g. sha256, it may not be a big issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants