Fix null pointer crash and incorrect error statuses

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 := "foo@example.com"
+
+	// 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