https://bz.apache.org/bugzilla/show_bug.cgi?id=59437
Bug ID: 59437
Summary: JASPIC: SimpleAuthConfigProvider with
CallbackHandlerImpl MT-unsafe?
Product: Tomcat 9
Version: 9.0.0.M4
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: thomas.mpp.mas...@gmail.com
Created attachment 33828
--> https://bz.apache.org/bugzilla/attachment.cgi?id=33828&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
