Bug 59437 - JASPIC: SimpleAuthConfigProvider with CallbackHandlerImpl MT-unsafe?
Summary: JASPIC: SimpleAuthConfigProvider with CallbackHandlerImpl MT-unsafe?
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.0.M4
Hardware: All All
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2016-05-08 01:18 UTC by Thomas Maslen
Modified: 2016-05-10 07:28 UTC (History)
1 user (show)

patch against trunk (1.06 KB, patch)
2016-05-08 01:18 UTC, Thomas Maslen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Maslen 2016-05-08 01:18:36 UTC
Created attachment 33828 [details]
patch against trunk

[This isn't a report from a concurrency-checking tool, it's just me looking at
the code, so I may be missing something, but...]

org.apache.catalina.authenticator.jaspic.CallbackHandlerImpl uses mutable
instance variables, so it definitely can't be used concurrently.

org.apache.catalina.authenticator.AuthenticatorBase _looks_ as though it's OK:
for each request it creates a new CallbackHandlerImpl instance and uses it in
a fresh getServerAuthConfig() invocation to obtain a ServerAuthConfig instance
that encapsulates the callback handler.

But SimpleAuthConfigProvider.getServerAuthConfig() caches its result (the
ServerAuthConfig) in an instance variable;  on the second and subsequent
invocations it ignores the input arguments (including the CallbackHandler)
and returns the cached ServerAuthConfig instance.

The result, I believe, is that an AuthenticatorBase instance will end up
using that same, cached, ServerAuthConfig instance to process every request
(including any concurrent requests).  That's right w.r.t. performance (and
I certainly wouldn't want to abolish SimpleAuthConfigProvider's instance
cache), but nasty w.r.t. MT-safety.

The guilty party, I believe, is CallbackHandlerImpl -- it ought to be safe
for concurrent use.

[Aside:  I wish that JSR 196 hadn't reused the CallbackHandler API here,
because while the method signatures are the same, the way it is used runs
counter to the mental model that everyone has from JAAS, and it has
discombobulated multiple JASPIC implementations].

I have attached a possible patch for CallbackHandlerImpl.

The good:  it fixes the MT-safety problem, and also means that if
AuthenticatorBase wanted to, it could obtain the ServerAuthConfig
instance once (as it currently does for the AuthConfigProvider)
and use that ServerAuthConfig instance for all requests.
[But yes, it should still generate a new authContextID and
ServerAuthContext for each request].

The bad:  if there are any ServerAuthModule implementations out there
that invoke the CallbackHandler twice (once for CallerPrincipalCallback
and once for GroupPrincipalCallback) then CallbackHandlerImpl's current
code may kinda sorta work (never mind MT-safety) whereas the patch that
I'm suggesting would not.

The ugly:  I opted for minimum # lines in the diff;  the result is more
cheesy than necessary.
Comment 1 Mark Thomas 2016-05-09 18:24:40 UTC
Thanks for the report and the patch.

Working through putting together a simple test case found a handful of additional JASPIC related issues that have also been fixed.

My reading of the spec is that state is maintained by the modules, not the CallbackHandler so making this thread-safe is the way to go.

I have applied a patch for this to trunk for 9.0.0.M5 onwards and 8.5.x for 8.5.1 onwards. I went for a slightly different approach than the one you proposed since minimising the diff was not my primary concern.
Comment 2 Arjan Tijms 2016-05-09 22:39:13 UTC
>The guilty party, I believe, is CallbackHandlerImpl -- it ought to be safe
for concurrent use.

Indeed, I explicitly asked the spec lead for clarification about this a while and the CallbackHandler has to be thread-safe. It would even be allowed to have one global instance of the SAM that handles all requests, which keeps re-using a single CallbackHandler instance.

>if there are any ServerAuthModule implementations out there
that invoke the CallbackHandler twice [...]

Hmmm, Soteria does just that:

   public static void notifyContainerAboutLogin(Subject clientSubject, CallbackHandler handler, Principal callerPrincipal, List<String> roles) {
       try {
            handler.handle(new Callback[] { new CallerPrincipalCallback(clientSubject, callerPrincipal) });
            if (!isEmpty(roles)) {
                  handler.handle(new Callback[] { new GroupPrincipalCallback(clientSubject, roles.toArray(new String[roles.size()])) });
        } catch (IOException | UnsupportedCallbackException e) {
            // Should not happen
            throw new IllegalStateException(e);
Comment 3 Mark Thomas 2016-05-10 07:28:10 UTC
(In reply to Arjan Tijms from comment #2)
> Hmmm, Soteria does just that:

Should be fine. That is no different to what a CallbackHandler implementation is going to have to do under the covers. State is maintained in clientSubject.