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

Add granular privileges for API keys #42020

Closed
wants to merge 32 commits into from

Conversation

bizybot
Copy link
Contributor

@bizybot bizybot commented May 9, 2019

In the current implementation of API keys, to create/get/invalidate
API keys one needs to be a superuser which limits the usage of API keys.
We would want to have fine-grained privileges rather than system-wide
privileges for using API keys.

  • manage_api_key cluster privilege which allows users to create, retrieve
    and invalidate any API keys in the system. This allows for limited access than
    manage_security or all privileges.

  • manage_own_api_key is a conditional cluster privilege which allows creating,
    retrieving and invalidation of API keys owned by the currently authenticated user.

This commit adds support for conditional cluster privileges for API keys so we can create privileges like manage_own_api_key. The conditional cluster privilege names can be used to specify cluster privileges.

This commit does not:-

  • documentation changes

In the current implementation of API keys, to create/get/invalidate
API keys one needs to be super user which limits the usage of API keys.
We would want to have fine grained privileges rather than system wide
privileges for using API keys.

- `manage_api_key` cluster privilege which allows users to create, retrieve
  and invalidate any API keys in the system. This allows for limited
  access than `manage_security` or `all` privileges.

To support scenario's where we want to give granular privileges this commit
adds support for conditional cluster privileges where we can create different
combinations of `create`, `get`, `invalidate` API key privileges for
restricting actions to API keys:
- owned by the user
- owned by group of users
- owned by group of users under specified realms
- any user from any realm

This commit does not:-
- define any new API for ease, this will be taken up later after we decide if this
approach works
- HLRC changes
@bizybot bizybot added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.2.0 labels May 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

this.users = (users == null) ? Collections.emptySet() : Set.copyOf(users);
this.usersPredicate = Automatons.predicate(this.users);

