From 77c99959d5a46a02e343fabf93fb5d3b50a07f3f Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Sat, 29 Aug 2026 10:38:10 +0200 Subject: [PATCH] ateapi: fail cleanly on a malformed actor JWT signing pool MintJWT indexes Authorities[0] from the actor JWT signing pool file, and actoridjwt.Sign type-asserts the signing key without checking it, so a pool file carrying no authorities (which localjwtauthority. Unmarshal accepts without error) or an algorithm that does not match its key panics instead of returning an error. Nothing in the server installs a panic-recovery interceptor, so that configuration error takes down ate-api-server on the first MintJWT call, and MintCert already guards the same situation for its own pool. Adds the missing checks plus the first tests for both packages, which also pin the behavior of the paths that already worked. --- .../internal/actoridentity/actoridentity.go | 23 +++- .../actoridentity/actoridentity_test.go | 48 ++++++++ cmd/ateapi/internal/actoridjwt/actoridjwt.go | 20 ++- .../internal/actoridjwt/actoridjwt_test.go | 115 ++++++++++++++++++ 4 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 cmd/ateapi/internal/actoridjwt/actoridjwt_test.go diff --git a/cmd/ateapi/internal/actoridentity/actoridentity.go b/cmd/ateapi/internal/actoridentity/actoridentity.go index 665f351527..649f1c4f02 100644 --- a/cmd/ateapi/internal/actoridentity/actoridentity.go +++ b/cmd/ateapi/internal/actoridentity/actoridentity.go @@ -134,8 +134,13 @@ func (s *Server) MintJWT(ctx context.Context, req *ateapipb.MintJWTRequest) (*at return nil, fmt.Errorf("while making actor JWT claims: %w", err) } - // Assume the first authority is the one to use for signing. - actorJWT, err := actoridjwt.Sign(actorWireClaims, 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 an actor JWT signing authority", slog.Any("err", err)) + return nil, status.Errorf(codes.Internal, "Failed to load actor signing key") + } + + actorJWT, err := actoridjwt.Sign(actorWireClaims, authority.SigningKey, authority.Algorithm, authority.ID) if err != nil { return nil, fmt.Errorf("while signing actor JWT: %w", err) } @@ -145,6 +150,20 @@ func (s *Server) MintJWT(ctx context.Context, req *ateapipb.MintJWTRequest) (*at }, nil } +// signingAuthority picks the authority to sign an actor 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 actor 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) { caller, err := authenticateAtelet(ctx) if err != nil { diff --git a/cmd/ateapi/internal/actoridentity/actoridentity_test.go b/cmd/ateapi/internal/actoridentity/actoridentity_test.go index e236f775b9..de749a7eae 100644 --- a/cmd/ateapi/internal/actoridentity/actoridentity_test.go +++ b/cmd/ateapi/internal/actoridentity/actoridentity_test.go @@ -31,6 +31,7 @@ import ( "github.com/agent-substrate/substrate/cmd/ateapi/internal/store/storetest" "github.com/agent-substrate/substrate/cmd/ateapi/internal/workercache" "github.com/agent-substrate/substrate/internal/localca" + "github.com/agent-substrate/substrate/internal/localjwtauthority" "github.com/agent-substrate/substrate/internal/principal" "github.com/agent-substrate/substrate/internal/resources" "github.com/agent-substrate/substrate/internal/substratex509" @@ -41,6 +42,53 @@ import ( "google.golang.org/grpc/status" ) +// 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 actor 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) + } +} + const ( testAtespace = "team-alpha" testActorName = "counter-1" diff --git a/cmd/ateapi/internal/actoridjwt/actoridjwt.go b/cmd/ateapi/internal/actoridjwt/actoridjwt.go index 9e8b44b31b..65a1c32d05 100644 --- a/cmd/ateapi/internal/actoridjwt/actoridjwt.go +++ b/cmd/ateapi/internal/actoridjwt/actoridjwt.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/actoridjwt/actoridjwt_test.go b/cmd/ateapi/internal/actoridjwt/actoridjwt_test.go new file mode 100644 index 0000000000..21b002767d --- /dev/null +++ b/cmd/ateapi/internal/actoridjwt/actoridjwt_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 actoridjwt + +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") + } +}