Repository: smallstep/crypto
Pull request #9 (2020-09-01 22:48:36)

Do not enforce signature algorithm.
### Description

This PR ignores the SignatureAlgorithm when no templates are used or in the template variable `.Insecure.CR`.

If SignatureAlgorithm is set in those cases, x509.CreateCertificate will fail if the signer cannot sign with that algorithm, e.g: RSA signer trying to sign ECDSA-SHA256.

This PR fixes some problematic cases with https://github.com/smallstep/crypto/pull/7



Patch:
diff --git a/x509util/certificate.go b/x509util/certificate.go
index 9702741..9c880e5 100644
--- a/x509util/certificate.go
+++ b/x509util/certificate.go
@@ -53,12 +53,9 @@ func NewCertificate(cr *x509.CertificateRequest, opts ...Option) (*Certificate,
 	}
 
 	// If no template use only the certificate request with the default leaf key
-	// usages. And do not enforce signature algorithm from the CSR, it might not
-	// be compatible with the certificate signer.
+	// usages.
 	if o.CertBuffer == nil {
-		cert := newCertificateRequest(cr).GetLeafCertificate()
-		cert.SignatureAlgorithm = 0
-		return cert, nil
+		return newCertificateRequest(cr).GetLeafCertificate(), nil
 	}
 
 	// With templates
diff --git a/x509util/certificate_request.go b/x509util/certificate_request.go
index 56dbb77..dbba214 100644
--- a/x509util/certificate_request.go
+++ b/x509util/certificate_request.go
@@ -65,6 +65,12 @@ func NewCertificateRequest(signer crypto.Signer, opts ...Option) (*CertificateRe
 
 // newCertificateRequest is an internal method that creates a CertificateRequest
 // from an x509.CertificateRequest.
+//
+// This method is used to create the template variable .Insecure.CR or to
+// initialize the Certificate when no templates are used. newCertificateRequest
+// will always ignore the SignatureAlgorithm because we cannot guarantee that
+// the signer will be able to sign a certificate template if
+// Certificate.SignatureAlgorithm is set.
 func newCertificateRequest(cr *x509.CertificateRequest) *CertificateRequest {
 	// Set SubjectAltName extension as critical if Subject is empty.
 	if len(cr.Extensions) > 0 {
@@ -89,7 +95,9 @@ func newCertificateRequest(cr *x509.CertificateRequest) *CertificateRequest {
 		PublicKey:          cr.PublicKey,
 		PublicKeyAlgorithm: cr.PublicKeyAlgorithm,
 		Signature:          cr.Signature,
-		SignatureAlgorithm: SignatureAlgorithm(cr.SignatureAlgorithm),
+		// Do not enforce signature algorithm from the CSR, it might not
+		// be compatible with the certificate signer.
+		SignatureAlgorithm: 0,
 	}
 }
 
@@ -103,7 +111,7 @@ func (c *CertificateRequest) GetCertificateRequest() (*x509.CertificateRequest,
 		EmailAddresses:     cert.EmailAddresses,
 		URIs:               cert.URIs,
 		ExtraExtensions:    cert.ExtraExtensions,
-		SignatureAlgorithm: cert.SignatureAlgorithm,
+		SignatureAlgorithm: x509.SignatureAlgorithm(c.SignatureAlgorithm),
 	}, c.Signer)
 	if err != nil {
 		return nil, errors.Wrap(err, "error creating certificate request")
@@ -114,6 +122,10 @@ func (c *CertificateRequest) GetCertificateRequest() (*x509.CertificateRequest,
 
 // GetCertificate returns the Certificate representation of the
 // CertificateRequest.
+//
+// GetCertificate will not specify a SignatureAlgorithm, it's not possible to
+// guarantee that the certificate signer can sign with the CertificateRequest
+// SignatureAlgorithm.
 func (c *CertificateRequest) GetCertificate() *Certificate {
 	return &Certificate{
 		Subject:            c.Subject,
@@ -125,12 +137,16 @@ func (c *CertificateRequest) GetCertificate() *Certificate {
 		Extensions:         c.Extensions,
 		PublicKey:          c.PublicKey,
 		PublicKeyAlgorithm: c.PublicKeyAlgorithm,
-		SignatureAlgorithm: c.SignatureAlgorithm,
+		SignatureAlgorithm: 0,
 	}
 }
 
 // GetLeafCertificate returns the Certificate representation of the
 // CertificateRequest, including KeyUsage and ExtKeyUsage extensions.
+//
+// GetLeafCertificate will not specify a SignatureAlgorithm, it's not possible
+// to guarantee that the certificate signer can sign with the CertificateRequest
+// SignatureAlgorithm.
 func (c *CertificateRequest) GetLeafCertificate() *Certificate {
 	keyUsage := x509.KeyUsageDigitalSignature
 	if _, ok := c.PublicKey.(*rsa.PublicKey); ok {


Test patch:
diff --git a/x509util/certificate_request_test.go b/x509util/certificate_request_test.go
index f595b96..1e294f9 100644
--- a/x509util/certificate_request_test.go
+++ b/x509util/certificate_request_test.go
@@ -75,15 +75,17 @@ func Test_newCertificateRequest(t *testing.T) {
 	}{
 		{"ok", args{&x509.CertificateRequest{}}, &CertificateRequest{}},
 		{"complex", args{&x509.CertificateRequest{
-			Extensions: []pkix.Extension{{Id: []int{1, 2, 3}, Critical: true, Value: []byte{3, 2, 1}}},
-			Subject:    pkix.Name{Province: []string{"CA"}, CommonName: "commonName"},
-			DNSNames:   []string{"foo"},
-			PublicKey:  []byte("publicKey"),
+			Extensions:         []pkix.Extension{{Id: []int{1, 2, 3}, Critical: true, Value: []byte{3, 2, 1}}},
+			Subject:            pkix.Name{Province: []string{"CA"}, CommonName: "commonName"},
+			DNSNames:           []string{"foo"},
+			PublicKey:          []byte("publicKey"),
+			SignatureAlgorithm: x509.PureEd25519,
 		}}, &CertificateRequest{
-			Extensions: []Extension{{ID: []int{1, 2, 3}, Critical: true, Value: []byte{3, 2, 1}}},
-			Subject:    Subject{Province: []string{"CA"}, CommonName: "commonName"},
-			DNSNames:   []string{"foo"},
-			PublicKey:  []byte("publicKey"),
+			Extensions:         []Extension{{ID: []int{1, 2, 3}, Critical: true, Value: []byte{3, 2, 1}}},
+			Subject:            Subject{Province: []string{"CA"}, CommonName: "commonName"},
+			DNSNames:           []string{"foo"},
+			PublicKey:          []byte("publicKey"),
+			SignatureAlgorithm: SignatureAlgorithm(x509.UnknownSignatureAlgorithm),
 		}},
 	}
 	for _, tt := range tests {
@@ -251,7 +253,7 @@ func TestCertificateRequest_GetCertificate(t *testing.T) {
 				Extensions:         []Extension{{ID: []int{1, 2, 3}, Critical: true, Value: []byte{3, 2, 1}}},
 				PublicKey:          []byte("publicKey"),
 				PublicKeyAlgorithm: x509.Ed25519,
-				SignatureAlgorithm: SignatureAlgorithm(x509.PureEd25519),
+				SignatureAlgorithm: SignatureAlgorithm(x509.UnknownSignatureAlgorithm),
 			},
 		},
 	}
@@ -324,7 +326,7 @@ func TestCertificateRequest_GetLeafCertificate(t *testing.T) {
 				}),
 				PublicKey:          []byte("publicKey"),
 				PublicKeyAlgorithm: x509.Ed25519,
-				SignatureAlgorithm: SignatureAlgorithm(x509.PureEd25519),
+				SignatureAlgorithm: SignatureAlgorithm(x509.UnknownSignatureAlgorithm),
 			},
 		},
 		{"rsa",
@@ -355,7 +357,7 @@ func TestCertificateRequest_GetLeafCertificate(t *testing.T) {
 				}),
 				PublicKey:          &rsa.PublicKey{},
 				PublicKeyAlgorithm: x509.RSA,
-				SignatureAlgorithm: SignatureAlgorithm(x509.SHA256WithRSA),
+				SignatureAlgorithm: SignatureAlgorithm(x509.UnknownSignatureAlgorithm),
 			},
 		},
 	}