Add option to unify signed and log timestamps #51

haydentherapper · 2023-12-18T23:51:59Z

Summary

This adds the WithObserverTimestamps option to verify that a threshold is
met using both RFC3161 signed timestamps and log integrated timestamps.

This also adds WithIntegratedTimestamps to verify a threshold for log integrated
timestamps.

This updates the verification API for the log to only verify log
inclusion proofs or SETs, so that log and timestamp verification are not
conflated.

This also fixes a bug where the integrated time is used with an
inclusion proof. This is not safe to do, since the integrated time is
not authenticated metadata without a SignedEntryTimestamp signature.

Fixes #44

Release Note

Documentation

haydentherapper · 2023-12-19T00:02:22Z

@steiza @kommendorkapten @phillmv PTAL (at commit 1a0fa2b, PR will be rebased once the other PR is merged)

The summary of the various options is in #44 (comment)

This is a WIP, looking for early comments. Remaining TODOs:

Fix validation logic for what combo of options are allowed
Add tests for WithObserverTimestamp
Clean up TODOs I left in code (documentation)
Separate PR: ~~Add configuration to not count SETs towards log threshold (Default: false for v0.2 bundles, true for v0.1)~~

Also, this fixes a bug during log entry verification. When an inclusion proof is verified, the integrated timestamp was marked as verified. This is unsafe, because the integrated timestamp is unauthenticated metadata. It can only be trusted when signed over in the case of an SET. Given this library is not widely used, I don't think this needs a CVE (unless you think otherwise), but we should at least ping those who we know use this to upgrade.

pkg/verify/signed_entity.go

pkg/verify/tlog_test.go

kommendorkapten

Looks good 🚀

haydentherapper · 2024-01-02T17:30:28Z

Thank you @kommendorkapten for the discussions and reviews!

@steiza any comments?

Like sigstore#45, as long as the threshold of expected timestamps is met, then verification should succeed. Otherwise, entries without trust root material should be skipped. One benefit of having the log key ID be used to look up the correct trust root material is that we can still error out if the signature is invalid. Ref: sigstore#43 Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

This adds the WithObserverTimestamp option to verify that a threshold is met using both RFC3161 signed timestamps and log integrated timestamps. This updates the verification API for the log to only verify log inclusion proofs or SETs, so that log and timestamp verification are not conflated. This also fixes a bug where the integrated time is used with an inclusion proof. This is not safe to do, since the integrated time is not authenticated metadata without a SignedEntryTimestamp signature. Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper · 2024-01-03T17:33:04Z

