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

fix: quill describe panics on ad_hoc unsigned binary #15

Merged
merged 10 commits into from
Oct 19, 2022
Merged

Conversation

spiffcs
Copy link
Contributor

@spiffcs spiffcs commented Oct 18, 2022

Fix: update quill details to not panic on ad_hoc unsigned binary

Reproduction Steps on OSX:
make build
./snapshot/darwin-build_darwin_amd64_v1/quill describe -vv ./snapshot/darwin-build_darwin_amd64_v1/quill 

Panic: on main:
Screen Shot 2022-10-18 at 2 29 37 PM

No Panic with fix:
Screen Shot 2022-10-18 at 2 30 13 PM

Details:

getSignatures features a couple of error branches where a panic is possible.

This PR refactors getSignatures into a few smaller functions and removes those panics

  • move BlobDetails out of the signature code and into its own function
  • warn if content info or signed data cannot be parsed
  • build signers, certs, verifiedCerts, and signature timestamps separately
  • return SignatureDetails early and append BlobDetails

Signed-off-by: Christopher Phillips [email protected]

@spiffcs spiffcs changed the title fix: quill details would panic on unsigned binary fix: quill details panics on unsigned binary Oct 18, 2022
@spiffcs spiffcs changed the title fix: quill details panics on unsigned binary fix: quill describe panics on unsigned binary Oct 18, 2022
@spiffcs spiffcs marked this pull request as ready for review October 18, 2022 20:18
@spiffcs spiffcs changed the title fix: quill describe panics on unsigned binary fix: quill describe panics on ad_hoc unsigned binary Oct 18, 2022
@spiffcs spiffcs requested a review from a team October 18, 2022 20:51
Signed-off-by: Christopher Phillips <[email protected]>
Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

really solid clean up 🙌

@spiffcs
Copy link
Contributor Author

spiffcs commented Oct 19, 2022

@wagoodman reduced the output to debug so now -vv is needed to see the debug output which were previously Warnf

A new error message has been included in the report in stdout showing there is no cryptographic signature
Screen Shot 2022-10-19 at 10 34 42 AM

@spiffcs spiffcs merged commit 2b60a5d into main Oct 19, 2022
@spiffcs spiffcs deleted the fix-details-panic branch October 19, 2022 17:02
@wagoodman wagoodman added the bug Something isn't working label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working changelog-ignore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants