Refactor challenge verification (#580)

* Break up token parsing and proof of possession verification Signed-off-by: Nathan Smith <[email protected]> * 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 <[email protected]>
sigstore · May 17, 2022 · c041c98 · c041c98
1 parent a1b85d4
commit c041c98
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 71 deletions.
diff --git a/pkg/api/error.go b/pkg/api/error.go
@@ -33,7 +33,9 @@ const (
 	insecurePublicKey      = "The public key supplied in the request is insecure"
 	//nolint
 	invalidCredentials = "There was an error processing the credentials for this request"
-	genericCAError     = "error communicating with CA backend"
+	// nolint
+	invalidIdentityToken = "There was an error processing the identity token"
+	genericCAError       = "error communicating with CA backend"
 )
 
 func handleFulcioGRPCError(ctx context.Context, code codes.Code, err error, message string, fields ...interface{}) error {

diff --git a/pkg/api/grpc_server.go b/pkg/api/grpc_server.go
@@ -17,7 +17,7 @@ package api
 
 import (
 	"context"
-	"crypto/x509"
+	"crypto"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -70,43 +70,65 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu
 		}
 	}
 
-	principal, err := authorize(ctx, token)
+	// Authenticate OIDC ID token by checking signature
+	idtoken, err := authorize(ctx, token)
 	if err != nil {
 		return nil, handleFulcioGRPCError(ctx, codes.Unauthenticated, err, invalidCredentials)
 	}
+	// Parse authenticated ID token into principal
+	// TODO:(nsmith5) replace this and authorize call above with
+	// just identity.IssuerPool.Authenticate()
+	principal, err := challenges.PrincipalFromIDToken(ctx, idtoken)
+	if err != nil {
+		return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidIdentityToken)
+	}
 
-	// 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: Verify CSR
+		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)
-	}
+		if err := csr.CheckSignature(); err != nil {
+			return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
+		}
+	} else {
+		// Option 2: Check the signature for proof of possession of a private 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
+		}
 
-	// verify challenge
-	subject, err := challenges.ExtractSubject(ctx, principal, publicKey, csr, proofOfPossession)
-	if err != nil {
-		return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature)
+		// 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)
+		}
 	}
 
 	var csc *certauth.CodeSigningCertificate
@@ -115,7 +137,7 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu
 	// For CAs that do not support embedded SCTs or if the CT log is not configured
 	if sctCa, ok := g.ca.(certauth.EmbeddedSCTCA); !ok || g.ct == nil {
 		// currently configured CA doesn't support pre-certificate flow required to embed SCT in final certificate
-		csc, err = g.ca.CreateCertificate(ctx, subject, publicKey)
+		csc, err = g.ca.CreateCertificate(ctx, principal, publicKey)
 		if err != nil {
 			// if the error was due to invalid input in the request, return HTTP 400
 			if _, ok := err.(certauth.ValidationError); ok {
@@ -165,7 +187,7 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu
 			result.GetSignedCertificateDetachedSct().SignedCertificateTimestamp = sctBytes
 		}
 	} else {
-		precert, err := sctCa.CreatePrecertificate(ctx, subject, publicKey)
+		precert, err := sctCa.CreatePrecertificate(ctx, principal, publicKey)
 		if err != nil {
 			// if the error was due to invalid input in the request, return HTTP 400
 			if _, ok := err.(certauth.ValidationError); ok {

diff --git a/pkg/challenges/challenges.go b/pkg/challenges/challenges.go
@@ -493,7 +493,7 @@ func validateAllowedDomain(subjectHostname, issuerHostname string) error {
 	return fmt.Errorf("hostname top-level and second-level domains do not match: %s, %s", subjectHostname, issuerHostname)
 }
 
-func ExtractSubject(ctx context.Context, tok *oidc.IDToken, publicKey crypto.PublicKey, csr *x509.CertificateRequest, challenge []byte) (identity.Principal, error) {
+func PrincipalFromIDToken(ctx context.Context, tok *oidc.IDToken) (identity.Principal, error) {
 	iss, ok := config.FromContext(ctx).GetIssuer(tok.Issuer)
 	if !ok {
 		return nil, fmt.Errorf("configuration can not be loaded for issuer %v", tok.Issuer)
@@ -520,31 +520,15 @@ func ExtractSubject(ctx context.Context, tok *oidc.IDToken, publicKey crypto.Pub
 		return nil, err
 	}
 
-	// verify the proof of possession of the private key
-	if csr != nil {
-		err = csr.CheckSignature()
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		if err := CheckSignature(publicKey, challenge, principal.Name(ctx)); err != nil {
-			return nil, err
-		}
-	}
-
 	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 {

diff --git a/pkg/challenges/challenges_test.go b/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"
@@ -671,28 +669,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")
@@ -701,22 +684,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)
 	}