CT static API support #73
Conversation
AlCutter
force-pushed
the
ct_static_api_support
branch
8 times, most recently
from
c0989b7 to
46930b4
Compare
AlCutter
force-pushed
the
ct_static_api_support
branch
2 times, most recently
from
1e70afd to
7ef226c
Compare
ct_only.go
Outdated
| type Storage interface { | ||
| // Add should duably assign an index to the provided Entry, and return it. | ||
| // | ||
| // Implementations MUST call the AssignIndex() method on the entry before marshaling and persisting it. |
There was a problem hiding this comment.
What is AssignIndex referring to?
There was a problem hiding this comment.
Good spot, that was a function from an earlier incarnation which went away.
I've updated the comments to remove the reference.
entry.go
Outdated
| } | ||
|
|
||
| // indexFunc will be called when an index is assigned to the entry. |
There was a problem hiding this comment.
I think this function MUST NOT modify the assigned index?
There was a problem hiding this comment.
Added a comment to that effect.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
- Coverage 35.80% 33.33% -2.48%
==========================================
Files 16 18 +2
Lines 1363 1440 +77
==========================================
- Hits 488 480 -8
- Misses 801 888 +87
+ Partials 74 72 -2 ☔ View full report in Codecov by Sentry. |
AlCutter
force-pushed
the
ct_static_api_support
branch
from
a805415 to
dd4b26e
Compare
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package tessera |
There was a problem hiding this comment.
tessera.Storage is somewhat independent from CT, but is defined inside ct_only.go. It seems a bit odd. Would it make sense to define NewCertificateTransparencySequencedWriter and convertCTEntry in their own package?
There was a problem hiding this comment.
That interface is there currently just because there's not a corresponding tessera.NewSequencedWriter yet.
Ideally the CT stuff would be in its own package, but to prevent other new use-cases from using these patterns it needs be in here to do "private" stuff internal to Entry when setting those marshaling funcs.
ct_only.go
Outdated
| type Storage interface { | ||
| // Add should duably assign an index to the provided Entry, and return it. | ||
| // | ||
| // Implementations MUST call MarshalBundleData method on the entry before marshaling and persisting it. |
There was a problem hiding this comment.
This now reads a bit funny because we must call the marshaling function before marshaling it? Maybe just remove the second marshaling?
There was a problem hiding this comment.
d'oh, fixed - ta.
AlCutter
force-pushed
the
ct_static_api_support
branch
from
272b47e to
d1fc00d
Compare
|
Summarizing our conversation here for the record. The logic makes sense to me, so I'll approve this PR. We could improve naming though to make it easier to read the code: |
|
This PR enables Tessera to support a CT Static API personality.
There's broadly two things which are necessary to be able to support that personality:
EntryBundleis different and incompatibletlog-tiles specifies a
length/valuescheme, whereas CT Static API specifies justvalueand relies on the fact thatvaluein this case is self-describing.The SCT returned to the submitter contains the sequence number of the entry in the log in the
CtExtensionsfield of the SCT struct. What's not obvious from the spec is that this must also be part of the Merkle leaf (otherwise compatibility with RFC6962 would be broken).This means that we don't actually know the
LeafDataor itsMerkleLeafHashuntil we've sequenced it.Beyond enabling a CT Static API personality to be built, other large goals for this PR are:
Toward #41