Skip to content

Commit 6123f01

Browse files
perf(artifact-cas): parse the EC public key once instead of per request
The JWT verification keyfunc re-parsed the public key PEM on every authenticated request across the unary, stream and HTTP download paths. Parse the EC public key once at server construction and share a single keyfunc closure across all interceptors. A malformed key now fails at startup instead of as a per-request auth error. Signed-off-by: Matías Insaurralde <matias@chainloop.dev>
1 parent 175338b commit 6123f01

3 files changed

Lines changed: 101 additions & 31 deletions

File tree

app/artifact-cas/internal/server/grpc.go

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package server
1717

1818
import (
1919
"context"
20+
"crypto/ecdsa"
2021
"crypto/tls"
2122
"fmt"
2223
"os"
@@ -52,22 +53,16 @@ import (
5253

5354
// NewGRPCServer new a gRPC server.
5455
func NewGRPCServer(c *conf.Server, authConf *conf.Auth, byteService *service.ByteStreamService, rSvc *service.ResourceService, providers backend.Providers, validator protovalidate.Validator, logger log.Logger) (*grpc.Server, error) {
55-
log := log.NewHelper(logger)
56-
// Load the key on initialization instead of on every request
56+
// Parse the public key once on initialization instead of on every request
5757
// TODO: implement jwks endpoint
58-
publicKeyPath := authConf.GetPublicKeyPath()
59-
if publicKeyPath == "" {
60-
// Maintain backwards compatibility
61-
publicKeyPath = authConf.RobotAccountPublicKeyPath
62-
}
63-
64-
log.Debugw("msg", "loading public key from file", "file", publicKeyPath)
65-
66-
rawKey, err := os.ReadFile(publicKeyPath)
58+
publicKey, err := parsePublicKey(authConf, logger)
6759
if err != nil {
68-
return nil, fmt.Errorf("failed to load public key: %w", err)
60+
return nil, err
6961
}
7062

63+
// Share a single keyfunc closure over the parsed key across all interceptors
64+
keyFunc := loadPublicKey(publicKey)
65+
7166
var opts = []grpc.ServerOption{
7267
// Kratos middleware are in practice unary interceptors
7368
grpc.Middleware(
@@ -83,7 +78,7 @@ func NewGRPCServer(c *conf.Server, authConf *conf.Auth, byteService *service.Byt
8378
// If we require a logged in user we
8479
selector.Server(
8580
jwtMiddleware.Server(
86-
loadPublicKey(rawKey),
81+
keyFunc,
8782
jwtMiddleware.WithSigningMethod(casJWT.SigningMethod),
8883
jwtMiddleware.WithClaims(func() jwt.Claims { return &casJWT.Claims{} })),
8984
).Match(requireAuthentication()).Build(),
@@ -92,7 +87,7 @@ func NewGRPCServer(c *conf.Server, authConf *conf.Auth, byteService *service.Byt
9287
// Streaming interceptors
9388
grpc.StreamInterceptor(
9489
grpcselector.StreamServerInterceptor(
95-
grpc_auth.StreamServerInterceptor(jwtAuthFunc(loadPublicKey(rawKey), casJWT.SigningMethod)),
90+
grpc_auth.StreamServerInterceptor(jwtAuthFunc(keyFunc, casJWT.SigningMethod)),
9691
grpcselector.MatchFunc(allButReflectionAPI),
9792
),
9893
// grpc prometheus metrics
@@ -163,10 +158,38 @@ func allButReflectionAPI(_ context.Context, callMeta interceptors.CallMeta) bool
163158
return !reflectionServiceRegexp.MatchString(callMeta.Service)
164159
}
165160

166-
// load key for verification
167-
func loadPublicKey(rawKey []byte) jwt.Keyfunc {
168-
return func(token *jwt.Token) (interface{}, error) {
169-
return jwt.ParseECPublicKeyFromPEM(rawKey)
161+
// parsePublicKey resolves the configured public key path, reads the file and parses
162+
// the EC public key once. A malformed key therefore fails at server construction
163+
// instead of surfacing as a per-request authentication error.
164+
func parsePublicKey(authConf *conf.Auth, logger log.Logger) (*ecdsa.PublicKey, error) {
165+
l := log.NewHelper(logger)
166+
167+
publicKeyPath := authConf.GetPublicKeyPath()
168+
if publicKeyPath == "" {
169+
// Maintain backwards compatibility with the deprecated field.
170+
publicKeyPath = authConf.RobotAccountPublicKeyPath //nolint:staticcheck // intentional fallback to the deprecated field
171+
}
172+
173+
l.Debugw("msg", "loading public key from file", "file", publicKeyPath)
174+
175+
rawKey, err := os.ReadFile(publicKeyPath)
176+
if err != nil {
177+
return nil, fmt.Errorf("failed to load public key: %w", err)
178+
}
179+
180+
publicKey, err := jwt.ParseECPublicKeyFromPEM(rawKey)
181+
if err != nil {
182+
return nil, fmt.Errorf("failed to parse public key: %w", err)
183+
}
184+
185+
return publicKey, nil
186+
}
187+
188+
// loadPublicKey returns a jwt.Keyfunc that hands back the pre-parsed public key,
189+
// avoiding a PEM re-parse on every request.
190+
func loadPublicKey(publicKey *ecdsa.PublicKey) jwt.Keyfunc {
191+
return func(_ *jwt.Token) (interface{}, error) {
192+
return publicKey, nil
170193
}
171194
}
172195

app/artifact-cas/internal/server/grpc_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@ import (
2222
"testing"
2323
"time"
2424

25+
"github.com/chainloop-dev/chainloop/app/artifact-cas/internal/conf"
2526
robotaccount "github.com/chainloop-dev/chainloop/internal/robotaccount/cas"
27+
"github.com/go-kratos/kratos/v2/log"
2628
jwtMiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt"
2729
jwt "github.com/golang-jwt/jwt/v5"
2830
"github.com/grpc-ecosystem/go-grpc-middleware/util/metautils"
@@ -181,6 +183,60 @@ func TestAllButReflectionAPI(t *testing.T) {
181183
}
182184
}
183185

186+
func TestLoadPublicKey(t *testing.T) {
187+
rawKey, err := os.ReadFile("./testdata/test-key.ec.pub")
188+
require.NoError(t, err)
189+
want, err := jwt.ParseECPublicKeyFromPEM(rawKey)
190+
require.NoError(t, err)
191+
192+
// The keyfunc must hand back the pre-parsed key without re-parsing the PEM
193+
got, err := loadPublicKey(want)(&jwt.Token{})
194+
require.NoError(t, err)
195+
assert.Same(t, want, got)
196+
}
197+
198+
func TestParsePublicKey(t *testing.T) {
199+
testCases := []struct {
200+
name string
201+
authConf *conf.Auth
202+
wantErr string
203+
}{
204+
{
205+
name: "valid public key via PublicKeyPath",
206+
authConf: &conf.Auth{PublicKeyPath: "./testdata/test-key.ec.pub"},
207+
},
208+
{
209+
name: "valid public key via deprecated RobotAccountPublicKeyPath",
210+
authConf: &conf.Auth{RobotAccountPublicKeyPath: "./testdata/test-key.ec.pub"},
211+
},
212+
{
213+
name: "missing file",
214+
authConf: &conf.Auth{PublicKeyPath: "./testdata/does-not-exist.pub"},
215+
wantErr: "failed to load public key",
216+
},
217+
{
218+
name: "not a public key PEM",
219+
authConf: &conf.Auth{PublicKeyPath: "./testdata/test-key.ec.pem"},
220+
wantErr: "failed to parse public key",
221+
},
222+
}
223+
224+
for _, tc := range testCases {
225+
t.Run(tc.name, func(t *testing.T) {
226+
got, err := parsePublicKey(tc.authConf, log.DefaultLogger)
227+
if tc.wantErr != "" {
228+
require.Error(t, err)
229+
assert.ErrorContains(t, err, tc.wantErr)
230+
assert.Nil(t, got)
231+
return
232+
}
233+
234+
require.NoError(t, err)
235+
assert.NotNil(t, got)
236+
})
237+
}
238+
}
239+
184240
func loadTestPublicKey(path string) jwt.Keyfunc {
185241
rawKey, _ := os.ReadFile(path)
186242
return func(token *jwt.Token) (interface{}, error) {

app/artifact-cas/internal/server/http.go

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@
1616
package server
1717

1818
import (
19-
"fmt"
20-
"os"
21-
2219
api "github.com/chainloop-dev/chainloop/app/artifact-cas/api/cas/v1"
2320
"github.com/chainloop-dev/chainloop/app/artifact-cas/internal/conf"
2421
"github.com/chainloop-dev/chainloop/app/artifact-cas/internal/service"
@@ -50,22 +47,16 @@ func NewHTTPServer(c *conf.Server, authConf *conf.Auth, downloadSvc *service.Dow
5047
opts = append(opts, http.Timeout(c.Http.Timeout.AsDuration()))
5148
}
5249

53-
// Load the key on initialization instead of on every request
50+
// Parse the public key once on initialization instead of on every request
5451
// TODO: implement jwks endpoint
55-
publicKeyPath := authConf.GetPublicKeyPath()
56-
if publicKeyPath == "" {
57-
// Maintain backwards compatibility
58-
publicKeyPath = authConf.RobotAccountPublicKeyPath
59-
}
60-
61-
rawKey, err := os.ReadFile(publicKeyPath)
52+
publicKey, err := parsePublicKey(authConf, logger)
6253
if err != nil {
63-
return nil, fmt.Errorf("failed to load public key: %w", err)
54+
return nil, err
6455
}
6556

6657
srv := http.NewServer(opts...)
6758

68-
downloadHandler := middlewares_http.AuthFromQueryParam(loadPublicKey(rawKey), claimsFunc(), casJWT.SigningMethod, downloadSvc)
59+
downloadHandler := middlewares_http.AuthFromQueryParam(loadPublicKey(publicKey), claimsFunc(), casJWT.SigningMethod, downloadSvc)
6960
srv.Handle(service.DownloadPath, CORSMiddleware(c.GetHttp().GetCors().GetAllowOrigins(), downloadHandler))
7061
api.RegisterStatusServiceHTTPServer(srv, service.NewStatusService(Version, providers))
7162
return srv, nil

0 commit comments

Comments
 (0)