From 8784317f7776a29d018e86b5793dd5c085687f6c Mon Sep 17 00:00:00 2001
From: "R.B. Boyer" <rb@hashicorp.com>
Date: Thu, 17 Jun 2021 14:08:11 -0500
Subject: [PATCH] crypto/x509: always emit a critical SAN extension if the
 Subject is empty in a CSR

When using x509.CreateCertificate empty Subject fields correctly set the
SAN extension as critical. This changeset ensures that
x509.CreateCertificateRequest does the same thing.

This was previously fixed in x509.CreateCertificate with:
https://go-review.googlesource.com/70852
---
 src/crypto/x509/x509.go      | 28 ++++++++++++++++------------
 src/crypto/x509/x509_test.go | 27 +++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 4304ab54e1..bc535befa7 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1309,7 +1309,7 @@ func marshalCertificatePolicies(policyIdentifiers []asn1.ObjectIdentifier) (pkix
 	return ext, nil
 }
 
-func buildCSRExtensions(template *CertificateRequest) ([]pkix.Extension, error) {
+func buildCSRExtensions(template *CertificateRequest, subjectIsEmpty bool) ([]pkix.Extension, error) {
 	var ret []pkix.Extension
 
 	if (len(template.DNSNames) > 0 || len(template.EmailAddresses) > 0 || len(template.IPAddresses) > 0 || len(template.URIs) > 0) &&
@@ -1320,8 +1320,12 @@ func buildCSRExtensions(template *CertificateRequest) ([]pkix.Extension, error)
 		}
 
 		ret = append(ret, pkix.Extension{
-			Id:    oidExtensionSubjectAltName,
-			Value: sanBytes,
+			Id: oidExtensionSubjectAltName,
+			// From RFC 5280, Section 4.2.1.6:
+			// “If the subject field contains an empty sequence ... then
+			// subjectAltName extension ... is marked as critical”
+			Critical: subjectIsEmpty,
+			Value:    sanBytes,
 		})
 	}
 
@@ -1859,7 +1863,15 @@ func CreateCertificateRequest(rand io.Reader, template *CertificateRequest, priv
 		return nil, err
 	}
 
-	extensions, err := buildCSRExtensions(template)
+	asn1Subject := template.RawSubject
+	if len(asn1Subject) == 0 {
+		asn1Subject, err = asn1.Marshal(template.Subject.ToRDNSequence())
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	extensions, err := buildCSRExtensions(template, bytes.Equal(asn1Subject, emptyASN1Subject))
 	if err != nil {
 		return nil, err
 	}
@@ -1946,14 +1958,6 @@ func CreateCertificateRequest(rand io.Reader, template *CertificateRequest, priv
 		rawAttributes = append(rawAttributes, rawValue)
 	}
 
-	asn1Subject := template.RawSubject
-	if len(asn1Subject) == 0 {
-		asn1Subject, err = asn1.Marshal(template.Subject.ToRDNSequence())
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	tbsCSR := tbsCertificateRequest{
 		Version: 0, // PKCS #10, RFC 2986
 		Subject: asn1.RawValue{FullBytes: asn1Subject},
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 802bce0f9a..64402d67ed 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -2298,6 +2298,33 @@ func TestEmptySubject(t *testing.T) {
 	t.Fatal("SAN extension is missing")
 }
 
+func TestEmptySubjectInCSR(t *testing.T) {
+	template := CertificateRequest{
+		DNSNames: []string{"example.com"},
+	}
+
+	derBytes, err := CreateCertificateRequest(rand.Reader, &template, testPrivateKey)
+	if err != nil {
+		t.Fatalf("failed to create certificate request: %s", err)
+	}
+
+	csr, err := ParseCertificateRequest(derBytes)
+	if err != nil {
+		t.Fatalf("failed to parse certificate request: %s", err)
+	}
+
+	for _, ext := range csr.Extensions {
+		if ext.Id.Equal(oidExtensionSubjectAltName) {
+			if !ext.Critical {
+				t.Fatal("SAN extension is not critical")
+			}
+			return
+		}
+	}
+
+	t.Fatal("SAN extension is missing")
+}
+
 // multipleURLsInCRLDPPEM contains two URLs in a single CRL DistributionPoint
 // structure. It is taken from https://crt.sh/?id=12721534.
 const multipleURLsInCRLDPPEM = `