Fix null pointer crash and incorrect error statuses (#526)

The gRPC and legacy servers did not check if the public
key was provided before accesses it, causing a null pointer exception if
no public key was provided in the request.

The errors from the gRPC server to the legacy server were being wrapped,
and the original error status was being lost.

Signed-off-by: Hayden Blauzvern <[email protected]>

fulcio_legacy.proto

@@ -17,6 +17,7 @@ syntax = "proto3";
 package dev.sigstore.fulcio.v1beta;
 
 import "google/api/annotations.proto";
+import "google/api/field_behavior.proto";
 import "google/api/httpbody.proto";
 import "google/protobuf/empty.proto";
 
@@ -55,11 +56,17 @@ message CreateSigningCertificateRequest {
     /*
      * The public key to be stored in the requested certificate
      */
-    PublicKey publicKey       = 1 [ deprecated=true ];
+    PublicKey publicKey       = 1 [
+        deprecated=true,
+        (google.api.field_behavior) = REQUIRED
+    ];
     /*
      * Proof that the client possesses the private key
      */
-    bytes signedEmailAddress  = 2 [ deprecated=true ];
+    bytes signedEmailAddress  = 2 [
+        deprecated=true,
+        (google.api.field_behavior) = REQUIRED
+    ];
 }
 
 message PublicKey {
@@ -70,5 +77,8 @@ message PublicKey {
     /*
      * DER or PEM encoded public key
      */
-    bytes content    = 2 [ deprecated=true ];
+    bytes content    = 2 [
+        deprecated=true,
+        (google.api.field_behavior) = REQUIRED
+    ];
 }

pkg/api/grpc_server.go

@@ -20,6 +20,7 @@ import (
 	"crypto/x509"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -75,6 +76,10 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu
 		return nil, handleFulcioGRPCError(ctx, codes.Unauthenticated, err, invalidCredentials)
 	}
 
+	if request.PublicKey == nil {
+		return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, errors.New("public key not provided"), invalidPublicKey)
+	}
+
 	publicKeyBytes := request.PublicKey.Content
 	// try to unmarshal as PEM
 	publicKey, err := cryptoutils.UnmarshalPEMToPublicKey([]byte(publicKeyBytes))

pkg/api/grpc_server_test.go

@@ -47,7 +47,9 @@ import (
 	"github.com/sigstore/fulcio/pkg/generated/protobuf"
 	"github.com/sigstore/sigstore/pkg/cryptoutils"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/status"
 	"google.golang.org/grpc/test/bufconn"
 	"gopkg.in/square/go-jose.v2"
 	"gopkg.in/square/go-jose.v2/jwt"
@@ -117,6 +119,9 @@ func TestMissingGetTrustBundleFails(t *testing.T) {
 	if err.Error() != expectedNoRootMessage {
 		t.Errorf("got an unexpected error: %q wanted: %q", err, expectedNoRootMessage)
 	}
+	if status.Code(err) != codes.Internal {
+		t.Fatalf("expected invalid argument, got %v", status.Code(err))
+	}
 }
 
 func TestGetTrustBundleSuccess(t *testing.T) {
@@ -649,6 +654,66 @@ func TestAPIWithInsecurePublicKey(t *testing.T) {
 	if err == nil || !strings.Contains(err.Error(), "The public key supplied in the request is insecure") {
 		t.Fatalf("expected insecure public key error, got %v", err)
 	}
+	if status.Code(err) != codes.InvalidArgument {
+		t.Fatalf("expected invalid argument, got %v", status.Code(err))
+	}
+}
+
+// Tests API with no public key
+func TestAPIWithoutPublicKey(t *testing.T) {
+	emailSigner, emailIssuer := newOIDCIssuer(t)
+
+	// Create a FulcioConfig that supports these issuers.
+	cfg, err := config.Read([]byte(fmt.Sprintf(`{
+		"OIDCIssuers": {
+			%q: {
+				"IssuerURL": %q,
+				"ClientID": "sigstore",
+				"Type": "email"
+			}
+		}
+	}`, emailIssuer, emailIssuer)))
+	if err != nil {
+		t.Fatalf("config.Read() = %v", err)
+	}
+
+	emailSubject := "[email protected]"
+
+	// Create an OIDC token using this issuer's signer.
+	tok, err := jwt.Signed(emailSigner).Claims(jwt.Claims{
+		Issuer:   emailIssuer,
+		IssuedAt: jwt.NewNumericDate(time.Now()),
+		Expiry:   jwt.NewNumericDate(time.Now().Add(30 * time.Minute)),
+		Subject:  emailSubject,
+		Audience: jwt.Audience{"sigstore"},
+	}).Claims(customClaims{Email: emailSubject, EmailVerified: true}).CompactSerialize()
+	if err != nil {
+		t.Fatalf("CompactSerialize() = %v", err)
+	}
+
+	ctClient, eca := createCA(cfg, t)
+	ctx := context.Background()
+	server, conn := setupGRPCForTest(ctx, t, cfg, ctClient, eca)
+	defer func() {
+		server.Stop()
+		conn.Close()
+	}()
+
+	client := protobuf.NewCAClient(conn)
+
+	_, err = client.CreateSigningCertificate(ctx, &protobuf.CreateSigningCertificateRequest{
+		Credentials: &protobuf.Credentials{
+			Credentials: &protobuf.Credentials_OidcIdentityToken{
+				OidcIdentityToken: tok,
+			},
+		},
+	})
+	if err == nil || !strings.Contains(err.Error(), "The public key supplied in the request could not be parsed") {
+		t.Fatalf("expected parsing public key error, got %v", err)
+	}
+	if status.Code(err) != codes.InvalidArgument {
+		t.Fatalf("expected invalid argument, got %v", status.Code(err))
+	}
 }
 
 // Stand up a very simple OIDC endpoint.

pkg/api/legacy_server.go

@@ -18,14 +18,15 @@ package api
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"strings"
 
 	empty "github.com/golang/protobuf/ptypes/empty"
-	"github.com/pkg/errors"
 	fulciogrpc "github.com/sigstore/fulcio/pkg/generated/protobuf"
 	"github.com/sigstore/fulcio/pkg/generated/protobuf/legacy"
 	"google.golang.org/genproto/googleapis/api/httpbody"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 )
 
@@ -62,6 +63,10 @@ func (l *legacyGRPCCAServer) CreateSigningCertificate(ctx context.Context, reque
 		},
 	}
 
+	if request.PublicKey == nil {
+		return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, errors.New("public key not provided"), invalidPublicKey)
+	}
+
 	// create new CA request mapping fields from legacy to actual
 	algorithmEnum, ok := fulciogrpc.PublicKeyAlgorithm_value[strings.ToUpper(request.PublicKey.Algorithm)] //lint:ignore SA1019 this is valid because we're converting from v1beta to v1 API
 	if !ok {
@@ -79,12 +84,12 @@ func (l *legacyGRPCCAServer) CreateSigningCertificate(ctx context.Context, reque
 
 	v2Response, err := l.v2Server.CreateSigningCertificate(ctx, &v2Request)
 	if err != nil {
-		return nil, errors.Wrap(err, "legacy handler")
+		return nil, err
 	}
 
 	// we need to return a HTTP 201 Created response code to be backward compliant
 	if err = grpc.SetHeader(ctx, metadata.Pairs(HTTPResponseCodeMetadataKey, "201")); err != nil {
-		return nil, errors.Wrap(err, "legacy handler")
+		return nil, err
 	}
 
 	detachedResponse := v2Response.GetSignedCertificateDetachedSct()
@@ -93,7 +98,7 @@ func (l *legacyGRPCCAServer) CreateSigningCertificate(ctx context.Context, reque
 		sctString := base64.StdEncoding.EncodeToString(detachedResponse.SignedCertificateTimestamp)
 		if sctString != "" {
 			if err := grpc.SetHeader(ctx, metadata.Pairs(SCTMetadataKey, sctString)); err != nil {
-				return nil, errors.Wrap(err, "legacy handler")
+				return nil, err
 			}
 		}
 	}
@@ -119,7 +124,7 @@ func (l *legacyGRPCCAServer) CreateSigningCertificate(ctx context.Context, reque
 func (l *legacyGRPCCAServer) GetRootCertificate(ctx context.Context, _ *empty.Empty) (*httpbody.HttpBody, error) {
 	v2Response, err := l.v2Server.GetTrustBundle(ctx, &fulciogrpc.GetTrustBundleRequest{})
 	if err != nil {
-		return nil, errors.Wrap(err, "legacy handler")
+		return nil, err
 	}
 
 	var concatCerts strings.Builder