Add granular API key privileges

[bizybot]

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.
This commit adds:

• 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.
• owner_manage_api_key cluster privilege which allows user to create,
  retrieve and invalidate API keys owned by this user only.
• create_api_key is a sub privilege which allows for user to create but
  not invalidate API keys.
• an API key with no api key manage privilege can retrieve its own information

Also introduces following rest APIs to manage owned API keys for a user:
GET /_security/api_key/my
DELETE /_security/api_key/my

Closes #40031

[elasticmachine]

Pinging @elastic/es-security

[albertzaharovits]

I confidently started the review by nit-picking, but then discovered a more important thing that I feel we need to discuss. Please feel free to ignore/dismiss my nitpicks.

Do we need to add another endpoint for my API keys? As a client of the API, I don't like to have two endpoints for the same thing. I also find it hard to explicate what "my" API key actually means; I don't like it because it forces the user to keep ownership info (client credentials) together with the api key names so that it can decide which endpoint to use. And ownership is internal to the ES authorization process.

More to that, we have a perfectly working mechanism for "same user" authorization that we can adapt for the API key action ("same user" actions don't require cluster privileges, but we can make them so, as a matter of fact I think we should, because change passwd/has priv is as much a DOS as creating API keys).
The problem with this is that we have to decide a "default" value for the user query parameter, depending on the principal's privileges (if it has manage_api_keys the user defaults to all, otherwise it defaults to self). For this we can call has_privileges on the transport handler. That's a fine enough solution, IMO. We can also just default the user query parameter always to self, but this is a bit limiting for the superuser.

I think that we have the opportunity to make the API cleaner by doing just a bit of extra logic on the transport action (although code wise it would be leaner as well).
@bizybot @tvernum what you reckon?

[albertzaharovits]

This is doing request validation in the business logic. I see this is duplicated in InvalidateApiKeyRequest#validate .

[bizybot]

The ApiKeyService is an interface defining some contracts.
Now in our case, maybe we never directly invoke methods on ApiKeyService and we always go via transport service. If that is the case and the validation is guaranteed, then we can remove this. This was the reason behind adding the validations in the service layer.
Let me know your thoughts on this. Thank you.

[albertzaharovits]

I see. Thanks for carefully considering it.

My impression is that the interface contract for the ApiKeyService#invalidateApi is at a too high level, it is the interface of the transport action. And this is the reason that validation has to be duplicated here. Instead, I would design the ApiKeyService interfaces at a lower level and have the transport action do some work on its abstraction level before calling these lower level interfaces. For example:

ApiKeyService would contain:

• findApiKeysByName
• findApiKeysByUserAndRealm
• InvalidateApiKeysById

The transport action does:

if (Strings.hasText(realmName) == false && Strings.hasText(userName) == false && Strings.hasText(apiKeyId) == false && Strings.hasText(apiKeyName) == false) {
  listener.onFailure(new IllegalArgumentException("one of [api key id, api key name, username, realm name] must be specified"));
} else if (Strings.hasText(apiKeyId) || Strings.hasText(apiKeyName)) {
  if (Strings.hasText(realmName) || Strings.hasText(userName)) {
    listener.onFailure(new IllegalArgumentException("username or realm name must not be specified when the api key id or api key name is specified"));
  } else if (Strings.hasText(apiKeyId) && Strings.hasText(apiKeyName)) {
    listener.onFailure(new IllegalArgumentException("only one of [api key id, api key name] can be specified"));
  } else if (Strings.hasText(apiKeyId)) {
    InvalidateApiKeysById(apiKeyId);
  } else {
    findApiKeysByName(apiKeyName, ids -> InvalidateApiKeysById(ids));
  }
} else {
  findApiKeysByUserAndRealm(userName, realmName, ids -> InvalidateApiKeysById(ids));
}

What do you think?

[tvernum]

Per our offline discussion, I think you need to decide what role ApiKeyService wants to play.

• If it's a generic service to be used by a variety of actions, then it shouldn't be tied to a request object, and should instead have logical method names.
• If it's just a way to collect together a bunch of behaviour for a few transport actions, then it doesn't really need the same set of validation.

Like @albertzaharovits, I would prefer it be a real service with properly named methods & logical parameter. But at the moment it doesn't know what it is, so let's work that out first.

[albertzaharovits]

Why is this necessary? We're not authorizing GetMyApiKeyRequest and InvalidateMyApiKeyRequest as "same user permission" type of requests, aren't we?

But maybe we should...

[bizybot]

By default any user who can create API key request can get its own API key. But for invalidating API key user must either have manage_api_key or owner_manage_api_key privilege. This is the reason this checks for only GetMyApiKeyRequest and allows the action to get but not others.

I think the assert is here in case someone accidentally adds action to the SAME_USER_PRIVILEGE privilege but skips the check for request. Most of the request except for GetMyApiKeyRequest is a UserRequest.

[albertzaharovits]

I see, GetMyApiKey is authorized implicitly, but create and invalidate are not.
But, isn't the condition if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) checking that the authn has been performed using the API key?
If this is the case, what would be the benefit of the client that authn with the API key to get his own api keys, but if he authn with the password to be rejected? (Assuming he will be rejected, because it is unlikely, since to "get" them they must have been created by the "self" and the privilege to create grants get as well.)

[albertzaharovits]

I don't like that we are doing request validation too deep into the handler logic.
Also, I would not pass the GetApiRequest around, but unpack it in the transport handler. This is so that the ApiKeyService methods be usable in other places without requiring the callers to build a GetApiKeyRequest.

[bizybot]

As mentioned above in one of my earlier comments, ApiKeyService if we allow calling from other places directly we need this validation at the service layer.
As you mention about callers invoking the ApiKeyService from other places without going via transport service I feel that the validation at service layer is good, though would like to reduce the duplicity.

If we are not okay with the passing the *Request or building the *Response at service layer then we would be introducing additional conversion between the models and I am not sure if we need this. If you strongly feel we should not be using them then I can pick this as a separate refactoring change as I have used not just request but also constructing response object in API key service. Let me know what you think?

[albertzaharovits]

As mentioned above in one of my earlier comments, ApiKeyService if we allow calling from other places directly we need this validation at the service layer.
As you mention about callers invoking the ApiKeyService from other places without going via transport service I feel that the validation at service layer is good, though would like to reduce the duplicity.

I've detailed my thinking for the interface at the ApiKeyService level in #41488 (comment) . I think the argument validation has to be done by the callee, so not relying on the caller doing it, but the validation should be done for the proper abstraction level. For example, findApiKeysByUserAndRealm would be validating that userName and realmName are not null simultaneously.

[albertzaharovits]

I would also move these in the transport handler. We're doing too much request processing here; we're not getting the "api key for the current user" but something more complex...

[albertzaharovits]

Also, I see the user of the API key is confined to see only that API key. Then I believe we should also confine the invalidation.

[albertzaharovits]

Do we need another REST endpoint? I am leaning towards not adding another endpoint.

Reasons:
As a client of the API, if I get a hold of such a token name (apiKeyName), which endpoint should I use to get/invalidate the associated key? To decide on the endpoint, as a client, I have to know the ownership of the token, and my principal on ES. Basically the privilege on the ES side correlates with the endpoint on the client; if my user was restricted to only its own keys, but then the admin grants the service user more privileges,I will also have to change the application logic to modify the URL endpoint, to take advantage of the elevated privileges.

[bizybot]

Thank you, Albert. Your comments are valuable.

[I am commenting here as this would create the discussion thread]

In my opinion, usage of my, me, mine is okay and we can choose if we want something else here.

I would like to start with the use cases for which we require invalidation API:

• Admin user who wants to invalidate 1 or many API keys using combinations of username, realm name, id and key
• User who has created 1 or many API keys (it is the owner of these API keys) and wants to invalidate API keys owned by it using either key id or name
• An API key authenticating and invalidating itself using either key id or name, mostly nothing as it is logged in using API key

I did give a thought of having just one API and we implicitly try and do the right thing depending on the request parameters, privilege this user has.
Your solution would work till the point of authorizing the action. In the transport action we need to either invoke invalidateApiKeysForCurrentUser or invalidateApiKeys we need to
know the cluster privilege or else we will be passing what is in the InvalidateApiKeyRequest. In case of self invalidation of API keys the parameters like realm name or username may be wrong and so this is important. We can do this by fetching the privileges for the authenticated user not that it cannot be done.

For ex. User who is trying to invalidate API keys it owns,

DELETE /api_key
{
  "id": "api-key-id",
  "name": "api-key-name",
  "realm_name": "some-realm"
  "username": [_'all'_, _'self'_, 'actual-user-name']
}

In this case we have unnecessary parameters like realm_name and username in the request which say defaults to self.
This requires us to compare the privilege, see if it is owner_manage_api_key and then deny access if the realm_name and username do not match our expectations (authenticated user details). This seems to be an authorization check performed outside authorization engine.

I think the APIs, though extra work bring out clarity into the responsibility and also enforce the behavior, not trying handle internally based on the request parameters.

The application scenario that you described can be designed with two user options:

• invalidate API keys
• invalidate my API keys

The user using the application can invoke any of those APIs, and depending on the privilege on ES the user will be authorized for the action. Say admin user logs on to the application, it can use any of the actions. For a user with owner_manage_api_key can only invoke invalidate my API keys but for other operation it gets access denied. And say for user with no api key privilege will be denied access to both APIs.

But I am happy to revisit this if we all think that the extra API does not serve its purpose.
I think looking at the comments, I feel we should discuss this offline, will connect on slack and then we can summarize.
Thank you for your comments.

[albertzaharovits]

I understand your reasoning and you're right, but I still think we can use the authz framework we have and still have a single REST endpoint. Let me try to detail it here:

• Looking at RestGetApiKeyAction#innerPrepareRequest, it is building theGetApiKeyRequest, and then calls the transport action that will be authorized. The username parameter is optional (as are all the others). I propose we "infer" the username parameter, in case it is not specified in the request (otherwise we leave it be).
• The username will be null (or * or _all depending on the interface we decide further) if the user has the manage_api_key privilege or Authentication.getAuthentication(threadPool.getThreadContext()).getUser().principal() otherwise. To check the privilege we make a has privilege call.
• We would then tweak the RBACEngine to do the same thing it does for UserRequests but for GetApiKey and InvalidateApiKey. Basically, authorize the action if the request has the same name as the principal.
• at the ApiKeyService level, call findApiKeysForUserRealmApiKeyIdAndNameCombination . Note that it is no longer correct to check the

        if (Strings.hasText(apiKeyId) || Strings.hasText(apiKeyName)) {
            if (Strings.hasText(realmName) || Strings.hasText(userName)) {
                listener.onFailure(new IllegalArgumentException(
                        "username or realm name must not be specified when the api key id or api key name is specified"));
            }
        }

condition at this level, because the username can be specified by the Rest handler, as detailed. This validation should be performed in the REST handler.

What do you think?

[tvernum]

I haven't had time to digest @albertzaharovits's proposal, and I want to submit my review comments, so let me just say:

• I would love to not have too many new Rest APIs for this. None is great, but even 1 new API is better 2 or 3.
• But we do need:
  • a security model that makes sense and we can review & understand
  • a user-level API that is clear enough that we can document it (including the security rules)

So if we need a new REST API to satisfy those last 2 requirements, then so be it. But if we can avoid it, that's great.

[albertzaharovits]

And I realize this was to be discussed in the issue, and I apologize for not bringing it up then, I haven't been thinking it thoroughly at that time... I'm sorry Yogesh!

[tvernum]

I wanted to get some comments done today, but I haven't had time to review this completely.

[tvernum]

Suggested change

	NOTE: If none of the parameters are set, it will invalidate API keys owned by the authenticated user.
	NOTE: If none of the parameters are set, it will invalidate all API keys owned by the authenticated user.

[tvernum]

I don't think we want separate actions for this. I know it was discussed in a previous meeting, but I don't think it's the right approach.

When we discussed this previously, what I was trying to propose was:

• the existing requests & transport actions would be changed so that it's possible to specify a username on any request, regardless of other parameters (e.g. it would be possible to specify invalidate the key with id {foo} owner by user {bar})
• the api key service would be updated to enforce the above, so, for example, there's a method with a contract of "delete the api key with id {foo}, iff it is owned by {bar}"
• the specific "my" rest actions (if we choose to have them) would simply call the same underlying transport actions with the same request objects, but always set the username to the logged in user.
• create_api_key would be a conditional cluster privilege that only allows either:
  1. requests that explicitly specify a "username" that matches the effective authz user
  2. requests that explicitly specify the "id" that was used to authenticate the effective authz user

The consequences/benefits of that approach are that

1. the /my endpoints are simply a convenience, but all the existing APIs continue to work if you specify the correct parameters. You can use /_security/api-key?username=bar and it will work (if you are user bar), or /_security/api-key?id=xyzzy (if you are authenticating with the xyzzy key).
2. we don't end up with a bunch of duplicated transport actions, requests, etc
3. the privileges model is a little more complex, but it's far more consistent - each privilege grants the right actions for the use case rather than having 2 security models that operate side-by-side.

[albertzaharovits]

This is what I have in my mind as well.
Just two notes to add:

• my suggestion about the has_privilege user inference can be seen as an enhancement, for when the REST endpoint does not contain the "username", only the "id" or "name", (in get and invalidate, NOT in create) but the client expects it to "work". For example, an invalidate by api key name.
• I have qualms if the create_api_key priv can be made to work without changes to the engine.

[tvernum]

This is a problem, because it implies that if I have create_api_key I can call get/my but not get/{id}.
Having to pick which API I call based on which permissions I have is not very friendly.

[tvernum]

It's a bike shed, but I prefer manage_own_api_key

[tvernum]

Per our offline discussion, I think you need to decide what role ApiKeyService wants to play.

• If it's a generic service to be used by a variety of actions, then it shouldn't be tied to a request object, and should instead have logical method names.
• If it's just a way to collect together a bunch of behaviour for a few transport actions, then it doesn't really need the same set of validation.

Like @albertzaharovits, I would prefer it be a real service with properly named methods & logical parameter. But at the moment it doesn't know what it is, so let's work that out first.

[tvernum]

This looks like code that should live in the privileges layer, not in a service.

[tvernum]

I would like us to move away from adding more special cases to RBACEngine.
We should aim to put as much as we can in the privileges model, not the engine.

[tvernum]

I haven't had time to digest @albertzaharovits's proposal, and I want to submit my review comments, so let me just say:

• I would love to not have too many new Rest APIs for this. None is great, but even 1 new API is better 2 or 3.
• But we do need:
  • a security model that makes sense and we can review & understand
  • a user-level API that is clear enough that we can document it (including the security rules)

So if we need a new REST API to satisfy those last 2 requirements, then so be it. But if we can avoid it, that's great.

[tvernum]

On a separate note: For changes that add new rest endpoints can we aim to do the HLRC in a separate PR?
I'd rather we argue out the design on a minimal PR that just implements the API and then add it to the client rather than starting with an 1800 line PR that potentially needs to be reworked from top-to-bottom.

[bizybot]

Hi @albertzaharovits and @tvernum, Thank you for your comments.
I will close this PR as the approach after your comments seem to be very different, so avoiding modifying this PR.
I have put a bare minimum change which allows us to go through the proposal in #42020. Thank you.