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