From 6a0c4f5a3d2c70b26bf118edec9ac2991bf23dac Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Thu, 3 Apr 2025 11:56:17 -0400 Subject: [PATCH] auth: oidc: enable pkce only on new auth methods (#25593) trying not to violate the principle of least astonishment. we want to only auto-enable PKCE on *new* auth methods, rather than *new or updated* auth methods, to avoid a scenario where a Nomad admin updates an auth method sometime in the future -- something innocent like a new client secret -- and their OIDC provider doesn't like PKCE. the main concern is that the provider won't like PKCE in a totally confusing way. error messages rarely say PKCE directly, so why the user's auth method suddenly broke would be a big mystery. this means that to enable it on existing auth methods, you would set `OIDCDisablePKCE = false`, and the double- negative doesn't feel right, so instead, swap the language, so enabling it on *existing* methods reads sensibly, and to disable it on *new* methods reads ok-enough: `OIDCEnablePKCE = false` --- api/acl.go | 5 +-- command/acl_auth_method.go | 2 +- internal/testing/apitests/acl_test.go | 2 +- nomad/acl_endpoint.go | 10 +++--- nomad/acl_endpoint_test.go | 36 +++++++++++++++++-- nomad/mock/acl.go | 2 +- nomad/structs/acl.go | 10 +++--- .../content/docs/upgrade/upgrade-specific.mdx | 15 +++----- .../partials/api-docs/auth-method-params.mdx | 8 ++--- 9 files changed, 61 insertions(+), 29 deletions(-) diff --git a/api/acl.go b/api/acl.go index c391e26e7..12d39b6a9 100644 --- a/api/acl.go +++ b/api/acl.go @@ -829,8 +829,9 @@ type ACLAuthMethodConfig struct { // Optionally send a signed JWT ("private key jwt") as a client assertion // to the OIDC provider OIDCClientAssertion *OIDCClientAssertion - // Disable S256 PKCE challenge verification - OIDCDisablePKCE *bool + // Enable S256 PKCE challenge verification. If nil, the Nomad server sets + // this to true when creating an auth method. I.e. it is enabled by default. + OIDCEnablePKCE *bool // Disable claims from the OIDC UserInfo endpoint OIDCDisableUserInfo bool // List of OIDC scopes diff --git a/command/acl_auth_method.go b/command/acl_auth_method.go index 113c22db8..6eb893cd1 100644 --- a/command/acl_auth_method.go +++ b/command/acl_auth_method.go @@ -96,7 +96,7 @@ func formatAuthMethodConfig(config *api.ACLAuthMethodConfig) string { } out = append(out, formatClientAssertion(config.OIDCClientAssertion)...) out = append(out, - fmt.Sprintf("OIDC Disable PKCE|%t", config.OIDCDisablePKCE != nil && *config.OIDCDisablePKCE), + fmt.Sprintf("OIDC Enable PKCE|%t", config.OIDCEnablePKCE != nil && *config.OIDCEnablePKCE), fmt.Sprintf("OIDC Disable UserInfo|%t", config.OIDCDisableUserInfo), fmt.Sprintf("OIDC Scopes|%s", strings.Join(config.OIDCScopes, ",")), fmt.Sprintf("Bound audiences|%s", strings.Join(config.BoundAudiences, ",")), diff --git a/internal/testing/apitests/acl_test.go b/internal/testing/apitests/acl_test.go index a84aaf716..a0905b3ff 100644 --- a/internal/testing/apitests/acl_test.go +++ b/internal/testing/apitests/acl_test.go @@ -100,7 +100,7 @@ func TestACLOIDC_CompleteAuth(t *testing.T) { OIDCClientSecret: "verysecretsecret", // PKCE is hard to test at this level, because the verifier only // exists on the server. this functionality is covered elsewhere. - OIDCDisablePKCE: pointer.Of(true), + OIDCEnablePKCE: pointer.Of(false), OIDCDisableUserInfo: false, BoundAudiences: []string{"mock"}, AllowedRedirectURIs: []string{"http://127.0.0.1:4649/oidc/callback"}, diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index dfe92ba2e..7c2a708ae 100644 --- a/nomad/acl_endpoint.go +++ b/nomad/acl_endpoint.go @@ -1912,9 +1912,11 @@ func (a *ACL) UpsertAuthMethods( } } - // if PKCE is not explicitly disabled, enable it. - if authMethod.Config.OIDCDisablePKCE == nil { - authMethod.Config.OIDCDisablePKCE = pointer.Of(false) + // if this is a new auth method, and PKCE is not explicitly disabled + // (by setting Enable=false), then enable it. existing auth methods + // need it to be enabled explicitly (Enable=True). + if existingMethod == nil && authMethod.Config.OIDCEnablePKCE == nil { + authMethod.Config.OIDCEnablePKCE = pointer.Of(true) } // if there is a client assertion, ensure it is valid. if authMethod.Config.OIDCClientAssertion.IsSet() { @@ -3069,7 +3071,7 @@ func (a *ACL) oidcRequest(nonce, redirect string, config *structs.ACLAuthMethodC opts = append(opts, capOIDC.WithAudiences(config.BoundAudiences...)) } - if config.OIDCDisablePKCE != nil && !*config.OIDCDisablePKCE { + if config.OIDCEnablePKCE != nil && *config.OIDCEnablePKCE { verifier, err := capOIDC.NewCodeVerifier() if err != nil { return nil, fmt.Errorf("failed to make pkce verifier: %w", err) diff --git a/nomad/acl_endpoint_test.go b/nomad/acl_endpoint_test.go index a7d7e466d..34881971d 100644 --- a/nomad/acl_endpoint_test.go +++ b/nomad/acl_endpoint_test.go @@ -3141,6 +3141,38 @@ func TestACLEndpoint_UpsertACLAuthMethods(t *testing.T) { } must.NoError(t, msgpackrpc.CallWithCodec(codec, structs.ACLUpsertAuthMethodsRPCMethod, req, &resp)) must.Eq(t, resp.AuthMethods[0].TokenLocality, am3.TokenLocality) + + // default PKCE behavior + // * for new auth methods, it should default to true + // * for existing auth methods, it should remain nil + t.Run("pkce", func(t *testing.T) { + amPKCE := mock.ACLOIDCAuthMethod() + + // new auth method, should default to true + amPKCE.Config.OIDCEnablePKCE = nil + req = &structs.ACLAuthMethodUpsertRequest{ + AuthMethods: []*structs.ACLAuthMethod{amPKCE}, + WriteRequest: structs.WriteRequest{ + Region: "global", + AuthToken: root.SecretID, + }, + } + must.NoError(t, msgpackrpc.CallWithCodec(codec, structs.ACLUpsertAuthMethodsRPCMethod, req, &resp)) + out, err = s1.fsm.State().GetACLAuthMethodByName(nil, amPKCE.Name) + must.NoError(t, err) + must.NotNil(t, out) + must.NotNil(t, out.Config) + must.True(t, *out.Config.OIDCEnablePKCE, must.Sprint("pkce should be enabled on new auth methods")) + + // but should remain disabled on existing auth methods + // upsert it directly to state to set it back to nil (not possible in rpc upsert) + must.NoError(t, s1.fsm.State().UpsertACLAuthMethods(resp.Index+1, []*structs.ACLAuthMethod{amPKCE})) + must.NoError(t, msgpackrpc.CallWithCodec(codec, structs.ACLUpsertAuthMethodsRPCMethod, req, &resp)) + out, err = s1.fsm.State().GetACLAuthMethodByName(nil, amPKCE.Name) + must.NoError(t, err) + must.NotNil(t, out) + must.Nil(t, out.Config.OIDCEnablePKCE, must.Sprint("pkce should remain disabled on existing auth methods")) + }) } func TestACL_UpsertBindingRules(t *testing.T) { @@ -3636,7 +3668,7 @@ func TestACL_OIDCAuthURL(t *testing.T) { t.Run("pkce", func(t *testing.T) { authMethod := mockedAuthMethod.Copy() authMethod.Name = mockedAuthMethod.Name + "-pkce" - authMethod.Config.OIDCDisablePKCE = pointer.Of(false) + authMethod.Config.OIDCEnablePKCE = pointer.Of(true) authMethod.SetHash() must.NoError(t, testServer.fsm.State().UpsertACLAuthMethods(20, []*structs.ACLAuthMethod{authMethod})) @@ -3923,7 +3955,7 @@ func TestACL_OIDCCompleteAuth(t *testing.T) { t.Run("pkce", func(t *testing.T) { - mockedAuthMethod.Config.OIDCDisablePKCE = pointer.Of(false) + mockedAuthMethod.Config.OIDCEnablePKCE = pointer.Of(true) must.NoError(t, testServer.fsm.State().UpsertACLAuthMethods(60, []*structs.ACLAuthMethod{mockedAuthMethod})) req := structs.ACLOIDCCompleteAuthRequest{ diff --git a/nomad/mock/acl.go b/nomad/mock/acl.go index 5824d3045..b35e0266f 100644 --- a/nomad/mock/acl.go +++ b/nomad/mock/acl.go @@ -278,7 +278,7 @@ func ACLOIDCAuthMethod() *structs.ACLAuthMethod { OIDCClientSecret: "very secret secret", // PKCE is hard to test outside the server/RPC layer, // because the verifier is only accessible there. - OIDCDisablePKCE: pointer.Of(true), + OIDCEnablePKCE: pointer.Of(false), OIDCDisableUserInfo: false, OIDCScopes: []string{"groups"}, BoundAudiences: []string{"sales", "engineering"}, diff --git a/nomad/structs/acl.go b/nomad/structs/acl.go index d037e001e..47e8fb1f5 100644 --- a/nomad/structs/acl.go +++ b/nomad/structs/acl.go @@ -796,8 +796,8 @@ func (a *ACLAuthMethod) SetHash() []byte { _, _ = hash.Write([]byte(a.Config.OIDCDiscoveryURL)) _, _ = hash.Write([]byte(a.Config.OIDCClientID)) _, _ = hash.Write([]byte(a.Config.OIDCClientSecret)) - if a.Config.OIDCDisablePKCE != nil { - _, _ = hash.Write([]byte(strconv.FormatBool(*a.Config.OIDCDisablePKCE))) + if a.Config.OIDCEnablePKCE != nil { + _, _ = hash.Write([]byte(strconv.FormatBool(*a.Config.OIDCEnablePKCE))) } _, _ = hash.Write([]byte(strconv.FormatBool(a.Config.OIDCDisableUserInfo))) _, _ = hash.Write([]byte(strconv.FormatBool(a.Config.VerboseLogging))) @@ -1061,8 +1061,10 @@ type ACLAuthMethodConfig struct { // Optional client assertion ("private key jwt") config OIDCClientAssertion *OIDCClientAssertion - // Disable PKCE challenge verification - OIDCDisablePKCE *bool + // Enable PKCE challenge verification + // If nil, the ACL Upsert RPC endpoint sets it to &true, + // if the auth method is brand new. + OIDCEnablePKCE *bool // Disable claims from the OIDC UserInfo endpoint OIDCDisableUserInfo bool diff --git a/website/content/docs/upgrade/upgrade-specific.mdx b/website/content/docs/upgrade/upgrade-specific.mdx index 1f67c473d..13c2e63e0 100644 --- a/website/content/docs/upgrade/upgrade-specific.mdx +++ b/website/content/docs/upgrade/upgrade-specific.mdx @@ -91,18 +91,13 @@ rendering must include a [`consul` block](/nomad/docs/job-specification/consul) #### OIDC login with PKCE Nomad now enables [Proof Key for Code Exchange (PKCE)](https://oauth.net/2/pkce/) -by default for new or updated OIDC auth methods. Existing auth methods remain -unaffected until changed using the -[acl auth-method create](/nomad/docs/commands/acl/auth-method/create) or -[acl auth-method update](/nomad/docs/commands/acl/auth-method/update) -CLI commands, or with the -[auth method API](/nomad/api-docs/acl/auth-methods) directly. +by default for new OIDC auth methods. Existing auth methods remain unaffected. -Set the [`OIDCDisablePKCE`](/nomad/api-docs/acl/auth-methods#oidcdisablepkce) -option to turn off this extra security. +Note that even if PKCE is enabled in Nomad, some OIDC providers may require you +to also enable it in their configuration. -Note that although Nomad enables PKCE by default, some OIDC providers may -require you to also enable it in their configuration. +Set the [`OIDCEnablePKCE`](/nomad/api-docs/acl/auth-methods#oidcenablepkce) +option to `false` to turn off this extra security. ## Nomad 1.9.5 diff --git a/website/content/partials/api-docs/auth-method-params.mdx b/website/content/partials/api-docs/auth-method-params.mdx index 4052139e1..5fb77f0d9 100644 --- a/website/content/partials/api-docs/auth-method-params.mdx +++ b/website/content/partials/api-docs/auth-method-params.mdx @@ -98,10 +98,10 @@ for the auth method. alongside "kid" and "type". Setting the "kid" header here is not allowed; use `PrivateKey.KeyID`. - - `OIDCDisablePKCE` `(bool: false)` - When set to `true`, Nomad will not - include [PKCE][] verification in the auth flow. Even with PKCE enabled in - Nomad, which is the default setting, you may still need to enable it in the - OIDC provider. + - `OIDCEnablePKCE` `(bool: true)` - When set to `true`, Nomad will include + [PKCE][] verification in the auth flow. Even with PKCE enabled in Nomad, + which is the default setting, you may still need to enable it in your OIDC + provider. - `OIDCDisableUserInfo` `(bool: false)` - When set to `true`, Nomad will not make a request to the identity provider to get OIDC UserInfo. You may wish to