fix: Authentication changes to better support FlightSQL clients #6032
Conversation
niloc132
added
grpc
NoDocumentationNeeded
ReleaseNotesNeeded
server/src/main/java/io/deephaven/server/session/SessionService.java
Outdated
Show resolved
Hide resolved
| SessionState session = sessionService.getSessionForToken(uuid); | ||
| respondWithAuthTokenBin(session); | ||
| return; | ||
| } catch (IllegalArgumentException | InvalidUuidException ignored) { |
There was a problem hiding this comment.
Do we expect IllegalArgumentException to ever get hit?
There was a problem hiding this comment.
Looks like this one is a real one - if session were null, respondWithAuthTokenBin would NPE (which inherts from IAE).
Probably better to wrap in a null check, in conjunction with the change below.
There was a problem hiding this comment.
Okay, there's another way IAE can get hit here - if the req's payload isn't valid ascii, the String constructor will throw.
This is only an error in terms of "we cant read it as a uuid", this might be valid non-ascii bytes for some other auth handler.
server/src/main/java/io/deephaven/server/arrow/FlightServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
|
Added another fix to handle v1 auth uuid being "invalid" |
| @@ -345,7 +346,7 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(final ServerCall<Re | |||
| if (altToken != null) { | |||
| try { | |||
| session = service.getSessionForToken(UUID.fromString(new String(altToken))); | |||
| } catch (IllegalArgumentException ignored) { | |||
| } catch (IllegalArgumentException | InvalidUuidException ignored) { | |||
There was a problem hiding this comment.
We aren't using the f4b6a3 library for parsing here, so I don't think we should explicitly try to catch this exception; should we update to using f4b6a3 library? I guess wider Q: what's the difference between UUID and f4b6a3?
There was a problem hiding this comment.
The UuidCreator is capable of generating random uuids, unlike the default java impl.
According to its documentation, it is also substantially faster at parsing uuids than the default impl, so we're preferring it if we have the choice.
There was a problem hiding this comment.
In which case we should swap out the call from UUID.fromString to the UuidCreator?
There was a problem hiding this comment.
Changed to UuidCreator, and specified that the altToken bytes are ascii - but we need the IAE here as above in case the bytes aren't acsii? I'm not 100% that this is an invalid state though, just "we don't accept your token", so we move on?
There was a problem hiding this comment.
I don't think String throws IAE...
This method always replaces malformed-input and unmappable-character sequences with this charset's default replacement string
There was a problem hiding this comment.
That is what the javadoc says, but doesn't quite match the bytecode I'm looking at (for Java 11, our default runtime for gradle). This might not strictly throw IAE for ascii, but there are paths to throw in this constructor:
String(byte[]) calls
String(byte[], int, int) calls
java.lang.StringCoding#decode(byte[], int, int)
- For utf8 this can end up in
java.lang.StringCoding#decodeUTF8_0which clearly callsthrowMalformed(), which will be a IAE - For ascii does seem to be safe
- for latin1 also seems to be safe
- for other charsets, this ends up in
java.lang.StringCoding.StringDecoder#decode, which explicitly throws Error in response toCharacterEncodingExceptions
So I think you're right that this can't break, but I'm skeptical about trusting that it can't throw IAE at least in case we change our handling here (note that we used to decode as file.encoding, this patch fixes that to be ascii, and file.encoding defaults to utf-8 which according to the above can throw IAE) or Java weakens its implementation.
| @@ -345,7 +346,7 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(final ServerCall<Re | |||
| if (altToken != null) { | |||
| try { | |||
| session = service.getSessionForToken(UUID.fromString(new String(altToken))); | |||
| } catch (IllegalArgumentException ignored) { | |||
| } catch (IllegalArgumentException | InvalidUuidException ignored) { | |||
There was a problem hiding this comment.
I don't think String throws IAE...
This method always replaces malformed-input and unmappable-character sequences with this charset's default replacement string
Handshake calls previously required the client to send at least one message before sending a response, even if the required auth information was included in headers, now the server will respond and close the stream right away. This change could cause problems for older DH clients (specifically JS and Go clients), but the fix for this was released in 0.36.
Bearer authentication would fail if the token wasn't in the form of a UUID, even if an alternative authentication handler was going to accept the token.
Some Flight clients are unable to send auth types other than Basic and Bearer, our authentication interceptor can now support the auth type as a space-separated prefix of the auth token.
Fixes #5922