Authentication logic should only allow one provider be the source of truth for any request

[epixa]

For every request, we iterate through all auth providers in the order they are configured and execute their individual authenticate operation. That operation returns one of three things:

• fail - we attempted to authenticate and failed
• notHandled - we didn't attempt to authenticate because the request was missing required details
• success - we attempted and succeeded in authenticating

If success, we do not execute authentication with any subsequent auth providers.

If notHandled, we move on to the next configured auth provider and try there.

If fail, we move on to the next configured auth provider and try there. This is unexpected to me, as a provider should only fail to authenticate if it had enough information to authenticate the request, an attempt was made to do so, and the attempt failed.

Using pseudo-code as an example, I'd expect the logic work roughly like this:

while (provider = providers.next()) {
  if (provider.succeed()) {
    return;
  }

  if (provider.notHandled()) {
    continue;
  }

  if (provider.fail()) {
    throw;
  }
}

Instead, it works roughly like this:

while (provider = providers.next()) {
  if (provider.succeed()) {
    return;
  }

  if (provider.notHandled() || provider.fail()) {
    continue;
  }
}

I suspect it was implemented this way so if you had multiple providers that could technically support the same details on a request, you wouldn't need to make a choice on which provider had the final say in case of failure. For example, if you had two providers that could each handle basic authentication headers, failing on the first provider doesn't necessarily mean the second would fail.

In practice, I don't think this is something we need to support, and I think the certainty that comes from having only one provider handle a request is preferable to the flexibility that exists today.

For example, when I have [token, basic] configured for auth providers, I want token to be the only thing that handles login attempts, but I want the option of sending an authorization: basic ... header that would go notHandled by the token provider and use the basic provider as the source of truth. With the current implementation, if a login attempt fails due to incorrect credentials on the token provider, it attempts the same login on the basic provider as well. That's not ideal since it's generally a wasted request, but in theory a race condition here could result in a valid session from the basic provider, which from a security perspective is something I never want in this configuration.

I could solve that login attempt problem by adding a special configuration for which provider is the source of truth for login attempts. I could also solve this problem by changing providers to differentiate between API handling and UI handling so admins get more control. Ultimately though, I don't think it's valuable to even have this possibility in the first place.

[elasticmachine]

Pinging @elastic/kibana-security

[epixa]

@azasypkin I'm also interested in your thoughts here since you created the current implementation.

[epixa]

I believe this is the extent of the change necessary (plus some test updates, I'm sure) ab788ef

[legrego]

I'm generally +1 on this change, so long as we are comfortable saying that the authC providers are mutually exclusive of each other (which I think we are). It makes sense to me that if an auth provider attempts and fails to authenticate, that we abort the request at that point.

I think we can get away with this because Kibana defers to Elasticsearch for a lot of the heavy lifting, so the flexibility with different realms is needed more-so on their side than ours.

[kobelb]

The changes that you've proposed seem reasonable to me as well, but I'm also interested on hearing from @azasypkin whether there's any specific configuration that requires the current behavior.

[azasypkin]

I suspect it was implemented this way so if you had multiple providers that could technically support the same details on a request, you wouldn't need to make a choice on which provider had the final say in case of failure. For example, if you had two providers that could each handle basic authentication headers, failing on the first provider doesn't necessarily mean the second would fail.

In practice, I don't think this is something we need to support, and I think the certainty that comes from having only one provider handle a request is preferable to the flexibility that exists today.

Just trying to remember the reason behind this desicion in my memory and I think I was trying to replicate ES behavior in some way (it may have changed since then, but see https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java#L267-L300 as a reference):

• notHandled is similar to ES's AuthenticationResult.notHandled (https://github.com/elastic/elasticsearch/blob/8d83688328f17c05ad7a7404a6e4319b9428e477/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationResult.java#L104) or I can't handle this auth token, please try next auth provider.

• failed is similar to ES's AuthenticationResult.unsuccessful (https://github.com/elastic/elasticsearch/blob/8d83688328f17c05ad7a7404a6e4319b9428e477/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationResult.java#L117) or I tried to authenticate user, but couldn't. Please try next auth provider.

But the idea was to eventually have terminated that is similar to ES's AuthenticationResult.terminate (https://github.com/elastic/elasticsearch/blob/8d83688328f17c05ad7a7404a6e4319b9428e477/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationResult.java#L132) or Damn, I couldn't authenticate user and failed so badly that we should terminate authentication process and return error to the user. As far as I remember ES returns terminate when reserved user was involved and couldn't authenticate (see ReservedRealm.java), maybe there are more cases.

At that time I couldn't find use cases for terminated in Kibana, so I'm fine if we say that failed should terminate authentication flow and reconsider if we ever find a use case for that (failed loginAttempt in token provider could be that use case though).

[epixa]

Great, thanks for the details @azasypkin. I'll go ahead and make the change to how we treat fail in Kibana.

[epixa]

I guess I'll leave this open to track.