-
Notifications
You must be signed in to change notification settings - Fork 43
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
Test against a matrix of golang versions and update various actions versions #34
Conversation
Test fail on go1.20.4 and go1.20.5.
|
The test failure begins appearing with go1.19 (release notes). Go1.18.10 doesn't exhibit this.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check in the softhsm token directory rather than initialize it on each run like before? Were you finding that importing the keys was taking up a lot of time in each test run?
If so, I think it would be better to not check in the softhsm token directory (since its format is not guaranteed consistent between versions, I think), but instead have the test generate it once in a gitignored directory and then reuse the cached version. This would have the added advantage the the initialization steps would regularly be run in CI so we would know they still work.
Also, the current version does git operations in the cleanup step of the test. That creates some possibilities for unpleasant surprises that I'd like to avoid. If we go to the "cache instead of check in" approach, we can avoid the need to do git operations during cleanup.
The only reason I have is that I liked the way it was done in the ceremony-demos repo and wanted to replicate it here. Was it the right thing to do? I don't know, but there was precedent.
I took that from ceremony-demos too. I can remove it here and explicitly rm files as needed. |
@jsha this PR is blocked on your approval (requested changes). |
Talked in standup - Phil is planning to go back to the Old Ways on this. The reason the ceremony-demos repo does it differently is (we think): (a) we wanted to have the .yaml files need to include the slot IDs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking nice! One shell nit left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two comments that don't have to be addressed
cd v4/testdata | ||
openssl ecparam -name prime256v1 -genkey | openssl pkcs8 -topk8 -nocrypt > entropic_ecdsa.key | ||
openssl req -new -x509 -key entropic_ecdsa.key -out entropic_ecdsa.pem -days 1000 -subj /CN=entropic\ ECDSA | ||
openssl req -new -newkey rsa:2048 -nodes -x509 -keyout silly_signer.key -out silly_signer.pem -days 1000 -subj /CN=silly\ signer | ||
cd - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not for this PR, but for the future: it might be cool to replace this comment and the comment in //v4/testdata/readme.md with a //v4/testdata/generate.go so that the instructions can be simplified to "if you need to regenerate key material, run go generate".)
Co-authored-by: Aaron Gable <aaron@aarongable.com>
v4/testdata/
.Background
Starting in Golang 1.18, crypto/elliptic: IsOnCurve began returning true for invalid field elements (elliptic curve points X and Y). [1]
Relatedly in Golang 1.19, crypto/elliptic: IsOnCurve was updated to explicitly panic when operating on invalid field elements. [2]
However, this should have still been failing on Golang 1.18 (even though it wasn't!!) because the points (1,1) are invalid. I believe what happened here was [3]: