[MTC] Fetch subtree cosignatures and populate AddTBS responses with them. - #1132
Conversation
5092e3c to
4d6c4d4
Compare
phbnf
changed the title
phbnf
changed the title
|
I forgot to mention in the description, |
| k := witnessKey{name: s.Name, keyHash: keyHash} | ||
| // SPEC: draft-ietf-plants-merkle-tree-certs section 6.2. | ||
| // "An MTCProof parser MUST reject the input if there are duplicate cosigner_id values" | ||
| if _, ok := verifiedSubtreeSigs[k]; ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The logic here verifies the witness keys instead of cosigner ID. Is the witness key always one-to-one mapped to the cosigner ID? If multiple keys are mapped to the same cosigner ID due to key rotation, the rejection check requirement from the specification seems to be wrong.
There was a problem hiding this comment.
s.Name() is the cosigner ID, I've added a comment. We already verify that it is a valid cosigner ID on line 44.
| if len(gw.witnesses) == 0 { | ||
| if gw.policy.Satisfied(rawCp) { | ||
| return nil, nil | ||
| } | ||
| return nil, ErrPolicyNotSatisfied | ||
| } |
There was a problem hiding this comment.
Is this part of the TODO?
If a witness group consists of non-subtree verifiers, the len(gw.witnesses) is 0, the logic will then go to return nil, nil. It is possible to bypass the policy.
There was a problem hiding this comment.
You are correct, and I think it is fine for now. I've added a TODO.
For now, it is the responsibility of log operators to set a valid policy for their operations with ML-DSA verifiers.
Further down the line, I don't think it should be possible to start an MTC log with a policy that lists non-subtree verifiers. It shouldn't be possible to start one with a policy that doesn't comply with Chrome requirements either. In the meantime, I've put TODOs in main.go. I don't really know where we'll enforce that, maybe this will be a special policy parser. There's also a TODO and warning if not enough signatures have been collected.
| } | ||
| n := unverified.Note | ||
|
|
||
| // reconstructCp is used for policy checking. |
There was a problem hiding this comment.
Is it reconstructedCp or reconstructCp?
There was a problem hiding this comment.
It's reconstructedCp, fixed, thanks.
| // "Standalone certificates MUST have at least 2 cosignatures. One of these | ||
| // MUST be from the MTC CA Operator, and one MUST be from a Mirroring | ||
| // Cosigner recognized by Chrome and not operated by the MTC CA Operator." | ||
| slog.WarnContext(ctx, "collected less than 2 subtree signatures", slog.Int("num_sigs", numSigs)) |
There was a problem hiding this comment.
| slog.WarnContext(ctx, "collected less than 2 subtree signatures", slog.Int("num_sigs", numSigs)) | |
| slog.WarnContext(ctx, "Collected less than 2 subtree signatures", slog.Int("num_sigs", numSigs)) |
| "slices" | ||
| "sync" | ||
|
|
||
| f_note "github.com/transparency-dev/formats/note" |
There was a problem hiding this comment.
In Go, package names must be concise and use only lowercase letters and numbers (e.g., k8s, oauth2). Multi-word package names should remain unbroken and in all lowercase (e.g., tabwriter instead of tabWriter, TabWriter, or tab_writer).
https://google.github.io/styleguide/go/decisions.html#package-names
There was a problem hiding this comment.
I'm following the pattern we've used in this repo so far, https://github.com/search?q=repo%3Atransparency-dev%2Ftessera+f_note&type=code. Maybe we can change this in a followup PR?
2 participants
Towards #945.
With this PR, an MTC log requests subtree cosignatures when processing
AddTBS()requests, and inserts them accordingly in theMTCProof.For the time being, only accept subtree cosignatures from witnesses that have also cosigned a checkpoint.
This design allows two things:
I expect that some of this implementation will change in the near future, as we iterate on the mirror client with request deduplication against multiple instances, and maybe providing a way to pass all checkpoint signatures to the MTC log.