From a3a03dff782b7700afc8f65a06dd2b229f2144dc Mon Sep 17 00:00:00 2001 From: James Rasell Date: Tue, 9 Jan 2024 14:03:26 +0000 Subject: [PATCH] acl: ensure auth method configs are correctly and fully hashed. (#19677) --- .changelog/19677.txt | 3 + nomad/structs/acl.go | 14 +++ nomad/structs/acl_test.go | 242 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 246 insertions(+), 13 deletions(-) create mode 100644 .changelog/19677.txt diff --git a/.changelog/19677.txt b/.changelog/19677.txt new file mode 100644 index 000000000..31cf86e12 --- /dev/null +++ b/.changelog/19677.txt @@ -0,0 +1,3 @@ +```release-note:bug +acl: Fixed auth method hashing which meant changing some fields would be silently ignored +``` diff --git a/nomad/structs/acl.go b/nomad/structs/acl.go index da0a91ab2..bd96a3dbe 100644 --- a/nomad/structs/acl.go +++ b/nomad/structs/acl.go @@ -779,21 +779,35 @@ func (a *ACLAuthMethod) SetHash() []byte { _, _ = hash.Write([]byte(strconv.FormatBool(a.Default))) if a.Config != nil { + _, _ = hash.Write([]byte(a.Config.JWKSURL)) + _, _ = hash.Write([]byte(a.Config.JWKSCACert)) _, _ = hash.Write([]byte(a.Config.OIDCDiscoveryURL)) _, _ = hash.Write([]byte(a.Config.OIDCClientID)) _, _ = hash.Write([]byte(a.Config.OIDCClientSecret)) + _, _ = hash.Write([]byte(a.Config.ExpirationLeeway.String())) + _, _ = hash.Write([]byte(a.Config.NotBeforeLeeway.String())) + _, _ = hash.Write([]byte(a.Config.ClockSkewLeeway.String())) for _, ba := range a.Config.BoundAudiences { _, _ = hash.Write([]byte(ba)) } + for _, bi := range a.Config.BoundIssuer { + _, _ = hash.Write([]byte(bi)) + } for _, uri := range a.Config.AllowedRedirectURIs { _, _ = hash.Write([]byte(uri)) } for _, pem := range a.Config.DiscoveryCaPem { _, _ = hash.Write([]byte(pem)) } + for _, scope := range a.Config.OIDCScopes { + _, _ = hash.Write([]byte(scope)) + } for _, sa := range a.Config.SigningAlgs { _, _ = hash.Write([]byte(sa)) } + for _, key := range a.Config.JWTValidationPubKeys { + _, _ = hash.Write([]byte(key)) + } for k, v := range a.Config.ClaimMappings { _, _ = hash.Write([]byte(k)) _, _ = hash.Write([]byte(v)) diff --git a/nomad/structs/acl_test.go b/nomad/structs/acl_test.go index 266c7d08b..0f0fb81d3 100644 --- a/nomad/structs/acl_test.go +++ b/nomad/structs/acl_test.go @@ -851,21 +851,237 @@ func Test_ACLAuthMethodGetRequest(t *testing.T) { func TestACLAuthMethodSetHash(t *testing.T) { ci.Parallel(t) - am := &ACLAuthMethod{ - Name: "foo", - Type: "bad type", + testCases := []struct { + name string + fistACLAuthMethod *ACLAuthMethod + secondACLAuthMethod *ACLAuthMethod + expectedEqual bool + }{ + { + name: "type_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "bad type", + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "good type", + }, + expectedEqual: false, + }, + { + name: "oidc_scopes_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + OIDCScopes: []string{"groups"}, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + OIDCScopes: []string{"groups", "roles"}, + }, + }, + expectedEqual: false, + }, + { + name: "jwksurl_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWKSURL: "some-url", + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWKSURL: "some-other-url", + }, + }, + expectedEqual: false, + }, + { + name: "jwkscacert_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWKSCACert: "some-ca-cert", + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWKSCACert: "some-other-ca-cert", + }, + }, + expectedEqual: false, + }, + { + name: "expiration_leeway_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + ExpirationLeeway: 1 * time.Second, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + ExpirationLeeway: 60 * time.Second, + }, + }, + expectedEqual: false, + }, + { + name: "not_before_leeway_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + NotBeforeLeeway: 1 * time.Second, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + NotBeforeLeeway: 60 * time.Second, + }, + }, + expectedEqual: false, + }, + { + name: "clock_skew_leeway_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + ClockSkewLeeway: 1 * time.Second, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + ClockSkewLeeway: 60 * time.Second, + }, + }, + expectedEqual: false, + }, + { + name: "bound_issuer_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + BoundIssuer: []string{"issuer"}, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + BoundIssuer: []string{"issuer", "other-issuer"}, + }, + }, + expectedEqual: false, + }, + { + name: "jtw_validation_pub_keys_change", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWTValidationPubKeys: []string{"pubkey"}, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWTValidationPubKeys: []string{"pubkey", "other-pubkey"}, + }, + }, + expectedEqual: false, + }, + { + name: "full_unchanged", + fistACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWTValidationPubKeys: []string{"some-jwt-validation-pubkey"}, + JWKSURL: "some-jks-url", + OIDCDiscoveryURL: "some-oidc-discovery-url", + OIDCClientID: "some-oidc-client-id", + OIDCClientSecret: "some-oidc-client-secret", + BoundAudiences: []string{"audience1", "audience2"}, + AllowedRedirectURIs: []string{"foo", "bar"}, + DiscoveryCaPem: []string{"foo"}, + SigningAlgs: []string{"bar"}, + OIDCScopes: []string{"some-oidc-scope"}, + BoundIssuer: []string{"some-bound-issuer"}, + JWKSCACert: "some-jks-ca-cert", + ExpirationLeeway: 5 * time.Second, + NotBeforeLeeway: 5 * time.Second, + ClockSkewLeeway: 5 * time.Second, + ClaimMappings: map[string]string{"foo": "bar"}, + ListClaimMappings: map[string]string{"foo": "bar"}, + }, + }, + secondACLAuthMethod: &ACLAuthMethod{ + Name: "foo", + Type: "type", + Config: &ACLAuthMethodConfig{ + JWTValidationPubKeys: []string{"some-jwt-validation-pubkey"}, + JWKSURL: "some-jks-url", + OIDCDiscoveryURL: "some-oidc-discovery-url", + OIDCClientID: "some-oidc-client-id", + OIDCClientSecret: "some-oidc-client-secret", + BoundAudiences: []string{"audience1", "audience2"}, + AllowedRedirectURIs: []string{"foo", "bar"}, + DiscoveryCaPem: []string{"foo"}, + SigningAlgs: []string{"bar"}, + OIDCScopes: []string{"some-oidc-scope"}, + BoundIssuer: []string{"some-bound-issuer"}, + JWKSCACert: "some-jks-ca-cert", + ExpirationLeeway: 5 * time.Second, + NotBeforeLeeway: 5 * time.Second, + ClockSkewLeeway: 5 * time.Second, + ClaimMappings: map[string]string{"foo": "bar"}, + ListClaimMappings: map[string]string{"foo": "bar"}, + }, + }, + expectedEqual: true, + }, } - out1 := am.SetHash() - must.NotNil(t, out1) - must.NotNil(t, am.Hash) - must.Eq(t, out1, am.Hash) - am.Type = "good type" - out2 := am.SetHash() - must.NotNil(t, out2) - must.NotNil(t, am.Hash) - must.Eq(t, out2, am.Hash) - must.NotEq(t, out1, out2) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + must.NotNil(t, tc.fistACLAuthMethod.SetHash()) + must.NotNil(t, tc.fistACLAuthMethod.Hash) + + hash2 := tc.secondACLAuthMethod.SetHash() + must.NotNil(t, hash2) + must.NotNil(t, tc.secondACLAuthMethod.Hash) + must.Eq(t, hash2, tc.secondACLAuthMethod.Hash) + + if tc.expectedEqual { + must.Eq(t, tc.fistACLAuthMethod.Hash, tc.secondACLAuthMethod.Hash) + } else { + must.NotEq(t, tc.fistACLAuthMethod.Hash, tc.secondACLAuthMethod.Hash) + } + }) + } } func TestACLAuthMethod_Stub(t *testing.T) {