Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timestamp Authority Verification #1206

Merged
merged 17 commits into from
Nov 15, 2024
Merged

Conversation

DarkaMaul
Copy link
Contributor

Summary

This PR verifies signed timestamp within a bundle using Timestamp Authorities provided within the Trusted Root.

Release Note

Added:

  • Signed timestamps embedded in a bundle are now automatically verified.

Documentation

The documentation is already written in Verifying Timestamp

/cc @woodruffw @facutuesca

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is already written in Verifying Timestamp

At least for the PR/issue I don't think it's quite true. It's not obvious to me how this feature is going to be used.

  • Is there spec for what clients SHOULD/MUST do with timestamps in bundles? If not how is the policy decided?
  • how do we end up with bundles with timestamps? Does the signer need to actively decide to include them?
  • if there are timestamps in the bundle, how does the verifier get the cert chain? (theoretically from the trusted root sure, but is there a practical path there? is this just for private instances?)
  • who sets the verification threshold policy? If there can be a variable number of timestamps in the bundle and the trusted root can contain a variable number of TSAs, how can you choose a reasonable value for threshold?

I realize these are not really questions that relate to the PR but ... I did try to start a discussion in the issue earlier already. I left some actual code comments but those are more to make sure I understand the intent correctly.

I think this feature would really benefit from a design doc (by which I mean just a detailed comment in the issue describing how it's all going to be used in the future): I don't think this client should just implement things because cosign does. Cosign is famous for kitchen sinks (with 6 water temp knobs).

sigstore/verify/verifier.py Outdated Show resolved Hide resolved
sigstore/verify/verifier.py Show resolved Hide resolved
sigstore/verify/verifier.py Outdated Show resolved Hide resolved
@DarkaMaul
Copy link
Contributor Author

  • Is there spec for what clients SHOULD/MUST do with timestamps in bundles? If not how is the policy decided?

According to the Sigstore Client Spec, it should be decided by the policy.

If the verification policy uses the Timestamping Service, the Verifier MUST verify the timestamping response using the Timestamping Service root key material, as described in Spec: Timestamping Service, with the raw bytes of the signature as the timestamped data. The Verifier MUST then extract a timestamp from the timestamping response. If verification or timestamp parsing fails, the Verifier MUST abort.

The current implementation follows a minimal approach - verifying timestamps only when they are provided. While this allows for a non-disruptive initial integration that maintains compatibility with existing workflows, I agree we should make our policy more explicit here.

(Note: the document linked here is restricted and I don't have access to it)

  • how do we end up with bundles with timestamps? Does the signer need to actively decide to include them?

Currently, timestamps are included through explicit opt-in by the signer. For example, using cosign, you would specify a timestamp server URL:

cosign sign-blob --yes --bundle bundle.sigstore --new-bundle-format=true \
--timestamp-server-url="http://localhost:3000/api/v1/timestamp" \  # here
--oidc-issuer "https://oauth2.sigstage.dev/auth" --fulcio-url "https://fulcio.sigstage.dev" --rekor-url "https://rekor.sigstage.dev" ./bundle.txt

While timestamps currently require explicit opt-in, we could consider making them opt-out in the future. As a next step, I plan to implement signing a bundle with timestamping support in sig store-python as a follow-up PR.

  • if there are timestamps in the bundle, how does the verifier get the cert chain? (theoretically from the trusted root sure, but is there a practical path there? is this just for private instances?)

You raise a valid point about the certificate chain verification path. While theoretically, the chain can be sourced from the trusted root (and there are fields specified for it in the bundle format), we need to ensure they are used within the community.

  • who sets the verification threshold policy? If there can be a variable number of timestamps in the bundle and the trusted root can contain a variable number of TSAs, how can you choose a reasonable value for threshold?

The verification threshold is a parameter specified in the spec. In practice, sigstore-go currently uses a fixed value of 1. This simplifies the initial implementation, though we should document this decision and its implications more explicitly.

I realize these are not really questions that relate to the PR but ... I did try to start a discussion in the issue earlier already. I left some actual code comments but those are more to make sure I understand the intent correctly.
I think this feature would really benefit from a design doc (by which I mean just a detailed comment in the issue describing how it's all going to be used in the future): I don't think this client should just implement things because cosign does. Cosign is famous for kitchen sinks (with 6 water temp knobs).

You're absolutely right, and I apologize for missing the earlier discussion in the issue. Your point about not simply mirroring cosign's features without clear design rationale is well-taken. Let's continue the discussion there.

@woodruffw
Copy link
Member

/gcbrun

@woodruffw woodruffw added the enhancement New feature or request label Nov 14, 2024
@woodruffw
Copy link
Member

/gcbrun

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: I moved this to sigstore._internal since these don't need to be public APIs quite yet.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member

/gcbrun

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DarkaMaul, nice work!

@woodruffw woodruffw merged commit 918f867 into sigstore:main Nov 15, 2024
23 checks passed
@DarkaMaul DarkaMaul deleted the dm/tsp branch November 15, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants