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

Add SCAI reusable actions (KubeCon NA '23 Demo) #13

Merged

Conversation

marcelamelara
Copy link
Contributor

@marcelamelara marcelamelara commented Oct 31, 2023

This PR adds GitHub Actions for running the Go scai-gen tools through GHA workflows. This PR also adds a new (experimental) command scai-gen sigstore for Sigstore integration (cosign signing and Rekor upload) via GHA. The primary use of these Actions is as part of the in-toto KubeCon NA '23 demo. The demo implementation lives here.

* Download evidence artifact in assertion generator
* Take evidence type as input param
* Pass generated intermediate files as step outputs

Signed-off-by: Marcela Melara <[email protected]>
Signed-off-by: Marcela Melara <[email protected]>
@marcelamelara marcelamelara force-pushed the add-scai-reusable-workflows branch from a576262 to ed74e18 Compare October 31, 2023 17:31
Copy link
Member

@adityasaky adityasaky 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 an expert in actions and parts of sigstore but some quick thoughts.

.github/workflows/test-e2e-flow.yml Outdated Show resolved Hide resolved
scai-gen/cmd/sigstore.go Show resolved Hide resolved
@marcelamelara marcelamelara force-pushed the add-scai-reusable-workflows branch from cd38563 to b828f1a Compare November 1, 2023 17:30
@marcelamelara marcelamelara force-pushed the add-scai-reusable-workflows branch from b828f1a to 5eced77 Compare November 1, 2023 17:42
Copy link
Member

@adityasaky adityasaky left a comment

Choose a reason for hiding this comment

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

Mostly nits but I think at this point this is fine.

shell: bash
run: |
mkdir -p ${{ inputs.assertion-path }}
scai-gen assert -e ${{ steps.gen-rd.outputs.file-rd-name }} -o ${{ inputs.assertion-path }}/${{ inputs.assertion-name }} ${{ inputs.attribute}}
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 use the full flag names instead of the short ones? I think it makes it more readable in this context and possibly more maintainable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll make these changes as part of a follow-up PR that focuses on readability and usability of the demo, so I can unblock testing on the implementer end.

id: scai-gen-assert
shell: bash
run: |
mkdir -p ${{ inputs.assertion-path }}
Copy link
Member

Choose a reason for hiding this comment

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

Possibly out of scope but perhaps the output dir creation should be handled by scai-gen assert so that the action's usable even without mkdir. IDK if mkdir is aliased / supported on Windows, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point, and shouldn't be too hard to implement, I hope. What I'll do is merge this PR so I can start testing this with my test build. I have some documentation for the demo in-flight (nothing major) that I think would fit well into a "make the demo more legible/usable" PR.


// Add certificate to envelope. This is needed for
// Rekor compatibility.
signedAttWithCert, err := envelope.AddCertToEnvelope(signedAtt, k.Cert)
Copy link
Member

Choose a reason for hiding this comment

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

This is technically not part of the DSSE spec. I don't want to block this PR on this but this is another data point for us to get secure-systems-lab/dsse#61 merged and implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%

@adityasaky
Copy link
Member

Sounds good, I'm fine with this being merged now.

@marcelamelara marcelamelara merged commit e89d20c into in-toto:main Nov 1, 2023
2 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.

2 participants