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.

Sign up for GitHub

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

Jump to bottom

Add v0.2 bundle tests #112

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Add v0.2 bundle tests #112

merged 1 commit into from
Dec 11, 2023

Conversation

bdehamer
Copy link
Contributor

@bdehamer bdehamer commented Dec 9, 2023 •
edited
Loading

Adds a handful of new bundle verification tests to the suite:

  • Happy path for v0.2 bundle with DSSE payload
  • Error case where the signing certificate was issued outside the validity window for the root CA certificate
  • Error case where the TLog entry is missing an inclusion proof
  • Error case where the integrated time of the TLog entry falls outside the validity window for the signing certificate
  • Error case where the canonicalized body of the TLog entry does NOT match the signed content
  • Error case where the (TSA-issued) signed timestamp falls outside the validity window for the signing certificate

All of these bundles were signed with a private Sigstore instance and must be verified using a custom trusted_root.json.

@bdehamer bdehamer force-pushed the bdehamer/dsse-bundle-verify branch from b1da294 to 8025e0a Compare December 9, 2023 01:28
@bdehamer bdehamer marked this pull request as ready for review December 9, 2023 15:41
@bdehamer
add dsse bundle tests
2d7481f 
Signed-off-by: Brian DeHamer <bdehamer@github.com>
@bdehamer bdehamer force-pushed the bdehamer/dsse-bundle-verify branch 3 times, most recently from db8862a to 2d7481f Compare December 9, 2023 23:50
@woodruffw
Copy link
Member

Thanks @bdehamer!

@woodruffw woodruffw requested a review from tnytown December 11, 2023 15:03
@woodruffw woodruffw added the component:tests Unit and integration tests label Dec 11, 2023
@woodruffw woodruffw self-requested a review December 11, 2023 15:03
woodruffw
woodruffw approved these changes Dec 11, 2023
View reviewed changes
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.

I did a quick pass, and LGTM! Tagging @tnytown for an additional review.

Just for xrefs: sigstore/sigstore-python#804 will add DSSE support to sigstore-python, once complete.

@bdehamer
Copy link
Contributor Author

FYI @phillmv @steiza

tnytown
tnytown approved these changes Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@tnytown tnytown left a comment

Choose a reason for hiding this comment

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

All of these bundles were signed with a private Sigstore instance and must be verified using a custom trusted_root.json.

This is sigstore/sigstore-python#821, right?

I didn't check the test assets, but the harness additions look good to me!

@woodruffw
Copy link
Member

This is sigstore/sigstore-python#821, right?

Yep, that's right. I'd like to get sigstore/sigstore-python#814 merged in and then move on that.

@woodruffw woodruffw merged commit 303dd4b into sigstore:main Dec 11, 2023
6 of 7 checks passed
steiza
steiza reviewed Dec 11, 2023
View reviewed changes
test/test_bundle.py
Test the happy path of verification for DSSE bundle w/ custom trust root
"""
materials: BundleMaterials
input_path, materials = make_materials_by_type("d.stmt.json", BundleMaterials)
Copy link
Member

Choose a reason for hiding this comment

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

I know I'm a bit late, but I don't think is quite right.

test/assets/d.stmt.json should be the artifact whose hash matches what's in the in-toto statement, not the in-toto statement itself.

sigstore-go says this test should fail, because the test/assets/d.stmt.good.sigstore DSSE payload says the hash of the artifact should be sha512:90f22..., and the hash of the current test/assets/d.stmt.json is sha512:9163d....

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 you're right -- this is an unfortunate consequence of having sigstore-python be the only "acceptance" suite currently running 😅

Would you mind sending a PR for this? Otherwise I can poke at it when I have a free moment.

Copy link
Member

Choose a reason for hiding this comment

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

Sure! See #114.

This might have uncovered another bug in test_verify_rejects_bad_tsa_timestamp, although I'm not sure yet if the bug is in sigstore-go or the conformance test!

steiza added a commit to sigstore/sigstore-go that referenced this pull request Dec 11, 2023
@steiza
Support Fulcio certificate "chains" that just have a root
c67baca 
sigstore/sigstore-conformance#112 includes
confromance tests with a mock Sigstore where there are no Fulcio
intermediate certificates.

Signed-off-by: Zach Steindler <steiza@github.com>
@steiza steiza mentioned this pull request Dec 11, 2023
Merged
@woodruffw woodruffw mentioned this pull request Dec 11, 2023
Open
@steiza steiza mentioned this pull request Dec 11, 2023
Merged
codysoyland pushed a commit to sigstore/sigstore-go that referenced this pull request Dec 12, 2023
@steiza
Support Fulcio certificate "chains" that just have a root (#40)
b5aa2fb 
* Support Fulcio certificate "chains" that just have a root

sigstore/sigstore-conformance#112 includes
confromance tests with a mock Sigstore where there are no Fulcio
intermediate certificates.

Signed-off-by: Zach Steindler <steiza@github.com>

* Clarify leaf CT certificate

Signed-off-by: Zach Steindler <steiza@github.com>

---------

Signed-off-by: Zach Steindler <steiza@github.com>
steiza added a commit to sigstore/sigstore-go that referenced this pull request Dec 12, 2023
@steiza
Have conformance test require signed timestamps for bundles v02
efdbf47 
It seems like this is the behavior that
`test_verify_rejects_bad_tsa_timestamp` is assuming, that was added in
sigstore/sigstore-conformance#112.

Signed-off-by: Zach Steindler <steiza@github.com>
@steiza steiza mentioned this pull request Dec 12, 2023
Merged
steiza added a commit to sigstore/sigstore-go that referenced this pull request Dec 14, 2023
@steiza
Use the trust root and bundle contents to construct verification poli…
0946840 
…cy (#42)

`test_verify_rejects_bad_tsa_timestamp`, which was added in
sigstore/sigstore-conformance#112, expects us reject bundles that have a bad TSA timestamp when the trust root has TSA information in it.

---------

Signed-off-by: Zach Steindler <steiza@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw woodruffw approved these changes

@steiza steiza steiza left review comments

@haydentherapper haydentherapper haydentherapper approved these changes

@tnytown tnytown tnytown approved these changes

Assignees

@bdehamer bdehamer

Labels
component:tests Unit and integration tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bdehamer @woodruffw @steiza @tnytown @haydentherapper