You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2016/05/08 01:18:36 UTC
[Bug 59437] New: JASPIC: SimpleAuthConfigProvider with
CallbackHandlerImpl MT-unsafe?
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.maslen@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59437] JASPIC: SimpleAuthConfigProvider with
CallbackHandlerImpl MT-unsafe?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59437
--- Comment #2 from Arjan Tijms <ar...@gmail.com> ---
>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);
}
}
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59437] JASPIC: SimpleAuthConfigProvider with
CallbackHandlerImpl MT-unsafe?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59437
--- Comment #3 from Mark Thomas <ma...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59437] JASPIC: SimpleAuthConfigProvider with
CallbackHandlerImpl MT-unsafe?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59437
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|NEW |RESOLVED
--- Comment #1 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 59437] JASPIC: SimpleAuthConfigProvider with
CallbackHandlerImpl MT-unsafe?
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=59437
Thomas Maslen <th...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |thomas.mpp.maslen@gmail.com
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org