From 318a2e72fae810609d7d2c14e8662c417c43af71 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Thu, 30 Jul 2026 00:09:10 +0200 Subject: [PATCH] ateapi: fail cleanly on a malformed session JWT signing pool MintJWT reads the signing pool file on every call and indexes Authorities[0] directly. localjwtauthority.Unmarshal reports no error for JSON that carries no authorities, so an empty or incompletely written pool file reaches that index and panics. sessionidjwt.Sign then type-asserts the signing key without checking, so a pool entry whose algorithm does not match its key type panics as well. Neither is recoverable: nothing in the server installs a panic-recovery interceptor, so a configuration error in the pool file takes down ate-api-server on the first MintJWT call rather than returning an error to the caller. Check the pool for authorities before selecting one, and check each key type assertion in Sign. MintCert already guards an empty session CA pool the same way; this brings the JWT path in line with it. The signing-authority selection moves into a helper so it can be tested without standing up an OIDC issuer to satisfy the client-JWT verification that precedes it. Both packages had no tests; the ones added here also pin the existing behavior of the working paths. --- .../sessionidentity/sessionidentity.go | 23 +++- .../sessionidentity/sessionidentity_test.go | 68 +++++++++++ .../internal/sessionidjwt/sessionidjwt.go | 20 ++- .../sessionidjwt/sessionidjwt_test.go | 115 ++++++++++++++++++ 4 files changed, 220 insertions(+), 6 deletions(-) create mode 100644 cmd/ateapi/internal/sessionidentity/sessionidentity_test.go create mode 100644 cmd/ateapi/internal/sessionidjwt/sessionidjwt_test.go diff --git a/cmd/ateapi/internal/sessionidentity/sessionidentity.go b/cmd/ateapi/internal/sessionidentity/sessionidentity.go index a9a581271..f43fa2a54 100644 --- a/cmd/ateapi/internal/sessionidentity/sessionidentity.go +++ b/cmd/ateapi/internal/sessionidentity/sessionidentity.go @@ -130,8 +130,13 @@ func (s *Server) MintJWT(ctx context.Context, req *ateapipb.MintJWTRequest) (*at return nil, fmt.Errorf("while making session JWT claims: %w", err) } - // Assume the first authority is the one to use for signing. - sessionJWT, err := sessionidjwt.Sign(sessionWireClaims, signingPool.Authorities[0].SigningKey, signingPool.Authorities[0].Algorithm, signingPool.Authorities[0].ID) + authority, err := signingAuthority(signingPool) + if err != nil { + slog.ErrorContext(ctx, "Failed to select a session JWT signing authority", slog.Any("err", err)) + return nil, status.Errorf(codes.Internal, "Failed to load session signing key") + } + + sessionJWT, err := sessionidjwt.Sign(sessionWireClaims, authority.SigningKey, authority.Algorithm, authority.ID) if err != nil { return nil, fmt.Errorf("while signing session JWT: %w", err) } @@ -141,6 +146,20 @@ func (s *Server) MintJWT(ctx context.Context, req *ateapipb.MintJWTRequest) (*at }, nil } +// signingAuthority picks the authority to sign a session JWT with. +// +// localjwtauthority.Unmarshal reports no error for JSON that carries no +// authorities, so an empty or incompletely written signing pool file arrives +// here as an empty pool. Report that instead of indexing into it, the way +// MintCert already handles an empty session CA pool. +func signingAuthority(pool *localjwtauthority.Pool) (*localjwtauthority.Authority, error) { + if pool == nil || len(pool.Authorities) == 0 { + return nil, fmt.Errorf("signing pool contains no authorities") + } + // Assume the first authority is the one to use for signing. + return pool.Authorities[0], nil +} + func (s *Server) MintCert(ctx context.Context, req *ateapipb.MintCertRequest) (*ateapipb.MintCertResponse, error) { p, ok := peer.FromContext(ctx) if !ok { diff --git a/cmd/ateapi/internal/sessionidentity/sessionidentity_test.go b/cmd/ateapi/internal/sessionidentity/sessionidentity_test.go new file mode 100644 index 000000000..fd48ad8e7 --- /dev/null +++ b/cmd/ateapi/internal/sessionidentity/sessionidentity_test.go @@ -0,0 +1,68 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sessionidentity + +import ( + "testing" + + "github.com/agent-substrate/substrate/internal/localjwtauthority" +) + +// TestSigningAuthorityEmptyPool covers the pool shapes that carry no signing +// authority. localjwtauthority.Unmarshal accepts JSON with no authorities +// without reporting an error, so MintJWT reaches this selection with an empty +// pool whenever the signing pool file is empty or was written incompletely. +// Selecting from it must report an error rather than panic and take the whole +// ate-api-server down, matching how MintCert treats an empty session CA pool. +func TestSigningAuthorityEmptyPool(t *testing.T) { + tests := []struct { + name string + pool *localjwtauthority.Pool + }{ + {"nil pool", nil}, + {"no authorities", &localjwtauthority.Pool{}}, + {"empty authority slice", &localjwtauthority.Pool{Authorities: []*localjwtauthority.Authority{}}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := signingAuthority(tc.pool) + if err == nil { + t.Fatalf("signingAuthority() error = nil, want an error; authority = %+v", got) + } + if got != nil { + t.Errorf("signingAuthority() = %+v alongside an error, want nil", got) + } + }) + } +} + +// TestSigningAuthorityPicksFirst pins the selection rule the signer relies on. +func TestSigningAuthorityPicksFirst(t *testing.T) { + first := &localjwtauthority.Authority{ID: "kid-first", Algorithm: "ES256"} + pool := &localjwtauthority.Pool{ + Authorities: []*localjwtauthority.Authority{ + first, + {ID: "kid-second", Algorithm: "RS256"}, + }, + } + + got, err := signingAuthority(pool) + if err != nil { + t.Fatalf("signingAuthority() error = %v, want nil", err) + } + if got != first { + t.Errorf("signingAuthority() = %+v, want the first authority %+v", got, first) + } +} diff --git a/cmd/ateapi/internal/sessionidjwt/sessionidjwt.go b/cmd/ateapi/internal/sessionidjwt/sessionidjwt.go index ebd52fb91..4bb19000f 100644 --- a/cmd/ateapi/internal/sessionidjwt/sessionidjwt.go +++ b/cmd/ateapi/internal/sessionidjwt/sessionidjwt.go @@ -120,21 +120,30 @@ func Sign(wireClaims *WireClaims, signingKey crypto.PrivateKey, algorithm, keyID var sigBytes []byte switch algorithm { case "RS256": - rsaKey := signingKey.(*rsa.PrivateKey) + rsaKey, ok := signingKey.(*rsa.PrivateKey) + if !ok { + return "", fmt.Errorf("algorithm %s requires an RSA key, got %T", algorithm, signingKey) + } toBeSignedDigest := hashBytes(crypto.SHA256.New(), []byte(toBeSigned)) sigBytes, err = rsa.SignPKCS1v15(rand.Reader, rsaKey, crypto.SHA256, toBeSignedDigest) if err != nil { return "", fmt.Errorf("while performing RSA PKCS1v15 signature: %w", err) } case "RS384": - rsaKey := signingKey.(*rsa.PrivateKey) + rsaKey, ok := signingKey.(*rsa.PrivateKey) + if !ok { + return "", fmt.Errorf("algorithm %s requires an RSA key, got %T", algorithm, signingKey) + } toBeSignedDigest := hashBytes(crypto.SHA384.New(), []byte(toBeSigned)) sigBytes, err = rsa.SignPKCS1v15(rand.Reader, rsaKey, crypto.SHA384, toBeSignedDigest) if err != nil { return "", fmt.Errorf("while performing RSA PKCS1v15 signature: %w", err) } case "RS512": - rsaKey := signingKey.(*rsa.PrivateKey) + rsaKey, ok := signingKey.(*rsa.PrivateKey) + if !ok { + return "", fmt.Errorf("algorithm %s requires an RSA key, got %T", algorithm, signingKey) + } toBeSignedDigest := hashBytes(crypto.SHA512.New(), []byte(toBeSigned)) sigBytes, err = rsa.SignPKCS1v15(rand.Reader, rsaKey, crypto.SHA512, toBeSignedDigest) if err != nil { @@ -142,7 +151,10 @@ func Sign(wireClaims *WireClaims, signingKey crypto.PrivateKey, algorithm, keyID } case "ES256": // JOSE ES256 defined at https://datatracker.ietf.org/doc/rfc7518/ section 3.4 - ecdsaKey := signingKey.(*ecdsa.PrivateKey) + ecdsaKey, ok := signingKey.(*ecdsa.PrivateKey) + if !ok { + return "", fmt.Errorf("algorithm %s requires an ECDSA key, got %T", algorithm, signingKey) + } if ecdsaKey.Curve != elliptic.P256() { return "", fmt.Errorf("ES256 requires a P256 key") } diff --git a/cmd/ateapi/internal/sessionidjwt/sessionidjwt_test.go b/cmd/ateapi/internal/sessionidjwt/sessionidjwt_test.go new file mode 100644 index 000000000..77ebb08dc --- /dev/null +++ b/cmd/ateapi/internal/sessionidjwt/sessionidjwt_test.go @@ -0,0 +1,115 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sessionidjwt + +import ( + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "strings" + "testing" +) + +func testRSAKey(t *testing.T) *rsa.PrivateKey { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generating RSA key: %v", err) + } + return key +} + +func testECKey(t *testing.T, curve elliptic.Curve) *ecdsa.PrivateKey { + t.Helper() + key, err := ecdsa.GenerateKey(curve, rand.Reader) + if err != nil { + t.Fatalf("generating EC key: %v", err) + } + return key +} + +// TestSignKeyAlgorithmMismatch covers every algorithm branch with a key of the +// wrong type. The algorithm and the key come from the same signing pool entry, +// which is operator-supplied, so a mismatched pair is a configuration error and +// must surface as an error rather than taking the process down. +func TestSignKeyAlgorithmMismatch(t *testing.T) { + rsaKey := testRSAKey(t) + ecKey := testECKey(t, elliptic.P256()) + + tests := []struct { + name string + algorithm string + key crypto.PrivateKey + }{ + {"RS256 with EC key", "RS256", ecKey}, + {"RS384 with EC key", "RS384", ecKey}, + {"RS512 with EC key", "RS512", ecKey}, + {"ES256 with RSA key", "ES256", rsaKey}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := Sign(&WireClaims{Issuer: "https://issuer.example"}, tc.key, tc.algorithm, "kid-1") + if err == nil { + t.Fatalf("Sign(%s) error = nil, want an error; token = %q", tc.algorithm, got) + } + if got != "" { + t.Errorf("Sign(%s) returned token %q alongside an error, want empty", tc.algorithm, got) + } + }) + } +} + +// TestSignUnimplementedAlgorithm pins the pre-existing behavior for an algorithm +// the signer does not implement, so the mismatch handling above cannot swallow it. +func TestSignUnimplementedAlgorithm(t *testing.T) { + if _, err := Sign(&WireClaims{}, testRSAKey(t), "HS256", "kid-1"); err == nil { + t.Fatal("Sign(HS256) error = nil, want an error") + } +} + +// TestSignMatchingKey checks the algorithms that do have a matching key still +// produce a three-segment compact JWT. +func TestSignMatchingKey(t *testing.T) { + tests := []struct { + algorithm string + key crypto.PrivateKey + }{ + {"RS256", testRSAKey(t)}, + {"RS384", testRSAKey(t)}, + {"RS512", testRSAKey(t)}, + {"ES256", testECKey(t, elliptic.P256())}, + } + for _, tc := range tests { + t.Run(tc.algorithm, func(t *testing.T) { + token, err := Sign(&WireClaims{Issuer: "https://issuer.example"}, tc.key, tc.algorithm, "kid-1") + if err != nil { + t.Fatalf("Sign(%s) error = %v, want nil", tc.algorithm, err) + } + if n := len(strings.Split(token, ".")); n != 3 { + t.Errorf("Sign(%s) produced %d segments, want 3", tc.algorithm, n) + } + }) + } +} + +// TestSignES256WrongCurve keeps the existing curve check covered: ES256 is +// defined over P-256 only, and a P-384 key must be rejected, not signed. +func TestSignES256WrongCurve(t *testing.T) { + if _, err := Sign(&WireClaims{}, testECKey(t, elliptic.P384()), "ES256", "kid-1"); err == nil { + t.Fatal("Sign(ES256) with a P-384 key error = nil, want an error") + } +}