authentication token extraction should be moved to its own entity

[elasticmachine]

Original comment by @jaymode:

I was doing some experimenting using the custom realms and saw some interesting behavior. In the example custom realm, the authentication mechanism is not basic authentication but is a custom authentication "scheme" based on username and passwords in headers. The realm doesn't check for basic authentication, but when I provided valid credentials for the custom realm using basic auth and had the esusers realm enabled, I was able to authenticate. This is not the way it should work IMO.

This happened because the custom realm uses the provided UsernamePasswordToken. This token does not imply that it must be used for Basic authentication; if it did then this should be named a BasicAuthenticationToken. However, we have a lot of static methods in this token that deal with BasicAuthentication, so maybe we should split this into two classes?

What I am proposing is to change to our authentication service. Rather than iterating over all realms and calling the token method until we find one and then iterate over the realms again until the token is supported and can be authenticated, we do the following:

• A single loop in the Authentication service over the realms
  • token is called. If a token is returned, call authenticate. If authenticate returns a user, break the loop
• We keep track if a token was returned by any realm. If there was no token, consider the request anonymous
• remove the supports method in the realm since we do not need it anymore.
• create a BasicAuthenticationToken that extends UsernamePasswordToken

In the case where there are multiple realms of the same type/token type, this means that we extract a token more than once. This requires more computation/memory but overall I don't think it will have that big of an impact and the authentication is correct.

[elasticmachine]

Original comment by @uboness:

not sure about this.

The intention was never to bind any realm to basic auth, but instead to an authentication token. And auth token represents the information a realm needs in order to perform authentication. That's why the name UsernamePasswordToken, because the information that is required is a username and a password. The fact that we're now bound to basic auth is simply because that's the simplest and widely used standard mechanism for attaching/extracting username/password to/from a request.

if there are 2 realms - A and B, and they both require a username/password pair to perform authentication. I care less about were these username/password pairs came from, and care more that once we have them, and both realms support this kind of authentication token, we try them both. At least that's the rational behind the current implementation.

It depends on how we would like to view realms and work with the same auth token (as I described auth token above). If esusers and LDAP both support username/password... lets say that you create a new BasicAuthToken... what does it change in the context of these two realms? they still both need to extract the same token from the request, in the same way. And lets say that tomorrow we'll come up with another mechanism to extract the username/password from the request, we'd still want to implement that for both right?... so if we're extracting it for one, why not just extract it for both in one go. At the end of the day, they both require us to do the exact same thing... call it basic auth, call it username/password token... I don't think it matters... except that with BasicAuthToken you're "forcing" the realm to define a single mechanism for extracting the username/password from the request.

I'm not saying what you're proposing is wrong... I'm just saying that I'm not sure if it's more right. If you strongly believe we need to change how it works now, I'm fine with it. But I don't think renaming UsernamePasswordToken to BasicAuthToken makes sense in the context of the proposed change.

[elasticmachine]

Original comment by @jaymode:

But I don't think renaming UsernamePasswordToken to BasicAuthToken makes sense in the context of the proposed change.

+1 (more on why below)

I care less about were these username/password pairs came from, and care more that once we have them, and both realms support this kind of authentication token, we try them both. At least that's the rational behind the current implementation.

This makes sense to me and now I want to take a different approach. I think what we should decouple the token extraction from the realm and the token itself. The token is just a representation of authentication credentials; UsernamePasswordToken merely represents that a username and password combination was presented, it doesn't matter how it was presented like you say. The Realm is an authentication source. So we could have the concept of token extractors or something that will return the token found in the request. The realm will still keep the supports method. What do you think?

[tomcallahan]

@jaymode is this still relevant?

[jaymode]

It is still relevant but should be a team discuss (cc @elastic/es-security)