-
Notifications
You must be signed in to change notification settings - Fork 140
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(deps): Update Sigstore Dep to Sigstore 2.2.2 #3491
fix(deps): Update Sigstore Dep to Sigstore 2.2.2 #3491
Conversation
Signed-off-by: Noah Elzner <nge1@rice.edu>
@@ -17,11 +17,6 @@ import { sigstore } from "sigstore"; | |||
import * as path from "path"; | |||
import * as tscommon from "tscommon"; | |||
|
|||
const signOptions = { |
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.
Are these now default params in v1.9?
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.
Yes, in v1.9 it defaults to CI provider when no other strategy has been configured, instead of defaulting as the first strategy like before in v1.8. Removing signOptions, lets it fallback to CI provider to get OIDC token for GHA.
You can see that change here: https://github.com/sigstore/sigstore-js/blob/sigstore%401.9.0/packages/client/src/config.ts#L130-L139
Signed-off-by: Noah Elzner <nge1@rice.edu>
I also just updated |
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.
Thanks for the contribution!
Signed-off-by: Noah Elzner <nge1@rice.edu>
@kpk47 I bumped to Sigstore 2.2.2 to make it the latest since it seemed to be a simple change following this commit from Sigstore 2.0.0 where they had some major changes. I seem to be failing check-dist-matrix pre-submit tests for the The functionality is correct on my personal workflow test here Thanks! |
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.
I'm not sure, but I think the pre-submit problems you're having might be around node version.
The tests all use node18, but we still have some actions using node 16.
slsa-github-generator/.github/workflows/pre-submit.actions.yml
Lines 67 to 83 in 2512315
matrix: | |
action: | |
- .github/actions/compute-sha256 | |
- .github/actions/privacy-check | |
- .github/actions/generate-attestations | |
- .github/actions/sign-attestations | |
- .github/actions/create-container_based-predicate | |
- ./actions/delegator/setup-generic | |
- .github/actions/verify-token | |
- .github/actions/detect-workflow-js | |
steps: | |
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | |
- name: Set Node.js 18 | |
uses: actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d # v3.8.1 | |
with: | |
node-version: 18 |
@@ -26,7 +26,7 @@ | |||
"@actions/github": "5.1.1", | |||
"@octokit/webhooks-types": "7.3.1", | |||
"@sigstore/rekor-types": "1.0.0", | |||
"sigstore": "1.8.0", | |||
"sigstore": "2.2.2", |
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.
Did you mean for this to be 2.2.2
?
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.
The package seems to exist at this verison, iiuc https://github.com/sigstore/sigstore-js/releases/tag/sigstore%402.2.2
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.
I ask because your PR description says you mean to upgrade to "1.9", so I thought you might have meant 1.9.0
.
https://github.com/sigstore/sigstore-js/releases/tag/sigstore%401.9.0
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.
Yes! I should have updated the title and description. Will do that now. Perhaps, I should have done 2.2.2 in a separate pr. My thinking was that since 2.2.2 was the latest and required only a small change that it would be best to do that here.
You need to recompile the code from typescript (TS) to javascript. In each of these projects, run something like |
I'm removing my approval for now. Please recompile the TypeScript and request review again when the tests pass. Also, please update the PR description when you re-send it. |
Signed-off-by: Noah Elzner <nge1@rice.edu>
@laurentsimon It is recompiled in each using Honestly, I am not sure what the problem is. I think it might be best to reset to last v1.9 commit, draft a pr to continue exploring this issue for 2.2.2. It seems that the tests fail because of this git diff (same for other fail as well):
What is |
Signed-off-by: Noah Elzner <nge1@rice.edu>
Signed-off-by: Noah Elzner <nge1@rice.edu>
Looks like tests are passing now? |
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. Wdut @ianlewis @ramonpetgrave64 ?
@laurentsimon Yes! Tests are passing now. It seems that |
The |
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
Summary
Updates sigstore version from 1.8 -> 2.2.2 for the root dependency version and for the Github Action
sign-attestation
,verify-token
, andsetup-generic
.Per 1.9, signing options needed to be removed. More information on it here on this Sigstore Issue. This fixes revert from #2913
The actions were refactored to make use of to explicitly use Sigstore's individual functions/types on imports from this v2.0.0 change
Testing Process
Testing Removal of Signing Options
After updating
sign-attestation
on a personal workflow pointing to the branch. Check it out hereAfter updating
verify-token
andsetup-generic
to 1.9, I tested using this workflow.Testing 2.2.2
After updating the actions to Sigstore 2.2.2, I tested using this workflow. Note: it says Sigstore 1.9 on workflow title, but it was used to test 2.2.2. I used the same workflow.
Final Test
This workflow test shows successful functionality after all the changes.
Checklist