SQL-76 Add pgwire password authentication to OIDC authenticator #34801
+1,871
−246
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SangJunBak
force-pushed
the
jun/add-password-auth-to-oidc
branch
7 times, most recently
from
a8d789b to
aed6472
Compare
SangJunBak
changed the title
SangJunBak
force-pushed
the
jun/add-password-auth-to-oidc
branch
from
aed6472 to
1ccd67a
Compare
We extract - authenticate - validate_access_token Two methods used to authenticate HTTP and pgwire sessions out of the Frontegg authenticator. The goal is we can reuse these methods for a generic OIDC authenticator, used for self managed SSO
- Create an OIDC authenticator kind and a minimal set of config variables using CLI args - Implement JWK fetch on validate and also cache by the JWK key id.
I noticed that we were doing this weird round trip of getting internal user metadata from the catalog during authentication, then passing it back when initializing the session. By just doing this on startup, we: - Remove extraneous code - Open up ease of creating a unified interface for OIDC clients
- Introduced a new `mz-oidc-mock` package - Implemented tests for OIDC authentication
- Add functionality to extract `oidc_auth_enabled` from startup options, allowing us to use the password authenticator in the future
Before we were mistakenly fetching from jwks.json instead of getting it from the configuration endpoint
…in authentication - Updated comments to use backticks for OIDC issuer URL - Changed password handling in oidc http/ws auth to include the username when validating access tokens. - Simplified OIDC mock server structure by removing the base URL field - Remove unneeded assertion on role existence
- Add aud claim - Introduced `oidc_audience` field in OidcConfig to validate JWT's `aud` claim. This follows the spec in the design doc - Added OIDC mock server audience claims support - Added tests for audience validation and when we don't need to
Didn't realize we already had a shared crate!
Due to changed restrictions after a discussion with an external advisor, we decided we no longer need to implement the refresh token flow. However, this also means we no longer have the need for a shared OidcAuthenticator trait.
Before when we had to return internal user metadata data from the auth response, it meant we couldn't forget to call adapter_client.authenticate. By introducing a sentinel type, we make it harder for a developer to. We also combine `validate_access_token` into `authenticate` for GenericOidcAuthenticator.
- Add external_metadata_rx() method to OidcAuthSessionHandle trait with default None impl
This allows us to create a helper functions for anything implementing OidcAuthenticator.
- Update Authenticator::Oidc to use named fields: {oidc, password}
- Add authenticate_with_oidc_token for token-based auth (Frontegg/OIDC JWT) - Add authenticate_with_password for password-based auth
- Enables tests to pass connection-level options like --oidc_auth_enabled - Verifies that when oidc_auth_enabled is not set in the connection options, the OIDC authenticator falls back to password authentication.
Call stacks above the critical recursion can grow as we add code elsewhere in the system
SangJunBak
force-pushed
the
jun/add-password-auth-to-oidc
branch
from
1ccd67a to
0d5b665
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see commit messages for details, starting from commit "Add password fallback for OIDC pgwire". Commits before are. being reviewed in #34690.
Will do http pgwire password authentication in a followup PR.
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.