-
Notifications
You must be signed in to change notification settings - Fork 541
Support for external principals #5119
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
339ccce
Support for external principals
adutra c489cd7
review
adutra acda76a
add changelog
adutra fcac24a
fix merge conflict
adutra 05f209b
introduce new attribute
adutra 697da81
fix merge conflict artifact
adutra e623f65
review
adutra 48136fd
introduce InternalPolarisCredential
adutra 63a9771
spotless
adutra 264b525
deny self-rotate to external principals
adutra 14a384b
introduce ExternalPolarisCredential
adutra e489b70
regression test
adutra 5a88922
validate principal name in authenticator
adutra 0789a78
it module
adutra e169a3e
flatten type hierarchy
adutra f0a6ac7
documentation + production readiness checks
adutra 5ed8434
nits
adutra 7042109
fix compilation failures after rebase
adutra 52f01fe
fix Helm tests
adutra 19488d9
switch to CredentialMode
adutra File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,3 +67,5 @@ oidc: | |
| secret: | ||
| name: polaris-oidc | ||
| key: client-secret | ||
| principalMapper: | ||
| nameClaimPath: preferred_username | ||
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this refactor is not related to the external principal effort. We may not conflate it with the principal change, but correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this PR introduces a new
OpaContainer, a required container for external principal testing.As a result, the code in this class is now largely duplicating what
OpaContainerdoes, hence the refactor. If you prefer, I can revert the changes in this class, and fix the duplication in a follow-up PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to move
ExternalPrincipalKeycloakOpaITinto the OPA module’s integration tests? The OPA module already owns the OPA extension, its container lifecycle, and the external-server test setup. It would only need Keycloak as a test-scoped dependency.This would keep
runtime/serviceindependent of a concrete authorizer, remove the new production runtimeOnly dependency onpolaris-extensions-auth-opa, and avoid introducing the sharedOpaContainerinfrastructure in this PR. The reusable container refactor could still be done separately if it is useful beyond this test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you suggest was actually my first design, but then I moved to the current design. Let me explain why:
The system under test here is the external-principals feature itself: it lives in
polaris-core(Resolver) andruntime/service(DefaultAuthenticator,ExternalPolarisCredential). OPA and Keycloak are just a concrete authorizer + IDP used to exercise it end-to-end.Moving the test into the OPA module inverts that relationship: OPA becomes the SUT and Polaris the fixture. It would also force Keycloak (as a test dependency) and a Keycloak-specific
PolarisServerStartupActioninto the OPA extension module, which is unrelated couplingThe natural neighbor for this new test is really
RestCatalogKeycloakFileIT, which already tests Keycloak/OIDC external auth inruntime/service.That said, I agree with the underlying concern about the
runtimeOnlyOPA dependency (your other comment); see my reply there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a single MockAuthorizer for that, more details are in #5119 (comment).