From 6383d5f54d79e9170957b67cb9068d729f8f0b02 Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Thu, 3 Apr 2025 12:53:37 -0400 Subject: [PATCH] auth: oidc client assertion tweaks (#25565) * allow for newline flexibility in client assertion key/cert * if client assertion, don't send the client secret, but do keep the client secret in both places in state (on the parent Config, and within the OIDCClientAssertion) mainly so that it shows up as "redacted" instead of empty when inspecting the auth method config via API. --- lib/auth/oidc/client_assertion.go | 25 +++++++++++ lib/auth/oidc/client_assertion_test.go | 61 ++++++++++++++++++++++++++ lib/auth/oidc/provider.go | 10 ++++- nomad/structs/acl.go | 5 +-- nomad/structs/acl_test.go | 1 - 5 files changed, 97 insertions(+), 5 deletions(-) diff --git a/lib/auth/oidc/client_assertion.go b/lib/auth/oidc/client_assertion.go index 33f6f0885..a95e7d01d 100644 --- a/lib/auth/oidc/client_assertion.go +++ b/lib/auth/oidc/client_assertion.go @@ -4,7 +4,9 @@ package oidc import ( + "bytes" "crypto/rsa" + // sha1 is used to derive an "x5t" jwt header from an x509 certificate, // per the OIDC JWS spec: // https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.7 @@ -131,6 +133,9 @@ func getCassPrivateKey(k *structs.OIDCClientAssertionKey) (key *rsa.PrivateKey, bts = []byte(k.PemKey) } + // ensure newlines around pem header/footer + bts = newlineHeaders(bts) + key, err = gojwt.ParseRSAPrivateKeyFromPEM(bts) if err != nil { return nil, fmt.Errorf("error parsing %s: %w", source, err) @@ -162,6 +167,9 @@ func getCassCert(k *structs.OIDCClientAssertionKey) (*x509.Certificate, error) { bts = []byte(k.PemCert) } + // ensure newlines around pem header/footer + bts = newlineHeaders(bts) + block, _ := pem.Decode(bts) if block == nil { return nil, fmt.Errorf("failed to decode %s PEM block", source) @@ -195,3 +203,20 @@ func hashKeyID(cert *x509.Certificate, header structs.OIDCClientAssertionKeyIDHe hashed := hasher.Sum(nil) return base64.RawURLEncoding.EncodeToString(hashed), nil } + +// newlineHeaders allows flexible copy-paste of a one-line key/cert PEM +// by adding newlines around "----BEGIN.*-----" and +// "-----END.*(KEY|CERTIFICATE)-----" +// it's okay to have extra whitespace, but it's imperative that there be +// at least one newline between the header/footer and the content. +func newlineHeaders(bts []byte) []byte { + cp := bytes.Clone(bts) + cp = bytes.TrimSpace(cp) + cp = bytes.ReplaceAll(cp, []byte("-----BEGIN"), []byte("\n-----BEGIN")) + cp = bytes.ReplaceAll(cp, []byte("-----END"), []byte("\n-----END")) + // key may be "PRIVATE KEY" or "RSA PRIVATE KEY", so just look for "KEY" + cp = bytes.ReplaceAll(cp, []byte("KEY-----"), []byte("KEY-----\n")) + cp = bytes.ReplaceAll(cp, []byte("CERTIFICATE-----"), []byte("CERTIFICATE-----\n")) + cp = bytes.TrimSpace(cp) + return cp +} diff --git a/lib/auth/oidc/client_assertion_test.go b/lib/auth/oidc/client_assertion_test.go index e526fc4bd..827fb2d10 100644 --- a/lib/auth/oidc/client_assertion_test.go +++ b/lib/auth/oidc/client_assertion_test.go @@ -518,3 +518,64 @@ func generateInvalidTestPrivateKey(t *testing.T) *rsa.PrivateKey { return key } +func TestNewlineHeaders(t *testing.T) { + cases := []struct { + name string + content string + expect string + }{ + { + name: "empty", + content: "", + expect: "", + }, + { + name: "nonsense", + content: "not a key or cert", + expect: "not a key or cert", + }, + { + name: "pem-shaped nonsense", + content: "-----BEGIN RANDOM PEM-----stuff-----END RANDOM PEM-----", + expect: "-----BEGIN RANDOM PEM-----stuff\n-----END RANDOM PEM-----", + }, + { + name: "no newlines key", + content: "-----BEGIN ANY KIND OF PRIVATE KEY-----stuff-----END ANY PRIVATE KEY-----", + expect: "-----BEGIN ANY KIND OF PRIVATE KEY-----\nstuff\n-----END ANY PRIVATE KEY-----", + }, + { + name: "no newlines cert", + content: "-----BEGIN ANY KIND OF CERTIFICATE-----stuff-----END ANY CERTIFICATE-----", + expect: "-----BEGIN ANY KIND OF CERTIFICATE-----\nstuff\n-----END ANY CERTIFICATE-----", + }, + // extra newlines between header/footer and content is okay. + { + name: "with newlines key", + content: "-----BEGIN ANY KIND OF PRIVATE KEY-----\nstuff\n-----END ANY PRIVATE KEY-----", + expect: "-----BEGIN ANY KIND OF PRIVATE KEY-----\n\nstuff\n\n-----END ANY PRIVATE KEY-----", + }, + { + name: "with newlines cert", + content: "-----BEGIN ANY KIND OF CERTIFICATE-----\nstuff\nmore\nstuff\n-----END ANY CERTIFICATE-----", + expect: "-----BEGIN ANY KIND OF CERTIFICATE-----\n\nstuff\nmore\nstuff\n\n-----END ANY CERTIFICATE-----", + }, + // extra junk outside the header/footer is okay. + { + name: "extra junk key", + content: "note to self\n-----BEGIN ANY KIND OF PRIVATE KEY-----\nstuff\n-----END ANY PRIVATE KEY-----\nanother note", + expect: "note to self\n\n-----BEGIN ANY KIND OF PRIVATE KEY-----\n\nstuff\n\n-----END ANY PRIVATE KEY-----\n\nanother note", + }, + { + name: "extra junk cert", + content: "note to self\n-----BEGIN ANY KIND OF CERTIFICATE-----\nstuff\n-----END ANY CERTIFICATE-----\nanother note", + expect: "note to self\n\n-----BEGIN ANY KIND OF CERTIFICATE-----\n\nstuff\n\n-----END ANY CERTIFICATE-----\n\nanother note", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := newlineHeaders([]byte(tc.content)) + must.Eq(t, tc.expect, string(got)) + }) + } +} diff --git a/lib/auth/oidc/provider.go b/lib/auth/oidc/provider.go index 9d8c15297..2e0821eb1 100644 --- a/lib/auth/oidc/provider.go +++ b/lib/auth/oidc/provider.go @@ -25,10 +25,18 @@ func providerConfig(authMethod *structs.ACLAuthMethod) (*oidc.Config, error) { algs = []oidc.Alg{oidc.RS256} } + // if client assertion is enabled, do not send a client secret normally; + // if it is set to anything, it will be used as an HMAC to sign the client + // assertion JWT, instead. + clientSecret := authMethod.Config.OIDCClientSecret + if authMethod.Config.OIDCClientAssertion != nil { + clientSecret = "" + } + return oidc.NewConfig( authMethod.Config.OIDCDiscoveryURL, authMethod.Config.OIDCClientID, - oidc.ClientSecret(authMethod.Config.OIDCClientSecret), + oidc.ClientSecret(clientSecret), algs, authMethod.Config.AllowedRedirectURIs, oidc.WithAudiences(authMethod.Config.BoundAudiences...), diff --git a/nomad/structs/acl.go b/nomad/structs/acl.go index 47e8fb1f5..9cfb748a8 100644 --- a/nomad/structs/acl.go +++ b/nomad/structs/acl.go @@ -1123,10 +1123,9 @@ func (a *ACLAuthMethodConfig) Canonicalize() { if len(a.OIDCClientAssertion.Audience) == 0 { a.OIDCClientAssertion.Audience = []string{a.OIDCDiscoveryURL} } - // move the client secret into the client assertion + // the client assertion inherits the client secret, + // in case KeySource = "client_secret" a.OIDCClientAssertion.ClientSecret = a.OIDCClientSecret - // do not also send the client secret normally - a.OIDCClientSecret = "" a.OIDCClientAssertion.Canonicalize() } } diff --git a/nomad/structs/acl_test.go b/nomad/structs/acl_test.go index 7a9d4b76e..d93d7c698 100644 --- a/nomad/structs/acl_test.go +++ b/nomad/structs/acl_test.go @@ -1403,7 +1403,6 @@ func TestACLAuthMethodConfig_Canonicalize(t *testing.T) { am.Canonicalize() must.Eq(t, []string{"test-disco-url"}, cass.Audience, must.Sprint("should inherit audience")) must.Eq(t, "super secret", cass.ClientSecret, must.Sprint("should inherit secret")) - must.Eq(t, "", am.OIDCClientSecret, must.Sprint("secret should move to assertion")) } func TestACLAuthMethodConfig_Validate(t *testing.T) {