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

@@ -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

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, ",")),

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"},

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)

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{

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"},

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

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
 

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