ca: Use a pre-baked OCSP Must-Staple extension if requested

Also adds unit tests.

ca/ca.go

@@ -375,8 +375,9 @@ func New(log *log.Logger, db *db.MemoryStore, ocspResponderURL string, alternate
 	return ca
 }
 
-func isOCSPMustStapleExtension(ext pkix.Extension) bool {
-	return ext.Id.Equal(asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 24}) && bytes.Equal(ext.Value, []byte{0x30, 0x03, 0x02, 0x01, 0x05})
+var ocspMustStapleExt = pkix.Extension{
+	Id:    asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 24},
+	Value: []byte{0x30, 0x03, 0x02, 0x01, 0x05},
 }
 
 func (ca *CAImpl) CompleteOrder(order *core.Order) {
@@ -405,10 +406,10 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) {
 
 	// Build a list of approved extensions to include in the certificate
 	var extensions []pkix.Extension
-	for _, ext := range order.ParsedCSR.Extensions {
-		if isOCSPMustStapleExtension(ext) {
-			extensions = append(extensions, ext)
-		}
+	if extensionsContainsOCSPMustStaple(order.ParsedCSR.Extensions) {
+		// If the user requested an OCSP Must-Staple extension, use our
+		// pre-baked one to ensure a reasonable value for Critical
+		extensions = append(extensions, ocspMustStapleExt)
 	}
 
 	// issue a certificate for the csr
@@ -426,6 +427,15 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) {
 	order.Unlock()
 }
 
+func extensionsContainsOCSPMustStaple(extensions []pkix.Extension) bool {
+	for _, ext := range extensions {
+		if ext.Id.Equal(ocspMustStapleExt.Id) && bytes.Equal(ext.Value, ocspMustStapleExt.Value) {
+			return true
+		}
+	}
+	return false
+}
+
 func (ca *CAImpl) GetNumberOfRootCerts() int {
 	return len(ca.chains)
 }

ca/ca_test.go

@@ -1,6 +1,7 @@
 package ca
 
 import (
+	"bytes"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
@@ -18,43 +19,118 @@ import (
 	"github.com/letsencrypt/pebble/v2/db"
 )
 
-func TestOCSPMustStaple(t *testing.T) {
+var ocspId asn1.ObjectIdentifier = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 24}
+var ocspValue []byte = []byte{0x30, 0x03, 0x02, 0x01, 0x05}
+
+func makeCa() *CAImpl {
 	logger := log.New(os.Stdout, "Pebble ", log.LstdFlags)
+	db := db.NewMemoryStore()
+	return New(logger, db, "", 0, 1, 0)
+}
+
+func makeCertOrderWithExtensions(extensions []pkix.Extension) core.Order {
 	privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	if err != nil {
 		panic(err)
 	}
-	db := db.NewMemoryStore()
-	ca := New(logger, db, "", 0, 1, 0)
 	csr := x509.CertificateRequest{
-		DNSNames:    []string{"test.org"},
-		IPAddresses: []net.IP{[]byte{10, 255, 0, 0}},
+		DNSNames:    []string{"fake.domain"},
+		IPAddresses: []net.IP{[]byte{192, 0, 2, 1}},
 		PublicKey:   &privateKey.PublicKey,
-		Extensions: []pkix.Extension{
-			{
-				Id:       asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 24},
-				Critical: false,
-				Value:    []byte{0x30, 0x03, 0x02, 0x01, 0x05},
-			},
-		},
+		Extensions:  extensions,
 	}
-	var uniquenames []acme.Identifier
-	uniquenames = append(uniquenames, acme.Identifier{Value: "13.12.13.12", Type: acme.IdentifierIP})
-	order := &core.Order{
+	return core.Order{
 		ID:              "randomstring",
 		AccountID:       "accountid",
 		ParsedCSR:       &csr,
 		BeganProcessing: true,
 		Order: acme.Order{
 			Status:      acme.StatusPending,
 			Expires:     time.Now().AddDate(0, 0, 1).UTC().Format(time.RFC3339),
-			Identifiers: uniquenames,
+			Identifiers: []acme.Identifier{},
 			NotBefore:   time.Now().UTC().Format(time.RFC3339),
 			NotAfter:    time.Now().AddDate(30, 0, 0).UTC().Format(time.RFC3339),
 		},
 		ExpiresDate: time.Now().AddDate(0, 0, 1).UTC(),
 	}
+}
+
+func getOCSPMustStapleExtension(cert *x509.Certificate) *pkix.Extension {
+	for _, ext := range cert.Extensions {
+		if ext.Id.Equal(ocspId) && bytes.Equal(ext.Value, ocspValue) {
+			return &ext
+		}
+	}
+	return nil
+}
 
-	ca.CompleteOrder(order)
-	log.Printf("cert: %+v", order.CertificateObject.Cert)
+func TestNoExtensions(t *testing.T) {
+	ca := makeCa()
+	order := makeCertOrderWithExtensions([]pkix.Extension{})
+	ca.CompleteOrder(&order)
+	foundOCSPExtension := getOCSPMustStapleExtension(order.CertificateObject.Cert)
+	if foundOCSPExtension != nil {
+		t.Error("Expected no OCSP Must-Staple extension in complete cert, but found one")
+	}
+}
+
+func TestSettingOCSPMustStapleExtension(t *testing.T) {
+	// Base case
+	ca := makeCa()
+	order := makeCertOrderWithExtensions([]pkix.Extension{
+		{
+			Id:       ocspId,
+			Critical: false,
+			Value:    ocspValue,
+		},
+	})
+	ca.CompleteOrder(&order)
+	foundOCSPExtension := getOCSPMustStapleExtension(order.CertificateObject.Cert)
+	if foundOCSPExtension == nil {
+		t.Error("Expected OCSP Must-Staple extension in complete cert, but didn't find it")
+	} else if foundOCSPExtension.Critical {
+		t.Error("Expected foundOCSPExtension.Critical to be false, but it was true")
+	}
+
+	// Test w/ improperly set Critical value
+	ca = makeCa()
+	order = makeCertOrderWithExtensions([]pkix.Extension{
+		{
+			Id:       ocspId,
+			Critical: true,
+			Value:    ocspValue,
+		},
+	})
+	ca.CompleteOrder(&order)
+	foundOCSPExtension = getOCSPMustStapleExtension(order.CertificateObject.Cert)
+	if foundOCSPExtension == nil {
+		t.Error("Expected OCSP Must-Staple extension in complete cert, but didn't find it")
+	} else if foundOCSPExtension.Critical {
+		t.Error("Expected foundOCSPExtension.Critical to be false, but it was true")
+	}
+
+	// Test w/ several extensions
+	ca = makeCa()
+	order = makeCertOrderWithExtensions([]pkix.Extension{
+		{
+			Id:       ocspId,
+			Critical: true,
+			Value:    ocspValue,
+		},
+		{
+			Id:       ocspId,
+			Critical: true,
+			Value:    ocspValue,
+		},
+	})
+	ca.CompleteOrder(&order)
+	numOCSPMustStapleExtensions := 0
+	for _, ext := range order.CertificateObject.Cert.Extensions {
+		if ext.Id.Equal(ocspId) && bytes.Equal(ext.Value, ocspValue) {
+			numOCSPMustStapleExtensions += 1
+		}
+	}
+	if numOCSPMustStapleExtensions != 1 {
+		t.Errorf("Expected exactly 1 OCSP Must-Staple extension, found %d", numOCSPMustStapleExtensions)
+	}
 }