Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configurable client signing algorithms #1938

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions cmd/app/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"sync"
"syscall"

"github.com/sigstore/sigstore/pkg/signature"

"github.com/fsnotify/fsnotify"
"github.com/goadesign/goa/grpc/middleware"
ctclient "github.com/google/certificate-transparency-go/client"
Expand Down Expand Up @@ -155,7 +157,7 @@ func (c *cachedTLSCert) GRPCCreds() grpc.ServerOption {
}))
}

func createGRPCServer(cfg *config.FulcioConfig, ctClient *ctclient.LogClient, baseca ca.CertificateAuthority, ip identity.IssuerPool) (*grpcServer, error) {
func createGRPCServer(cfg *config.FulcioConfig, ctClient *ctclient.LogClient, baseca ca.CertificateAuthority, algorithmRegistry *signature.AlgorithmRegistryConfig, ip identity.IssuerPool) (*grpcServer, error) {
logger, opts := log.SetupGRPCLogging()

serverOpts := []grpc.ServerOption{
Expand Down Expand Up @@ -186,7 +188,7 @@ func createGRPCServer(cfg *config.FulcioConfig, ctClient *ctclient.LogClient, ba

myServer := grpc.NewServer(serverOpts...)

grpcCAServer := server.NewGRPCCAServer(ctClient, baseca, ip)
grpcCAServer := server.NewGRPCCAServer(ctClient, baseca, algorithmRegistry, ip)

health.RegisterHealthServer(myServer, grpcCAServer)
// Register your gRPC service implementations.
Expand Down
14 changes: 12 additions & 2 deletions cmd/app/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (

"github.com/sigstore/fulcio/pkg/ca"
"github.com/sigstore/fulcio/pkg/identity"
v1 "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1"
"github.com/sigstore/sigstore/pkg/signature"
"github.com/spf13/viper"

"google.golang.org/grpc"
Expand All @@ -47,7 +49,11 @@ func setupHTTPServer(t *testing.T) (httpServer, string) {

viper.Set("grpc-host", "")
viper.Set("grpc-port", 0)
grpcServer, err := createGRPCServer(nil, nil, &TrivialCertificateAuthority{}, nil)
algorithmRegistry, err := signature.NewAlgorithmRegistryConfig([]v1.PublicKeyDetails{})
if err != nil {
t.Error(err)
}
grpcServer, err := createGRPCServer(nil, nil, &TrivialCertificateAuthority{}, algorithmRegistry, nil)
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -93,7 +99,11 @@ func setupHTTPServerWithGRPCTLS(t *testing.T) (httpServer, string) {

viper.Set("grpc-host", "")
viper.Set("grpc-port", 0)
grpcServer, err := createGRPCServer(nil, nil, &TrivialCertificateAuthority{}, nil)
algorithmRegistry, err := signature.NewAlgorithmRegistryConfig([]v1.PublicKeyDetails{})
if err != nil {
t.Error(err)
}
grpcServer, err := createGRPCServer(nil, nil, &TrivialCertificateAuthority{}, algorithmRegistry, nil)
if err != nil {
t.Error(err)
}
Expand Down
46 changes: 42 additions & 4 deletions cmd/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ import (
"syscall"
"time"

v1 "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1"
"github.com/sigstore/sigstore/pkg/signature"

"chainguard.dev/go-grpc-kit/pkg/duplex"
"github.com/goadesign/goa/grpc/middleware"
ctclient "github.com/google/certificate-transparency-go/client"
Expand Down Expand Up @@ -113,6 +116,15 @@ func newServeCmd() *cobra.Command {
cmd.Flags().String("grpc-tls-key", "", "the private key file to use for secure connections (without passphrase) - only applies to grpc-port")
cmd.Flags().Duration("idle-connection-timeout", 30*time.Second, "The time allowed for connections (HTTP or gRPC) to go idle before being closed by the server")
cmd.Flags().String("ct-log.tls-ca-cert", "", "Path to TLS CA certificate used to connect to ct-log")
cmd.Flags().StringSlice("client-signing-algorithms", buildDefaultClientSigningAlgorithms([]v1.PublicKeyDetails{
v1.PublicKeyDetails_PKIX_ECDSA_P256_SHA_256,
ret2libc marked this conversation as resolved.
Show resolved Hide resolved
v1.PublicKeyDetails_PKIX_ECDSA_P384_SHA_384,
v1.PublicKeyDetails_PKIX_ECDSA_P521_SHA_512,
v1.PublicKeyDetails_PKIX_RSA_PKCS1V15_2048_SHA256,
v1.PublicKeyDetails_PKIX_RSA_PKCS1V15_3072_SHA256,
v1.PublicKeyDetails_PKIX_RSA_PKCS1V15_4096_SHA256,
v1.PublicKeyDetails_PKIX_ED25519,
}), "the list of allowed client signing algorithms")

// convert "http-host" flag to "host" and "http-port" flag to be "port"
cmd.Flags().SetNormalizeFunc(func(_ *pflag.FlagSet, name string) pflag.NormalizedName {
Expand Down Expand Up @@ -211,6 +223,20 @@ func runServeCmd(cmd *cobra.Command, args []string) { //nolint: revive
// Setup the logger to dev/prod
log.ConfigureLogger(viper.GetString("log_type"))

algorithmStrings := viper.GetStringSlice("client-signing-algorithms")
var algorithmConfig []v1.PublicKeyDetails
for _, s := range algorithmStrings {
algorithmValue, err := signature.ParseSignatureAlgorithmFlag(s)
if err != nil {
log.Logger.Fatal(err)
}
algorithmConfig = append(algorithmConfig, algorithmValue)
}
algorithmRegistry, err := signature.NewAlgorithmRegistryConfig(algorithmConfig)
if err != nil {
log.Logger.Fatalf("error loading --client-signing-algorithms=%s: %v", algorithmConfig, err)
}

// from https://github.com/golang/glog/commit/fca8c8854093a154ff1eb580aae10276ad6b1b5f
_ = flag.CommandLine.Parse([]string{})

Expand Down Expand Up @@ -324,7 +350,7 @@ func runServeCmd(cmd *cobra.Command, args []string) { //nolint: revive
port := viper.GetInt("port")
metricsPort := viper.GetInt("metrics-port")
// StartDuplexServer will always return an error, log fatally if it's non-nil
if err := StartDuplexServer(ctx, cfg, ctClient, baseca, viper.GetString("host"), port, metricsPort, ip); err != http.ErrServerClosed {
if err := StartDuplexServer(ctx, cfg, ctClient, baseca, algorithmRegistry, viper.GetString("host"), port, metricsPort, ip); err != http.ErrServerClosed {
log.Logger.Fatal(err)
}
return
Expand All @@ -337,7 +363,7 @@ func runServeCmd(cmd *cobra.Command, args []string) { //nolint: revive

reg := prometheus.NewRegistry()

grpcServer, err := createGRPCServer(cfg, ctClient, baseca, ip)
grpcServer, err := createGRPCServer(cfg, ctClient, baseca, algorithmRegistry, ip)
if err != nil {
log.Logger.Fatal(err)
}
Expand Down Expand Up @@ -415,7 +441,7 @@ func checkServeCmdConfigFile() error {
return nil
}

func StartDuplexServer(ctx context.Context, cfg *config.FulcioConfig, ctClient *ctclient.LogClient, baseca certauth.CertificateAuthority, host string, port, metricsPort int, ip identity.IssuerPool) error {
func StartDuplexServer(ctx context.Context, cfg *config.FulcioConfig, ctClient *ctclient.LogClient, baseca certauth.CertificateAuthority, algorithmRegistry *signature.AlgorithmRegistryConfig, host string, port, metricsPort int, ip identity.IssuerPool) error {
logger, opts := log.SetupGRPCLogging()

d := duplex.New(
Expand All @@ -437,7 +463,7 @@ func StartDuplexServer(ctx context.Context, cfg *config.FulcioConfig, ctClient *
)

// GRPC server
grpcCAServer := server.NewGRPCCAServer(ctClient, baseca, ip)
grpcCAServer := server.NewGRPCCAServer(ctClient, baseca, algorithmRegistry, ip)
protobuf.RegisterCAServer(d.Server, grpcCAServer)
if err := d.RegisterHandler(ctx, protobuf.RegisterCAHandlerFromEndpoint); err != nil {
return fmt.Errorf("registering grpc ca handler: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we either add a comment that the legacy path (`NewLegacyGRPCCAServer) will not be updated to support this feature, or if it's not much work, can we add it? I'm OK with either approach, with a slight preference for uniformity between the two if it's not much work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but I see that the legacy grpc server creates a v2 request from the legacy request (https://github.com/sigstore/fulcio/blob/main/pkg/server/legacy_server.go#L100) and then the new logic takes place anyway. So this is automatically supported on the legacy grpc server as well.

Are you referring to any edge case I'm not considering?

Expand Down Expand Up @@ -470,3 +496,15 @@ func StartDuplexServer(ctx context.Context, cfg *config.FulcioConfig, ctClient *
}
return nil
}

func buildDefaultClientSigningAlgorithms(allowedAlgorithms []v1.PublicKeyDetails) []string {
var algorithmStrings []string
for _, algorithm := range allowedAlgorithms {
algorithmString, err := signature.FormatSignatureAlgorithmFlag(algorithm)
if err != nil {
log.Logger.Fatal(err)
}
algorithmStrings = append(algorithmStrings, algorithmString)
}
return algorithmStrings
}
8 changes: 7 additions & 1 deletion cmd/app/serve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import (
"github.com/sigstore/fulcio/pkg/ca/ephemeralca"
"github.com/sigstore/fulcio/pkg/config"
"github.com/sigstore/fulcio/pkg/generated/protobuf"
v1 "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1"
"github.com/sigstore/sigstore/pkg/signature"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
)
Expand All @@ -48,9 +50,13 @@ func TestDuplex(t *testing.T) {
t.Fatal(err)
}
metricsPort := 2114
algorithmRegistry, err := signature.NewAlgorithmRegistryConfig([]v1.PublicKeyDetails{})
if err != nil {
t.Error(err)
}

go func() {
if err := StartDuplexServer(ctx, config.DefaultConfig, nil, ca, "localhost", port, metricsPort, nil); err != nil {
if err := StartDuplexServer(ctx, config.DefaultConfig, nil, ca, algorithmRegistry, "localhost", port, metricsPort, nil); err != nil {
log.Fatalf("error starting duplex server: %v", err)
}
}()
Expand Down
5 changes: 3 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ require (
github.com/prometheus/client_model v0.6.1
github.com/prometheus/common v0.62.0
github.com/rs/cors v1.11.1
github.com/sigstore/sigstore v1.8.12
github.com/sigstore/protobuf-specs v0.4.0
github.com/sigstore/sigstore v1.8.13-0.20250204232249-4b62818325b7
github.com/sigstore/sigstore/pkg/signature/kms/aws v1.8.12
github.com/sigstore/sigstore/pkg/signature/kms/azure v1.8.12
github.com/sigstore/sigstore/pkg/signature/kms/gcp v1.8.12
Expand All @@ -44,7 +45,7 @@ require (
google.golang.org/api v0.218.0
google.golang.org/genproto/googleapis/api v0.0.0-20241219192143-6b3ec007d9bb
google.golang.org/grpc v1.70.0
google.golang.org/protobuf v1.36.3
google.golang.org/protobuf v1.36.4
gopkg.in/yaml.v3 v3.0.1
sigs.k8s.io/release-utils v0.9.0
)
Expand Down
10 changes: 6 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,10 @@ github.com/segmentio/ksuid v1.0.4 h1:sBo2BdShXjmcugAMwjugoGUdUV0pcxY5mW4xKRn3v4c
github.com/segmentio/ksuid v1.0.4/go.mod h1:/XUiZBD3kVx5SmUOl55voK5yeAbBNNIed+2O73XgrPE=
github.com/shopspring/decimal v1.4.0 h1:bxl37RwXBklmTi0C79JfXCEBD1cqqHt0bbgBAGFp81k=
github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME=
github.com/sigstore/sigstore v1.8.12 h1:S8xMVZbE2z9ZBuQUEG737pxdLjnbOIcFi5v9UFfkJFc=
github.com/sigstore/sigstore v1.8.12/go.mod h1:+PYQAa8rfw0QdPpBcT+Gl3egKD9c+TUgAlF12H3Nmjo=
github.com/sigstore/protobuf-specs v0.4.0 h1:yoZbdh0kZYKOSiVbYyA8J3f2wLh5aUk2SQB7LgAfIdU=
github.com/sigstore/protobuf-specs v0.4.0/go.mod h1:FKW5NYhnnFQ/Vb9RKtQk91iYd0MKJ9AxyqInEwU6+OI=
github.com/sigstore/sigstore v1.8.13-0.20250204232249-4b62818325b7 h1:dKgngAj90pDWGKB/yBt/AmSvNFzLOPvYGfEgEKRQWc4=
github.com/sigstore/sigstore v1.8.13-0.20250204232249-4b62818325b7/go.mod h1:2lXojNsjZjkqu1//FWxq7qUcPB8Lq1KsR5hc+GkcC/4=
github.com/sigstore/sigstore/pkg/signature/kms/aws v1.8.12 h1:EC3UmIaa7nV9sCgSpVevmvgvTYTkMqyrRbj5ojPp7tE=
github.com/sigstore/sigstore/pkg/signature/kms/aws v1.8.12/go.mod h1:aw60vs3crnQdM/DYH+yF2P0MVKtItwAX34nuaMrY7Lk=
github.com/sigstore/sigstore/pkg/signature/kms/azure v1.8.12 h1:FPpliDTywSy0woLHMAdmTSZ5IS/lVBZ0dY0I+2HmnSY=
Expand Down Expand Up @@ -528,8 +530,8 @@ google.golang.org/grpc v1.27.0/go.mod h1:qbnxyOmOxrQa7FizSgH+ReBfzJrCY1pSN7KXBS8
google.golang.org/grpc v1.29.1/go.mod h1:itym6AZVZYACWQqET3MqgPpjcuV5QH3BxFS3IjizoKk=
google.golang.org/grpc v1.70.0 h1:pWFv03aZoHzlRKHWicjsZytKAiYCtNS0dHbXnIdq7jQ=
google.golang.org/grpc v1.70.0/go.mod h1:ofIJqVKDXx/JiXrwr2IG4/zwdH9txy3IlF40RmcJSQw=
google.golang.org/protobuf v1.36.3 h1:82DV7MYdb8anAVi3qge1wSnMDrnKK7ebr+I0hHRN1BU=
google.golang.org/protobuf v1.36.3/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE=
google.golang.org/protobuf v1.36.4 h1:6A3ZDJHn/eNqc1i+IdefRzy/9PokBTPvcqMySR7NNIM=
google.golang.org/protobuf v1.36.4/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
Expand Down
60 changes: 54 additions & 6 deletions pkg/server/grpc_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ package server
import (
"context"
"crypto"
"crypto/ed25519"
"crypto/x509"
"encoding/json"
"errors"
"fmt"

"github.com/sigstore/sigstore/pkg/signature"

ctclient "github.com/google/certificate-transparency-go/client"
health "google.golang.org/grpc/health/grpc_health_v1"

Expand All @@ -44,11 +48,12 @@ type GRPCCAServer interface {
health.HealthServer
}

func NewGRPCCAServer(ct *ctclient.LogClient, ca certauth.CertificateAuthority, ip identity.IssuerPool) GRPCCAServer {
func NewGRPCCAServer(ct *ctclient.LogClient, ca certauth.CertificateAuthority, algorithmRegistry *signature.AlgorithmRegistryConfig, ip identity.IssuerPool) GRPCCAServer {
return &grpcaCAServer{
ct: ct,
ca: ca,
IssuerPool: ip,
ct: ct,
ca: ca,
algorithmRegistry: algorithmRegistry,
IssuerPool: ip,
}
}

Expand All @@ -58,8 +63,9 @@ const (

type grpcaCAServer struct {
fulciogrpc.UnimplementedCAServer
ct *ctclient.LogClient
ca certauth.CertificateAuthority
ct *ctclient.LogClient
ca certauth.CertificateAuthority
algorithmRegistry *signature.AlgorithmRegistryConfig
identity.IssuerPool
}

Expand Down Expand Up @@ -88,6 +94,7 @@ func (g *grpcaCAServer) CreateSigningCertificate(ctx context.Context, request *f
}

var publicKey crypto.PublicKey
var hashFunc crypto.Hash
// Verify caller is in possession of their private key and extract
// public key from request.
if len(request.GetCertificateSigningRequest()) > 0 {
Expand All @@ -106,6 +113,11 @@ func (g *grpcaCAServer) CreateSigningCertificate(ctx context.Context, request *f
if err := csr.CheckSignature(); err != nil {
return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
}

hashFunc, err = getHashFuncForSignatureAlgorithm(csr.SignatureAlgorithm)
if err != nil {
return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, err.Error())
}
} else {
// Option 2: Check the signature for proof of possession of a private key
var (
Expand Down Expand Up @@ -133,6 +145,22 @@ func (g *grpcaCAServer) CreateSigningCertificate(ctx context.Context, request *f
if err := challenges.CheckSignature(publicKey, proofOfPossession, principal.Name(ctx)); err != nil {
return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
}

// The proof of possession signature always uses SHA-256, unless the key algorithm is ED25519
hashFunc = crypto.SHA256
if _, ok := publicKey.(ed25519.PublicKey); ok {
hashFunc = crypto.Hash(0)
}
Comment on lines +149 to +153
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing the Cosign changes and I've realized this is not good in general.

Something like this seems to work:

		switch pk := publicKey.(type) {
		case ed25519.PublicKey:
			// Fulcio only works with PureEd25519
			hashFunc = crypto.Hash(0)
		case *ecdsa.PublicKey:
			switch pk.Curve {
			case elliptic.P256():
				hashFunc = crypto.SHA256
			case elliptic.P384():
				hashFunc = crypto.SHA384
			case elliptic.P521():
				hashFunc = crypto.SHA512
			default:
				hashFunc = crypto.SHA256
			}
		case *rsa.PublicKey:
			hashFunc = crypto.SHA256
		default:
			hashFunc = crypto.SHA256
		}

		// Check proof of possession signature
		if err := challenges.CheckSignatureWithOpts(publicKey, proofOfPossession, principal.Name(ctx), options.WithHash(hashFunc)); err != nil {
			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
		}

@haydentherapper what do you think about this?

}

// Check whether the public-key/hash algorithm combination is allowed
isPermitted, err := g.algorithmRegistry.IsAlgorithmPermitted(publicKey, hashFunc)
if err != nil {
return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, err.Error())
}
if !isPermitted {
err = fmt.Errorf("Signing algorithm not permitted: %T, %s", publicKey, hashFunc)
return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, err.Error())
}

var csc *certauth.CodeSigningCertificate
Expand Down Expand Up @@ -280,3 +308,23 @@ func (g *grpcaCAServer) Check(_ context.Context, _ *health.HealthCheckRequest) (
func (g *grpcaCAServer) Watch(_ *health.HealthCheckRequest, _ health.Health_WatchServer) error {
return status.Error(codes.Unimplemented, "unimplemented")
}

func getHashFuncForSignatureAlgorithm(signatureAlgorithm x509.SignatureAlgorithm) (crypto.Hash, error) {
switch signatureAlgorithm {
case x509.ECDSAWithSHA256:
return crypto.SHA256, nil
case x509.ECDSAWithSHA384:
return crypto.SHA384, nil
case x509.ECDSAWithSHA512:
return crypto.SHA512, nil
case x509.SHA256WithRSA:
return crypto.SHA256, nil
case x509.SHA384WithRSA:
return crypto.SHA384, nil
case x509.SHA512WithRSA:
return crypto.SHA512, nil
case x509.PureEd25519:
return crypto.Hash(0), nil
}
return crypto.Hash(0), fmt.Errorf("unrecognized signature algorithm: %s", signatureAlgorithm)
}
Loading