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.
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.
>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); } }
(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.