Group proof of possession logic by challenge type

There are two challenges a caller can use to prove they possess their
private key:
- Submit a CSR
- Sign the subject or email from their ID token
Previously the logic to verify these two types of challenges was
interweaved. This work splits the verification into two different
branches and groups the logic of each type of verification together.

Signed-off-by: Nathan Smith <nathan@chainguard.dev>

pkg/api/grpc_server.go

@@ -17,7 +17,7 @@ package api
 
 import (
 	"context"
-	"crypto/x509"
+	"crypto"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -83,41 +83,51 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu
 		return nil, handleFulcioGRPCError(ctx, codes.Unauthenticated, err, invalidCredentials)
 	}
 
-	// optionally parse CSR
-	var csr *x509.CertificateRequest
+	var publicKey crypto.PublicKey
+	// Verify caller is in possession of their private key and extract
+	// public key from request.
 	if len(request.GetCertificateSigningRequest()) > 0 {
-		csr, err = cryptoutils.ParseCSR(request.GetCertificateSigningRequest())
+		// Option 1: If they gave us a CSR we can verify that to check
+		csr, err := cryptoutils.ParseCSR(request.GetCertificateSigningRequest())
 		if err != nil {
 			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidCSR)
 		}
-	}
 
-	// fetch public key from request or CSR
-	var pubKeyContent string
-	var proofOfPossession []byte
-	if request.GetPublicKeyRequest() != nil {
-		if request.GetPublicKeyRequest().PublicKey != nil {
-			pubKeyContent = request.GetPublicKeyRequest().PublicKey.Content
+		// Parse public key and check for weak key parameters
+		publicKey = csr.PublicKey
+		if err := cryptoutils.ValidatePubKey(publicKey); err != nil {
+			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, insecurePublicKey)
 		}
-		proofOfPossession = request.GetPublicKeyRequest().ProofOfPossession
-	}
-	publicKey, err := challenges.ParsePublicKey(pubKeyContent, csr)
-	if err != nil {
-		return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidPublicKey)
-	}
 
-	// validate public key, checking for weak key parameters
-	if err := cryptoutils.ValidatePubKey(publicKey); err != nil {
-		return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, insecurePublicKey)
-	}
-
-	// verify the challenge
-	if csr != nil {
+		// Check signature of CSR
 		err = csr.CheckSignature()
 		if err != nil {
 			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
 		}
 	} else {
+		// Option 2: If no CSR, we need to check that they signed something with theirprivate key
+		var (
+			pubKeyContent     string
+			proofOfPossession []byte
+			err               error
+		)
+		if request.GetPublicKeyRequest() != nil {
+			if request.GetPublicKeyRequest().PublicKey != nil {
+				pubKeyContent = request.GetPublicKeyRequest().PublicKey.Content
+			}
+			proofOfPossession = request.GetPublicKeyRequest().ProofOfPossession
+		}
+
+		// Parse public key and check for weak parameters
+		publicKey, err = challenges.ParsePublicKey(pubKeyContent)
+		if err != nil {
+			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidPublicKey)
+		}
+		if err := cryptoutils.ValidatePubKey(publicKey); err != nil {
+			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, insecurePublicKey)
+		}
+
+		// Check proof of possession signature
 		if err := challenges.CheckSignature(publicKey, proofOfPossession, principal.Name(ctx)); err != nil {
 			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
 		}

pkg/challenges/challenges.go

@@ -523,16 +523,12 @@ func PrincipalFromIDToken(ctx context.Context, tok *oidc.IDToken) (identity.Prin
 	return principal, nil
 }
 
-// ParsePublicKey parses a PEM or DER encoded public key, or extracts the public
-// key from the provided CSR. Returns an error if decoding fails or if no public
-// key is found.
-func ParsePublicKey(encodedPubKey string, csr *x509.CertificateRequest) (crypto.PublicKey, error) {
-	if csr == nil && len(encodedPubKey) == 0 {
+// ParsePublicKey parses a PEM or DER encoded public key. Returns an error if
+// decoding fails or if no public key is found.
+func ParsePublicKey(encodedPubKey string) (crypto.PublicKey, error) {
+	if len(encodedPubKey) == 0 {
 		return nil, errors.New("public key not provided")
 	}
-	if csr != nil {
-		return csr.PublicKey, nil
-	}
 	// try to unmarshal as PEM
 	publicKey, err := cryptoutils.UnmarshalPEMToPublicKey([]byte(encodedPubKey))
 	if err != nil {

pkg/challenges/challenges_test.go

@@ -25,9 +25,7 @@ import (
 	"crypto/rsa"
 	"crypto/sha256"
 	"crypto/x509"
-	"crypto/x509/pkix"
 	"encoding/asn1"
-	"encoding/pem"
 	"errors"
 	"fmt"
 	"net/url"
@@ -466,28 +464,13 @@ func TestCheckSignatureRSA(t *testing.T) {
 }
 
 func TestParsePublicKey(t *testing.T) {
-	// succeeds with CSR
 	priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	failErr(t, err)
-	csrTmpl := &x509.CertificateRequest{Subject: pkix.Name{CommonName: "test"}}
-	derCSR, err := x509.CreateCertificateRequest(rand.Reader, csrTmpl, priv)
-	failErr(t, err)
-	pemCSR := pem.EncodeToMemory(&pem.Block{
-		Type:  "CERTIFICATE REQUEST",
-		Bytes: derCSR,
-	})
-	parsedCSR, err := cryptoutils.ParseCSR(pemCSR)
-	failErr(t, err)
-	pubKey, err := ParsePublicKey("", parsedCSR)
-	failErr(t, err)
-	if err := cryptoutils.EqualKeys(pubKey, priv.Public()); err != nil {
-		t.Fatalf("expected equal public keys")
-	}
 
 	// succeeds with PEM-encoded key
 	pemKey, err := cryptoutils.MarshalPublicKeyToPEM(priv.Public())
 	failErr(t, err)
-	pubKey, err = ParsePublicKey(string(pemKey), nil)
+	pubKey, err := ParsePublicKey(string(pemKey))
 	failErr(t, err)
 	if err := cryptoutils.EqualKeys(pubKey, priv.Public()); err != nil {
 		t.Fatalf("expected equal public keys")
@@ -496,22 +479,22 @@ func TestParsePublicKey(t *testing.T) {
 	// succeeds with DER-encoded key
 	derKey, err := cryptoutils.MarshalPublicKeyToDER(priv.Public())
 	failErr(t, err)
-	pubKey, err = ParsePublicKey(string(derKey), nil)
+	pubKey, err = ParsePublicKey(string(derKey))
 	failErr(t, err)
 	if err := cryptoutils.EqualKeys(pubKey, priv.Public()); err != nil {
 		t.Fatalf("expected equal public keys")
 	}
 
 	// fails with no public key
-	_, err = ParsePublicKey("", nil)
+	_, err = ParsePublicKey("")
 	if err == nil || err.Error() != "public key not provided" {
 		t.Fatalf("expected error parsing no public key, got %v", err)
 	}
 
 	// fails with invalid public key (private key)
 	pemPrivKey, err := cryptoutils.MarshalPrivateKeyToPEM(priv)
 	failErr(t, err)
-	_, err = ParsePublicKey(string(pemPrivKey), nil)
+	_, err = ParsePublicKey(string(pemPrivKey))
 	if err == nil || err.Error() != "error parsing PEM or DER encoded public key" {
 		t.Fatalf("expected error parsing invalid public key, got %v", err)
 	}