Skip to content

Verify artifact signing time against all timestamps#1381

Merged
jku merged 12 commits intosigstore:mainfrom
ramonpetgrave64:verify-artifact-signing-time
May 16, 2025
Merged

Verify artifact signing time against all timestamps#1381
jku merged 12 commits intosigstore:mainfrom
ramonpetgrave64:verify-artifact-signing-time

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)...

  • @woodruffw or others, please leave comment if you have opinions
  • @ramonpetgrave64 can you leave a comment in #1226 that links here and states that during verification we plan to require that every included established time is within the window -- at least we'll leave a bread crumb trail for someone to follow

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