From 5c8e436de98b18bd3b7d0da05d8fa83c1529b4b2 Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Mon, 7 Apr 2025 13:36:09 -0400 Subject: [PATCH] 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 | 5 +-- command/acl_auth_method.go | 2 +- internal/testing/apitests/acl_test.go | 11 ++---- nomad/acl_endpoint.go | 10 +---- nomad/acl_endpoint_test.go | 37 +------------------ nomad/mock/acl.go | 11 ++---- nomad/structs/acl.go | 8 +--- .../partials/api-docs/auth-method-params.mdx | 5 +-- 8 files changed, 18 insertions(+), 71 deletions(-) diff --git a/api/acl.go b/api/acl.go index 12d39b6a9..dd61541aa 100644 --- a/api/acl.go +++ b/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 diff --git a/command/acl_auth_method.go b/command/acl_auth_method.go index 6eb893cd1..0e862934d 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 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, ",")), diff --git a/internal/testing/apitests/acl_test.go b/internal/testing/apitests/acl_test.go index a0905b3ff..f47030f37 100644 --- a/internal/testing/apitests/acl_test.go +++ b/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"}, diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index 7c2a708ae..997643455 100644 --- a/nomad/acl_endpoint.go +++ b/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) diff --git a/nomad/acl_endpoint_test.go b/nomad/acl_endpoint_test.go index 34881971d..13a5b5896 100644 --- a/nomad/acl_endpoint_test.go +++ b/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{ diff --git a/nomad/mock/acl.go b/nomad/mock/acl.go index b35e0266f..90757cde0 100644 --- a/nomad/mock/acl.go +++ b/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"}, diff --git a/nomad/structs/acl.go b/nomad/structs/acl.go index 9cfb748a8..2bb7a04b1 100644 --- a/nomad/structs/acl.go +++ b/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 diff --git a/website/content/partials/api-docs/auth-method-params.mdx b/website/content/partials/api-docs/auth-method-params.mdx index 5fb77f0d9..4fa934ce4 100644 --- a/website/content/partials/api-docs/auth-method-params.mdx +++ b/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