ra: Guarantee an audit completion line for every issuance attempt - #8846
ra: Guarantee an audit completion line for every issuance attempt#8846beautifulentropy wants to merge 1 commit into
Conversation
issueCertificateOuter logs "Authz used for issuance" audit lines up
front, with a comment promising they always correlate (via the shared
id attr) with a completion line. Two exits broke that promise: the
renewal-check (FQDNSetTimestampsForWindow) error return, and the entire
MTC branch.
Log "Certificate request - error" on the renewal-check failure path.
Move MTC dispatch from issueCertificateOuter into
issueCertificateInner, so a failed MTC issuance flows through the same
failOrder and "Certificate request - error" handling as classic
issuance. Because the dispatch sits below the CAA recheck block, MTC
orders now also receive CAA rechecks under the CAARechecksFailOrder
flag, matching their behavior when the flag is off. Upgrade the MTC
success line ("issued MTC") to an audit log so MTC issuance has an
audit-tagged completion record. Also fix the comment (which referenced
"Certificate request complete", a message renamed during #8606
review).
beautifulentropy
changed the title
| } else if cert == nil { | ||
| // MTC issuance produced no classic certificate, so there is no serial | ||
| // or validity period to record on the order. Its completion record is | ||
| // the "issued MTC" audit line logged by issueMTC. | ||
| return order, nil |
There was a problem hiding this comment.
We shouldn't special case this, for two reasons:
- We want all of our issuance to have log lines of the same shape. The whole point of moving issueMTC inside issueInner was so that we could take advantage of the log line on line 1127. This conditional makes that pointless.
- We do have the serial number: it's computable from the entryIndex returned by issueMTC.
There was a problem hiding this comment.
We do want our log lines to have the same shape, but there's a trickiness: issuerCertificateInner returns a *x509.Certificate, and the log line below pulls fields from it to log.
By the time issueMTC returns, there is not yet a certificate. We have "issued" a TBSCertificateLogEntry, but are awaiting a mirror signature to make a standalone certificate.
I think we can square the circle like so:
- Define a TBSCertificateLogEntry type with serialization and deserialization (we need to do this anyhow for the MTCA work).
- Return that as part of the MTCA.Issue() call.
- In RA, parse that TBSCertificateLogEntry.
- In RA, define a wrapper that can extract useful fields (serial, CommonName, NotBefore, NotAfter) from either an
*x509.Certificateor a TBSCertificateLogEntry. - Use that wrapper to emit the log line.
However, since this requires some refactorings and more implementation I think we shouldn't let it block the slog work. Let's leave the issueMTC call where it is and file an issue for followup as part of MTC implementation.
| return nil, err | ||
| } | ||
|
|
||
| _, err = ra.SA.FinalizeOrder(ctx, &sapb.FinalizeOrderRequest{ |
There was a problem hiding this comment.
This review has made me realize that I think issueMTC is missing a call to SA.FinalizeOrder (cc: @jsha)
There was a problem hiding this comment.
It doesn't have that call because once issueMTC returns, the order will still be in "processing" state until we have a mirror cosignature on the relevant checkpoint.
| err = fmt.Errorf("checking if certificate is a renewal: %w", err) | ||
| ra.log.AuditInfo(ctx, "Certificate request - error", | ||
| slog.String("result", "error"), | ||
| slog.Time("responseTime", ra.clk.Now()), | ||
| blog.Error(err), | ||
| ) | ||
| return nil, err |
There was a problem hiding this comment.
The goal of this PR is to fulfill the comment on lines 1045-1047:
these authz logs will always correspond to (and carry the same
idfield as) a completion line logged below
But for this part of the change, think it would be a lot simpler to move the whole "log the authzs" stanza to just before the issueCertificateInner call. That would mean that failures to check whether a certificate is a renewal wouldn't log "Certificate request - error", and also would would not log the authz lines. I think that's okay.
| } else if cert == nil { | ||
| // MTC issuance produced no classic certificate, so there is no serial | ||
| // or validity period to record on the order. Its completion record is | ||
| // the "issued MTC" audit line logged by issueMTC. | ||
| return order, nil |
There was a problem hiding this comment.
We do want our log lines to have the same shape, but there's a trickiness: issuerCertificateInner returns a *x509.Certificate, and the log line below pulls fields from it to log.
By the time issueMTC returns, there is not yet a certificate. We have "issued" a TBSCertificateLogEntry, but are awaiting a mirror signature to make a standalone certificate.
I think we can square the circle like so:
- Define a TBSCertificateLogEntry type with serialization and deserialization (we need to do this anyhow for the MTCA work).
- Return that as part of the MTCA.Issue() call.
- In RA, parse that TBSCertificateLogEntry.
- In RA, define a wrapper that can extract useful fields (serial, CommonName, NotBefore, NotAfter) from either an
*x509.Certificateor a TBSCertificateLogEntry. - Use that wrapper to emit the log line.
However, since this requires some refactorings and more implementation I think we shouldn't let it block the slog work. Let's leave the issueMTC call where it is and file an issue for followup as part of MTC implementation.
|
I'm going to close this out and let it be fixed as part of the MTC project. |
4 participants
issueCertificateOuter logs "Authz used for issuance" audit lines up front, with a comment promising they always correlate (via the shared id attr) with a completion line. Two exits broke that promise: the renewal-check (FQDNSetTimestampsForWindow) error return, and the entire MTC branch.
Log "Certificate request - error" on the renewal-check failure path. Move MTC dispatch from issueCertificateOuter into issueCertificateInner, so a failed MTC issuance flows through the same failOrder and "Certificate request - error" handling as classic issuance. Upgrade the MTC success line ("issued MTC") to an audit log so MTC issuance has an audit-tagged completion record. Also fix the comment (which referenced "Certificate request complete", a message renamed during #8606 review).
This PR was generated as part of an audit of #8606 using Claude Fable 5.