Sigstore sign #15108

mtrmac · 2022-07-28T22:21:39Z

Allow creating sigstore signatures via --sign-by-sigstore-private-key . Like existing --sign-by, it does not work remote (in this case because we would have to copy the private key to the server).
Allow passing a passphrase (which is mandatory for sigstore private keys) via --sign-passphrase-file; if it is not provided, prompt interactively.
Also, use that passphrase for --sign-by as well, allowing non-interactive GPG use. (But --sign-passphrase-file can only be used with one of --sign-by and --sign-by-sigstore-private-key.)

Note that unlike the existing code, podman build does not yet implement sigstore (I'm not sure why it needs to, it seems not to push images?) because Buildah does not expose the feature yet.

Also, podman image sign was not extended to support sigstore.

See individual commit messages for details.

Does this PR introduce a user-facing change?

Added the ability to create sigstore signatures in `podman push` and `podman manifest push`.
Added an option to read image signing passphrase from a file.

openshift-ci · 2022-07-28T22:21:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mtrmac]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mtrmac · 2022-07-28T22:22:26Z

Draft:

~~Depends on unmerged libimage: Allow adding sigstore signatures, and submitting passphrases common#1106~~
~~I didn’t even compile-test this yet; I’ll try this tomorrow.~~

mtrmac · 2022-07-29T18:05:02Z

This implements the podman push part of #7705. (Not the podman image sign part.)

test/e2e/push_test.go

htpasswd is no longer included in docker.io/library/distribution after 2.7.0, per distribution/distribution-library-image#107 , and we want to upgrade to a recent version. At least system tests currently execute htpasswd from the OS, so it seems that it is likely to be available. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2022-07-30T03:30:45Z

This now includes (via a merge commit) #15122, so that tests can run.

After the registry update PR is merged is merged, this can be rebased on top — or, I guess, merged as is.

... instead of hard-coding a copy of the value. Notably this makes hack/podman_registry actually support the documented -i option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... primarily so that it can support OCI artifacts. 2.8 already seems to exist in the repo. This requires changing WaitContainerReady to also check stderr (ultimately because docker/distribution was updated to a more recent sirupsen/logrus, which logs by default to stderr instead of stdout). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to get containers/common#1106 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

AFAICS it is not used anywhere. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... because it is documented to be ignored. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

- Allow creating sigstore signatures via --sign-by-sigstore-private-key . Like existing --sign-by, it does not work remote (in this case because we would have to copy the private key to the server). - Allow passing a passphrase (which is mandatory for sigstore private keys) via --sign-passphrase-file; if it is not provided, prompt interactively. - Also, use that passphrase for --sign-by as well, allowing non-interactive GPG use. (But --sign-passphrase-file can only be used with _one of_ --sign-by and --sign-by-sigstore-private-key.) Note that unlike the existing code, (podman build) does not yet implement sigstore (I'm not sure why it needs to, it seems not to push images?) because Buildah does not expose the feature yet. Also, (podman image sign) was not extended to support sigstore. The test for this follows existing (podman image sign) tests and doesn't work rootless; that could be improved by exposing a registries.d override option. The test for push is getting large; I didn't want to start yet another registry container, but that would be an alternative. In the future, Ginkgo's Ordered/BeforeAll would allow starting a registry once and using it for two tests. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2022-08-01T01:29:28Z

Ready for review, see #15108 (comment) about whether it should be merged.

vrothberg

LGTM

vrothberg · 2022-08-01T06:29:05Z

Ready for review, see #15108 (comment) about whether it should be merged.

I am good to merge as is.

@rhatdan @Luap99 PTAL

Luap99 · 2022-08-01T09:29:34Z

test/e2e/testdata/sigstore-key.key

@@ -0,0 +1,11 @@
+-----BEGIN ENCRYPTED COSIGN PRIVATE KEY-----


Do we really want to commit private keys for testing? IMO keys should be automatically generated before the tests and we should not used fixed ones.

The format for this is cosign-specific. Right now, to generate this, we would need to install cosign during tests.

Eventually Skopeo will be taught to generate the key pair, and/or we might accept keys in more standardized formats, but that doesn’t exist yet.

(To be explicit, this key pair was generated specifically for the tests and has no other use.)

I am fine with this, looks like we already have other private keys committed anyway.

My main concern is that we are testing this one key and not random generated keys like normal users would do.

flouthoc · 2022-08-01T10:49:45Z

test/utils/utils.go

@@ -225,7 +225,7 @@ func (p *PodmanTest) WaitContainerReady(id string, expStr string, timeout int, s
 			return false
 		}

-		if strings.Contains(s.OutputToString(), expStr) {
+		if strings.Contains(s.OutputToString(), expStr) || strings.Contains(s.ErrorToString(), expStr) {


This looks a bit weird, why are we matching stderr for a successful case. Should it be a different commit ?

Never mind I think reason for this is specified in the commit Update the registry server we test against from 2.6 to 2.8 .

Yes, that’s the reason. WaitContainerReady is most commonly used for the registry, so I took this fairly lazy route.

Alternatively, we could add an option whether to check stderr to this function, or maybe a specialized “start a registry and wait until ready” helper.

Luap99

LGTM

rhatdan · 2022-08-01T11:32:09Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jul 28, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2022

mtrmac mentioned this pull request Jul 28, 2022

libimage: Allow adding sigstore signatures, and submitting passphrases containers/common#1106

Merged

mtrmac force-pushed the sigstore-sign branch from c773da0 to 4769abe Compare July 29, 2022 18:02

mtrmac force-pushed the sigstore-sign branch 2 times, most recently from 0009b84 to 893e630 Compare July 29, 2022 21:58

mtrmac commented Jul 29, 2022

View reviewed changes

test/e2e/push_test.go Outdated Show resolved Hide resolved

mtrmac force-pushed the sigstore-sign branch from 893e630 to cd0148b Compare July 29, 2022 22:22

mtrmac mentioned this pull request Jul 30, 2022

RFC: tag v0.49.1 containers/common#1107

Merged

mtrmac force-pushed the sigstore-sign branch from cd0148b to e61afd6 Compare July 30, 2022 03:29

mtrmac force-pushed the sigstore-sign branch 2 times, most recently from 3bcb3d4 to 8d21c66 Compare July 30, 2022 15:17

mtrmac added 8 commits July 30, 2022 17:23

Use existing REGISTRY_IMAGE variables in more places

7599fde

... instead of hard-coding a copy of the value. Notably this makes hack/podman_registry actually support the documented -i option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Merge branch 'registry-2.8' into HEAD

b014780

Update c/common to an unreleased version

63d77da

... to get containers/common#1106 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove libpod/common

02f570e

AFAICS it is not used anywhere. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Use signByFlagName instead of copy&pasting the string

68cf116

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Hide podman manifest push --sign-by on remote

7075e2e

... because it is documented to be ignored. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the sigstore-sign branch from 8d21c66 to d462da6 Compare July 30, 2022 15:26

vrothberg reviewed Aug 1, 2022

View reviewed changes

vrothberg marked this pull request as ready for review August 1, 2022 06:29

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2022

vrothberg mentioned this pull request Aug 1, 2022

Update the registry server we test against from 2.6 to 2.8 #15122

Closed

Luap99 reviewed Aug 1, 2022

View reviewed changes

flouthoc mentioned this pull request Aug 1, 2022

manifest,test: inspect should contain OCI annotations. #15097

Merged

flouthoc reviewed Aug 1, 2022

View reviewed changes

Luap99 reviewed Aug 1, 2022

View reviewed changes

openshift-ci bot assigned rhatdan Aug 1, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022

openshift-merge-robot merged commit 82137dc into containers:main Aug 1, 2022

mtrmac deleted the sigstore-sign branch August 1, 2022 16:41

mtrmac mentioned this pull request Aug 1, 2022

[v4.2] Sigstore sign #15142

Merged

mtrmac mentioned this pull request Aug 23, 2022

Unattended Image signing #7705

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sigstore sign #15108

Sigstore sign #15108

mtrmac commented Jul 28, 2022

openshift-ci bot commented Jul 28, 2022

mtrmac commented Jul 28, 2022 •

edited

Loading

mtrmac commented Jul 29, 2022

mtrmac commented Jul 30, 2022

mtrmac commented Aug 1, 2022

vrothberg left a comment

vrothberg commented Aug 1, 2022

Luap99 Aug 1, 2022

mtrmac Aug 1, 2022

Luap99 Aug 1, 2022

flouthoc Aug 1, 2022

flouthoc Aug 1, 2022

mtrmac Aug 1, 2022

Luap99 left a comment

rhatdan commented Aug 1, 2022

		@@ -0,0 +1,11 @@
		-----BEGIN ENCRYPTED COSIGN PRIVATE KEY-----

Sigstore sign #15108

Sigstore sign #15108

Conversation

mtrmac commented Jul 28, 2022

Does this PR introduce a user-facing change?

openshift-ci bot commented Jul 28, 2022

mtrmac commented Jul 28, 2022 • edited Loading

mtrmac commented Jul 29, 2022

mtrmac commented Jul 30, 2022

mtrmac commented Aug 1, 2022

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Aug 1, 2022

Luap99 Aug 1, 2022

Choose a reason for hiding this comment

mtrmac Aug 1, 2022

Choose a reason for hiding this comment

Luap99 Aug 1, 2022

Choose a reason for hiding this comment

flouthoc Aug 1, 2022

Choose a reason for hiding this comment

flouthoc Aug 1, 2022

Choose a reason for hiding this comment

mtrmac Aug 1, 2022

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

rhatdan commented Aug 1, 2022

mtrmac commented Jul 28, 2022 •

edited

Loading