diff --git a/go.mod b/go.mod index 836247d..aafb15e 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ godebug default=go1.25 require ( github.com/approvals/go-approval-tests v1.8.0 - github.com/nats-io/jwt/v2 v2.8.0 + github.com/nats-io/jwt/v2 v2.8.1 github.com/nats-io/nats.go v1.49.0 github.com/nats-io/nkeys v0.4.15 github.com/onsi/ginkgo/v2 v2.28.1 @@ -15,7 +15,6 @@ require ( k8s.io/api v0.35.2 k8s.io/apimachinery v0.35.2 k8s.io/client-go v0.35.2 - k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 sigs.k8s.io/controller-runtime v0.23.3 sigs.k8s.io/yaml v1.6.0 ) @@ -80,15 +79,15 @@ require ( go.uber.org/zap v1.27.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect - golang.org/x/crypto v0.47.0 // indirect + golang.org/x/crypto v0.48.0 // indirect golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect golang.org/x/mod v0.32.0 // indirect golang.org/x/net v0.49.0 // indirect golang.org/x/oauth2 v0.30.0 // indirect golang.org/x/sync v0.19.0 // indirect - golang.org/x/sys v0.40.0 // indirect - golang.org/x/term v0.39.0 // indirect - golang.org/x/text v0.33.0 // indirect + golang.org/x/sys v0.41.0 // indirect + golang.org/x/term v0.40.0 // indirect + golang.org/x/text v0.34.0 // indirect golang.org/x/time v0.9.0 // indirect golang.org/x/tools v0.41.0 // indirect gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect @@ -104,6 +103,7 @@ require ( k8s.io/component-base v0.35.0 // indirect k8s.io/klog/v2 v2.130.1 // indirect k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 // indirect + k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.31.2 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect diff --git a/go.sum b/go.sum index 7b6cc2c..10a6d71 100644 --- a/go.sum +++ b/go.sum @@ -109,8 +109,8 @@ github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFd github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= -github.com/nats-io/jwt/v2 v2.8.0 h1:K7uzyz50+yGZDO5o772eRE7atlcSEENpL7P+b74JV1g= -github.com/nats-io/jwt/v2 v2.8.0/go.mod h1:me11pOkwObtcBNR8AiMrUbtVOUGkqYjMQZ6jnSdVUIA= +github.com/nats-io/jwt/v2 v2.8.1 h1:V0xpGuD/N8Mi+fQNDynXohVvp7ZztevW5io8CUWlPmU= +github.com/nats-io/jwt/v2 v2.8.1/go.mod h1:nWnOEEiVMiKHQpnAy4eXlizVEtSfzacZ1Q43LIRavZg= github.com/nats-io/nats.go v1.49.0 h1:yh/WvY59gXqYpgl33ZI+XoVPKyut/IcEaqtsiuTJpoE= github.com/nats-io/nats.go v1.49.0/go.mod h1:fDCn3mN5cY8HooHwE2ukiLb4p4G4ImmzvXyJt+tGwdw= github.com/nats-io/nkeys v0.4.15 h1:JACV5jRVO9V856KOapQ7x+EY8Jo3qw1vJt/9Jpwzkk4= @@ -195,8 +195,8 @@ go.yaml.in/yaml/v2 v2.4.3 h1:6gvOSjQoTB3vt1l+CU+tSyi/HOjfOjRLJ4YwYZGwRO0= go.yaml.in/yaml/v2 v2.4.3/go.mod h1:zSxWcmIDjOzPXpjlTTbAsKokqkDNAVtZO0WOMiT90s8= go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= -golang.org/x/crypto v0.47.0 h1:V6e3FRj+n4dbpw86FJ8Fv7XVOql7TEwpHapKoMJ/GO8= -golang.org/x/crypto v0.47.0/go.mod h1:ff3Y9VzzKbwSSEzWqJsJVBnWmRwRSHt/6Op5n9bQc4A= +golang.org/x/crypto v0.48.0 h1:/VRzVqiRSggnhY7gNRxPauEQ5Drw9haKdM0jqfcCFts= +golang.org/x/crypto v0.48.0/go.mod h1:r0kV5h3qnFPlQnBSrULhlsRfryS2pmewsg+XfMgkVos= golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/mod v0.32.0 h1:9F4d3PHLljb6x//jOyokMv3eX+YDeepZSEo3mFJy93c= @@ -207,12 +207,12 @@ golang.org/x/oauth2 v0.30.0 h1:dnDm7JmhM45NNpd8FDDeLhK6FwqbOf4MLCM9zb1BOHI= golang.org/x/oauth2 v0.30.0/go.mod h1:B++QgG3ZKulg6sRPGD/mqlHQs5rB3Ml9erfeDY7xKlU= golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= -golang.org/x/sys v0.40.0 h1:DBZZqJ2Rkml6QMQsZywtnjnnGvHza6BTfYFWY9kjEWQ= -golang.org/x/sys v0.40.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= -golang.org/x/term v0.39.0 h1:RclSuaJf32jOqZz74CkPA9qFuVTX7vhLlpfj/IGWlqY= -golang.org/x/term v0.39.0/go.mod h1:yxzUCTP/U+FzoxfdKmLaA0RV1WgE0VY7hXBwKtY/4ww= -golang.org/x/text v0.33.0 h1:B3njUFyqtHDUI5jMn1YIr5B0IE2U0qck04r6d4KPAxE= -golang.org/x/text v0.33.0/go.mod h1:LuMebE6+rBincTi9+xWTY8TztLzKHc/9C1uBCG27+q8= +golang.org/x/sys v0.41.0 h1:Ivj+2Cp/ylzLiEU89QhWblYnOE9zerudt9Ftecq2C6k= +golang.org/x/sys v0.41.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +golang.org/x/term v0.40.0 h1:36e4zGLqU4yhjlmxEaagx2KuYbJq3EwY8K943ZsHcvg= +golang.org/x/term v0.40.0/go.mod h1:w2P8uVp06p2iyKKuvXIm7N/y0UCRt3UfJTfZ7oOpglM= +golang.org/x/text v0.34.0 h1:oL/Qq0Kdaqxa1KbNeMKwQq0reLCCaFtqu2eNuSeNHbk= +golang.org/x/text v0.34.0/go.mod h1:homfLqTYRFyVYemLBFl5GgL/DWEiH5wcsQ5gSh1yziA= golang.org/x/time v0.9.0 h1:EsRrnYcQiGH+5FfbgvV4AP7qEZstoyrHB0DzarOQ4ZY= golang.org/x/time v0.9.0/go.mod h1:3BpzKBy/shNhVucY/MWOyx10tF3SFh9QdLuxbVysPQM= golang.org/x/tools v0.41.0 h1:a9b8iMweWG+S0OBnlU36rzLp20z1Rp10w+IY2czHTQc= diff --git a/internal/account/account.go b/internal/account/account.go index d2f68ae..227e69d 100644 --- a/internal/account/account.go +++ b/internal/account/account.go @@ -143,13 +143,11 @@ func (a *Manager) Create(ctx context.Context, state *v1alpha1.Account) (*control signingKey(accountSigningPublicKey). build() if err != nil { - accountName := fmt.Sprintf("%s-%s", state.GetNamespace(), state.GetName()) - return nil, fmt.Errorf("failed to build NATS account claims for %s during creation: %w", accountName, err) + return nil, fmt.Errorf("failed to build NATS account claims during creation: %w", err) } - signedJwt, err := natsClaims.Encode(cluster.OperatorSigningKey) + signedJwt, err := signAccountJWT(natsClaims, cluster.OperatorSigningKey) if err != nil { - accountName := fmt.Sprintf("%s-%s", state.GetNamespace(), state.GetName()) - return nil, fmt.Errorf("failed to sign account jwt for %s during creation: %w", accountName, err) + return nil, fmt.Errorf("failed to sign account jwt during creation: %w", err) } conn, err := a.natsClient.Connect(cluster.NatsURL, cluster.SystemAdminCreds) @@ -172,6 +170,15 @@ func (a *Manager) Create(ctx context.Context, state *v1alpha1.Account) (*control }, nil } +func signAccountJWT(claims *jwt.AccountClaims, operatorSigningKey nkeys.KeyPair) (string, error) { + claimsVal := &jwt.ValidationResults{} + claims.Validate(claimsVal) + if errs := claimsVal.Errors(); len(errs) > 0 { + return "", fmt.Errorf("account claims validation failed: %v", errs) + } + return claims.Encode(operatorSigningKey) +} + func (a *Manager) Update(ctx context.Context, state *v1alpha1.Account) (*controller.AccountResult, error) { accountRef := domain.NewNamespacedName(state.GetNamespace(), state.GetName()) if err := accountRef.Validate(); err != nil { @@ -368,6 +375,11 @@ func (a *Manager) SignUserJWT(ctx context.Context, accountRef domain.NamespacedN if claims.IssuerAccount == "" { claims.IssuerAccount = accountID } + claimsVal := &jwt.ValidationResults{} + claims.Validate(claimsVal) + if errs := claimsVal.Errors(); len(errs) > 0 { + return nil, fmt.Errorf("claims validation failed during user JWT signing: %v", claimsVal.Errors()) + } accountSecrets, err := a.secretManager.GetSecrets(ctx, accountRef, accountID) if err != nil { return nil, fmt.Errorf("failed to get account secrets for user JWT signing: %w", err) diff --git a/internal/account/account_test.go b/internal/account/account_test.go index 28616b0..e0e53fd 100644 --- a/internal/account/account_test.go +++ b/internal/account/account_test.go @@ -363,6 +363,36 @@ func (t *ManagerTestSuite) Test_Delete_ShouldSucceed() { t.Equal([]interface{}{accountID}, deleteClaims.Data["accounts"]) } +func (t *ManagerTestSuite) Test_signAccountJWT_ShouldFailWhenInvalidClaims() { + // Given + acRoot, _ := nkeys.CreateAccount() + acPub, _ := acRoot.PublicKey() + opSign, _ := nkeys.CreateOperator() + claims := jwt.NewAccountClaims(acPub) + + acOtherRoot, _ := nkeys.CreateAccount() + acOtherPub, _ := acOtherRoot.PublicKey() + claims.Imports.Add(&jwt.Import{ + Name: "import-once", + Type: jwt.Service, + Subject: "foo", + Account: acOtherPub, + }) + claims.Imports.Add(&jwt.Import{ + Name: "import-twice", + Type: jwt.Service, + Subject: "foo", + Account: acOtherPub, + }) + + // When + accountJWT, err := signAccountJWT(claims, opSign) + + // Then + t.Empty(accountJWT) + t.ErrorContains(err, "account claims validation failed: [overlapping subject namespace for \"foo\" and \"foo\"") +} + func (t *ManagerTestSuite) Test_SignUserJWT_ShouldSucceed() { // Given accountRef := domain.NewNamespacedName("account-namespace", "account-name") @@ -461,6 +491,36 @@ func (t *ManagerTestSuite) Test_SignUserJWT_ShouldFailWhenClaimsIssuerAccountDoe accountID+" bound to account \"account-namespace/account-name\" during user JWT signing") } +func (t *ManagerTestSuite) Test_SignUserJWT_ShouldFailWhenClaimsValidationFails() { + // Given + accountRef := domain.NewNamespacedName("account-namespace", "account-name") + accountRootKey, _ := nkeys.CreateAccount() + accountID, _ := accountRootKey.PublicKey() + + account := &v1alpha1.Account{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "account-namespace", + Name: "account-name", + Labels: map[string]string{ + k8s.LabelAccountID: accountID, + }, + }, + } + t.accountReaderMock.mockGet(t.ctx, accountRef, account) + + userKey, _ := nkeys.CreateUser() + userKeyPublic, _ := userKey.PublicKey() + claims := jwt.NewUserClaims(userKeyPublic) + claims.Locale = "funky-BUSINESS" + + // When + result, err := t.unitUnderTest.SignUserJWT(t.ctx, accountRef, claims) + + // Then + t.Nil(result) + t.ErrorContains(err, "claims validation failed during user JWT signing: [could not parse iana time zone by name") +} + /* **************************************************** * Helpers *****************************************************/ diff --git a/internal/account/approvals/claims_test.TestClaims.exports.input.yaml b/internal/account/approvals/claims_test.TestClaims.exports.input.yaml index b9fc8d5..89a5376 100644 --- a/internal/account/approvals/claims_test.TestClaims.exports.input.yaml +++ b/internal/account/approvals/claims_test.TestClaims.exports.input.yaml @@ -26,6 +26,6 @@ exports: serviceLatency: sampling: 25 results: "metrics.latency.results" - accountTokenPosition: 1234 + accountTokenPosition: 2 advertise: true - allowTrace: true \ No newline at end of file + allowTrace: true diff --git a/internal/account/approvals/claims_test.TestClaims.exports.output.nats.approved.json b/internal/account/approvals/claims_test.TestClaims.exports.output.nats.approved.json index 1c17056..de0475a 100644 --- a/internal/account/approvals/claims_test.TestClaims.exports.output.nats.approved.json +++ b/internal/account/approvals/claims_test.TestClaims.exports.output.nats.approved.json @@ -16,7 +16,7 @@ "type": "stream" }, { - "account_token_position": 1234, + "account_token_position": 2, "advertise": true, "allow_trace": true, "name": "my-complex", diff --git a/internal/account/approvals/claims_test.TestClaims.exports.output.nauth.approved.yaml b/internal/account/approvals/claims_test.TestClaims.exports.output.nauth.approved.yaml index 615dc80..1cdac6c 100644 --- a/internal/account/approvals/claims_test.TestClaims.exports.output.nauth.approved.yaml +++ b/internal/account/approvals/claims_test.TestClaims.exports.output.nauth.approved.yaml @@ -9,7 +9,7 @@ exports: - name: my-stream subject: events.> type: stream -- accountTokenPosition: 1234 +- accountTokenPosition: 2 advertise: true allowTrace: true name: my-complex diff --git a/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.input.yaml b/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.input.yaml new file mode 100644 index 0000000..16f0989 --- /dev/null +++ b/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.input.yaml @@ -0,0 +1,13 @@ +imports: + - name: account-a-fc + accountRef: + namespace: my-namespace + name: account-a + subject: $JS.FC.> + type: service + - name: account-b-fc + accountRef: + namespace: my-namespace + name: account-b + subject: $JS.FC.> + type: service diff --git a/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.output.nats.approved.json b/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.output.nats.approved.json new file mode 100644 index 0000000..2664ae0 --- /dev/null +++ b/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.output.nats.approved.json @@ -0,0 +1,51 @@ +{ + "iat": 1700000000, + "iss": "OD32J3IJ3TNSPNAG2MHKB7F77ETVXGUBYMGXQ2ONNJCKPM5RXO7XWTFD", + "jti": "TEST-JWT-ID-STATIC-FOR-APPROVAL-TESTS", + "name": "test-namespace/test-account", + "nats": { + "authorization": {}, + "default_permissions": { + "pub": {}, + "sub": {} + }, + "imports": [ + { + "account": "A000000000000000000000000000000000000ACCOUNTAMYNAMESPACE", + "name": "account-a-fc", + "subject": "$JS.FC.\u003e", + "type": "service" + }, + { + "account": "A000000000000000000000000000000000000ACCOUNTBMYNAMESPACE", + "name": "account-b-fc", + "subject": "$JS.FC.\u003e", + "type": "service" + } + ], + "limits": { + "conn": -1, + "consumer": -1, + "data": -1, + "disk_max_stream_bytes": -1, + "disk_storage": -1, + "exports": -1, + "imports": -1, + "leaf": -1, + "max_ack_pending": -1, + "mem_max_stream_bytes": -1, + "mem_storage": -1, + "payload": -1, + "streams": -1, + "subs": -1, + "wildcards": true + }, + "signing_keys": [ + "ACI73NE4LXWVHSYSFXY73WTZVKIKE54PQUMRDYA4EUFYFGEGHKTPCOI4", + "ADCECGT44IBBMSNGOEZTVK2QUQSVTJW6FABW7JBFFTITDBHMP6TXM4XG" + ], + "type": "account", + "version": 2 + }, + "sub": "AAJCK7774DXTQZAFJLSQIVU76UHGXFZNJVWMT4F7PNRBCYM75LS75UYE" +} diff --git a/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.output.nauth.approved.yaml b/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.output.nauth.approved.yaml new file mode 100644 index 0000000..4e9d395 --- /dev/null +++ b/internal/account/approvals/claims_test.TestClaims.imports-multi-js-fc.output.nauth.approved.yaml @@ -0,0 +1,37 @@ +accountLimits: + conn: -1 + exports: -1 + imports: -1 + leaf: -1 + wildcards: true +displayName: test-namespace/test-account +imports: +- account: A000000000000000000000000000000000000ACCOUNTAMYNAMESPACE + accountRef: + name: "" + namespace: "" + name: account-a-fc + subject: $JS.FC.> + type: service +- account: A000000000000000000000000000000000000ACCOUNTBMYNAMESPACE + accountRef: + name: "" + namespace: "" + name: account-b-fc + subject: $JS.FC.> + type: service +jetStreamLimits: + consumer: -1 + diskMaxStreamBytes: -1 + diskStorage: -1 + maxAckPending: -1 + memMaxStreamBytes: -1 + memStorage: -1 + streams: -1 +natsLimits: + data: -1 + payload: -1 + subs: -1 +signingKeys: +- key: ACI73NE4LXWVHSYSFXY73WTZVKIKE54PQUMRDYA4EUFYFGEGHKTPCOI4 +- key: ADCECGT44IBBMSNGOEZTVK2QUQSVTJW6FABW7JBFFTITDBHMP6TXM4XG diff --git a/internal/account/claims.go b/internal/account/claims.go index 0fd0097..216d033 100644 --- a/internal/account/claims.go +++ b/internal/account/claims.go @@ -186,12 +186,6 @@ func newClaimsBuilder( } } claim.Imports = imports - - err := validateImports(imports) - if err != nil { - errs = append(errs, err) - claim.Imports = nil - } } return &claimsBuilder{ @@ -205,24 +199,6 @@ func (b *claimsBuilder) signingKey(signingKey string) *claimsBuilder { return b } -func validateImports(imports jwt.Imports) error { - seenSubjects := make(map[string]bool, len(imports)) - - for _, importClaim := range imports { - subject := string(importClaim.Subject) - if importClaim.LocalSubject != "" { - subject = string(importClaim.LocalSubject) - } - - if seenSubjects[subject] { - return fmt.Errorf("conflicting import subject found: %s", subject) - } - seenSubjects[subject] = true - } - - return nil -} - func (b *claimsBuilder) build() (*jwt.AccountClaims, error) { if err := errors.Join(b.errs...); err != nil { return nil, err diff --git a/internal/account/claims_test.go b/internal/account/claims_test.go index f344d95..a170107 100644 --- a/internal/account/claims_test.go +++ b/internal/account/claims_test.go @@ -68,7 +68,7 @@ func TestClaims(t *testing.T) { require.NoError(t, err) require.NotNil(t, natsClaims) // Ensure that the NATS JWT can be encoded - natsJwt, err := natsClaims.Encode(opSigningKey) + natsJwt, err := signAccountJWT(natsClaims, opSigningKey) require.NoError(t, err) require.NotEmpty(t, natsJwt)