RA: Always provide profile name to the CA

letsencrypt · Jan 30, 2025 · f35ea40 · f35ea40
1 parent 1262939
commit f35ea40
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 63 deletions.
diff --git a/ra/ra.go b/ra/ra.go
@@ -1138,9 +1138,16 @@ func (ra *RegistrationAuthorityImpl) issueCertificateOuter(
 		logEvent.PreviousCertificateIssued = timestamps.Timestamps[0].AsTime()
 	}
 
+	// Ensure that we always provide a profile name to the CA, even if the order
+	// didn't request a specific profile.
+	profileName := order.CertificateProfileName
+	if profileName == "" {
+		profileName = ra.defaultProfileName
+	}
+
 	// Step 3: Issue the Certificate
 	cert, cpId, err := ra.issueCertificateInner(
-		ctx, csr, isRenewal, order.CertificateProfileName, accountID(order.RegistrationID), orderID(order.Id))
+		ctx, csr, isRenewal, profileName, accountID(order.RegistrationID), orderID(order.Id))
 
 	// Step 4: Fail the order if necessary, and update metrics and log fields
 	var result string

diff --git a/ra/ra_test.go b/ra/ra_test.go
@@ -10,6 +10,7 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
+	"encoding/hex"
 	"encoding/json"
 	"encoding/pem"
 	"errors"
@@ -3366,11 +3367,12 @@ func (sa *mockSAWithFinalize) FQDNSetTimestampsForWindow(ctx context.Context, in
 	}, nil
 }
 
-func TestIssueCertificateInnerWithProfile(t *testing.T) {
+func TestIssueCertificateOuter(t *testing.T) {
 	_, _, ra, _, fc, cleanup := initAuthorities(t)
 	defer cleanup()
+	ra.SA = &mockSAWithFinalize{}
 
-	// Generate a reasonable-looking CSR and cert to pass the matchesCSR check.
+	// Create a CSR to submit and a certificate for the fake CA to return.
 	testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	test.AssertNotError(t, err, "generating test key")
 	csrDER, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{DNSNames: []string{"example.com"}}, testKey)
@@ -3387,71 +3389,68 @@ func TestIssueCertificateInnerWithProfile(t *testing.T) {
 	test.AssertNotError(t, err, "creating test cert")
 	certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER})
 
