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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

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

Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
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
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
Signed-off-by: Alexis <alexis.challande@trailofbits.com>
@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.

Signed-off-by: Alexis <alexis.challande@trailofbits.com>
@woodruffw
Copy link
Member

/gcbrun

Comment on lines +234 to +239
if len(self._inner.signatures) == 0:
return b""

signature_bytes = self._inner.signatures[0].sig
if not signature_bytes:
return b""
Copy link
Member

Choose a reason for hiding this comment

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

Rather than forwarding these invalid states, we should enforce the invariant that a dsse.Envelope always has exactly one non-empty signature member.

This should be done in the Envelope constructor -- there's an example of this pattern here:

https://github.com/trail-of-forks/sigstore-python/blob/5f23327c1520b757670f6e3aeccda1da66c4805f/sigstore/models.py#L444

Comment on lines +256 to +257
f"Not enough Timestamp validated to meet the Validation "
f"Threshold ({verified_timestamp}/{VERIFY_TIMESTAMP_THRESHOLD})"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"Not enough Timestamp validated to meet the Validation "
f"Threshold ({verified_timestamp}/{VERIFY_TIMESTAMP_THRESHOLD})"
f"Not enough timestamps validated to meet the validation "
f"threshold ({verified_timestamp}/{VERIFY_TIMESTAMP_THRESHOLD})"


return False

def _verify_timestamp_authority(self, bundle: Bundle) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to return the verified timestamp, not the number of verified timestamps.

(It needs to return the verified timestamp, because that becomes the source of trusted time for the rest of the verification flow.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants