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 support for Fulcio and Rekor, and --sign-by-sigstore=param-file #1849

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Jan 11, 2023

skopeo copy and skopeo sync now support --sign-by-sigstore=param-file,
using the containers-sigstore-signing-params.yaml(5) file format.

That notably adds support for Fulcio and Rekor signing.

Depends on unmerged containers/image#1787 ; see that PR for documentation of the YAML file format, as well as example files.

Interoperability with cosign tested manually, similarly to those examples. Integration tests to come later, tracked in #1704 .

(Yes, this is costly in binary size. On macOS, the binary goes from 27 MB to 33 MB. That can almost certainly be brought down by not using the official client packages, at the cost of replicating (the relevant subset of) their functionality.)

@mtrmac
Copy link
Contributor Author

mtrmac commented Jan 11, 2023

@vrothberg PTAL. @rhatdan FYI.

@mtrmac mtrmac mentioned this pull request Jan 11, 2023
6 tasks
@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2023

Awesome work @mtrmac

@@ -93,6 +93,11 @@ Do not copy signatures, if any, from _source-image_. Necessary when copying a si

Add a “simple signing” signature using that key ID for an image name corresponding to _destination-image_

**--sign-by-sigstore** _param-file_

Add a sigstore signature based on further optinos specified in a containers sigstore signing parameter file _param-file_.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add a sigstore signature based on further optinos specified in a containers sigstore signing parameter file _param-file_.
Add a sigstore signature based on further options specified in a container's sigstore signing parameter file _param-file_.

Copy link
Contributor Author

@mtrmac mtrmac Jan 11, 2023

Choose a reason for hiding this comment

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

Thanks, fixed the options typo.

WRT the other part, this is not “a parameter file owned by / related to a container”, it’s “a signing parameter file defined by the GitHub.com/containers organization” (i.e. not a cosign-defined parameter file).

I fully appreciate that a “containers sigstore signing parameter file” is a horrible mouthful that desperately needs replacing; what would be a better name? Compare a tiny bit more discussion in containers/image#1787 . Ultimately the name for this concept is decided by the man page in that other PR, and this one would follow the lead.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's entertaining. Thanks for the explanation. I'll try another whack at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

“entertaining”. Seriously, I would love to replace that name.

@@ -76,6 +76,11 @@ Print usage statement.

Add a “simple signing” signature using that key ID for an image name corresponding to _destination-image_

**--sign-by-sigstore** _param-file_

Add a sigstore signature based on further optinos specified in a containers sigstore signing parameter file _param-file_.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add a sigstore signature based on further optinos specified in a containers sigstore signing parameter file _param-file_.
Add a sigstore signature based on further options specified in a container's sigstore signing parameter file _param-file_.

@TomSweeneyRedHat
Copy link
Member

Couple doc nits, otherwise LGTM.
Nicely done, I was hoping you'd keep this PR under 967 changed or new files! ;^)

@mtrmac
Copy link
Contributor Author

mtrmac commented Jan 11, 2023

Nicely done, I was hoping you'd keep this PR under 967 changed or new files! ;^)

Ugh, I didn’t realize this is +170K lines. That’s just profoundly sad.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jan 11, 2023
@@ -93,6 +93,11 @@ Do not copy signatures, if any, from _source-image_. Necessary when copying a si

Add a “simple signing” signature using that key ID for an image name corresponding to _destination-image_

**--sign-by-sigstore** _param-file_

Add a sigstore signature based on further options specified in a containers sigstore signing parameter file _param-file_.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps?

Suggested change
Add a sigstore signature based on further options specified in a containers sigstore signing parameter file _param-file_.
Add a sigstore signature based on the options in the specified containers sigstore signing parameter file, _param-file_.

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, updated.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@mtrmac mtrmac force-pushed the sign-by-sigstore branch 2 times, most recently from 0ffba91 to c6cc9b5 Compare January 13, 2023 15:39
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
(skopeo copy) and (skopeo sync) now support --sign-by-sigstore=param-file,
using the containers-sigstore-signing-params.yaml(5) file format.

That notably adds support for Fulcio and Rekor signing.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Contributor Author

mtrmac commented Jan 14, 2023

Now based on a merged c/image feature, ready for review and possible merging.

@mtrmac mtrmac marked this pull request as ready for review January 14, 2023 12:37
@rhatdan
Copy link
Member

rhatdan commented Jan 14, 2023

LGTM

@rhatdan rhatdan enabled auto-merge January 14, 2023 16:17
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@rhatdan rhatdan merged commit 8e09e64 into containers:main Jan 16, 2023
@vrothberg
Copy link
Member

@rhatdan what is this auto-merge thing you enabled?

@mtrmac mtrmac deleted the sign-by-sigstore branch January 16, 2023 12:16
@rhatdan
Copy link
Member

rhatdan commented Jan 17, 2023

I probably did it by accident, all I wanted to do was prevent force push to main.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature A request for, or a PR adding, new functionality locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants