Fix for ApiKeyIntegTests related to Expired API keys remover

[bizybot]

When API key is invalidated we do two things first it tries to trigger ExpiredApiKeysRemover task
and second, we do index the invalidation for the API key. The index invalidation may happen
before the ExpiredApiKeysRemover task is run and in that case, the API key
invalidated will also get deleted. If the ExpiredApiKeysRemover runs before the
API key invalidation is indexed then the API key is not deleted and will be
deleted in the future run.
This behavior was not captured in the tests related to ExpiredApiKeysRemover
causing intermittent failures.
This commit fixes those tests by checking if the API key invalidated is reported
back when we get API keys after invalidation and perform the checks based on that.

Closes #41747

[elasticmachine]

Pinging @elastic/es-security

[albertzaharovits]

The index invalidation may happen
before the ExpiredApiKeysRemover task is run and in that case, the API key
invalidated will also get deleted.

I believe this is not true. And I see the changes in the tests reflect this thinking.

In ExpiredApiKeysRemover there is this condition
.should(QueryBuilders.rangeQuery("expiration_time").lte(now.minus(EXPIRED_API_KEYS_RETENTION_PERIOD).toEpochMilli()))
such that expired API keys will not immediately be removed.
And I believe the tests don't change this?

[bizybot]

The index invalidation may happen
before the ExpiredApiKeysRemover task is run and in that case, the API key
invalidated will also get deleted.

I believe this is not true. And I see the changes in the tests reflect this thinking.

In ExpiredApiKeysRemover there is this condition
.should(QueryBuilders.rangeQuery("expiration_time").lte(now.minus(EXPIRED_API_KEYS_RETENTION_PERIOD).toEpochMilli()))
such that expired API keys will not immediately be removed.
And I believe the tests don't change this?

Hi @albertzaharovits Thank you for your comments.
The behavior is slightly different from what you see,

.setQuery(QueryBuilders.boolQuery()
    .filter(QueryBuilders.termsQuery("doc_type", "api_key"))
    .should(QueryBuilders.termsQuery("api_key_invalidated", true))
    .should(QueryBuilders.rangeQuery("expiration_time")
                  .lte(now.minus(EXPIRED_API_KEYS_RETENTION_PERIOD).toEpochMilli()))
    .minimumShouldMatch(1)

The expiration_time can be null (for API keys with no expiration) and the minimumShouldMatch
means at least one out of the 2 should clause must match. So if the indexing of the invalidate happens before the task is run and if the API key does not expire then it will be deleted by the ExpiredApiKeysRemover as api_key_invalidated would be true. The task may run before the indexing of invalidating happens and in that case, the API key would not be deleted.
As we do not know or there is no way to identify what happened, we have changed the test such that if the returned results contain or miss the recent invalidated API key. Hope this helps. Thank you.

[albertzaharovits]

The expiration_time can be null (for API keys with no expiration) and the minimumShouldMatch
means at least one out of the 2 should clause must match.

Ooops! You're right! I completely misunderstood the condition there. Good catch in the tests!

So, invalidated keys are removed as soon as possible but expired ones have a 7 days (configurable) grace period before they are deleted. I believe the reason for the grace period is for audit & diagnostic purposes (a service stops working some day, with its API key non-existent; did the API key existed and and was deleted automatically, or it never existed and the client must have messed up its credentials).

Should invalidated API keys also have the same grace period? I believe this might have been discussed before? Invalidation is an explicit action that we audit so this is a strong reason no to; but we don't audit or otherwise trace specifically which API key got invalidated.

Alternatively, instead of marking them as invalidated and then having a periodic job do the removal, could we just remove them?

I am proposing this only because the changes required in tests look unnecessary complicated (but correct).

[bizybot]

Hi, @albertzaharovits,

Thank you for your comments. I see your point and to do that we will need to modify the existing behavior which we would not be able to do for older released versions where the test is also failing.
On the other channel, we discussed another issue that we found where we would want to trigger the API keys/ Token remover on every API instead of only being done on invalidation.
I think as we would not be able to backport these changes (as they are bug fix/enhancement) to released versions, I think we should fix the test behavior for now and backport them.

I can create another issue for tracking the other changes that we discussed, wdyt?

[jkakavas]

@bizybot I think we're good to merge this and you can open the follow up issue to discuss the invalidation grace period ?

[albertzaharovits]

LGTM