Add MtcLogBuilder to cert_builder [chromium/src : main]
Matt Mueller (Gerrit)
Matt Mueller added 9 comments![]( "Open in Gerrit")
std::optional<bssl::UniquePtr<CRYPTO_BUFFER>>Matt Muellernit: can you specify in the comment the meaning of nullopt vs a null UniquePtr vs an empty buffer?
good point, removed the optional here since uniqueptr already can specify that by returnning nullptr.
std::pair<uint64_t, uint64_t> GetActiveLandmarkRange() {Matt Muellernit: make a type alias for landmark indices? It seems error-prone/confusing to use the same type for indices demarcating a subtree (which also seem to variously use either size_t or uint64_t; can we stick with one or the other?) and landmark numbers/indices.
Done
for (const auto& extension_it : extensions_) {Matt Muellernit: can use structured binding
`for (const auto& [extension_name, extension_value] : extensions_)`
Done
size_t split = bit_length(start ^ last) - 1;Matt Muellernit: make these all const
Done
node_cache_[level_index].resize(position_index);Matt Muellernit: maybe add the `value` argument to resize(), just to be more clear/explicit that this is adding default initialized elements of (presumably) std::nullopt?
(no longer relevant after updating to use the new boring MerkleTree design)
std::vector<std::vector<std::optional<bssl::TreeHash>>> node_cache_;Matt MuellerI'm not sure how this is going to be used, but would it make sense for this to be a sparse thing like maybe a map(tuple(level_index, position_index), TreeHash)?
(no longer relevant after updating to use the new boring MerkleTree design)
MtcLogBuilder::MtcLogEntry MtcLogBuilder::MtcLogEntry::NullEntry() {Matt Muelleroptional suggestion: maybe this could return a const reference to a singleton instance, like how GURL::EmptyGURL() works?
added as a TODO
landmarks_.push_back(data_->Size());Matt MuellerShould we CHECK that the landmarks' tree sizes never decrease?
not sure it's worth CHECKing since the interfaces only allow appending to the tree
std::vector<bssl::Subtree> MtcLogBuilder::GetLandmarkSubtrees() {Matt MuellerCould these just be precomputed (maybe when adding every new landmark) and cached so that we don't have to generate them repeatedly?
added that as a TODO
Related details
- Lily Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Harper (Gerrit)
Nick Harper added 4 comments![]( "Open in Gerrit")
std::pair<LandmarkNumber, LandmarkNumber> GetActiveLandmarkRange() {For whatever reason, the phrase "active landmark" sounds to me like it refers to the stuff that will go in the next landmark to be generated (hence "active"), instead of referring to all of the landmarks that have already been created (and are immutable).
if (start == last) {If [start, end) is a valid subtree, we can also return that as a single subtree rather than 2.
const uint64_t left_split = bit_length(~start & mask);I don't understand the function of `~start` here. Can you add some comments explaining the bit operations? (Alternatively, can this be rewritten similar to how `split` was written with bit_length(a XOR b), possibly using `mid` in place of `last`?)
// deduplicate this somehow?One way to dedupe the code is to remove it from MTCAnchor (and have MTCAnchor always return the same synthetic cert regardless of subject).
Related details
- Lily Chen
- Matt Mueller
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lily Chen (Gerrit)
Lily Chen added 1 comment![]( "Open in Gerrit")
- Matt Mueller
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Mueller (Gerrit)
Matt Mueller added 3 comments![]( "Open in Gerrit")
std::pair<LandmarkNumber, LandmarkNumber> GetActiveLandmarkRange() {For whatever reason, the phrase "active landmark" sounds to me like it refers to the stuff that will go in the next landmark to be generated (hence "active"), instead of referring to all of the landmarks that have already been created (and are immutable).
`active` is the term used in the draft (https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-6.3.1-7)
if (start == last) {If [start, end) is a valid subtree, we can also return that as a single subtree rather than 2.
hmm, interesting point. Perhaps the draft should be updated? I'm not clear if there is an implicit "only use this algorithm if it isn't already a valid subtree" here: https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-4.5
It does say the algorithm is for any interval so it seems like it should handle that case in the described algorithm if it's intended.
@davi...@chromium.org thoughts?
I don't understand the function of `~start` here. Can you add some comments explaining the bit operations? (Alternatively, can this be rewritten similar to how `split` was written with bit_length(a XOR b), possibly using `mid` in place of `last`?)
This is based on the sample code from the draft: https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-4.5-5
I'll add the draft reference in a comment.
Related details
- David Benjamin
- Nick Harper
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
if (start == last) {Matt MuellerIf [start, end) is a valid subtree, we can also return that as a single subtree rather than 2.
hmm, interesting point. Perhaps the draft should be updated? I'm not clear if there is an implicit "only use this algorithm if it isn't already a valid subtree" here: https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-4.5
It does say the algorithm is for any interval so it seems like it should handle that case in the described algorithm if it's intended.
@davi...@chromium.org thoughts?
(Haven't looked at the rest, just this question.)
As currently spec'd, the algorithm will actually split valid subtrees into their left and right subtrees. (It's not important for the core certificate verification state whether everyone agrees, but it is important for the negotiation bit because we just specify the subtrees implicitly by landmarks.)
We could make the `end - start == 1` special case be an `is_subtree(start, end)` instead, and it would have worked just as well. I went with this one just because:
- This is an easier operation to implement
- This edge case will approximately never happen in a high-issuance-rate CA, so it doesn't really matter
- If it does happen, since everyone has to budget for two hashes per landmark anyway, we may as well shave 32 bytes and split them down one later since it doesn't cost anything.
Related details
- Matt Mueller
- Nick Harper
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Harper (Gerrit)
Nick Harper voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nick Harper
| Code-Review | +1 |
1 comment
I didn't realize the things I had questions about were things that came from the draft. LGTM with citations to appropriate sections of the draft added.
Related details
- Matt Mueller
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Mueller (Gerrit)
Matt Mueller voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matt Mueller
| Commit-Queue | +2 |
2 comments
std::pair<LandmarkNumber, LandmarkNumber> GetActiveLandmarkRange() {Matt MuellerFor whatever reason, the phrase "active landmark" sounds to me like it refers to the stuff that will go in the next landmark to be generated (hence "active"), instead of referring to all of the landmarks that have already been created (and are immutable).
`active` is the term used in the draft (https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-6.3.1-7)
added a bit more comments and links
if (start == last) {Matt MuellerIf [start, end) is a valid subtree, we can also return that as a single subtree rather than 2.
David Benjaminhmm, interesting point. Perhaps the draft should be updated? I'm not clear if there is an implicit "only use this algorithm if it isn't already a valid subtree" here: https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-4.5
It does say the algorithm is for any interval so it seems like it should handle that case in the described algorithm if it's intended.
@davi...@chromium.org thoughts?
(Haven't looked at the rest, just this question.)
As currently spec'd, the algorithm will actually split valid subtrees into their left and right subtrees. (It's not important for the core certificate verification state whether everyone agrees, but it is important for the negotiation bit because we just specify the subtrees implicitly by landmarks.)
We could make the `end - start == 1` special case be an `is_subtree(start, end)` instead, and it would have worked just as well. I went with this one just because:
- This is an easier operation to implement
- This edge case will approximately never happen in a high-issuance-rate CA, so it doesn't really matter
- If it does happen, since everyone has to budget for two hashes per landmark anyway, we may as well shave 32 bytes and split them down one later since it doesn't cost anything.
- David Benjamin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: net/test/cert_builder.h
Insertions: 19, Deletions: 1.
@@ -494,15 +494,31 @@
bool AdvanceLandmark();
// Returns the range, inclusive, of active landmark numbers.
+ //
+ // Active landmarks are those that may contain un-expired certificates
+ // (https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-6.3.1-7)
+ //
+ // This implementation doesn't directly care about expiration or validity
+ // periods and leaves those details to the test to control. It does not
+ // currently support advancing the minimum landmark, but that could be added
+ // if a test needs it.
+ //
+ // Landmark numbers can be used to form Trust Anchor IDs
+ // https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-6.3.1-4
std::pair<LandmarkNumber, LandmarkNumber> GetActiveLandmarkRange() {
return {0, landmarks_.size() - 1};
}
// Returns the currently active landmark subtrees.
+ //
+ // https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#section-6.3.1-7
std::vector<bssl::Subtree> GetLandmarkSubtrees();
// Returns the subtrees and subtree hashes for the currently active
- // landmarks.
+ // landmarks. This information is needed by the client to verify
+ // signatureless certificates.
+ //
+ // https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#trusted-subtrees
std::vector<bssl::TrustedSubtree> GetLandmarkSubtreeHashes();
// Add entry to the log and return the index of the entry.
@@ -523,6 +539,8 @@
// Returns the DER-encoded certificate for entry `index`, which must be
// included in the active landmark subtrees. Returns nullopt otherwise.
+ //
+ // https://davidben.github.io/merkle-tree-certs/draft-davidben-tls-merkle-tree-certs.html#name-constructing-signatureless-
std::optional<std::vector<uint8_t>> CreateSignaturelessCertificate(
LogIndex index);
```
Change information
Add MtcLogBuilder to cert_builder
Simple testing implementation of a Merkle Tree Certificate log.
Only supports signatureless certificates currently.
- M net/test/cert_builder.cc
- M net/test/cert_builder.h
Code-Review: +1 by Nick Harper
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |