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(deps): Update Sigstore Dep to Sigstore 2.2.2 #3491

Merged
merged 6 commits into from Mar 28, 2024

Conversation

enteraga6
Copy link
Collaborator

@enteraga6 enteraga6 commented Mar 27, 2024

Summary

Updates sigstore version from 1.8 -> 2.2.2 for the root dependency version and for the Github Action sign-attestation, verify-token, and setup-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 here
After updating verify-token and setup-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

  • Review the contributing guidelines
  • Add a reference to related issues in the PR description.
  • Update documentation if applicable.
  • Add unit tests if applicable.
  • Add changes to the CHANGELOG if applicable.

Signed-off-by: Noah Elzner <nge1@rice.edu>
@enteraga6 enteraga6 changed the title Remove Sign Options and Bump to Sigstore 1.9 fix(deps): Update Sign Attestation to Sigstore 1.9 Mar 27, 2024
@@ -17,11 +17,6 @@ import { sigstore } from "sigstore";
import * as path from "path";
import * as tscommon from "tscommon";

const signOptions = {
Copy link
Collaborator

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?

Copy link
Collaborator Author

@enteraga6 enteraga6 Mar 27, 2024

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>
@enteraga6
Copy link
Collaborator Author

I also just updated verify-token and setup-generic because they also had Sigstore v1.8. I think that is all the actions that were running v1.8. Test that confirms functionality is in the edited PR description and here

kpk47
kpk47 previously approved these changes Mar 27, 2024
Copy link
Contributor

@kpk47 kpk47 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 the contribution!

Signed-off-by: Noah Elzner <nge1@rice.edu>
@enteraga6
Copy link
Collaborator Author

@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 sign-attestations and setup-generic, both of which I bumped. Is this because I pushed another commit after you approved the code? Or is there another reason I should look into?

The functionality is correct on my personal workflow test here

Thanks!

Copy link
Collaborator

@ramonpetgrave64 ramonpetgrave64 left a 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.

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",
Copy link
Collaborator

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?

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@laurentsimon
Copy link
Collaborator

laurentsimon commented Mar 28, 2024

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.

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

You need to recompile the code from typescript (TS) to javascript. In each of these projects, run something like npm run all (check the Makefile and package.json where these commands are defined). The reason we have this check is because the compiled code is not human-readable so we can't review it. But we want to ensure that what's being pushed and eventually run by the Action is the same as the TS code that we review.

@kpk47 kpk47 dismissed their stale review March 28, 2024 16:39

Version change post-review.

@kpk47
Copy link
Contributor

kpk47 commented Mar 28, 2024

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.

@enteraga6 enteraga6 changed the title fix(deps): Update Sign Attestation to Sigstore 1.9 fix(deps): Update Sign Attestation to Sigstore 2.2.2 Mar 28, 2024
Signed-off-by: Noah Elzner <nge1@rice.edu>
@enteraga6
Copy link
Collaborator Author

enteraga6 commented Mar 28, 2024

@laurentsimon It is recompiled in each using npm run all. I went ahead to double check did npm run all again, then cleared the node_modules in case there was a local cache issue, did npm install then npm run all. I refactored the code to explicitly use Sigstore's individual functions/types on imports from this 2.0.0 change. No luck.

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):

diff --git a/actions/delegator/setup-generic/dist/public-good-instance-root.json b/actions/delegator/setup-generic/dist/public-good-instance-root.json
deleted file mode 100644
index e95c7e8..0000000
--- a/actions/delegator/setup-generic/dist/public-good-instance-root.json

What is public-good-instance-root.json? What do you think?

Signed-off-by: Noah Elzner <nge1@rice.edu>
Signed-off-by: Noah Elzner <nge1@rice.edu>
@enteraga6 enteraga6 changed the title fix(deps): Update Sign Attestation to Sigstore 2.2.2 fix(deps): Update Sigstore Dep to Sigstore 2.2.2 Mar 28, 2024
@laurentsimon
Copy link
Collaborator

@laurentsimon It is recompiled in each using npm run all. I went ahead to double check did npm run all again, then cleared the node_modules in case there was a local cache issue, did npm install then npm run all. I refactored the code to explicitly use Sigstore's individual functions/types on imports from this 2.0.0 change. No luck.

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):

diff --git a/actions/delegator/setup-generic/dist/public-good-instance-root.json b/actions/delegator/setup-generic/dist/public-good-instance-root.json
deleted file mode 100644
index e95c7e8..0000000
--- a/actions/delegator/setup-generic/dist/public-good-instance-root.json

What is public-good-instance-root.json? What do you think?

Looks like tests are passing now?

Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

@enteraga6
Copy link
Collaborator Author

enteraga6 commented Mar 28, 2024

@laurentsimon Yes! Tests are passing now. It seems that npm run all created public-good-instance-root.json which the tests did not want. Looking over the the matrix dist tests, they run make clean package. Doing that removed public-good-instance-root.json which is what the tests needed to pass. I am still unsure what public-good-instance-root.json was or why it was generated with npm run all but not make clean package.

@ianlewis
Copy link
Member

@laurentsimon Yes! Tests are passing now. It seems that npm run all created public-good-instance-root.json which the tests did not want. Looking over the the matrix dist tests, they run make clean package. Doing that removed public-good-instance-root.json which is what the tests needed to pass. I am still unsure what public-good-instance-root.json was or why it was generated with npm run all but not make clean package.

The sigstore-js packages (or underlying TUF packages) have likely changed how they build the TUF root info into the package and public-good-instance-root.json was just hanging around from a previous build. Running make clean cleaned out the lib and dist directories and cleared things up.

Copy link
Member

@ianlewis ianlewis left a comment

Choose a reason for hiding this comment

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

LGTM

@laurentsimon laurentsimon merged commit e8c2dcf into slsa-framework:main Mar 28, 2024
79 checks passed
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.

None yet

5 participants