this.requestPredicate = (request, authentication) -> {
Copy link
Contributor Author

@bizybot bizybot May 9, 2019

Choose a reason for hiding this comment

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

This takes care of enforcing the authorization, makes use of Authentication details containing the principal or API key id from authentication metadata.
For owner API keys enforcement, the request must contain either (username, realmName) or apiKeyId

*/
public void getApiKeyForApiKeyName(String apiKeyName, ActionListener<GetApiKeyResponse> listener) {
public void getApiKeys(String realmName, String username, String apiKeyName, String apiKeyId,
Copy link
Contributor Author

@bizybot bizybot May 9, 2019

Choose a reason for hiding this comment

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

ApiKeyService has two APIs for retrieving and invalidating API keys.
They do not enforce any authorization but with validation that atleast one of the parameters must be specified.

getApiKeys(String realmName, String username, String apiKeyName, String apiKeyId);
invalidateApiKeys(String realmName, String username, String apiKeyName, String apiKeyId);

return sameUsername;
} else if (request instanceof GetApiKeyRequest) {
GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
Copy link
Contributor Author

@bizybot bizybot May 9, 2019

Choose a reason for hiding this comment

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

This is required for case where the API key is used for authentication and it should be able to get the details and it does not have any API key privileges in the assigned role.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this accounted for in ManageApiKeyConditionalClusterPrivilege#checkIfUserIsOwnerOfApiKeys:

        if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
            // API key id from authentication must match the id from request
            String authenticatedApiKeyId = (String) authentication.getMetadata().get("_security_api_key_id");
            if (Strings.hasText(apiKeyId)) {
                return apiKeyId.equals(authenticatedApiKeyId);
            }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code in the ManageApiKeyConditionalClusterPrivilege would be used only when the user has manage_own_api_key privilege assigned. In the case where an API key was created but we did not give it any permission related to API key management then, at the least it should be able to access its own details. This code checks for that condition.

import java.util.function.BiPredicate;
import java.util.function.Predicate;

public final class ManageApiKeyConditionalPrivileges implements ConditionalClusterPrivilege {
Copy link
Contributor Author

@bizybot bizybot May 9, 2019

Choose a reason for hiding this comment

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

With conditional privileges for API keys, the Rest APIs remain same but when conditional privileges restrict access of API keys to owner only then there is inherent requirement that requests must either contain (username & realmName) or (apiKeyId) or else the request fails.

* ability to execute actions related to the management of application privileges (Get, Put, Delete) for a subset
* of applications (identified by a wildcard-aware application-name).
*/
public static class ManageApplicationPrivileges implements ConditionalClusterPrivilege {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been moved to its own file.

@bizybot
Copy link
Contributor Author

bizybot commented May 10, 2019

@elasticmachine run elasticsearch-ci/1

@tvernum
Copy link
Contributor

tvernum commented May 20, 2019

To support scenario's where we want to give granular privileges this commit
adds support for conditional cluster privileges where we can create different
combinations of create, get, invalidate API key privileges for
restricting actions to API keys:

What's the rationale for adding this level of complexity?
I thought we had settled on there simply being a privilege to manage your own API keys, but this is taking it in another direction.

My primary concern is that the Kibana security management UI doesn't support "global" privileges, so they have to be edited manually, so I feel like we've made this much more complex for ourselves as well as for security admins, without know what use case that we're trying to solve.

this.usersPredicate = Automatons.predicate(this.users);

this.requestPredicate = (request, authentication) -> {
if (request instanceof CreateApiKeyRequest && privilege.predicate().test(CREATE_API_KEY_PATTERN)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to check the privilege here do you? ConditionalClusterPermission always checks the predicate against the action being executed, so why do we need to check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need not, removed the check. Thank you.

final GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
if (this.realms.contains("_self") && this.users.contains("_self")) {
return checkIfUserIsOwnerOfApiKeys(authentication, getApiKeyRequest.getApiKeyId(), getApiKeyRequest.getUserName(),
getApiKeyRequest.getRealmName());
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right - it means if you have _self then you can only ever get your own keys (it never falls through to the checkIfAccessAllowed condition to allow you to get other keys that you might have permission over).
But nothing in this class enforces that _self must be the only item in the list, so "realms": [ "_self", "native" ], "users": [ "_self", "*"] wouldn't actually let me manage API keys for the native realm, but nor will it give me an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I think I had this test case to enforce that when _self is used it must be the only item but missed it while writing the code. I have enforced the check.

}

private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, String apiKeyId, String username, String realmName) {
if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these string checks to somewhere central, like a method on ApiKeyService :

public static boolean isApiKeyAuthentication(Authentication auth) { ... }

or even better

public static String getAuthenticatedApiKey(Authentication auth) { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am with you on adding the check in ApiKeyService but the current code structure does not allow it to do so. ManageApiKeyConditionalPrivileges is in core due to it being used when we parse conditional privileges in role descriptor. It feels like we need some refactoring here to push the classes to security but unsure of what all things should we move. Do you want to tackle this here with this PR or as a separate PR for refactoring?

String authenticatedUserRealm = authentication.getAuthenticatedBy().getName();
if (Strings.hasText(username) && Strings.hasText(realmName)) {
return username.equals(authenticatedUserPrincipal) && realmName.equals(authenticatedUserRealm);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this behaviour is what we'd want when run_as is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. This does not take run_as scenario into consideration as we currently do not support run_as for token based schemes.
As I understand if we were to then the condition needs to be updated to check the realm name against the lookedUpBy realm instead of authenticatedBy realm.
Do we want to handle the scenario here with this PR as right now we do not support run_as for API keys?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow why run_as for API keys would come in to play here.

The question is What should the behaviour be if I authenticate as elastic and run-as bizybot and then try to delete one of bizybot's keys?

@bizybot
Copy link
Contributor Author

bizybot commented May 30, 2019

What's the rationale for adding this level of complexity?
I thought we had settled on there simply being a privilege to manage your own API keys, but this is taking it in another direction.

My primary concern is that the Kibana security management UI doesn't support "global" privileges, so they have to be edited manually, so I feel like we've made this much more complex for ourselves as well as for security admins, without know what use case that we're trying to solve.

I agree the current proposal is verbose. As in we need to specify: cluster actions, realms, users and create the conditional cluster privilege.

I started this to see if we can construct conditional privilege, for example, creation and retrieval of API keys limited to self. I must admit that I did not consider Kibana UI, though I feel that UI should not drive the decision here.

The next step that I thought was to create named conditional cluster privileges similar to the cluster privilege names and use these names when constructing the role.
Though this requires an additional field in the role descriptor (we can enable this in the UI similar to cluster field) instead of nesting it under global field.
I was not sure if we would want this and so I went with the current structure which allows the user to create its own conditional privilege based on its requirements.

I have restricted the model to be limited to manage API keys with this PR, but we could enable this conditional cluster privileges generically so any combination of cluster actions could be used giving flexibility in the field to construct role where we miss some cluster privileges (Ref: #30078)

I agree specifying the cluster permissions and creation of conditional cluster can be complex for security admins and so I think we would like to have some notion of named conditional privileges which come by default and also allow the flexibility to do combinations of cluster actions as an advanced capability.

Also, this does not require us to add any extra APIs and is solely driven by the user's role.

WDYT @tvernum @albertzaharovits

@tvernum
Copy link
Contributor

tvernum commented May 30, 2019

The next step that I thought was to create named conditional cluster privileges similar to the cluster privilege names and use these names when constructing the role.
Though this requires an additional field in the role descriptor (we can enable this in the UI similar to cluster field) instead of nesting it under global field.

Why would this be the case?
It just means that Role.Builder.cluster() needs to know that some privilege names might actually be implemented as a ConditionalClusterPrivilege rather than a ClusterPrivilege

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

Overall, I think this tries too hard to hide that manage_own_api_key is a ConditionalClusterPrivilege. For example:

  • The ClusterPrivilegeResolver combines GlobalClusterPrivileges with ClusterPrivileges . The ability to name certain GlobalClusterPrivileges and to be able to refer to them by their name as a ClusterPrivilege is interesting but it complicates everything. I would suggest we have a separate PR for this. What do you think? Is there a subtler issue behind all this, in which case I completely missed it?
  • ManageApiKeyConditionalClusterPrivilege is not a GlobalClusterPrivilege because then it will be grouped as such in the role descriptor.
  • ManageApiKeyConditionalClusterPrivilege is configurable with restrictActionsToAuthenticatedUser , presumably so that manage_api_key and manage_own_api_key names as cluster privileges instantiate this conditional cluster privilege with different parameters.

I think I have an idea on how to simplify this, but I need to try and code it first so that we minimize the noise here.

return sameUsername;
} else if (request instanceof GetApiKeyRequest) {
GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this accounted for in ManageApiKeyConditionalClusterPrivilege#checkIfUserIsOwnerOfApiKeys:

        if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
            // API key id from authentication must match the id from request
            String authenticatedApiKeyId = (String) authentication.getMetadata().get("_security_api_key_id");
            if (Strings.hasText(apiKeyId)) {
                return apiKeyId.equals(authenticatedApiKeyId);
            }
        }

@albertzaharovits
Copy link
Contributor

I think I have an idea on how to simplify this, but I need to try and code it first so that we minimize the noise here.

What I had in mind is likewise nasty. I wanted to move checkSameUserPermissions from RBACEngine to the Role. This way manage_own_api_key would not authorize any action, but there would be a hack in checkSameUserPermissions that checks this privilege and the request&action.
In my opinion the root of the complication is that we try to have the manage_own_api_key label as a cluster privilege that actually behaves as a ConditionalClusterPrivilege. I think any implementation of this is bound to get nasty. If this is the case I prefer to contain the "nastiness" in as a narrower portion as possible, eg don't change class hierarchies.

I will spend some more time tomorrow, but I would very much enjoy smaller PRs, because this is difficult to review.

@tvernum
Copy link
Contributor

tvernum commented Jun 18, 2019

In my opinion the root of the complication is that we try to have the manage_own_api_key label as a cluster privilege that actually behaves as a ConditionalClusterPrivilege

I agree, but I see it in the reverse.
My thinking has two parts that clash slighty, but I have a strong preference on each:

  1. ConditionalClusterPrivilege is the preferred model moving forward. Authorizing on an action name only is historical and limiting us, and we need to move towards a model where we are able to look at the request and user (Authorization) for more privileges. But none of that should matter to end users. If we have to write docs that say "Because this privilege needs to check the request, it is handled differently", then I think we've failed.

  2. The "global" privileges model for the API is probably good (we haven't used it enough to be sure how it will play out), but it's far more complex for users than the "cluster" model of named privileges. Partly that is a UI issue, but it's more than that, it risks making simple thing complex. We want simple things to be simple, so if we start putting all cluster-level privileges into global, then we need to really think about how we would make that as simple as the current "cluster" privileges.

Based on those things, I feel pretty strongly that we can't tell users that "manage_own_api_keys" needs to be in "global" but "manage_api_keys" goes into "cluster", just because. There's no reason for users to consider those to be different types of things, so they should go into the same place in the role definition.

I think the problem you're seeing is more a case that we're trying to hold on to the plain old ClusterPrivilege rather than being caused by pushing ConditionalClusterPrivilege in.
I would like to resolve this weirdness (that definitely exists) by asking why we need to keep ClusterPrivilege around and working to minimise that problem of having two different types for what is logically the same concept: a name that describes a thing that a user can do on the cluster.

@tvernum
Copy link
Contributor

tvernum commented Jun 18, 2019

The question is how we get there. I'm worried about how long thing PR has been floating about and where we find the boundary for what it should deliver. From the beginning I've been pushing for this to provide the bare minimum of features, but to implement them correctly. And I think it's close to doing that - except that the ClusterPrivilegeResolver isn't quite right.

In my opinion, ClusterPrivilegeResolver does the correct thing (in broad terms) of encapsulating the resolution of "names" to privileges. But it falls down in the types that it returns. 80% of the callers of this method actually only want ClusterPrivilege and ignore ConditionalClusterPrivilege. That's a sure sign that something is not right.

I also have some ideas on how to clean that up, but I don't know if we can solve them completely in this PR. Like @albertzaharovits, I'd need to go away and actually play with the code and propose a set of refactorings that get us where we need to go.

So, my proposal would be to get this PR to the point where we can merge it into a feature branch, so that we can then have separate PRs to do that cleanup.
That would require that we reach agreement that the direction it's going in is correct (*) and that we can get it to completion by iterative evolution.
Let's do what we need to do to get to that consensus, and then we can merge & evolve.

(*) Or, probably correct - we can throw away/re-write a feature branch if we decide to, but let's at least avoid going down paths that we don't believe will work.

@albertzaharovits
Copy link
Contributor

Excellent write-up Tim! I agree with all your points.
Then if the sub-problem is framed as "find a way to make cluster and global privileges in the role descriptor map to the same abstract privilege in the code" we already have a more approachable issue, leaving aside the "self" aspect altogether. And this would be a nice independent refactoring task.

In my opinion, ClusterPrivilegeResolver does the correct thing (in broad terms) of encapsulating the resolution of "names" to privileges. But it falls down in the types that it returns. 80% of the callers of this method actually only want ClusterPrivilege and ignore ConditionalClusterPrivilege. That's a sure sign that something is not right.

In principle I agree, but practically it still needs iterations. For example ConditionalClusterPrivilege is not a Privilege but it contains a privilege. I believe making ConditionalClusterPrivilege a ClusterPrivilege would simplify ClusterPrivilegeResolver and its callers.

@tvernum
Copy link
Contributor

tvernum commented Jun 18, 2019

For example ConditionalClusterPrivilege is not a Privilege but it contains a privilege. I believe making ConditionalClusterPrivilege a ClusterPrivilege would simplify ClusterPrivilegeResolver and its callers.

I agree that a big part of the solution for clearing this up will be to find a common base class / interface for the things that the resolver returns.
I'm not quite so confident that ClusterPrivilege is that base class, but that's one of the things that will be easier to solve by trying than talking.

@bizybot
Copy link
Contributor Author

bizybot commented Jun 18, 2019

PR build failed due to existing intermittent issue #43315
@elasticmachine run elasticsearch-ci/bwc

@bizybot
Copy link
Contributor Author

bizybot commented Jun 18, 2019

@elasticmachine run elasticsearch-ci/1

@bizybot
Copy link
Contributor Author

bizybot commented Jun 19, 2019

Thank you @albertzaharovits and @tvernum.
I agree with the comments related to the ClusterPrivilegeResolver and the return type problem.
I was trying to avoid major refactoring as we had a couple of classes like ClusterPrivilege marked final and I did like the ConditionalClusterPrivilege which was a composition rather than tied to hierarchy. But looking at the behavior expected from ClusterPrivilegeResolver#resolve I liked the @albertzaharovits proposal of introducing a hierarchy to make it a ClusterPrivilege.
I have tried to limit again with the refactoring constraining myself with the cluster privilege only, though I have introduced an interface ConditionalPrivilege which may allow us in future to introduce for ex. ConditionalIndexPrivilege (IndexPrivilege constrained by what's in the request and in the context of current authentication). Please take a look at the current classes and see if this seems to be a direction that we might pursue. Thank you.

Note: As suggested we can merge into a branch and raise the refactoring PR which will be easier to review. I can revert and raise another PR if you feel the same. Thanks.

public List<Tuple<ClusterPrivilege, ConditionalClusterPrivilege>> privileges() {
return Collections.singletonList(new Tuple<>(super.privilege, null));
public List<ClusterPrivilege> privileges() {
return Collections.singletonList(super.privilege);
}
}

/**
* A permission that makes use of both cluster privileges and request inspection
*/
public static class ConditionalClusterPermission extends ClusterPermission {
Copy link
Contributor

Choose a reason for hiding this comment

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

ConditionalClusterPermission and SimpleClusterPermission seem to overlap in scope.


import static java.util.Map.entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

ClusterPrivilege got barren, we should probably remove it.

INVALIDATE_API_KEY_PATTERN);

private final boolean restrictActionsToAuthenticatedUser;
private final BiPredicate<TransportRequest, Authentication> requestPredicate;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not accounted for in hashCode and equals.

return username.equals(authenticatedUserPrincipal) && realmName.equals(authenticatedUserRealm);
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very comfortable with the option of interpreting values for user and realm based on the privilege it has as we do not know the intent of the user invoking the API. For eg. when the user does not specify user and realm, and the user has manage_own_api_key we interpret it as self and continue with the operation.

The API is designed to query by id, name, user and realm and to have visibility over the whole pool of keys.
If the API can query by user and we want to limit it, then we either forbid the parameter in a new API or authorize the parameter value. If we authorize the parameter value, what happens if the parameter is not specified; we either reject it or fill in a default value that will not reject (un-authorize) the request.

I honestly don't know what to do in this case, I would suggest we discuss this as a team.

@albertzaharovits
Copy link
Contributor

Sorry @bizybot I think this touches too much of the authz process at once for me to be confident to Okay it.
I think we can have a feature branch with this code, to see it all passing CI and having an overview of the goal, but then I would really want us to split it into small bits when we merge to master.

This is my suggestion, what do you think? I can help, and take parts of it. If you or others feel otherwise I could give it another thorough round.

@tvernum
Copy link
Contributor

tvernum commented Jun 28, 2019

Given where we're at with this, and the limited availability we've had for people to work on it, I think the first step we can take right now is to split manage_api_keys into a standalone PR so it can get merged and we can release something that doesn't require superuser / manage_security.

It won't be enough, but it's a step that we can take. I'll see if I can get that up in the next few hours.

The other option we can look at in the short term is to add a create_api_key privilege. I think that's a straight-forward implementation, but it's not a feature that's very useful without also being able to get them, so will need to see if it's worth bothering in the short term.

@bizybot
Copy link
Contributor Author

bizybot commented Jun 28, 2019

Thanks, @tvernum for raising a separate PR for the short term fix.

I was going to suggest the same.
This PR has grown from just addressing API key privileges to an additional refactoring of cluster privilege model. This makes the PR too big for a review and many ideas on how to do it.

I think the next step would be to split this PR into two:

  • PR which handles the cluster privileges refactoring first, making it more suitable for things like API keys to come in. This will also reduce the bias towards API keys when looking at the code.
  • the next PR which handles the API key privileges (namely manage_own_api_key. This will build on the refactoring which we would have agreed upon.
    This helps us in the review and focusing on only one thing at a time, reducing the noise and churn.

@jpountz jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019
@bizybot
Copy link
Contributor Author

bizybot commented Jul 30, 2019

Closing this in favor of splitting this into refactoring and API key privilege PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants