Bug 64715

Created attachment 37434 [details]
Proposed patch for bug 64715

- CallbackHandlerImpl changed from singleton to regular class
- added parameter context in constructor of CallbackHandlerImpl
- implemented PasswordValidationCallback in CallbackHandlerImpl
- updated initialization code for the callbackHandler in AuthenticatorBase
- removed direct initialization of CallbackHandlerImpl

Section 4.9.2 is part of the SOAP profile. Tomcat only targets the Servlet Container profile. Looking at the requirements for the SOAP profile, it does not look to be directly implementable in Tomcat. Therefore, I am wondering what is the purpose of this enhancement?

Sorry, I got the section number wrong, it's section 3.5

Chapter 3 is Servlet Container Profile.

Here is an excerpt from section 3.5:
The CallbackHandler passed to ServerAuthModule.initialize is determined by the handler argument passed in the AuthConfigProvider.getServerAuthConfig call that acquired the corresponding authentication context configuration object. The handler argument must not be null, and the argument handler and the CallbackHandler passed to ServerAuthModule.initialize MUST support the following callbacks:

• CallerPrincipalCallback
• GroupPrincipalCallback
• PasswordValidationCallback


So it is a bug, if Tomcat claims to be JASPIC 1.1 compatible in my opinion.

Yes, that does make it a bug.

Reviewing the patch:
- It doesn't handle all combinations of
  - Constructor with/without Context
  - Class defined in web app / in container
- The call to the "with Context" constructor will always fail (no Context)
- Use of an interface would be cleaner. The Contained interface is a good fit.

The rest looks good.

(In reply to Mark Thomas from comment #5)

> - It doesn't handle all combinations of
>   - Constructor with/without Context
I don't think a constructor for CallbackHandlerImpl without Context is needed. As this is a Tomcat internal class and AuthenticatorBase can always provide a context with realm (this is stated in the usage constraints of AuthenticatorBase). Or am I misinterpreting you?

>   - Class defined in web app / in container
That's a case I don't seem to understand. Could you please explain?

> - The call to the "with Context" constructor will always fail (no Context)
Ugh. Missing argument. I'll change this when the other fixes are clear.

> - Use of an interface would be cleaner. The Contained interface is a good
> fit.
Are you suggesting to pass the Realm (which is a Contained) to the CallbackHandler? Wouldn't that break dynamic configuration through JMX? I could use Container though.

I also saw that minor modifications to the tests will be necessary too. In addition I would add one or two tests for the Callbacks.

Users may wish to use a 3rd party custom CallbackHandler that knows nothing about Tomcat internals. A no-arg Constructor needs to be supported.

There are multiple class loaders involved and while the default configuration avoids most of the complexities, the non-default configs need to be handled.

Some users reconfigure the class loaders so they look like this:
http://tomcat.apache.org/tomcat-4.1-doc/class-loader-howto.html

and may want to put the custom CallbackHandler in the Catalina loader. This boils down to you need to try and load the specified class first with the web app class loader (TCCL) and then with the class loader that loaded the current class.

For adding the Context I'm suggesting something like:

if (callbackHandler instanceof Contained) {
    ((Contained) callbackHandler).setContainer(context);
}

Tomcat can then do:
if (callbackHandler instanceof Contained) {
    getContainer().getRealm()...
}

Created attachment 37438 [details]
Proposed patch for bug 64715 (second attempt), Tomcat 10

Changes to the first version:

- added Contained interface to CallbackHandlerImpl and changed access logic
- implemented check for Contained interface instead of different constructors in AuthenticatorBase
- added test cases for the three supported Callbacks


A questions that occurred to me:
- Should we throw UnsupportedCallbackException for unsupported callbacks instead of ignoring them? This breaks compatibility but would enable the user to check for unimplemented callbacks.

The use of the Contained interface is definitely a big plus. Thanks for the great feedback Mark.

Slightly off topic, but could someone explain why the package imports in CallbackHandlerImpl switched from

import javax.security.auth.callback.UnsupportedCallbackException;
import javax.security.auth.message.callback.CallerPrincipalCallback;
import javax.security.auth.message.callback.GroupPrincipalCallback;

in Tomcat 9 to the corresponding jakarta.security.auth equivalents in Tomcat 10? Is that a JASPIC 2.0 change?

Would we have to support both in Tomcat 10 so that the implementation is backwards compatible or am I mislead? I stumbled upon this while implementing the tests for my patch and I am quite sure that my own SAM implementation written for Tomcat 9 would not work with Tomcat 10 because of this.

Created attachment 37439 [details]
Proposed patch for bug 64715 (second attempt), Tomcat 10

- corrected a spelling error in Locale.properties

Created attachment 37440 [details]
Proposed patch for bug 64715 (second attempt), Tomcat 9

The Tomcat 9.0.x version of the patch.

Main difference is the difference in the package names for the callbacks.

(In reply to Mark Thomas from comment #7)
> Users may wish to use a 3rd party custom CallbackHandler that knows nothing
> about Tomcat internals. A no-arg Constructor needs to be supported.
> 
> There are multiple class loaders involved and while the default
> configuration avoids most of the complexities, the non-default configs need
> to be handled.
> 
> Some users reconfigure the class loaders so they look like this:
> http://tomcat.apache.org/tomcat-4.1-doc/class-loader-howto.html
> 
> and may want to put the custom CallbackHandler in the Catalina loader. This
> boils down to you need to try and load the specified class first with the
> web app class loader (TCCL) and then with the class loader that loaded the
> current class.
> 
> For adding the Context I'm suggesting something like:
> 
> if (callbackHandler instanceof Contained) {
>     ((Contained) callbackHandler).setContainer(context);
> }
> 
> Tomcat can then do:
> if (callbackHandler instanceof Contained) {
>     getContainer().getRealm()...
> }

Doesn't this tie the implementation class to Tomcata internals? It would be nice to implement a CallbackHandler which can be built (and run) independently of Tomcat classes. Or do I have an unrealistic expectation, here, of the way CallbackHandlers would be implemented? Is it possible for them to be container-agnostic?

(In reply to Robert Rodewald from comment #9)
> Slightly off topic, but could someone explain why the package imports in
> CallbackHandlerImpl switched from
> 
> import javax.security.auth.callback.UnsupportedCallbackException;
> import javax.security.auth.message.callback.CallerPrincipalCallback;
> import javax.security.auth.message.callback.GroupPrincipalCallback;
> 
> in Tomcat 9 to the corresponding jakarta.security.auth equivalents in Tomcat
> 10? Is that a JASPIC 2.0 change?

I don't know about the JASPIC version implications, but Java EE recently change to Jakarta EE an all the package names changed for their forthcoming versions. Presumably, this includes JASPIC because it's included in the Java EE specifications which have been "migrated" to the new Jakarta umbrella. [1]

For all the affected APIs, Tomcat has basically done a search-and-replace for javax.(api).whatever to jakarta.(api).whatever.

It is not (currently, maybe ever) expected that web applications written for Tomcat 9 can be directly run under Tomcat 10: they will require migration to the new API package names. That includes all servlets, etc.[2]

There is an experimental migration tool available[3] which aims to migrate your web applications from Java EE to Jakarta EE. I'd be interested to see if it works for you and gives you artifacts you can successfully deploy into Tomcat 10.

[1] https://jakarta.ee/specifications/authentication/1.1/apidocs/
[2] http://tomcat.apache.org/migration-10.html#Specification_APIs
[3] https://github.com/apache/tomcat-jakartaee-migration

> Would we have to support both in Tomcat 10 so that the implementation is
> backwards compatible or am I mislead?

I don't believe any backward-compatibility would be expected. We toyed with that idea, but it seems like a maintenance nightmare so we decided not to do it.

(In reply to Christopher Schultz from comment #12)
> Doesn't this tie the implementation class to Tomcata internals? It would be
> nice to implement a CallbackHandler which can be built (and run)
> independently of Tomcat classes. Or do I have an unrealistic expectation,
> here, of the way CallbackHandlers would be implemented? Is it possible for
> them to be container-agnostic?

In my understanding the CallbackHandler cannot be decoupled from Tomcat (the "runtime" in the JASPIC specification), because it represents the "glue code" to the runtime. A replacement of the implementing class is not intended anywhere in the JASPIC specification as far as I can see, but is a nice thing to have. This way the user can implement some very special non-standard Callbacks.

The inner workings of the CallbackHandler are not well defined in the specs, neither are the standard callbacks. 

For example the PasswordValidationCallback:
- Shall it set the principal in the client subject or shall it just return true or false for the getResult method?
- The password field may be null (as per spec) but how should the CallbackHandler handle this case? Set the roles/groups of the principal anyway?

I found an interesting differentiation on this page:
https://github.com/wildfly/wildfly/blob/master/docs/src/main/asciidoc/_elytron/Elytron_and_Java_Authentication_SPI_for_Containers-JASPI.adoc

If a SAM requires access to the configured identity management (the realm) of the runtime is uses "integrated" mode (this would be a CallbackHandler implementation that implements Contained).

If it establishes the identity and roles of the user by itself it can use "non-itegrated" mode (no access to the realm) and is what was supported by Tomcat before the patch (CallbackHandler does not implement Contained and has no access to the Realm).

All valid points regarding the expected behaviour of CallbackHandlers. I'd recommend raising issues against the Jakarta Authentication spec:
https://github.com/eclipse-ee4j/authentication/issues

(In reply to Mark Thomas from comment #16)
> All valid points regarding the expected behaviour of CallbackHandlers. I'd
> recommend raising issues against the Jakarta Authentication spec:
> https://github.com/eclipse-ee4j/authentication/issues

I raised the issue. Here is the link:
https://github.com/eclipse-ee4j/authentication/issues/110

Thanks for the work on this, and especially the patches. These have now been applied (with minor tweaks) and will be in the next set of releases.

Fixed in:
- master for 10.0.0-M9 onwards
- 9.0.x for 9.0.39 onwards
- 8.5.x for 8.5.59 onwards