ca: log cert signing using JSON objects #7742
Conversation
Previously, we logged csr.DNSNames along with the precertificate issuance events, but this was potentially misleading. For instance, if a CSR contained only a CN and no SANs, this set would be empty. Instead, log the uniquified, lowercased, sorted SANs that are actually issued. Also, emit precert=[] in issuePrecertificateInner, which is consistent with the field name used when the final certificate is issued.
This makes the log events easier to parse, and makes it easier to consistently use the correct fields from the issuance request. Also, reduce the number of fields that are logged on error events. Logging just the serial and the error in most cases should suffice to cross-reference the error with the item that we attempted to sign.
There was a problem hiding this comment.
Generally I like this approach. I'm a much bigger fan of logging a single consistent json object than our custom =[...] format. I have a few tiny questions/concerns about this precise approach, but nothing that would stop me from saying LGTM on this change essentially as-is.
- Is the change to the ca.json log levels intentional? Or is that just from your own debugging?
- I'd like to chat about the pros/cons of using
logEvent := map[string]any{...}versus usingvar logEvent struct{...}{...}or even defining atype logEvent struct {...}higher up in the file. Using a map certainly feels closer to the raw json that it will eventually produce, but also leads to IMO-unidiomatic code likedelete(logEvent, "csr")and the hand-writtenIssuanceRequest.MarshalJSON()method which has no mechanism to guarantee that a newly-added struct field gets reflected in the final output json.
Just for personal testing / if anyone else wanted to try this branch locally.
I think the
FWIW, this is the idiomatic (and only) way to delete an element from a map. And I don't think deleting elements from maps is un-idiomatic. In the struct case, it would be something like
This one is a little trickier. Probably what we need here is to define a newtype that wraps some of the fields - like |
I think that deleting elements with a hard-coded key (as opposed to, say, a key that you got from a
|
|
I've updated the top comment to show the most recent iteration of the JSON output. |
This makes the log events easier to parse, and makes it easier to consistently use the correct fields from the issuance request.
Also, reduce the number of fields that are logged on error events. Logging just the serial and the error in most cases should suffice to cross-reference the error with the item that we attempted to sign.
Some example log output from an integration test run, with the JSON manually prettified (using jq):
One downside is that this increases the total log size (10kB above, vs 7kB from a similar production issuance) due in part to more repetition. For example, both the "signing cert" and "signing cert success" log lines include the full precert DER.
Note that our long-term plan for more structured logs is to have a unique event id to join logs on, which can avoid this repetition. But since we don't currently have convenient ways to do that join, some duplication (as we currently have in the logs) seems reasonable.