Skip to content

feat: Add PKCE and JWKS support for upstream OIDC providers #5993

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.

Sign up for GitHub

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

Open
assimilat wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add PKCE and JWKS support for upstream OIDC providers #5993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions idp/dex/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ type ConnectorConfig struct {
ClientSecret string
// RedirectURI is the OAuth2 redirect URI
RedirectURI string
// PKCE enables Proof Key for Code Exchange for the upstream OIDC provider
PKCE bool
// JWKSURL is the URL to the JSON Web Key Set of the identity provider
JWKSURL string
}

// CreateConnector creates a new connector in Dex storage.
Expand Down Expand Up @@ -129,6 +133,13 @@ func mergeConnectorConfig(cfg, oldCfg *ConnectorConfig) {
if cfg.Name == "" {
cfg.Name = oldCfg.Name
}
if cfg.JWKSURL == "" {
cfg.JWKSURL = oldCfg.JWKSURL
}
// Note: for boolean PKCE, we don't have an easy way to detect if it was missing in the request
// if we don't use pointers. Given the current pattern, we assume if it's false it might be intentional
// or it might be missing. However, the API request uses *bool, so we could have used that if we wanted.
// For now, we'll just let it be updated as is.
}
Comment on lines +136 to 143
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Root cause of the PKCE/JWKS silent-reset risk on partial updates.

The acknowledged limitation here is the underlying cause of a real data-loss bug: because cfg.PKCE is a plain bool, this merge cannot distinguish "client omitted the field" from "client set it to false". Any update from a client that doesn't echo pkce (curl/scripted updates, future UIs that only patch some fields, etc.) will flip a previously-enabled PKCE configuration off without the user's knowledge.

