Skip to content

Commit c602381

Browse files
authored
CA: Stop using OCSP status NotReady (#8394)
Stop setting "OcspNotReady: true" when asking the SA to store a linting precertificate, and stop calling SetCertificateStatusReady after precertificate issuance has succeeded. Because these two calls are always made in sequence by the same CA instance, this change will not leave any database entries permanently hanging in the "NotReady" state (unless an error occurs between the two calls, as is the case today). A follow-up change will remove the corresponding support on the SA side. Part of #8343
1 parent ade2703 commit c602381

File tree

2 files changed

+13
-25
lines changed

2 files changed

+13
-25
lines changed

ca/ca.go

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,7 @@ type certProfileWithID struct {
8282
profile *issuance.Profile
8383
}
8484

85-
// caMetrics holds various metrics which are shared between caImpl, ocspImpl,
86-
// and crlImpl.
85+
// caMetrics holds various metrics which are shared between caImpl and crlImpl.
8786
type caMetrics struct {
8887
signatureCount *prometheus.CounterVec
8988
signErrorCount *prometheus.CounterVec
@@ -132,7 +131,6 @@ func (m *caMetrics) noteSignError(err error) {
132131
}
133132

134133
// certificateAuthorityImpl represents a CA that signs certificates.
135-
// It can sign OCSP responses as well, but only via delegation to an ocspImpl.
136134
type certificateAuthorityImpl struct {
137135
capb.UnsafeCertificateAuthorityServer
138136
sa sapb.StorageAuthorityCertificateClient
@@ -155,7 +153,7 @@ var _ capb.CertificateAuthorityServer = (*certificateAuthorityImpl)(nil)
155153

156154
// makeIssuerMaps processes a list of issuers into a set of maps for easy
157155
// lookup either by key algorithm (useful for picking an issuer for a precert)
158-
// or by unique ID (useful for final certs, OCSP, and CRLs). If two issuers with
156+
// or by unique ID (useful for final certs and CRLs). If two issuers with
159157
// the same unique ID are encountered, an error is returned.
160158
func makeIssuerMaps(issuers []*issuance.Issuer) (issuerMaps, error) {
161159
issuersByAlg := make(map[x509.PublicKeyAlgorithm][]*issuance.Issuer, 2)
@@ -203,8 +201,7 @@ func makeCertificateProfilesMap(profiles map[string]*issuance.ProfileConfig) (ma
203201
}
204202

205203
// NewCertificateAuthorityImpl creates a CA instance that can sign certificates
206-
// from any number of issuance.Issuers according to their profiles, and can sign
207-
// OCSP (via delegation to an ocspImpl and its issuers).
204+
// from any number of issuance.Issuers and for any number of profiles.
208205
func NewCertificateAuthorityImpl(
209206
sa sapb.StorageAuthorityCertificateClient,
210207
sctService rapb.SCTProviderClient,
@@ -294,11 +291,6 @@ func (ca *certificateAuthorityImpl) issuePrecertificate(ctx context.Context, cer
294291
return nil, err
295292
}
296293

297-
_, err = ca.sa.SetCertificateStatusReady(ctx, &sapb.Serial{Serial: serialHex})
298-
if err != nil {
299-
return nil, err
300-
}
301-
302294
return precertDER, nil
303295
}
304296

@@ -572,7 +564,6 @@ func (ca *certificateAuthorityImpl) issuePrecertificateInner(ctx context.Context
572564
RegID: issueReq.RegistrationID,
573565
Issued: timestamppb.New(ca.clk.Now()),
574566
IssuerNameID: int64(issuer.NameID()),
575-
OcspNotReady: true,
576567
})
577568
if err != nil {
578569
return nil, nil, err

test/integration/cert_storage_failed_test.go

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,8 @@ func getPrecertByName(db *sql.DB, reversedName string) (*x509.Certificate, error
6060

6161
// TestIssuanceCertStorageFailed tests what happens when a storage RPC fails
6262
// during issuance. Specifically, it tests that case where we successfully
63-
// prepared and stored a linting certificate plus metadata, but after
64-
// issuing the precertificate we failed to mark the certificate as "ready"
65-
// to serve an OCSP "good" response.
63+
// prepared and stored a linting certificate plus metadata, but failed to store
64+
// the corresponding final certificate after issuance completed.
6665
//
6766
// To do this, we need to mess with the database, because we want to cause
6867
// a failure in one specific query, without control ever returning to the
@@ -83,28 +82,26 @@ func TestIssuanceCertStorageFailed(t *testing.T) {
8382
_, err = db.ExecContext(ctx, `DROP TRIGGER IF EXISTS fail_ready`)
8483
test.AssertNotError(t, err, "failed to drop trigger")
8584

86-
// Make a specific update to certificateStatus fail, for this test but not others.
85+
// Make a specific insert into certificates fail, for this test but not others.
8786
// To limit the effect to this one test, we make the trigger aware of a specific
88-
// hostname used in this test. Since the UPDATE to the certificateStatus table
87+
// hostname used in this test. Since the INSERT to the certificates table
8988
// doesn't include the hostname, we look it up in the issuedNames table, keyed
90-
// off of the serial being updated.
91-
// We limit this to UPDATEs that set the status to "good" because otherwise we
92-
// would fail to revoke the certificate later.
89+
// off of the serial.
9390
// NOTE: CREATE and DROP TRIGGER do not work in prepared statements. Go's
9491
// database/sql will automatically try to use a prepared statement if you pass
9592
// any arguments to Exec besides the query itself, so don't do that.
9693
_, err = db.ExecContext(ctx, `
9794
CREATE TRIGGER fail_ready
98-
BEFORE UPDATE ON certificateStatus
95+
BEFORE INSERT ON certificates
9996
FOR EACH ROW BEGIN
10097
DECLARE reversedName1 VARCHAR(255);
10198
SELECT reversedName
10299
INTO reversedName1
103100
FROM issuedNames
104101
WHERE serial = NEW.serial
105102
AND reversedName LIKE "com.wantserror.%";
106-
IF NEW.status = "good" AND reversedName1 != "" THEN
107-
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Pretend there was an error updating the certificateStatus';
103+
IF reversedName1 != "" THEN
104+
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Pretend there was an error inserting into certificates';
108105
END IF;
109106
END
110107
`)
@@ -117,7 +114,7 @@ func TestIssuanceCertStorageFailed(t *testing.T) {
117114

118115
// ---- Test revocation by serial ----
119116
revokeMeDomain := "revokeme.wantserror.com"
120-
// This should fail because the trigger prevented setting the certificate status to "ready"
117+
// This should fail because the trigger prevented storing the final certificate.
121118
_, err = authAndIssue(nil, certKey, []acme.Identifier{{Type: "dns", Value: revokeMeDomain}}, true, "")
122119
test.AssertError(t, err, "expected authAndIssue to fail")
123120

@@ -140,7 +137,7 @@ func TestIssuanceCertStorageFailed(t *testing.T) {
140137

141138
// ---- Test revocation by key ----
142139
blockMyKeyDomain := "blockmykey.wantserror.com"
143-
// This should fail because the trigger prevented setting the certificate status to "ready"
140+
// This should fail because the trigger prevented storing the final certificate.
144141
_, err = authAndIssue(nil, certKey, []acme.Identifier{{Type: "dns", Value: blockMyKeyDomain}}, true, "")
145142
test.AssertError(t, err, "expected authAndIssue to fail")
146143

0 commit comments

Comments
 (0)