From acadb794006c9bb573a72e22bbc83059ad2551e6 Mon Sep 17 00:00:00 2001 From: Nathan Smith Date: Thu, 12 May 2022 15:59:16 -0700 Subject: [PATCH 1/2] Break up token parsing and proof of possession verification Signed-off-by: Nathan Smith --- pkg/api/error.go | 4 +++- pkg/api/grpc_server.go | 27 ++++++++++++++++++++------- pkg/challenges/challenges.go | 14 +------------- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/pkg/api/error.go b/pkg/api/error.go index 41634463d..695033271 100644 --- 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 index 9a91d82f2..b64bee060 100644 --- a/pkg/api/grpc_server.go +++ b/pkg/api/grpc_server.go @@ -70,10 +70,18 @@ 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 @@ -103,10 +111,15 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, insecurePublicKey) } - // verify challenge - subject, err := challenges.ExtractSubject(ctx, principal, publicKey, csr, proofOfPossession) - if err != nil { - return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature) + // verify the challenge + if csr != nil { + if err := csr.CheckSignature(); err != nil { + return nil, handleFulcioGRPCError(ctx, codes.InvalidArgument, err, invalidSignature) + } + } else { + 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 +128,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 +178,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 index 03ed7db0e..b81a56fd5 100644 --- 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,18 +520,6 @@ 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 } From 57dbda79ae40fc28c44ce2a9d3c50ed54d2d2fc8 Mon Sep 17 00:00:00 2001 From: Nathan Smith Date: Thu, 12 May 2022 16:14:43 -0700 Subject: [PATCH 2/2] 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 --- pkg/api/grpc_server.go | 57 ++++++++++++++++++------------- pkg/challenges/challenges.go | 12 +++---- pkg/challenges/challenges_test.go | 25 +++----------- 3 files changed, 41 insertions(+), 53 deletions(-) diff --git a/pkg/api/grpc_server.go b/pkg/api/grpc_server.go index b64bee060..240a5dc42 100644 --- 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" @@ -83,40 +83,49 @@ func (g *grpcCAServer) CreateSigningCertificate(ctx context.Context, request *fu 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) - } - - // verify the challenge - if csr != nil { 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 + } + + // 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) } diff --git a/pkg/challenges/challenges.go b/pkg/challenges/challenges.go index b81a56fd5..ad3797c24 100644 --- a/pkg/challenges/challenges.go +++ b/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 { diff --git a/pkg/challenges/challenges_test.go b/pkg/challenges/challenges_test.go index e58a0fb65..bd4a3ae2c 100644 --- 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" @@ -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,14 +479,14 @@ 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) } @@ -511,7 +494,7 @@ func TestParsePublicKey(t *testing.T) { // 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) }