ISPN-9841 Non-blocking Hot Rod authentication #6801
Conversation
|
3 related failures |
tristantarrant
force-pushed
the
ISPN-9841/nonblocking_auth
branch
2 times, most recently
from
3717f60
to
0abfdfd
Compare
|
Fixed the failures by reordering how we handle "complete" authentication. Before sending the final server response, we obtain the subject from the auth backend. |
|
There are couple of places where the auth process throws an exception. In the original code this was handled in the server (writing the error response) but I am afraid here it just gets logged and the client does not get any response until it times out. Could you make sure to write some response on any code path? |
|
@rvansa are you sure ? I'm just wrapping the exceptions into a SecurityException which will then be handled by HotRodDecoder.decode()'s catch |
tristantarrant
force-pushed
the
ISPN-9841/nonblocking_auth
branch
from
0abfdfd
to
0c92cfd
Compare
| saslServer = Subject.doAs(authenticationConfig.serverSubject(), (PrivilegedExceptionAction<SaslServer>) () -> | ||
| ssf.createSaslServer(mech, "hotrod", authenticationConfig.serverName(), | ||
| authenticationConfig.mechProperties(), callbackHandler)); | ||
| } else { | ||
| } catch (PrivilegedActionException e) { | ||
| throw new SecurityException(writeException(header, e.getCause())); |
There was a problem hiding this comment.
Why are you throwing that here? Can't you just log the PAE, writeException and return?
| saslServer = ssf.createSaslServer(mech, "hotrod", authenticationConfig.serverName(), | ||
| authenticationConfig.mechProperties(), callbackHandler); | ||
| } catch (SaslException e) { | ||
| throw new SecurityException(writeException(header, e)); |
There was a problem hiding this comment.
In general I find calling a method with side-effect in exception ctor a bit cryptic (couldn't spot where it is written for a while), though YMMV.
tristantarrant
force-pushed
the
ISPN-9841/nonblocking_auth
branch
from
0c92cfd
to
b812337
Compare
|
Yeah, that was really silly of me. I've changed that @rvansa |
| } | ||
| writeResponse(header, header.encoder().authResponse(header, server, channel.alloc(), serverChallenge)); | ||
|
|
||
| if (saslServer.isComplete()) { |
There was a problem hiding this comment.
Isn't here a race between responding with the authResponse and adding the QOP handler below?
There was a problem hiding this comment.
@rvansa @tristantarrant isn't this still valid?
| throw log.externalMechNotAllowedWithoutSSLClientCert(); | ||
| } catch (SSLPeerUnverifiedException e) { | ||
| throw log.externalMechNotAllowedWithoutSSLClientCert(); | ||
| executor.execute(() -> authInternal(header, mech, response)); |
There was a problem hiding this comment.
While you have handled all the declared exceptions, I would still feel a bit more warm&fuzzy if any unexpected exceptions (NPEs...) would be handled with a response...
tristantarrant
force-pushed
the
ISPN-9841/nonblocking_auth
branch
from
b812337
to
b891df7
Compare
tristantarrant
force-pushed
the
ISPN-9841/nonblocking_auth
branch
from
b891df7
to
8ebc474
Compare
|
@rvansa I believe I've addressed everything |
|
Kicked-off a new CI build as the previous one crashed in core (unrelated to this PR) |
|
A green build ! |
https://issues.jboss.org/browse/ISPN-9841