[v11] Backport enterprise SSO refactor PRs#20243
Closed
camscale wants to merge 5 commits intobranch/v11from
Closed
[v11] Backport enterprise SSO refactor PRs#20243camscale wants to merge 5 commits intobranch/v11from
camscale wants to merge 5 commits intobranch/v11from
Conversation
Export some names in the `lib/auth` package so that SSO auth plugins can be implemented from outside this package, adding doc comments where missing: * struct `ssoRequestParams` (including fields) * struct `ssoCallbackResponse` (including fields) * func `parseSSORequestParams` * func `ssoSetWebSessionAndRedirectURL` * func `redirectURLWithError` * var `ssoLoginConsoleErr`, renamed to `SSOLoginFailureMessage` * type `CachedSessionLingeringThreshold` (for TestSAML)
Create a new type - `SAMLAuthService` - and move the implementation of the SAML connector from the auth Service to this new type. This decoupling will make it easier to move out of tree in a later commit. This also lays the basic groundwork for running without the SAML connector at all, returning a NotImplemented error for all the SAML logic if the SAMLAuthService is not plugged in. A number of names in the `auth` package have been exported to facilitate moving the SAML connector out of this repository. The types exported do not need to be exported for this commit, but for subsequent commits that move the `SAMLAuthService` out of the `auth` package. Also apply a small fix to to emit `apievents.SAMLConnector*` events of `apievents.OIDCConnector*` events in the SAML connector.
Remove the Upsert and Delete methods from the SAMLService interface. It was intended for these to be part of the SAML "plugin", however they are needed for the operator tests to be able to create and delete the SAML connectors. The methods are back to being implemented directly in the `auth.Server`. This does not cause any real issues with the rest in the enterprise repo as the upsert/delete logic is just manipulating the local backend with no actual SAML logic in it. The `Get*` methods had already been kept in the `auth.Server` for the same reason. This narrows the `SAMLService` interface to just creating the SAML auth request and validating the response that comes back from the SAML identity provider. This is the core of the SAML-specific logic. Issue: gravitational/teleport.e#525
Provide defaults for `TestTLSServerConfig` when passed to `auth.NewTestServer()`, as some of those fields cannot be set from outside `NewTestServer()`. The defaults are the same as when called without a `TestTLSServerConfig`, which allows just some fields to be set/overridden. This is to allow `APIConfig.PluginRegistry` to be set.
Refactor the OIDC connector to its own type so it is simpler to move out of the `auth.Server` struct. This follows the same pattern as was done with the SAML connector - a new struct that the OIDC request methods are attached to, plugged into the `auth.Server`. Move some small amount of code around so when it comes to removing it later, it is largely confined to the oidc files and large contiguous blocks - makes it slightly simpler to review. Make `ValidateACRValues` a pure function as it did not use anything from the struct it was a method on, and move the test for it into `oidc_test.go`. Move `isHTTPS()` from `auth.go` to `oidc.go` as it is only used there and will later move out. Export `validateOIDCAuthCallbackReq` and `oidcAuthRawResponse` and move them to `oidc.go` so the web handler can also be moved to enterprise.
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
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.
Backport the enterprise SSO refactor PRs that are the first step to
moving the enterprise SSO connectors to the enterprise repository.
This is the first of 4 PRs to backport the enterprise SSO connector
migration to the v11 branch. The 4 PRs are (including this):
The PRs backported in this PR are:
Conflicts resolved:
textual conflict
now-exported methods: s/withAuth/WithAuth/
simple textual conflict again.
These changes are just a refactor to make it easier to migrate the code.
No functional changes are present.
Issue: https://github.com/gravitational/teleport.e/issues/525