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

feat(cli): context by environment #148

Closed

Conversation

Zenithar
Copy link

@Zenithar Zenithar commented Feb 25, 2022

Context

Consume all contexts (GITHUB_CONTEXT and RUNNER_CONTEXT) via environment variables. The previous strategy was to use a base64 encoded value to pass these contexts via command line. But the GITHUB_CONTEXT contains an active Github token value which can be retrieved by decoding the base64 encoded context.

The generated encoded payload is publicly accessible and depending on the permissions allocated to the workflow, this ephemeral token could have high privileges.

By the way, this is very cool project I use in my pro and personal projects, I'm happy to help you to make all of these CI/CD topics more secured as I could help.

Reference(s)

@Zenithar Zenithar requested a review from a team as a code owner February 25, 2022 16:27
@Zenithar Zenithar force-pushed the feat_cli_context_by_environment branch from 8509e46 to ab47f35 Compare February 25, 2022 16:28
@Brend-Smits
Copy link
Member

Heey @Zenithar,
Appreciate the pull request a lot. Glad to hear that you enjoy using the Action.
We will try to get a review in as soon as possible. May be a bit delayed due to holidays though.

Will keep you posted.

@Brend-Smits Brend-Smits added the enhancement New feature or request label Feb 25, 2022
@Brend-Smits Brend-Smits linked an issue Feb 25, 2022 that may be closed by this pull request
JeroenKnoops
JeroenKnoops previously approved these changes Mar 1, 2022
@JeroenKnoops
Copy link
Member

@Zenithar
Copy link
Author

Zenithar commented Mar 1, 2022

The linter is failing. https://github.com/Zenithar/slsa-provenance-action/runs/5336373702?check_suite_focus=true

I'm not sure why?

I stopped the process because it requires tokens I don't have for various steps.

@Zenithar Zenithar force-pushed the feat_cli_context_by_environment branch from c2b6ca6 to c2b5e37 Compare March 1, 2022 14:49
@Zenithar Zenithar requested a review from JeroenKnoops March 1, 2022 14:49
Copy link
Member

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

@Zenithar thanks a lot for your contribution.

I found a few nit picky things that could use improvement. Furthermore we should perform one additional test regarding the base64 encoding change also see #116.

Also would like to understand the reasoning for the limit reader.

Also happy to have a chat, to verbally discuss.

Thanks a lot.

install-slsa-provenance.sh Outdated Show resolved Hide resolved
cmd/slsa-provenance/cli/options/generate.go Show resolved Hide resolved
cmd/slsa-provenance/cli/options/generate.go Show resolved Hide resolved
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
steps:
-
Copy link
Member

Choose a reason for hiding this comment

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

Could you align the yaml formatting like we do in our other workflows?

- name:

I would also love to have this job integrated in our ci workflow.

It should run after the build
but before the job release

Copy link
Author

Choose a reason for hiding this comment

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

The idea of these tests is to test the released code in a multi-platform context. That's why they don't use local action source but a named release. Do you think that I should use local code only?

Copy link
Member

Choose a reason for hiding this comment

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

Using local code we can capture the bug you resolved before cutting out another release. Would be great to have that.

@@ -40,15 +40,8 @@ runs:
id: compose-args
shell: bash
run: |
encoded_github="$(echo ${GITHUB_CONTEXT} | base64 -w 0)"
Copy link
Member

Choose a reason for hiding this comment

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

Please do note we have added base64 encoding due to an earlier bug that broke the json due to

a commit message with quotes inside. #116

Could you confirm this isn't reintroducing that issue?

Copy link
Author

Choose a reason for hiding this comment

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

Probably a toJSON(github) error? I'm going to check that. Thank you to point me out to this issue.

@Zenithar Zenithar force-pushed the feat_cli_context_by_environment branch from 41cedb3 to f82f4a0 Compare March 1, 2022 15:30
@Zenithar Zenithar force-pushed the feat_cli_context_by_environment branch from 5771fc8 to 40367c5 Compare March 1, 2022 16:49
@Zenithar Zenithar requested a review from marcofranssen March 1, 2022 17:17
- name: Install cosign
uses: sigstore/[email protected]
with:
cosign-release: 'v1.5.1'
Copy link
Member

@marcofranssen marcofranssen Mar 2, 2022

Choose a reason for hiding this comment

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

Bumped cosign yesterday.

Suggested change
cosign-release: 'v1.5.1'
cosign-release: 'v1.6.0'

@marcofranssen
Copy link
Member

👋 @Zenithar

Is there anything I can assist you with to finalize this PR? You can find us on the gophers slack in our channel. https://gophers.slack.com/archives/C03563G8T4M

@Zenithar
Copy link
Author

Zenithar commented Mar 9, 2022

👋 @Zenithar

Is there anything I can assist you with to finalize this PR? You can find us on the gophers slack in our channel. https://gophers.slack.com/archives/C03563G8T4M

I'm a little busy for the moment :-) I need to build a test repo to test your CI. I have permissions problems during the run.

@Zenithar
Copy link
Author

Zenithar commented Jan 3, 2024

Closing this PR, I can't find time to finish it.

@Zenithar Zenithar closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use this action from macos-latest
4 participants