bundle: cap raw TlogEntries length before per-entry parse#630
Conversation
Move the existing maxAllowedTlogEntries = 32 cap earlier in the parse path so the bound is enforced on the raw decoded slice length before each entry is individually parsed. Previously the constant was consulted only in pkg/verify/tlog.go inside VerifyTlogEntry, which meant (*Bundle).TlogEntries() (and the validate() call that runs during Bundle.UnmarshalJSON) would iterate and call tlog.ParseTransparencyLogEntry for every element of VerificationMaterial.TlogEntries before the count check fired. Moving the check to the top of TlogEntries() keeps the cap intact and keeps the parse step's work proportional to the configured limit rather than to the bundle's declared entry count. Also lifts the constant (and the analogous maxAllowedTimestamps cap) into a new pkg/limits package so pkg/bundle and pkg/verify reference the same source of truth. Values are unchanged. This is a follow-up to the direction discussed in sigstore#567. Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
| // MaxAllowedTlogEntries is the upper bound on the number of transparency | ||
| // log entries a bundle may carry. The value matches the historical cap | ||
| // enforced inside VerifyTlogEntry. | ||
| const MaxAllowedTlogEntries = 32 |
There was a problem hiding this comment.
As I mentioned in the other PR, these shouldn't be publicly exported values.
There was a problem hiding this comment.
Good call — moved the package from pkg/limits/ to internal/limits/ in 463da5a. The constants remain uppercase since cross-package access requires it, but the internal/ location keeps them off the public Go API surface so external importers cannot depend on them. The 4 import paths in pkg/bundle/{bundle,bundle_test}.go and pkg/verify/{tlog,tsa}.go were updated to point at the new path; TestTlogEntriesCountCap and TestTlogEntriesCountAtCap pass.
Move the per-bundle upper bounds package from pkg/limits/ to internal/limits/. The constants remain uppercase since cross-package access requires it, but the internal/ location keeps them off the public Go API surface as requested in PR review. The single source of truth and the parse/verify symmetry are preserved; only the import path changes. Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
Hayden-IO
left a comment
There was a problem hiding this comment.
Left some comments on the code comments. These comments are fairly typical for LLM output in that they are either not needed or overly descriptive, so please review that they are meaningful and in line with Go best practices.
| // between the bundle parse path and the verify path. Keeping a single | ||
| // source of truth avoids drift between the two layers and lets the parse | ||
| // step short-circuit on counts that the verify step would later reject. | ||
| package limits |
There was a problem hiding this comment.
This package comment doesn't need to describe this being a single source of truth.
|
|
||
| // MaxAllowedTlogEntries is the upper bound on the number of transparency | ||
| // log entries a bundle may carry. The value matches the historical cap | ||
| // enforced inside VerifyTlogEntry. |
There was a problem hiding this comment.
This doesn't need to mention a "historical cap", same as below, as that isn't a meaningful description.
| // before iterating and parsing each one. The same cap is later | ||
| // re-checked in pkg/verify, so enforcing it here keeps the parse | ||
| // step's work proportional to the configured limit instead of to | ||
| // the bundle's declared entry count. |
There was a problem hiding this comment.
Code is self-describing, not sure this needs a comment
| } | ||
|
|
||
| func TestTlogEntriesCountCap(t *testing.T) { | ||
| // Build a slice of entries that exceeds the shared cap. The individual |
There was a problem hiding this comment.
The test code is self-describing, this comment isn't adding anything.
| } | ||
|
|
||
| func TestTlogEntriesCountAtCap(t *testing.T) { | ||
| // At exactly the cap the count check must not fire. The slice here |
Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
|
Thanks @Hayden-IO. Pushed 34db922 trimming the redundant comments per the five threads:
|
Follow-up to #567 — moves the existing
maxAllowedTlogEntries = 32cap earlier in the parse path so the bound is enforced on the raw decoded slice length before each entry is individually parsed.Today the constant is consulted in
pkg/verify/tlog.go:VerifyTlogEntry, which means(*Bundle).TlogEntries()(and thevalidate()call that runs duringBundle.UnmarshalJSON) iterate and calltlog.ParseTransparencyLogEntryfor every element ofVerificationMaterial.TlogEntriesbefore the count check fires. Moving the check to the start ofTlogEntries()keeps the cap intact and keeps the parse step's work proportional to the configured limit rather than to the bundle's declared entry count.Also lifts the constant (and the analogous
maxAllowedTimestampscap) into a newpkg/limitspackage sopkg/bundle/bundle.goandpkg/verify/{tlog,tsa}.goreference the same source of truth. Values are unchanged (32 in both cases).Summary
pkg/limitswith exportedMaxAllowedTlogEntriesandMaxAllowedTimestamps(both 32, matching today's behavior).(*Bundle).TlogEntries()so the cap fires before the per-entry parse loop.pkg/verify/tlog.goandpkg/verify/tsa.goto reference the shared constants.pkg/bundle/bundle_test.goasserting the early return surfaces the count-based error (entries are intentionally bare so per-entry parse would otherwise error first).Test plan
go test ./pkg/bundle/... ./pkg/verify/... ./pkg/limits/...— all existing tests pass; newTestTlogEntriesCountCapandTestTlogEntriesCountAtCappass.go vet ./pkg/bundle/... ./pkg/verify/... ./pkg/limits/...clean.pkg/verifyconstants were unexported; the new shared constants are additive.Notes for reviewers:
SignedEntity.protojson.Unmarshal, so it does not cap allocations during JSON decoding itself; callers should still enforce a bundle byte-size limit at ingestion (unchanged from current guidance).