Pkce support#2143
Conversation
There was a problem hiding this comment.
Pull request overview
Adds PKCE support to the Terraform provider’s OIDC-based auth config resources (Generic OIDC, Keycloak OIDC, Cognito) by surfacing a pkce_method field, mapping it to Rancher’s PKCEMethod, and updating the Rancher dependency versions to include the upstream field.
Changes:
- Add
pkce_methodto the shared OIDC schema and wire it into OIDC flatten/expand logic. - Update unit tests for Generic OIDC / Keycloak OIDC / Cognito auth config structures and add schema validation coverage for
pkce_method. - Bump Rancher (and related) module versions in
go.mod.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rancher2/structure_auth_config_generic_oidc.go | Maps Terraform pkce_method to/from Rancher PKCEMethod in shared OIDC flatten/expand. |
| rancher2/schema_auth_config_generic_oidc.go | Adds pkce_method to the shared OIDC schema fields used by multiple auth providers. |
| rancher2/schema_auth_config_generic_oidc_test.go | Adds schema validation test for pkce_method allowed values. |
| rancher2/structure_auth_config_generic_oidc_test.go | Extends generic OIDC structure tests to include PKCEMethod/pkce_method. |
| rancher2/structure_auth_config_keycloak_oidc_test.go | Extends Keycloak OIDC structure tests to include PKCEMethod/pkce_method. |
| rancher2/structure_auth_config_cognito_test.go | Extends Cognito structure tests to include PKCEMethod/pkce_method. |
| go.mod | Updates Rancher/NorMan and other module versions to pull in PKCE-related upstream changes. |
matttrach
added
the
area/authentication
| warns, errs := r.Validate(d) | ||
|
|
||
| assert.Empty(t, warns) | ||
| assert.ErrorContains(t, errors.Join(errs...), "expected pkce_method to be one of [S256 ]") |
There was a problem hiding this comment.
Is the extra space here going to mess up the search function?
There was a problem hiding this comment.
In what sense? We have two possible values for the pkce_method "S256" and "" ?
It would be easy enough to change the validation func not to use the slice version tho'
| if v, ok := oidcData["EndSessionEndpoint"]; ok { | ||
| d.Set("end_session_endpoint", v) | ||
| } | ||
|
|
||
| d.Set("pkce_method", oidcData["PKCEMethod"]) | ||
|
|
There was a problem hiding this comment.
flattenOIDCConfig sets pkce_method without checking whether PKCEMethod is present/non-nil in oidcData, and it ignores the d.Set error. If the upstream struct omits this field (or decodes to nil), d.Set can fail and the error will be silently dropped, leaving state inconsistent. Please guard the lookup like EndSessionEndpoint and return a wrapped error when d.Set("pkce_method", v) fails (e.g., return fmt.Errorf("setting pkce_method: %w", err)).
JonCrowther
left a comment
JonCrowther
left a comment
There was a problem hiding this comment.
LGTM. One minor nit
| } | ||
|
|
||
| if valString != "" && valString != "S256" { | ||
| errors = append(errors, fmt.Errorf("%q: only supported value is 256, got %q", k, valString)) |
There was a problem hiding this comment.
validatePKCEMethod checks for the string value "S256", but the returned error message says the only supported value is 256 (missing the leading "S" and missing quotes). This makes the error misleading and will also fail TestAuthConfigGenericOIDCResourcePKCEMethodValidation, which expects "S256" in the message.
| errors = append(errors, fmt.Errorf("%q: only supported value is 256, got %q", k, valString)) | |
| errors = append(errors, fmt.Errorf("%q: only supported value is %q, got %q", k, "S256", valString)) |
This adds support for enabling PKCE for Generic OIDC, Keycloak OIDC and Cognito.
Addresses # rancher/rancher#53638
Description
Add PKCE Support for Generic OIDC, Keycloak OIDC and Cognito Auth Providers.
This updates the version of Rancher pulled in by the terraform provider and configures the PKCEMethod field on OIDC Providers.
Testing
Configured the relevant providers with PKCE.
Not a breaking change.