Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to flight auth #3589

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

jmao-denver
Copy link
Contributor

@jmao-denver jmao-denver commented Mar 23, 2023

Fixes #3513

@jmao-denver jmao-denver added this to the Mar 2023 milestone Mar 23, 2023
@jmao-denver jmao-denver self-assigned this Mar 23, 2023
@jmao-denver jmao-denver marked this pull request as ready for review March 23, 2023 16:25
@jmao-denver jmao-denver requested a review from kosak March 23, 2023 17:55
py/client/pydeephaven/_config_service.py Outdated Show resolved Hide resolved
Comment on lines +93 to +98
auth_type (str): the authentication type string, can be "Anonymous', 'Basic", or any custom-built
authenticator in the server, such as "io.deephaven.authentication.psk.PskAuthenticationHandler",
default is 'Anonymous'.
auth_token (str): the authentication token string. When auth_type is 'Basic', it must be
"user:password"; when auth_type is "Anonymous', it will be ignored; when auth_type is a custom-built
authenticator, it must conform to the specific requirement of the authenticator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niloc132
At a low level, the API just takes a single string. Are we guaranteed that we will always be able to break auth into these two fields, or are there options that take 3+ fields that would make just doing a single field the right way to go?

Should the Go client be operating this way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They syntax of the Authorization header is in two parts, the "scheme" and "parameters", see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization#syntax

See also
https://httpwg.org/specs/rfc9110.html#field.authorization
and
https://httpwg.org/specs/rfc9110.html#credentials

Sending it as one value is probably fine, but most use cases will tend to think of it as two, likely a constant for the kind of authentication they are using, then the specific token that identifies them.

py/client/pydeephaven/session.py Show resolved Hide resolved
py/client/pydeephaven/_config_service.py Show resolved Hide resolved
if auth_type == "Anonymous":
self._auth_token = auth_type
elif auth_type == "Basic":
auth_token_base64 = base64.b64encode(auth_token.encode("ascii")).decode("ascii")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niloc132 If the Go client should be split up into two args, it probably also needs to base64 encode...right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the Basic type is expected/required to be base64 encoded, this isn't necessarily required for other formats. For example, if you base64 the already-hex encoded uuids that are used for bearer tokens, or the already-base64 encoded openId connect payloads, the server won't be able to read them.

py/client/pydeephaven/session.py Show resolved Hide resolved
py/client/pydeephaven/session.py Show resolved Hide resolved
py/client/pydeephaven/session.py Show resolved Hide resolved
py/client/pydeephaven/session.py Outdated Show resolved Hide resolved
py/client/pydeephaven/session.py Show resolved Hide resolved
chipkent
chipkent previously approved these changes Mar 29, 2023
chipkent
chipkent previously approved these changes Mar 29, 2023
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving post conflict resolution based on prior reviewers.

@jmao-denver jmao-denver merged commit 7e5f662 into deephaven:main Mar 30, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2023
@deephaven-internal
Copy link
Contributor

@jmao-denver jmao-denver deleted the 3513-pyclient-flight-auth branch March 31, 2023 03:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python client should migrate to flight auth
5 participants