More robust SCT creation for duplicate entries#886
Conversation
phbnf
force-pushed
the
dedup-retrieve-entry
branch
from
73d66a1 to
5500b24
Compare
… entry When handling duplicate submissions, retrieve the full stored ctonly.Entry from log storage rather than only extracting the timestamp. Validate that IsPrecert, Certificate, and IssuerKeyHash match the submitted entry to guard against deduplication collisions or antispam driver errors. Once validated, reassign the submitted entry to the stored entry so that all inputs to SCT generation come coherently from storage. Optimize bundle extraction via ExtractEntryFromBundle by skipping over non-target tiles and ignoring extensions and fingerprints while preserving field documentation comments. TAG=agy CONV=50366255-7cb8-46d8-b354-53839f838955
…leaves Update SCT generation so that signSCT accepts rfc6962.CertificateTimestamp directly rather than relying on intermediate data structures. For non-duplicate submissions and signing unit tests, derive the SCT signing input by generating the MerkleTreeLeaf via Tessera (entry.MerkleTreeLeaf) and unmarshalling it using ExtractCertificateTimestampFromLeaf. This guarantees 100% coherence between the data Tessera integrates into the Merkle tree and the data signed in the SCT while eliminating duplicated extension marshalling logic. TAG=agy CONV=50366255-7cb8-46d8-b354-53839f838955
phbnf
force-pushed
the
dedup-retrieve-entry
branch
from
5500b24 to
abfa3e7
Compare
| return http.StatusInternalServerError, []attribute.KeyValue{duplicateKey.Bool(index.IsDup)}, fmt.Errorf("could not resolve duplicate: %v", err) | ||
| } | ||
| if err := entrySCTsMatch(sctInput, entry, index.Index); err != nil { | ||
| return http.StatusInternalServerError, []attribute.KeyValue{duplicateKey.Bool(index.IsDup)}, fmt.Errorf("deduplicated entry in storage does not match submitted entry: %w", err) |
There was a problem hiding this comment.
Do you need the %w for the caller behave correctly, or is %v enough?
| func entrySCTsMatch(sct *rfc6962.CertificateTimestamp, e *ctonly.Entry, idx uint64) error { | ||
| extractedIdx, err := staticct.ParseCTExtensionsBytes(sct.Extensions) | ||
| if err != nil || extractedIdx != idx { | ||
| return fmt.Errorf("index mismatch: stored=%d, expected=%d", extractedIdx, idx) |
There was a problem hiding this comment.
This message seems like it could be misleading if err != nil?
|
|
||
| // ParseCTExtensions parses base64-encoded CTExtensions into an index. | ||
| // Code is inspired by https://github.com/FiloSottile/sunlight/blob/main/tile.go. | ||
| func ParseCTExtensions(ext string) (uint64, error) { |
There was a problem hiding this comment.
nit: could rename to ParseCTExtensionsB64? Looks like the only place this is used is in the hammer (could also just make the hammer do the de-b64)
phbnf
force-pushed
the
dedup-retrieve-entry
branch
3 times, most recently
from
a34a8ff to
a87a0fc
Compare
phbnf
force-pushed
the
dedup-retrieve-entry
branch
from
a87a0fc to
607ba53
Compare
| if sct == nil { | ||
| return errors.New("sct is nil") | ||
| } | ||
| if e == nil { | ||
| return errors.New("entry is nil") | ||
| } |
There was a problem hiding this comment.
Wondering if you can avoid all these nil checks here (and in Sign below)?
Is it ever acceptable for sctInput == nil && err == nil at L524 or for entry at L477?
Could the funcs which return them return by-value?
(Can totally be a follow-up PR!)
There was a problem hiding this comment.
We should never run into these edge cases with the current implementation, but we can't tell for sure what will call this method later, so I addd these defensive checks. I considered passing structs by value but ruled it out to avoid large copies since the cert is in there. After looking into it again, certs wouldn't be copied, only the reference to the byte array. Since copies will be small, it shouldn't be a problem. I've changed the signature of signSCT and DedupFuture. We can dereference the pointer for ctonly.Entry. EntryFromChain uses sync.Pool, so I'd apply extra care before returning a struct rather than a pointer.
I've left checks for sct.PrecertEntry and sct.X509Entry. They are not necessary with the current code paths, but better err on the safe side.
Change ExtractCertificateTimestampFromLeaf, ExtractSCTInputFromBundle, DedupFuture, sctMatchesEntry, and Sign to return and accept rfc6962.CertificateTimestamp and ctonly.Entry by value rather than pointer. This eliminates unnecessary defensive nil checks while preserving sync.Pool entry pooling and avoiding memory allocations during extraction. TAG=agy CONV=fdd8e21c-6548-461f-8ead-5ec8915de98d
2 participants
Towards #884
This PR modifies builds SCTs from the leaf data of deduplicated entries, rather than the submitted entry. These should always match, but this defensive change prevents TesseraCT from signing an SCT that would not correspond to a leaf.
This PR also: