jakartaee / authentication

Jakarta Authentication
https://eclipse.org/ee4j/jaspic
Other
24 stars 33 forks source link

Planned behaviour of CallbackHandler is not clearly defined #110

Closed rrodewald closed 2 years ago

rrodewald commented 4 years ago

I'm currently writing several JASPIC authentication modules for Tomcat and have encountered some uncertainties in the specs about the CallbackHandler and the planned behaviour of the required callbacks:

These uncertainties have led to some interesting interpretation in the open source community as you can see here: Elytron_and_Java_Authentication_SPI_for_Containers-JASPI In this case an integrated and non-integrated mode have been invented to solve the ambiguities.

There clearly is no "best" way to handle this, so it probably should be parametrized.

Example usage:

Many more cases come to mind if you add client cert auth.

arjantijms commented 3 years ago

CallerPrincipalCallback Is it intended that the goups of the user are set, if the user has been found in the identity store?

The CallerPrincipalCallback sets the caller principal into the container after the ServerAuthModule has acquired it in some way, for instance, by looking it up in an identity store.

So the CallerPrincipalCallback is not a "reader" into any container defined identity store, but it just sets whatever string (or principal) is passed in. The spec doesn't seem to require it, but practically this often means it "writes" the given principal into the also given subject, in a container specific way.

I think this should pretty much answer the GroupPrincipalCallback callback question as well. As the CallerPrincipalCallback doesn't do any identity store lookup, there are no groups to be found, so also no groups to be set.

Of course you could wonder about the case if the CallerPrincipalCallback sets a principal named, say, "foo", and there's also a "foo" in whatever identity store the container has configured by default. In that case, I believe, the above still holds. The container accepts the principal set by the CallerPrincipalCallback at face value. It's the ServerAuthModule which is coordinating the task of looking up identity stores or not.

arjantijms commented 3 years ago

PasswordValidationCallback It is not clear if the callback must only yield a result in it's getResult method or also set the current users principal in the > client subject. It is not clear if the groups must also be set for the identified user by the CallbackHandler.

That's all true. The PasswordValidationCallback is woefully underspecified even up to the point that I'm seriously wondering how we could have missed this to such degree for all this time.

My understanding is that the PasswordValidationCallback is, contrary to e.g. how Jakarta Security works, a 2-1 approach. It does the usual {credentials in, caller data out} function such as described in the Jakarta Security spec, but it also immediately sets the authenticated identity such as a combination of CallerPrincipalCallback and GroupPrincipalCallback would do.

The question is then, what if both PasswordValidationCallback and CallerPrincipalCallback \ GroupPrincipalCallback are used? I'd say it's simply an error then, and at the moment it's undefined. Cc @ggam @darranl what do you think?

rrodewald commented 3 years ago

Hi @arjantijms,

my intent is to replace multiple authentication mechanisms in Tomcat by JASPIC counterparts. My understanding is that JASPIC should handle the client communication part of the authentication and then use callbacks to interact with the application server. In the case of BASIC authentication this is a trivial task:

How should one implement other authentication mechanisms without having to reimplement the whole access logic to the identity store (which normally is part of the application servers configuration)? Some callbacks like PasswordValidationCallback and SecretKeyCallback indicate that this was the ambition? A very common case in my opinion is the establishment of the user identity by individual means (e.g. Bearer Auth with JWT) and the need to set groups/roles based on the identity store.

In my opinion there are different possible solutions, if you accept my argument.

I would be willing to contribute the reference implementation in Tomcat as I was already involved in fixing some bugs in the JASPIC implementation (9.0.38 and 9.0.39) if that's of any help.

arjantijms commented 3 years ago

PasswordValidationCallback and SecretKeyCallback indicate that this was the ambition?

Indeed, Jakarta Authentication (new name for JASPIC) did not really specify any access to the identity store. We talked for a long time about either enhancing Jakarta Authentication, or doing these enhancements as part of Jakarta Security, which builds directly on Jakarta Authentication.

The PasswordValidationCallback seems to have been an attempt indeed, but it's somewhat confusing it does two things in one. I, too, would rather have seen it just return the status, principal and groups, and not set them right away.

As for JWT, I implemented that with Jakarta Security, which is just a thin layer over a ServerAuthModule:

add a parameter to the CallerPrincipalCallback which allows to indicate that the server should also set the groups/roles for the given user/prinicipal (something like setGroups(true) or a constructor parameter)

I don't understand this one.

If you do something like:

