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

Improve Verify Error Responses #210

Merged
merged 12 commits into from
May 8, 2024
Merged

Improve Verify Error Responses #210

merged 12 commits into from
May 8, 2024

Conversation

ChaosInTheCRD
Copy link
Collaborator

No description provided.

colek42
colek42 reviewed Apr 12, 2024
View reviewed changes
policy/policy.go Outdated Show resolved Hide resolved
@colek42
Copy link
Member

colek42 commented Apr 12, 2024 •
edited 

➜  policy-service git:(main) ✗ ./test.sh
+ SHA=7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
+ go run . run --archivista-url https://archivista.testifysec.io/ --subject-digest 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5 --kms-url=awskms:///arn:aws:kms:us-east-2:184822323247:key/a5b221b1-0670-4c6d-8760-135cb29a0ad5 --sticky-keys-file=sticky.yaml --tsa=https://freetsa.org/files/cacert.pem
+ jq -r
+ witness sign -k policykey.pem -o auto-policy-signed.json -f auto-policy.json
+ witness verify -p auto-policy-signed.json -k policypub.pem -s -l debug --enable-archivista -s 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
INFO    policy signature verified
INFO    Verification succeeded
INFO    Evidence:
INFO    0: acaf3d0bb40b715a21e42320517276dae28e999b1f80f028ed7095fb2ae7275f
INFO    1: b43f217cc65b92ef2b1c0ee3041807a7867e475cac080592779fd2de4a3097ac
INFO    2: bb60e7f30f990fccc991649b8057ac385278666d894d04941b1e36b63e340c6a
INFO    3: 2a6eb1ba5f048799079fe941ac5929a557ac8b1667fdfaff6e59e5d85aafd0bd
INFO    4: b222369b43821e349c1f4cc9051c9875b35c50b431af6b00f3dedb08d4aa8dce
INFO    5: 24260cb11925061c988f134a7cfeb21ab16b02cfe51f091d37c96f37f221970c
INFO    6: f00e5177ee0fccd9b28e2299c8c1de3a5f5de96506e6cfba46ce1ca9b408eb4a
INFO    7: 62e2530eb74c2dead4b20872f67e9d0c1211663ad0904f64c45058265773484c
INFO    8: 9327c17ef676d7b35904faaa03065b61e6b3fc91caae0710c0cd4ee54fc196b2
INFO    9: e9be26c45b7311c02291924e25bd500ba9422632e3ccc1793b556956f2a68520
INFO    10: dcb4df0f81838f8917fce371de6bd6f11ec4413f044c32693b76f9027118dd9c
INFO    11: 2d3f4a544a61f0ee2cbd6426e364dd05d0b77c0ae58568fa31e721be7ecc160a
+ witness2 verify -p auto-policy-signed.json -k policypub.pem -s -l debug --enable-archivista -s 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
INFO    policy signature verified
ERROR   Verification failed
ERROR   Evidence:
ERROR   Step: docker-build
ERROR   Step: release
ERROR   Step: cve-scan
ERROR   Step: secret-scan
ERROR   Step: test
ERROR   Step: build
ERROR   Verification Failure: Reason: failed to verify artifacts for step build: no passed collections present
ERROR   Step: fmt
ERROR   Step: generate-sbom
ERROR   Step: vet
ERROR   Step: checkout
ERROR   Step: sast
ERROR   Step: lint

This is failing on one of the policies I am using for the GitLab demo. It is also interesting that it fails on a different step each time. I'll add this as a text fixture

@ChaosInTheCRD
Copy link
Collaborator Author 

