-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Introduce generic OIDC provider #45366
base: release/v2.9
Are you sure you want to change the base?
Conversation
Adding actions and refresh
Note: you will now need to set up the audience (value is same as client name) in the provider now since we're validating the access token rather than the id token directly
… to discovery with just the issuer.
} | ||
|
||
// getRedirectURL uses the AuthConfig map to build-up the redirect URL passed to the OIDC provider at login-time. | ||
func (g *GenOIDCProvider) getRedirectURL(config map[string]interface{}) string { |
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.
I wonder about using https://pkg.go.dev/golang.org/x/oauth2#Config.AuthCodeURL to generate the URL?
You'd create an oauth2.Config
value and then use the AuthCodeURL()
to generate the URL?
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.
Interesting thought, I'll give it a look. Thanks.
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.
I may opt to just make sure that this is all encoded correctly. AuthCodeURL does a bit more than what we're currently doing here. Currently, the dashboard is expecting to handle the addition of the state param (I don't have the history on that), so going with AuthCodeURL would be a departure from what they're expecting to get.
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.
Interestingly enough, the dashboard currently seems to be expecting the not-properly encoded version and then it does the encoding itself. If I return a nicely encoded version, the dashboard winds-up double-encoding...which fails eventually after login.
I will keep this comment open and work with the dashboard folks to see if they can adjust to a better-behaved backend.
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.
This looks good 👍
return fmt.Sprintf( | ||
"%s?client_id=%s&response_type=code&redirect_uri=%s", | ||
authURL, | ||
config["clientId"], | ||
config["rancherUrl"], |
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.
These need to be query encoded, or use AuthCodeURL
, as Kevin suggests, which already does it.
use http client with timeout for discovery error formatting cleanup use JoinPath instead of Sprintf
… and be able to handle colons in the externalID
Issue:
#10053
Problem
New feature: Generic support OIDC auth providers
Solution
New auth provider added that relies on the existing OIDC functionality as much as possible, but provides support for extra options (custom scopes, custom endpoints or use of discovery)
There is no periodic refresh for user attributes when using the generic provider.
Testing
A full test plan should be developed by QA for this feature.
Engineering Testing
Manual Testing
I've tested this manually against a few different OIDC providers. I have a containerized Keycloak OIDC setup that can be used for quick testing, but other OIDC providers should also be tested.
Automated Testing
Unit tests have been added
Summary: TODO
QA Testing Considerations
The new provider should be tested with multiple OIDC providers.
Regressions Considerations
We should also perform some regression tests against the existing OIDC providers. There was some minor refactoring that shouldn't impact anything, but it's worth testing.