handler.handle(new Callback[] {
            new CallerPrincipalCallback(
                clientSubject, 
                "jsmith")

Where should the groups come from? Getting them semi-automatically from the default identity store is perhaps a bit too much mixing up responsibilities, as the default identity store might not be used at all.

The only viable alternative I think would be a convenience handler:

handler.handle(new Callback[] {
            new CallerPrincipalCallback(
                clientSubject, 
                "jsmith", 
                new String[] { "user", "customer" }));

Or with a custom principal:

handler.handle(new Callback[] {
            new CallerPrincipalCallback(
                clientSubject, 
                new TestPrincipal("jsmith", 45)), 
                new String[] { "user", "customer" }));

create a new standard callback (GroupsCallback) that can be used for getting the groups/roles of a user; the callback could set them in the subject OR return them to the caller

There's two versions of this really, the first one is a callback that reads the groups back from a subject. This is something we need anyway.

E.g.

// Write groups to subject
handler.handle(new Callback[] {
            new GroupPrincipalCallback(
                clientSubject, 
                 new String[] { "user", "customer" }));

// Read groups from subject
GroupPrincipalCallback groupPrincipalCallback = 
   new GroupPrincipalCallback(
      clientSubject)

handler.handle(new Callback[] {
   groupPrincipalCallback });            

String[] groupsFromSubject = 
   groupPrincipalCallback.getGroups();

The second type is one that reads the groups from whatever identity store is set as the default, like PasswordValidationCallback does implicitly.

I would be willing to contribute the reference implementation in Tomcat as I was already involved in fixing some bugs in the JASPIC implementation (9.0.38 and 9.0.39) if that's of any help.

Thanks for that :) Though I did notice the PasswordValidationCallback still does not work in Tomcat. The code forgets to call passwordValidationCallback.setResult(principal != null currently. Otherwise it works indeed.

arjantijms commented 3 years ago

p.s. a while ago Tomcat did start with implementation the default Authentication mechanisms as JASPIC modules:

https://github.com/apache/tomcat/tree/dcba40e3cdf5d288d6a81b7832fa52cb926ae4c3/java/org/apache/catalina/authenticator/jaspic/provider/modules

This code was later removed again I think.

darranl commented 3 years ago

Based on our experience with WildFly and WildFly Elytron when it comes to authentication mechanisms I would recommend not focusing on BASIC authentication too much at the outset. It is a very simple mechanism so you can get something up and running very quickly but it does not really push the boundaries of any implementation or related specifications which can make it a lot harder to add the additional mechanisms later.

Generally I would suggest an approach of starting with a challenge response mechanism like DIGEST or SCRAM, in parallel adding something like SPNEGO, maybe also add one that uses information from the connection such as CLIENT_CERT - then come back and work out how you would add FORM authentication into the mix and at that point you have covered a good set of mechanisms that whatever else you can find should fit. Sorry this is a bit of a tangent but this is something I would like to suggest we look into for Jakarta Authentication during any discussions for Jakarta EE 10 and plug any of these gaps to ensure Jakarta Authentication is a solid base for any specifications which build upon the top.

Regarding the sequence of Callbacks passed to the CallbackHandler at this point I think the reality is the expected sequences have just not been defined, within the JASPIC specification we have a list of Callbacks that must be supported - each has a short description of their behaviour but no real definition of their interaction. That does mean implementations of mechanisms could have taken different interpretations so any attempts to restrict a sequence could be breaking existing code.

Within WildFly Elytron we worked with a lot of authentication mechanisms, not just HTTP mechanisms we also have a large number of SASL mechanisms and they all interact using a CallbackHandler. For the various mechanisms that exist we found that the authentication mechanism needs to be a part of the decision making process and not assume it is just a "dumb intermediary" passing a credential along for validation. Where we could extract a credential or "evidence" for validation we would pass it along but in some cases the response from the client is in response to a challenge issued by the mechanism so the mechanism needed to be involved in the validation decision otherwise we would just be leaking the mechanism detail into our password validation.

I don't think that last paragraph relates too closely to the questions here but I just wanted to provide some more background to our interpretation of the Callbacks and the collaboration with the mechanism.

So when it came to our JASPIC implementation we found we needed two different modes of operation, an integrated mode where we integrate with a configured SecurityDomain and a non-integrated mode where we allow the ServerAuthModule to completely describe it's own identity.

Our non integrated mode covers cases where the ServerAuthModule handles it's own validation, this is particularly appropriate for Jakarta Security - in that case Jakarta Security provides both the implementation of the mechanism and the IdentityStore containing the identities. It does not make sense to duplicate this information in our SecurityDomain so instead the ServerAuthModule describes the resulting identity using the Callbacks.

Now in the case of the non-integrated mode we support validation against our SecurityDomain. At the simplest level the PasswordValidationCallback contains everything needed to establish an identity which has a principal and set of groups. We don't however consider subsequent use of CallerPrincipalCallback or GroupPrincipalCallback to be invalid.

If we receive a subsequent CallerPrincipalCallback we treat that as a request to run as a different identity, i.e. authenticate as one identity but switch to another. In our case when this situation occurs we perform a permission check to verify the authenticated identity is allowed to run as the requested identity. For subsequent calls of GroupPrincipalCallback we take that as a request to augment the identity with additional roles.

So I don't think either of the callbacks are necessary after a PasswordValidationCallback but their use is not invalid.

arjantijms commented 3 years ago

Regarding the sequence of Callbacks passed to the CallbackHandler at this point I think the reality is the expected sequences have just not been defined, within the JASPIC specification we have a list of Callbacks that must be supported - each has a short description of their behaviour but no real definition of their interaction. That does mean implementations of mechanisms could have taken different interpretations so any attempts to restrict a sequence could be breaking existing code.

True, although lately there seems to be a bigger desire to "move fast and break things" then there has been in the past. Naturally we need to find the right balance there.

At a minimal I'd like to specify the PasswordValidationCallback better and clarify that it does those two-in-one things as mentioned, and that the CallerPrincipalCallback or GroupPrincipalCallback are not necessarily after that point.

What I also like to specify going forward, which influences existing implementations, is that the result of executing the PasswordValidationCallback, CallerPrincipalCallback and GroupPrincipalCallback MUST be written into the Subject and that the CallbackHandler MUST be stateless. This may solve the problem we talked about earlier, where WildFly/Elytron current stores the state in the CallbackHandler. Of course such change would need to be agreed upon by everyone involved.

In a way that would make the CallbackHandler vs Subject a but like the flyweight pattern. Jakarta Authentication already does that with the MessageInfo, which is the state that is swapped in and out of a ServerAuthModule (the ServerAuthModule itself doesn't keep any state).

The thing this would buy as directly is that we can use a callback to read data in a portable way from the Subject that we have (even though its internals are proprietary).

arjantijms commented 3 years ago

(p.s. more related to the discussion about the CallbackHandler and state from the other thread, but since it came up here, I found this interesting part in the specification:

The message processing runtime acquires the authentication context configuration object for the application 
from the provider. This step is typically done at application deployment, and may be accomplished as follows:

[...]
ServerAuthConfig serverConfig =
    provider.getServerAuthConfig(layer, appID, callbackHandler);

The resulting authentication context configuration object encapsulates all authentication contexts for the 
application at the layer. Its internal state will be kept up to date by the configuration system, and from this 
point until the application is undeployed, the configuration object represents a stable point of interaction
 between the runtime and the integrated authentication mechanisms for the purpose of securing the messages
 of the application at the layer.

So the ServerAuthConfig is created at deployment time, and its given a CallbackHandler then. It furthermore says that "configuration object represents a stable point of interaction between the runtime and the integrated authentication mechanisms".

Furthermore it says:

"A callback handler is associated with the configuration object when it is obtained from the provider. This callback handler will be passed to the authentication modules within the authentication contexts acquired from the configuration object. "

It still doesn't spell it out, but it seems to strongly indicative that the callback handler should indeed be stateless.)

darranl commented 3 years ago

I think lets move the CallbackHandler state discussion into it's own issue as that will be a tangent on it's own. I was thinking of starting with a couple of test cases to better illustrate the context of that one. I think there are issues to discuss in the overall API regarding state before we get to the CallbackHandler.

darranl commented 3 years ago

True, although lately there seems to be a bigger desire to "move fast and break things" then there has been in the past. Naturally we need to find the right balance there.

Overall I think it will be great if we can rework some of the Jakarta Authentication APIs which may lead to breakage but try and keep any breakage to necessary changes,

At a minimal I'd like to specify the PasswordValidationCallback better and clarify that it does those two-in-one things as mentioned, and that the CallerPrincipalCallback or GroupPrincipalCallback are not necessarily after that point.

+1 PasswordValidationCallback alone should be sufficient for the majority of cases.

The thing this would buy as directly is that we can use a callback to read data in a portable way from the Subject that we have (even though its internals are proprietary).

Well would it be unreasonable to discuss some minimal requirements on the resulting Subject? It seems as though it would be useful to have some common expectations set for the resulting Subject.

arjantijms commented 2 years ago

Addressed the main concern of this issue by clarification. See #140