Skip to content

Verify artifact signing time against all timestamps #1381

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

Merged
merged 12 commits into from
May 16, 2025

Conversation

ramonpetgrave64
Copy link
Contributor

Client support for Rekor V2: sigstore-python

Summary

Resolves #1380

Release Note

  • Timestamps: Verify the signature date against the validity period of both the
    Timestamp Authority or the Transperency Service, if either of such timestamps
    are present in the Bundle. We still require at lease one of such timestamps.

Documentation

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.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.

I spent some time thinking about whether we really want to require all established times to be within the signing cert lifetime -- it sounds like policy decision -- but in the end agreed with you: I think we can require correctness even if the policy only asks for one established time.

The only request is on the language. I'd like to be exact: Timestamps always come from a TSA. integration time is not a real Timestamp it's just a time.

I think using "established times" as the common term is fine as is spelling all the options out: "timestamps or the log integration time"

@jku
Copy link
Member

jku commented May 12, 2025

The only request is on the language. I'd like to be exact: Timestamps always come from a TSA. integration time is not a real Timestamp it's just a datetime.

I suppose another possibility might be to always refer to "TSA timestamps" when talking about those

ramonpetgrave64 and others added 4 commits May 12, 2025 19:57
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.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.

This looks good, thanks for figuring out the test.

I'm still flip flopping on whether we should outright fail when any time is outside the certificate validity as in this PR (or if we should just verify we have at least one valid established time, and not care if other times are outside the certificate window)...

jku
jku previously approved these changes May 13, 2025
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.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.

I like the intent of the new test but currently what happens is

  1. bundle is constructed
  2. bundle is validated (see Bundle._verify())
  3. test modifies bundle by deleting fields
  4. test runs verify

I don't like how the field deletion bypasses the validation. I think storing the new cases as separate assets would solve this issue

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@jku
Copy link
Member

jku commented May 16, 2025

/gcbrun

@jku jku merged commit 30a74ed into sigstore:main May 16, 2025
22 of 23 checks passed
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.

Verifying Signature time when no inclusion promise or integrated time
2 participants