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

Use ValidatePubKey from sigstore/sigstore #1676

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

haydentherapper
Copy link
Contributor

This improves RSA, ECDSA, and ED25519 (no-op) validation for imported keys.

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Summary

Ticket Link

Fixes

Release Note

Improved validation for imported RSA keys

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2022

Codecov Report

Merging #1676 (0b28ef1) into main (ac682db) will decrease coverage by 0.14%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##             main    #1676      +/-   ##
==========================================
- Coverage   29.37%   29.22%   -0.15%     
==========================================
  Files         141      141              
  Lines        8429     8413      -16     
==========================================
- Hits         2476     2459      -17     
  Misses       5684     5684              
- Partials      269      270       +1     
Impacted Files Coverage Δ
pkg/cosign/keys.go 61.36% <28.57%> (-4.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac682db...0b28ef1. Read the comment docs.

@haydentherapper
Copy link
Contributor Author

Hm, looks like we might have an issue due to Windows - golang/go#1108

@dlorenc
Copy link
Member

dlorenc commented Mar 29, 2022

Hm, looks like we might have an issue due to Windows - golang/go#1108

This is hitting a few things now... cc @cpanato any ideas what we should do here?

@cpanato
Copy link
Member

cpanato commented Mar 30, 2022

Hm, looks like we might have an issue due to Windows - golang/go#1108

This is hitting a few things now... cc @cpanato any ideas what we should do here?

maybe we can use that https://github.com/hashicorp/go-syslog but then we will need to patch upstream

@cpanato
Copy link
Member

cpanato commented Mar 30, 2022

did a change in a fork https://github.com/cpanato/boulder/commits/go-syslog and updated our side and push this change to a branch on my cosign fork and run CI here: cpanato#175

seems to compile and tests passed :)

I can submit this patch to boulder and then if they approve/merge we can update, in meantime we can use the replace directive, I guess

UPDATE: made a PR letsencrypt/boulder#6021

@dlorenc
Copy link
Member

dlorenc commented Mar 30, 2022

Thanks! Let's do the fork for now:)

@haydentherapper
Copy link
Contributor Author

Thanks @cpanato for the fix! Added a replace directive to the fork.

@haydentherapper
Copy link
Contributor Author

Looks to be building successfully on Windows, just an intermittent KinD E2E error now.

@haydentherapper
Copy link
Contributor Author

@cpanato Looks like there's another suggestion to remove the logging statement from a core package in boulder

@cpanato
Copy link
Member

cpanato commented Mar 31, 2022

based on the feedback from the letsencrypt maintainer I did a follow-up letsencrypt/boulder#6029 and did the testing on our side here #1676 which compile and tests passed

cross compile https://github.com/cpanato/cosign/runs/5768177620?check_suite_focus=true

Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

LGTM modulo testing/build/CI issues, which you all seem to have under control now :)

@haydentherapper
Copy link
Contributor Author

Thanks @cpanato! I’ll wait til end of day to see if they approve and merge the change, otherwise I’ll use your fork.

go.mod Outdated Show resolved Hide resolved
@dlorenc
Copy link
Member

dlorenc commented Mar 31, 2022

Looks like we need codegen still, but Windows is passing!

This improves RSA, ECDSA, and ED25519 (no-op) validation for imported keys.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

All tests are now passing, and the replace directive has been removed. PR is ready to go.

@dlorenc dlorenc merged commit c68d170 into sigstore:main Mar 31, 2022
@github-actions github-actions bot added this to the v1.7.0 milestone Mar 31, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
This improves RSA, ECDSA, and ED25519 (no-op) validation for imported keys.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper haydentherapper deleted the validate-key branch January 11, 2023 22:55
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.

6 participants