Rebasing against HEAD, no changes besides go.mod updates (https://github.com/sigstore/sigstore-go/compare/0c43326f2ecf7d6d6a294c9391ff7831cb173bcd..8684030bcc34417d4d61e94c804e8e3a6ca21f12)

steiza

I think this change gives us the flexibility to cover almost any use case.

I do worry about the complexity of the interface, but I think for many (most?) users, they should just use WithObserverTimestamps() and not need to dive into WithIntegratedTimestamps() vs WithSignedTimestamps().

One minor comment, but I don't feel strongly that the current implementation need to change.

steiza · 2024-01-03T19:36:41Z

cmd/conformance/main.go

@@ -237,7 +237,7 @@ func main() {

 		// Check bundle and trusted root for Tlog information
 		if len(tr.TlogAuthorities()) > 0 && b.HasInclusionPromise() {
-			verifierConfig = append(verifierConfig, verify.WithTransparencyLog(1))
+			verifierConfig = append(verifierConfig, verify.WithTransparencyLog(1), verify.WithObserverTimestamps(1))


Minor: In trying to make sure I understand this pull request, I'm wondering if this should be verify.WithIntegratedTimestamps(1) instead.

Generally, I think we want people to use WithObserverTimestamps() and not have to reason about the specifics of WithIntegratedTimestamps() or WithSignedTimestamps().

But I thought what we were saying on #42 is that the conformance test will infer the verification policy from the trust root, and if the trust root has tlogs entries, we should rely on them both for inclusion and for establishing a timestamp?

I guess when inferring the policy from the trust root, there isn't a way to differentiate today. This isn't a problem really, but it does highlight the complexity of the problem space generally!

You're right, this should be WithIntegratedTimestamps since this is inferring a policy based on if a promise is present. I'll update in a follow up.

In order to infer WithObserverTimestamps, which would be more permissive, it would probably look something like a check for either bundled timestamps or SETs. We could do that here, but we can be more precise with WithSignedTimestamps and WithIntegratedTimestamps.

haydentherapper · 2024-01-03T20:39:56Z

Thanks for the review @steiza! Agreed that the goal is that most if not all users would leverage WithObserverTimestamps. The other options would be for those who have specific requirements, eg only trusting a timestamp authority. At some point in the future, we might be able to deprecate WithIntegratedTimestamps if the log requires timestamps on entry inclusion.

Follow up from sigstore#51 Fixes sigstore#62 Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Follow up from #51 Fixes #62 Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper marked this pull request as draft December 18, 2023 23:52

haydentherapper marked this pull request as ready for review December 19, 2023 23:54

haydentherapper changed the title ~~WIP: Add option to unify signed and log timestamps~~ Add option to unify signed and log timestamps Dec 19, 2023

haydentherapper mentioned this pull request Dec 19, 2023

Add configuration to not count SETs towards log threshold #54

Closed

kommendorkapten reviewed Dec 21, 2023

View reviewed changes

pkg/verify/signed_entity.go Show resolved Hide resolved

kommendorkapten reviewed Dec 21, 2023

View reviewed changes

pkg/verify/tlog_test.go Outdated Show resolved Hide resolved

kommendorkapten previously approved these changes Dec 21, 2023

View reviewed changes

kommendorkapten mentioned this pull request Dec 21, 2023

Added an option for a more generic observer time, and aligned with v0.2 sigstore/protobuf-specs#179

Merged

haydentherapper mentioned this pull request Jan 2, 2024

Don't fail on Rekor entry verification for untrusted entries #47

Merged

haydentherapper mentioned this pull request Jan 2, 2024

Add flag for observer timestamps to OCI example #62

Closed

haydentherapper added 6 commits January 3, 2024 17:29

Address comments

b890502

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Fix conformance test

65e1e36

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Add WithIntegratedTimestamps

1aeef18

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Complete TODOs, add tests

8684030

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper dismissed kommendorkapten’s stale review via 8684030 January 3, 2024 17:32

haydentherapper force-pushed the refactor-rekor-ts branch from 0c43326 to 8684030 Compare January 3, 2024 17:32

steiza approved these changes Jan 3, 2024

View reviewed changes

haydentherapper merged commit 44c7c85 into sigstore:main Jan 3, 2024
8 checks passed

haydentherapper deleted the refactor-rekor-ts branch January 3, 2024 20:42

haydentherapper added a commit to haydentherapper/sigstore-go that referenced this pull request Jan 3, 2024

Update policy inference, add flags for OCI example

6fcf57a

Follow up from sigstore#51 Fixes sigstore#62 Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper mentioned this pull request Jan 3, 2024

Update policy inference, add flags for OCI example #67

Merged

haydentherapper added a commit to haydentherapper/sigstore-go that referenced this pull request Jan 4, 2024

Update policy inference, update flags for examples

e187ff9

Follow up from sigstore#51 Fixes sigstore#62 Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper added a commit that referenced this pull request Jan 5, 2024

Update policy inference, update flags for examples (#67)

1195bd5

Follow up from #51 Fixes #62 Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to unify signed and log timestamps #51

Add option to unify signed and log timestamps #51

haydentherapper commented Dec 18, 2023 •

edited

Loading

haydentherapper commented Dec 19, 2023 •

edited

Loading

kommendorkapten left a comment

haydentherapper commented Jan 2, 2024

haydentherapper commented Jan 3, 2024 •

edited

Loading

steiza left a comment

steiza Jan 3, 2024

haydentherapper Jan 3, 2024

haydentherapper commented Jan 3, 2024

Add option to unify signed and log timestamps #51

Add option to unify signed and log timestamps #51

Conversation

haydentherapper commented Dec 18, 2023 • edited Loading

Summary

Release Note

Documentation

haydentherapper commented Dec 19, 2023 • edited Loading

kommendorkapten left a comment

Choose a reason for hiding this comment

haydentherapper commented Jan 2, 2024

haydentherapper commented Jan 3, 2024 • edited Loading

steiza left a comment

Choose a reason for hiding this comment

steiza Jan 3, 2024

Choose a reason for hiding this comment

haydentherapper Jan 3, 2024

Choose a reason for hiding this comment

haydentherapper commented Jan 3, 2024

haydentherapper commented Dec 18, 2023 •

edited

Loading

haydentherapper commented Dec 19, 2023 •

edited

Loading

haydentherapper commented Jan 3, 2024 •

edited

Loading