auth: oidc: disable pkce by default (#25600)

our goal of "enable by default, only for new auth methods"
proved to be unwieldy, so instead make it a simple bool,
disabled by default.

api/acl.go

@@ -829,9 +829,8 @@ type ACLAuthMethodConfig struct {
 	// Optionally send a signed JWT ("private key jwt") as a client assertion
 	// to the OIDC provider
 	OIDCClientAssertion *OIDCClientAssertion
-	// 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
+	// Enable S256 PKCE challenge verification.
+	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 Enable PKCE|%t", config.OIDCEnablePKCE != nil && *config.OIDCEnablePKCE),
+		fmt.Sprintf("OIDC Enable PKCE|%t", 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

@@ -11,7 +11,6 @@ import (
 	capOIDC "github.com/hashicorp/cap/oidc"
 	"github.com/hashicorp/nomad/api"
 	"github.com/hashicorp/nomad/ci"
-	"github.com/hashicorp/nomad/helper/pointer"
 	"github.com/shoenig/test/must"
 )
 
@@ -95,12 +94,10 @@ func TestACLOIDC_CompleteAuth(t *testing.T) {
 		MaxTokenTTL:   10 * time.Hour,
 		Default:       true,
 		Config: &api.ACLAuthMethodConfig{
-			OIDCDiscoveryURL: oidcTestProvider.Addr(),
-			OIDCClientID:     "mock",
-			OIDCClientSecret: "verysecretsecret",
-			// PKCE is hard to test at this level, because the verifier only
-			// exists on the server. this functionality is covered elsewhere.
-			OIDCEnablePKCE:      pointer.Of(false),
+			OIDCDiscoveryURL:    oidcTestProvider.Addr(),
+			OIDCClientID:        "mock",
+			OIDCClientSecret:    "verysecretsecret",
+			OIDCEnablePKCE:      false,
 			OIDCDisableUserInfo: false,
 			BoundAudiences:      []string{"mock"},
 			AllowedRedirectURIs: []string{"http://127.0.0.1:4649/oidc/callback"},

nomad/acl_endpoint.go

@@ -20,8 +20,6 @@ import (
 	"github.com/hashicorp/go-memdb"
 	metrics "github.com/hashicorp/go-metrics/compat"
 	"github.com/hashicorp/go-set/v3"
-	"github.com/hashicorp/nomad/helper/pointer"
-
 	policy "github.com/hashicorp/nomad/acl"
 	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/helper/uuid"
@@ -1912,12 +1910,6 @@ func (a *ACL) UpsertAuthMethods(
 			}
 		}
 
-		// 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() {
 			_, err := a.oidcClientAssertion(authMethod.Config)
@@ -3071,7 +3063,7 @@ func (a *ACL) oidcRequest(nonce, redirect string, config *structs.ACLAuthMethodC
 		opts = append(opts, capOIDC.WithAudiences(config.BoundAudiences...))
 	}
 
-	if config.OIDCEnablePKCE != nil && *config.OIDCEnablePKCE {
+	if 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

@@ -20,7 +20,6 @@ import (
 	"github.com/hashicorp/go-memdb"
 	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc/v2"
 	"github.com/hashicorp/nomad/ci"
-	"github.com/hashicorp/nomad/helper/pointer"
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/lib/auth/oidc"
 	"github.com/hashicorp/nomad/nomad/mock"
@@ -3141,38 +3140,6 @@ 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) {
@@ -3668,7 +3635,7 @@ func TestACL_OIDCAuthURL(t *testing.T) {
 	t.Run("pkce", func(t *testing.T) {
 		authMethod := mockedAuthMethod.Copy()
 		authMethod.Name = mockedAuthMethod.Name + "-pkce"
-		authMethod.Config.OIDCEnablePKCE = pointer.Of(true)
+		authMethod.Config.OIDCEnablePKCE = true
 		authMethod.SetHash()
 		must.NoError(t, testServer.fsm.State().UpsertACLAuthMethods(20, []*structs.ACLAuthMethod{authMethod}))
 
@@ -3955,7 +3922,7 @@ func TestACL_OIDCCompleteAuth(t *testing.T) {
 
 	t.Run("pkce", func(t *testing.T) {
 
-		mockedAuthMethod.Config.OIDCEnablePKCE = pointer.Of(true)
+		mockedAuthMethod.Config.OIDCEnablePKCE = true
 		must.NoError(t, testServer.fsm.State().UpsertACLAuthMethods(60, []*structs.ACLAuthMethod{mockedAuthMethod}))
 
 		req := structs.ACLOIDCCompleteAuthRequest{

nomad/mock/acl.go

@@ -16,7 +16,6 @@ import (
 	"time"
 
 	"github.com/golang-jwt/jwt/v5"
-	"github.com/hashicorp/nomad/helper/pointer"
 	testing "github.com/mitchellh/go-testing-interface"
 	"github.com/stretchr/testify/assert"
 
@@ -273,12 +272,10 @@ func ACLOIDCAuthMethod() *structs.ACLAuthMethod {
 		MaxTokenTTL:   maxTokenTTL,
 		Default:       false,
 		Config: &structs.ACLAuthMethodConfig{
-			OIDCDiscoveryURL: "http://example.com",
-			OIDCClientID:     "mock",
-			OIDCClientSecret: "very secret secret",
-			// PKCE is hard to test outside the server/RPC layer,
-			// because the verifier is only accessible there.
-			OIDCEnablePKCE:      pointer.Of(false),
+			OIDCDiscoveryURL:    "http://example.com",
+			OIDCClientID:        "mock",
+			OIDCClientSecret:    "very secret secret",
+			OIDCEnablePKCE:      false,
 			OIDCDisableUserInfo: false,
 			OIDCScopes:          []string{"groups"},
 			BoundAudiences:      []string{"sales", "engineering"},

nomad/structs/acl.go

@@ -796,9 +796,7 @@ 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.OIDCEnablePKCE != nil {
-			_, _ = hash.Write([]byte(strconv.FormatBool(*a.Config.OIDCEnablePKCE)))
-		}
+		_, _ = hash.Write([]byte(strconv.FormatBool(a.Config.OIDCEnablePKCE)))
 		_, _ = hash.Write([]byte(strconv.FormatBool(a.Config.OIDCDisableUserInfo)))
 		_, _ = hash.Write([]byte(strconv.FormatBool(a.Config.VerboseLogging)))
 		_, _ = hash.Write([]byte(a.Config.ExpirationLeeway.String()))
@@ -1062,9 +1060,7 @@ type ACLAuthMethodConfig struct {
 	OIDCClientAssertion *OIDCClientAssertion
 
 	// Enable PKCE challenge verification
-	// If nil, the ACL Upsert RPC endpoint sets it to &true,
-	// if the auth method is brand new.
-	OIDCEnablePKCE *bool
+	OIDCEnablePKCE bool
 
 	// Disable claims from the OIDC UserInfo endpoint
 	OIDCDisableUserInfo bool

website/content/partials/api-docs/auth-method-params.mdx

@@ -98,10 +98,9 @@ for the auth method.
     alongside "kid" and "type". Setting the "kid" header here is not allowed;
     use `PrivateKey.KeyID`.
 
-  - `OIDCEnablePKCE` `(bool: true)` - When set to `true`, Nomad will include
+  - `OIDCEnablePKCE` `(bool: false)` - 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.
+  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