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

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

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))
+		})
+	}
+}

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

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()
 	}
 }

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) {