fix: Authentication changes to better support FlightSQL clients #6032
Conversation
niloc132
added
grpc
NoDocumentationNeeded
ReleaseNotesNeeded
| } | ||
|
|
||
| // No more options, log an error and return | ||
| log.info().append("No AuthenticationRequestHandler registered for type ").append(key).endl(); |
There was a problem hiding this comment.
I wonder if we should potentially log out bearerKey if bearerOffset >= 0? I guess technically, you could have an auth handler who's auth token just happens to have spaces in it, in which case printing out even part of the key would be bad...
Also, this log message does seem technically incorrect; there might be an appropriate handler registered, just that "login" did not succeed.
I wonder if we should collect the handler.getClass() in both cases and print that out for additional (non-leaky) info?
There was a problem hiding this comment.
I wonder if we should potentially log out bearerKey if bearerOffset >= 0? I guess technically, you could have an auth handler who's auth token just happens to have spaces in it, in which case printing out even part of the key would be bad...
I was leaning towards not logging in case there was a space in the token, yes.
Also, this log message does seem technically incorrect; there might be an appropriate handler registered, just that "login" did not succeed.
Nit, there might be a handler registered, but it might not be the "appropriate" one. Mostly I was aiming to keep the message the same, but we really have three cases here:
- return auth context, signaling that authentication was successful
- throw auth exception, signaling that authentication failed
- return empty, signaling that this handler is not the correct one to handle the request (or the request is ambiguous in some way that the handler doesn't want to say "this login failed")
If a handler doesn't outright succeed or fail, we want to say "no one claimed this auth attempt" - the handler might be registered with a matching name, but it wasn't the correct handler for this request.
I wonder if we should collect the handler.getClass() in both cases and print that out for additional (non-leaky) info?
Maybe as debug logging? It can be noisy otherwise when not actively debugging auth issues?
| @@ -333,21 +338,41 @@ public SessionState getSessionForAuthToken(final String token) throws Authentica | |||
| if (session != null) { | |||
| return session; | |||
| } | |||
| } catch (IllegalArgumentException ignored) { | |||
| } 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.
Not that I'm aware of, but wanted to broaden what we expected, continue to ignore things we've previously ignored. If a simple runtime error happens in trying to obtain the session, we can probably ignore that call?
Open to your feedback on how this should be handled, I went with the broader interpretation.
There was a problem hiding this comment.
I'm generally a fan of only catching the exceptions you can reasonably expect. That said, happy to only widen as part of this PR.
There was a problem hiding this comment.
The IAE was intended for the UUID parsing - which I didn't realize was incorrect when swapping from UUID to this UuidCreator impl.
| 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.
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?
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