-	// Use a mock CA that will record the profile name and profile hash included
-	// in the RA's request messages. Populate it with the cert generated above.
-	mockCA := MockCARecordingProfile{inner: &mocks.MockCA{PEM: certPEM}}
-	ra.CA = &mockCA
-
-	ra.SA = &mockSAWithFinalize{}
-
-	// Call issueCertificateInner with the CSR generated above and the profile
-	// name "default", which will cause the mockCA to return a specific hash.
-	_, cpId, err := ra.issueCertificateInner(context.Background(), csr, false, "default", 1, 1)
-	test.AssertNotError(t, err, "issuing cert with profile name")
-	test.AssertEquals(t, mockCA.profileName, cpId.name)
-	test.AssertByteEquals(t, mockCA.profileHash, cpId.hash)
-}
-
-func TestIssueCertificateOuter(t *testing.T) {
-	_, sa, ra, _, fc, cleanup := initAuthorities(t)
-	defer cleanup()
-
-	// Make some valid authorizations for some names
-	names := []string{"not-example.com", "www.not-example.com", "still.not-example.com", "definitely.not-example.com"}
-	exp := ra.clk.Now().Add(ra.validationProfiles[ra.defaultProfileName].orderLifetime)
-	var authzIDs []int64
-	for _, name := range names {
-		authzIDs = append(authzIDs, createFinalizedAuthorization(t, sa, name, exp, core.ChallengeTypeHTTP01, ra.clk.Now()))
-	}
-
-	// Create a pending order for all of the names
-	order, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
-		NewOrder: &sapb.NewOrderRequest{
-			RegistrationID:         Registration.Id,
-			Expires:                timestamppb.New(exp),
-			DnsNames:               names,
-			V2Authorizations:       authzIDs,
-			CertificateProfileName: "philsProfile",
+	for _, tc := range []struct {
+		name        string
+		profile     string
+		wantProfile string
+		wantHash    string
+	}{
+		{
+			name:        "select default profile when none specified",
+			wantProfile: "test", // matches ra.defaultProfileName
+			wantHash:    "9f86d081884c7d65",
 		},
-	})
-	test.AssertNotError(t, err, "Could not add test order with finalized authz IDs")
+		{
+			name:        "default profile specified",
+			profile:     "test",
+			wantProfile: "test",
+			wantHash:    "9f86d081884c7d65",
+		},
+		{
+			name:        "other profile specified",
+			profile:     "other",
+			wantProfile: "other",
+			wantHash:    "d9298a10d1b07358",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			// Use a mock CA that will record the profile name and profile hash included
+			// in the RA's request messages. Populate it with the cert generated above.
+			mockCA := MockCARecordingProfile{inner: &mocks.MockCA{PEM: certPEM}}
+			ra.CA = &mockCA
 
-	testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	test.AssertNotError(t, err, "generating test key")
-	csrDER, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{DNSNames: []string{"example.com"}}, testKey)
-	test.AssertNotError(t, err, "creating test csr")
-	csr, err := x509.ParseCertificateRequest(csrDER)
-	test.AssertNotError(t, err, "parsing test csr")
-	certDER, err := x509.CreateCertificate(rand.Reader, &x509.Certificate{
-		SerialNumber:          big.NewInt(1),
-		DNSNames:              []string{"example.com"},
-		NotBefore:             fc.Now(),
-		BasicConstraintsValid: true,
-		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
-	}, &x509.Certificate{}, testKey.Public(), testKey)
-	test.AssertNotError(t, err, "creating test cert")
-	certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER})
+			order := &corepb.Order{
+				RegistrationID:         Registration.Id,
+				Expires:                timestamppb.New(fc.Now().Add(24 * time.Hour)),
+				DnsNames:               []string{"example.com"},
+				CertificateProfileName: tc.profile,
+			}
 
-	// Use a mock CA that will record the profile name and profile hash included
-	// in the RA's request messages. Populate it with the cert generated above.
-	mockCA := MockCARecordingProfile{inner: &mocks.MockCA{PEM: certPEM}}
-	ra.CA = &mockCA
+			order, err = ra.issueCertificateOuter(context.Background(), order, csr, certificateRequestEvent{})
 
-	ra.SA = &mockSAWithFinalize{}
+			// The resulting order should have new fields populated
+			if order.Status != string(core.StatusValid) {
+				t.Errorf("order.Status = %+v, want %+v", order.Status, core.StatusValid)
+			}
+			if order.CertificateSerial != core.SerialToString(big.NewInt(1)) {
+				t.Errorf("CertificateSerial = %+v, want %+v", order.CertificateSerial, 1)
+			}
 
-	_, err = ra.issueCertificateOuter(context.Background(), order, csr, certificateRequestEvent{})
-	test.AssertNotError(t, err, "Could not issue certificate")
-	test.AssertMetricWithLabelsEquals(t, ra.newCertCounter, prometheus.Labels{"profileName": mockCA.profileName, "profileHash": fmt.Sprintf("%x", mockCA.profileHash)}, 1)
+			// The recorded profile and profile hash should match what we expect.
+			if mockCA.profileName != tc.wantProfile {
+				t.Errorf("recorded profileName = %+v, want %+v", mockCA.profileName, tc.wantProfile)
+			}
+			wantHash, err := hex.DecodeString(tc.wantHash)
+			if err != nil {
+				t.Fatalf("decoding test hash: %s", err)
+			}
+			if !bytes.Equal(mockCA.profileHash, wantHash) {
+				t.Errorf("recorded profileName = %x, want %x", mockCA.profileHash, wantHash)
+			}
+			test.AssertMetricWithLabelsEquals(t, ra.newCertCounter, prometheus.Labels{"profileName": tc.wantProfile, "profileHash": tc.wantHash}, 1)
+			ra.newCertCounter.Reset()
+		})
+	}
 }
 
 func TestNewOrderMaxNames(t *testing.T) {