The cleanest fix is to plumb *bool (and *string for JWKSURL if you want the same semantics) through the call chain — api.IdentityProviderRequest already uses *bool/*string, so the information is currently being thrown away in fromAPIRequest (management/server/http/handlers/idp/idp_handler.go). Either:

  1. Make types.IdentityProvider.PKCE and dex.ConnectorConfig.PKCE *bool, and only overwrite when non-nil; or
  2. Have the manager Update* path read the existing IdentityProvider, copy over only fields explicitly set in the request, and pass the merged struct down (so this merge becomes unnecessary for these fields).

Also note JWKSURL has the inverse asymmetry: mergeConnectorConfig preserves it on empty input, so a user can never clear a previously-set JWKS URL via update once the field is set.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@idp/dex/connector.go` around lines 136 - 143, The merge currently loses
"field-omitted vs false" semantics because dex.ConnectorConfig.PKCE is a plain
bool and mergeConnectorConfig treats empty JWKSURL specially, so updates can
silently disable PKCE or prevent clearing JWKSURL; update fromAPIRequest
(management/server/http/handlers/idp/idp_handler.go) and the types by changing
types.IdentityProvider.PKCE and dex.ConnectorConfig.PKCE to *bool (and
optionally JWKSURL to *string) and adjust fromAPIRequest/mergeConnectorConfig to
only overwrite cfg.PKCE or cfg.JWKSURL when the incoming pointer is non-nil (or
alternatively implement merging in the Update* path to read the existing
IdentityProvider, overlay only non-nil request fields, and pass the merged
struct down) so omitted fields are preserved and explicit false/empty values are
honored.


// DeleteConnector removes a connector from Dex storage.
Expand Down Expand Up @@ -210,7 +221,13 @@ func buildOIDCConnectorConfig(cfg *ConnectorConfig, redirectURI string) ([]byte,
"insecureEnableGroups": true,
//some providers don't return email verified, so we need to skip it if not present (e.g., Entra, Okta, Duo)
"insecureSkipEmailVerified": true,
"jwksURL": cfg.JWKSURL,
}

if cfg.PKCE {
oidcConfig["pkceChallenge"] = "S256"
}

switch cfg.Type {
case "zitadel":
oidcConfig["getUserInfo"] = true
Expand Down Expand Up @@ -264,6 +281,12 @@ func (p *Provider) parseStorageConnector(conn storage.Connector) (*ConnectorConf
if v, ok := configMap["issuer"].(string); ok {
cfg.Issuer = v
}
if v, ok := configMap["pkceChallenge"].(string); ok {
cfg.PKCE = v == "S256" || v == "plain"
}
if v, ok := configMap["jwksURL"].(string); ok {
cfg.JWKSURL = v
}

// Infer the original identity provider type from Dex connector type and ID
cfg.Type = inferIdentityProviderType(conn.Type, conn.ID, configMap)
Expand Down
11 changes: 10 additions & 1 deletion management/server/http/handlers/idp/idp_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ func toAPIResponse(idp *types.IdentityProvider) api.IdentityProvider {
Name: idp.Name,
Issuer: idp.Issuer,
ClientId: idp.ClientID,
Pkce: &idp.PKCE,
JwksUrl: &idp.JWKSURL,
}
if idp.ID != "" {
resp.Id = &idp.ID
Expand All @@ -186,11 +188,18 @@ func toAPIResponse(idp *types.IdentityProvider) api.IdentityProvider {
}

func fromAPIRequest(req *api.IdentityProviderRequest) *types.IdentityProvider {
return &types.IdentityProvider{
idp := &types.IdentityProvider{
Type: types.IdentityProviderType(req.Type),
Name: req.Name,
Issuer: req.Issuer,
ClientID: req.ClientId,
ClientSecret: req.ClientSecret,
}
if req.Pkce != nil {
idp.PKCE = *req.Pkce
}
if req.JwksUrl != nil {
idp.JWKSURL = *req.JwksUrl
}
Comment on lines +198 to +203
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

PKCE cannot be distinguished from "not provided" on update.

Because req.Pkce is *bool, fromAPIRequest correctly only sets idp.PKCE when the pointer is non-nil. However, when the request omits pkce, idp.PKCE is left at its zero value (false) and is later passed through identityProviderToConnectorConfig/UpdateConnector. In idp/dex/connector.go::mergeConnectorConfig, there is no preservation logic for PKCE (the comment at lines 139-142 acknowledges this), so an existing PKCE=true configuration will be silently flipped to false on any partial update that omits the field.

Consider propagating the pointer (or a tri-state) all the way down to the merge step, e.g. by carrying *bool on types.IdentityProvider/dex.ConnectorConfig, or by having the handler load the existing IdP and apply only fields that were provided in the request before delegating to the manager.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@management/server/http/handlers/idp/idp_handler.go` around lines 198 - 203,
The handler currently overwrites idp.PKCE with the zero value when req.Pkce is
nil; to avoid flipping existing PKCE you should load the existing IdP first and
only set idp.PKCE when req.Pkce != nil (leave existing value intact otherwise)
before calling identityProviderToConnectorConfig/UpdateConnector (or
alternatively change types.IdentityProvider and dex.ConnectorConfig to carry
*bool and pass the pointer through to mergeConnectorConfig). Concretely, in
fromAPIRequest/this handler: fetch the current IdentityProvider, initialize idp
from that, then apply fields from req only when their pointers (req.Pkce,
req.JwksUrl, etc.) are non-nil so mergeConnectorConfig/UpdateConnector receives
preserved PKCE unless explicitly changed.

return idp
}
11 changes: 11 additions & 0 deletions management/server/http/handlers/idp/idp_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,13 +400,16 @@ func TestDeleteIdentityProvider(t *testing.T) {
}

func TestToAPIResponse(t *testing.T) {
pkce := true
idp := &types.IdentityProvider{
ID: "test-id",
Name: "Test IDP",
Type: types.IdentityProviderTypeGoogle,
Issuer: "https://accounts.google.com",
ClientID: "client-id",
ClientSecret: "should-not-be-returned",
PKCE: pkce,
JWKSURL: "https://example.com/jwks",
}

response := toAPIResponse(idp)
Expand All @@ -416,16 +419,22 @@ func TestToAPIResponse(t *testing.T) {
assert.Equal(t, api.IdentityProviderTypeGoogle, response.Type)
assert.Equal(t, "https://accounts.google.com", response.Issuer)
assert.Equal(t, "client-id", response.ClientId)
assert.Equal(t, pkce, *response.Pkce)
assert.Equal(t, "https://example.com/jwks", *response.JwksUrl)
// Note: ClientSecret is not included in response type by design
}

func TestFromAPIRequest(t *testing.T) {
pkce := true
jwksURL := "https://example.com/jwks"
req := &api.IdentityProviderRequest{
Name: "New IDP",
Type: api.IdentityProviderTypeOkta,
Issuer: "https://dev-123456.okta.com",
ClientId: "okta-client-id",
ClientSecret: "okta-client-secret",
Pkce: &pkce,
JwksUrl: &jwksURL,
}

idp := fromAPIRequest(req)
Expand All @@ -435,4 +444,6 @@ func TestFromAPIRequest(t *testing.T) {
assert.Equal(t, "https://dev-123456.okta.com", idp.Issuer)
assert.Equal(t, "okta-client-id", idp.ClientID)
assert.Equal(t, "okta-client-secret", idp.ClientSecret)
assert.Equal(t, pkce, idp.PKCE)
assert.Equal(t, jwksURL, idp.JWKSURL)
}
4 changes: 4 additions & 0 deletions management/server/identity_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ func connectorConfigToIdentityProvider(conn *dex.ConnectorConfig, accountID stri
Issuer: conn.Issuer,
ClientID: conn.ClientID,
ClientSecret: conn.ClientSecret,
PKCE: conn.PKCE,
JWKSURL: conn.JWKSURL,
}
}

Expand All @@ -270,6 +272,8 @@ func identityProviderToConnectorConfig(idpConfig *types.IdentityProvider) *dex.C
Issuer: idpConfig.Issuer,
ClientID: idpConfig.ClientID,
ClientSecret: idpConfig.ClientSecret,
PKCE: idpConfig.PKCE,
JWKSURL: idpConfig.JWKSURL,
}
}

Expand Down
20 changes: 16 additions & 4 deletions management/server/store/sql_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2723,12 +2723,24 @@ func (s *SqlStore) SaveUserLastLogin(ctx context.Context, accountID, userID stri
return status.NewGetUserFromStoreError()
}

if !lastLogin.IsZero() {
user.LastLogin = &lastLogin
return s.db.Save(&user).Error
if lastLogin.IsZero() {
return nil
}

return nil
if err := user.DecryptSensitiveData(s.fieldEncrypt); err != nil {
return fmt.Errorf("decrypt user: %w", err)
}

user.LastLogin = &lastLogin

userCopy := user.Copy()
userCopy.Email = user.Email
userCopy.Name = user.Name
if err := userCopy.EncryptSensitiveData(s.fieldEncrypt); err != nil {
return fmt.Errorf("encrypt user: %w", err)
}

return s.db.Save(userCopy).Error
}

func (s *SqlStore) GetPostureCheckByChecksDefinition(accountID string, checks *posture.ChecksDefinition) (*posture.Checks, error) {
Expand Down
Loading