➜  policy-service git:(main) ✗ ./test.sh
+ SHA=7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
+ go run . run --archivista-url https://archivista.testifysec.io/ --subject-digest 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5 --kms-url=awskms:///arn:aws:kms:us-east-2:184822323247:key/a5b221b1-0670-4c6d-8760-135cb29a0ad5 --sticky-keys-file=sticky.yaml --tsa=https://freetsa.org/files/cacert.pem
+ jq -r
+ witness sign -k policykey.pem -o auto-policy-signed.json -f auto-policy.json
+ witness verify -p auto-policy-signed.json -k policypub.pem -s -l debug --enable-archivista -s 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
INFO    policy signature verified
INFO    Verification succeeded
INFO    Evidence:
INFO    0: acaf3d0bb40b715a21e42320517276dae28e999b1f80f028ed7095fb2ae7275f
INFO    1: b43f217cc65b92ef2b1c0ee3041807a7867e475cac080592779fd2de4a3097ac
INFO    2: bb60e7f30f990fccc991649b8057ac385278666d894d04941b1e36b63e340c6a
INFO    3: 2a6eb1ba5f048799079fe941ac5929a557ac8b1667fdfaff6e59e5d85aafd0bd
INFO    4: b222369b43821e349c1f4cc9051c9875b35c50b431af6b00f3dedb08d4aa8dce
INFO    5: 24260cb11925061c988f134a7cfeb21ab16b02cfe51f091d37c96f37f221970c
INFO    6: f00e5177ee0fccd9b28e2299c8c1de3a5f5de96506e6cfba46ce1ca9b408eb4a
INFO    7: 62e2530eb74c2dead4b20872f67e9d0c1211663ad0904f64c45058265773484c
INFO    8: 9327c17ef676d7b35904faaa03065b61e6b3fc91caae0710c0cd4ee54fc196b2
INFO    9: e9be26c45b7311c02291924e25bd500ba9422632e3ccc1793b556956f2a68520
INFO    10: dcb4df0f81838f8917fce371de6bd6f11ec4413f044c32693b76f9027118dd9c
INFO    11: 2d3f4a544a61f0ee2cbd6426e364dd05d0b77c0ae58568fa31e721be7ecc160a
+ witness2 verify -p auto-policy-signed.json -k policypub.pem -s -l debug --enable-archivista -s 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
INFO    policy signature verified
ERROR   Verification failed
ERROR   Evidence:
ERROR   Step: docker-build
ERROR   Step: release
ERROR   Step: cve-scan
ERROR   Step: secret-scan
ERROR   Step: test
ERROR   Step: build
ERROR   Verification Failure: Reason: failed to verify artifacts for step build: no passed collections present
ERROR   Step: fmt
ERROR   Step: generate-sbom
ERROR   Step: vet
ERROR   Step: checkout
ERROR   Step: sast
ERROR   Step: lint

This is failing on one of the policies I am using for the GitLab demo. It is also interesting that it fails on a different step each time. I'll add this as a text fixture

I think i've figured out why this is happening. Bear with me while I try and resolve.

@ChaosInTheCRD
Copy link
Collaborator Author 

➜  policy-service git:(main) ✗ ./test.sh
+ SHA=7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
+ go run . run --archivista-url https://archivista.testifysec.io/ --subject-digest 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5 --kms-url=awskms:///arn:aws:kms:us-east-2:184822323247:key/a5b221b1-0670-4c6d-8760-135cb29a0ad5 --sticky-keys-file=sticky.yaml --tsa=https://freetsa.org/files/cacert.pem
+ jq -r
+ witness sign -k policykey.pem -o auto-policy-signed.json -f auto-policy.json
+ witness verify -p auto-policy-signed.json -k policypub.pem -s -l debug --enable-archivista -s 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
INFO    policy signature verified
INFO    Verification succeeded
INFO    Evidence:
INFO    0: acaf3d0bb40b715a21e42320517276dae28e999b1f80f028ed7095fb2ae7275f
INFO    1: b43f217cc65b92ef2b1c0ee3041807a7867e475cac080592779fd2de4a3097ac
INFO    2: bb60e7f30f990fccc991649b8057ac385278666d894d04941b1e36b63e340c6a
INFO    3: 2a6eb1ba5f048799079fe941ac5929a557ac8b1667fdfaff6e59e5d85aafd0bd
INFO    4: b222369b43821e349c1f4cc9051c9875b35c50b431af6b00f3dedb08d4aa8dce
INFO    5: 24260cb11925061c988f134a7cfeb21ab16b02cfe51f091d37c96f37f221970c
INFO    6: f00e5177ee0fccd9b28e2299c8c1de3a5f5de96506e6cfba46ce1ca9b408eb4a
INFO    7: 62e2530eb74c2dead4b20872f67e9d0c1211663ad0904f64c45058265773484c
INFO    8: 9327c17ef676d7b35904faaa03065b61e6b3fc91caae0710c0cd4ee54fc196b2
INFO    9: e9be26c45b7311c02291924e25bd500ba9422632e3ccc1793b556956f2a68520
INFO    10: dcb4df0f81838f8917fce371de6bd6f11ec4413f044c32693b76f9027118dd9c
INFO    11: 2d3f4a544a61f0ee2cbd6426e364dd05d0b77c0ae58568fa31e721be7ecc160a
+ witness2 verify -p auto-policy-signed.json -k policypub.pem -s -l debug --enable-archivista -s 7ca1e69b7f80e02346819160df24503484ecf268c09a19528ab5a4cb12944af5
INFO    policy signature verified
ERROR   Verification failed
ERROR   Evidence:
ERROR   Step: docker-build
ERROR   Step: release
ERROR   Step: cve-scan
ERROR   Step: secret-scan
ERROR   Step: test
ERROR   Step: build
ERROR   Verification Failure: Reason: failed to verify artifacts for step build: no passed collections present
ERROR   Step: fmt
ERROR   Step: generate-sbom
ERROR   Step: vet
ERROR   Step: checkout
ERROR   Step: sast
ERROR   Step: lint

This is failing on one of the policies I am using for the GitLab demo. It is also interesting that it fails on a different step each time. I'll add this as a text fixture

I think the issue is that the depthened search creates a situation where the stepResult is getting overwritten. Therefore I have added logic so the results get merged together, and moved verifyArtifacts outside of the search loop

@ChaosInTheCRD ChaosInTheCRD marked this pull request as ready for review May 3, 2024 16:05
jkjell
jkjell reviewed May 3, 2024
View reviewed changes
Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

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

I think we need to retitle this PR... it's way more than just improving error responses. It's a refactor of a lot of the policy verification workflow.

dsse/verify.go Outdated Show resolved Hide resolved
dsse/verify.go Outdated Show resolved Hide resolved
policy/policy.go Show resolved Hide resolved
policy/policy.go Show resolved Hide resolved
verify.go Outdated Show resolved Hide resolved
verify.go Outdated Show resolved Hide resolved
policy/policy.go Show resolved Hide resolved
policy/policy.go Outdated
}
return resultsByStep, nil
Copy link
Member

Choose a reason for hiding this comment

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

I was super confused by the return at this point in the code. Would we want to want to continue in case other policy step ArtifactsFrom clauses fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah my apologies on this one, very unclear. I have now changed things so it continues if there are no passing results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually gone an backtracked a bit. We want to handle an error in the case of !ok because that implies that there was a problem fetching a step result (which should definitely be there)

@ChaosInTheCRD @jkjell
saving progress - working but a bit of tidy up needed
5db95ff 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
fixing verification issues observed by colek42 - recursive search was
94de08a 
deleting previous step results

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
Update policy/policy.go
4194306 
Signed-off-by: Tom Meadows <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
added description of analyze function
68dfbd0 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
adding more test scaffolding, bug fixes etc.
7063e90 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
changed info line to debug
08b6743 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
adding changes / fixes to tests, not finished yet
faaa245 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
fixing tests
8d89ec0 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
removing unnecessary sprint
0989b10 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
changed to avoid breaking API and using original error return style
4daed93 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
tidying things up
2ffe489 
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD @jkjell
modifying verifyArtifacts so it returns an error if a step result is not
78a4066 
found in the map

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
jkjell
jkjell approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes!

@jkjell jkjell merged commit 90e0da2 into in-toto:main May 8, 2024
11 checks passed
@ChaosInTheCRD ChaosInTheCRD mentioned this pull request May 8, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colek42 colek42 colek42 left review comments

@jkjell jkjell jkjell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ChaosInTheCRD @colek42 @jkjell