image sign using per user registries.d #7357

QiWang19 · 2020-08-18T14:52:01Z

Support per user ~/.config/containers/registries.d to allow rootless image sign configurations.

Signed-off-by: Qi Wang qiwan@redhat.com

rhatdan · 2020-08-18T18:36:16Z

@QiWang19 Rebase and you should be able to pass the tests.

docs/source/markdown/podman-image-sign.1.md

QiWang19 · 2020-08-28T00:13:44Z

@mtrmac @rhatdan PTAL

rhatdan · 2020-08-28T09:58:38Z

LGTM
@mtrmac PTAL

rhatdan · 2020-09-03T10:57:37Z

@mtrmac @vrothberg PTAL

vrothberg

I really want a head nod from @mtrmac. I may very well miss details/side effects I am may not be aware of.

vrothberg · 2020-09-07T10:06:09Z

docs/source/markdown/podman-image-sign.1.md

@@ -38,7 +40,7 @@ Sign the busybox image with the identify of foo@bar.com with a user's keyring an
 ## RELATED CONFIGURATION

 The write (and read) location for signatures is defined in YAML-based
-configuration files in /etc/containers/registries.d/.  When you sign
+configuration files in /etc/containers/registries.d/ for root user, or $HOME/.config/containers/registries.d for non-root user.  When you sign


Suggested change

configuration files in /etc/containers/registries.d/ for root user, or $HOME/.config/containers/registries.d for non-root user. When you sign

configuration files in /etc/containers/registries.d/ for root, or $HOME/.config/containers/registries.d for non-root users. When you sign

vrothberg · 2020-09-07T10:14:42Z

docs/source/markdown/podman-image-sign.1.md

-derived from the registry configuration files in /etc/containers/registries.d. By default, the signature will be written into /var/lib/containers/sigstore directory.
+derived from the registry configuration files in `$HOME/.config/containers/registries.d` if exists, otherwise `/etc/containers/registries.d` (unless overridden at compile-time), see **containers-registries.d(5)** for more information.
+By default, the signature will be written into `/var/lib/containers/sigstore` from `/etc/containers/registries.d/default.yaml`, if the rootless user has no privilege to write the signature, the YAML format files
+under `$HOME/.config/containers/registries.d` can be configured, otherwise, `$HOME/.local/share/containers/sigstore` will be used.


The comment doesn't seem to match the code. We don't seem to check for permissions but use the rootless paths if they exist even for root.

mtrmac

I’d prefer for this to be merged only after or together with containers/image#1035 to make sure the two are consistent.

Right now, the code LGTM, though the documentation does not match the code and needs updating.

The path lookup logic is complex enough that it probably makes sense to export c/image/docker.configuredSignatureStorageBase (with a better name) now, and use it here instead of maintaining a second copy of the code. (While overall we do want readers/writers to use the c/image API to access signatures instead of raw access, this has clearly not stopped podman sign, and modifying podman sign to use a c/image/docker ImageDestination would now incompatibly introduce a requirement to be able to connect to the registry.)

mtrmac · 2020-09-07T22:02:54Z

pkg/trust/trust.go

-		if sys.RootForImplicitAbsolutePaths != "" {
-			return filepath.Join(sys.RootForImplicitAbsolutePaths, systemRegistriesDirPath)
-		}
+	userRegistriesDir := filepath.FromSlash(".config/containers/registries.d")


Should these two be a top-level commented variables like the others?

mtrmac · 2020-09-07T22:04:06Z

pkg/domain/infra/abi/images.go

+				if rootless.IsRootless() {
+					sigStoreURI = trust.RootlessSignatureStoreURL()
+				} else {
+					sigStoreURI = trust.DefaultSignatureStoreURL()


Dealing with URLs here seems to be an unnecessary complication; move the localPathFromURI call to the registryInfo != nil case, and this can just work with the sigStoreDir path.

github-actions · 2020-10-20T00:20:08Z

A friendly reminder that this PR had no activity for 30 days.

rhatdan · 2020-10-20T11:31:06Z

@QiWang19 This needs a rebase. Is this still being worked on?

QiWang19 · 2020-10-20T15:50:33Z

@rhatdan still being worked on, waiting to get this commit from c/image containers/image@1a0dda7

rhatdan · 2020-10-27T20:16:04Z

@QiWang19 you should have the containers/image PR in podman now.

QiWang19 · 2020-10-28T13:36:50Z

@mtrmac @vrothberg PTAL

pkg/trust/trust.go

QiWang19 · 2020-11-03T15:25:25Z

@mtrmac PTAL

QiWang19 · 2020-12-02T19:29:00Z

@containers/podman-maintainers PTAL

TomSweeneyRedHat · 2020-12-02T20:52:06Z

docs/source/markdown/podman-image-sign.1.md

@@ -9,7 +9,9 @@ podman-image-sign - Create a signature for an image
 ## DESCRIPTION
 **podman image sign** will create a local signature for one or more local images that have
 been pulled from a registry. The signature will be written to a directory
-derived from the registry configuration files in /etc/containers/registries.d. By default, the signature will be written into /var/lib/containers/sigstore directory.
+derived from the registry configuration files in `$HOME/.config/containers/registries.d` if exists,


Suggested change

derived from the registry configuration files in `$HOME/.config/containers/registries.d` if exists,

derived from the registry configuration files in `$HOME/.config/containers/registries.d` if it exists,

TomSweeneyRedHat · 2020-12-02T20:53:54Z

docs/source/markdown/podman-image-sign.1.md

@@ -9,7 +9,9 @@ podman-image-sign - Create a signature for an image
 ## DESCRIPTION
 **podman image sign** will create a local signature for one or more local images that have
 been pulled from a registry. The signature will be written to a directory
-derived from the registry configuration files in /etc/containers/registries.d. By default, the signature will be written into /var/lib/containers/sigstore directory.
+derived from the registry configuration files in `$HOME/.config/containers/registries.d` if exists,
+otherwise `/etc/containers/registries.d` (unless overridden at compile-time), see **containers-registries.d(5)** for more information.


looks like we don't list containers-registries-d(5) at the bottom of this man page and I think we should.

mtrmac

These code changes LGTM.

To be consistent, though, I think this PR should also update getPolicyShowOutput, the other user of trust.LoadAndMergeConfig+trust.HaveMatchRegistry (and then delete them), so that that function also correctly shows the new built-in default paths.

A downside to that is that that function will now always show a sigstore, even for X-R-S-S registries (and there isn’t a way to tell whether a registry supports X-R-S-S without connecting to it), but that’s probably better than not showing a default sigstore path that would get used.

mtrmac · 2020-12-03T20:50:14Z

pkg/domain/infra/abi/images.go

+			if path.Clean(repo) != repo { // Coverage: This should not be reachable because /./ and /../ components are not valid in docker references
+				return nil, errors.Errorf("Unexpected path elements in Docker reference %s for signature storage", dockerReference.String())
+			}
+			sigStoreDir = filepath.Join(options.Directory, repo)
 		}
 		if sigStoreDir == "" {


Non-blocking: This can just be an else.

QiWang19 · 2020-12-04T17:46:59Z

These code changes LGTM.

To be consistent, though, I think this PR should also update getPolicyShowOutput, the other user of trust.LoadAndMergeConfig+trust.HaveMatchRegistry (and then delete them), so that that function also correctly shows the new built-in default paths.

replace the above with docker.SignatureStorageBaseURL needs pass in an ImageReference, is there a good way to form an imageReference from the transports configurations in policy.json?

mtrmac · 2020-12-04T18:51:44Z

These code changes LGTM.
To be consistent, though, I think this PR should also update getPolicyShowOutput, the other user of trust.LoadAndMergeConfig+trust.HaveMatchRegistry (and then delete them), so that that function also correctly shows the new built-in default paths.

replace the above with docker.SignatureStorageBaseURL needs pass in an ImageReference, is there a good way to form an imageReference from the transports configurations in policy.json?

Ouch… no. There is one case that works ( a policy.json scope that includes a tag or digest); all the rest does not:

For a host[:port]/…/repo scope, it would be possible to look up e.g. …/repo:latest, but that might not be correct if :latest has a different configuration section from the rest of the repo.
For a host[:port], it is impossible to look up without inventing a repo+tag/digest, and that again could clash with a more specific configuration section.
In the worst case, there will be new support for *.domain.com and the like in policy.json, so there could be multiple equally right answers if a.domain.com and b.domain.com are configured with different sigstores.

The last one is essentially unfixable given the current trust.Policy data structure and CLI output, so I suppose returning nothing if the existing code doesn’t find an exact match is as close as we can get.

mtrmac

LGTM.

openshift-ci-robot · 2020-12-04T18:52:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, QiWang19, rhatdan

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~~ [rhatdan]

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

umohnani8 · 2020-12-07T14:32:06Z

Changes LGTM, @QiWang19 needs a rebase though

Support per user ~/.config/containers/registries.d to allow rootless image sign configurations. Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 · 2020-12-07T17:49:17Z

Needs lgtm label.

umohnani8 · 2020-12-07T17:58:32Z

/lgtm
/hold

@mheon should we hold for the github-check failure?

mheon · 2020-12-07T18:59:57Z

We can't do anything about the RDO check, but it's not a blocking failure, we can merge without it.
/hold cancel

openshift-ci-robot requested review from mrunalp and rhatdan August 18, 2020 14:52

rhatdan approved these changes Aug 18, 2020

View reviewed changes

docs/source/markdown/podman-image-sign.1.md Outdated Show resolved Hide resolved

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2020

QiWang19 force-pushed the rootless-sign branch 2 times, most recently from e1bc559 to 54bf987 Compare August 18, 2020 19:03

QiWang19 force-pushed the rootless-sign branch 3 times, most recently from d7f9355 to b508366 Compare August 27, 2020 19:40

QiWang19 force-pushed the rootless-sign branch 2 times, most recently from e2703b0 to 725bb9b Compare September 2, 2020 18:46

vrothberg reviewed Sep 7, 2020

View reviewed changes

mtrmac reviewed Sep 7, 2020

View reviewed changes

QiWang19 marked this pull request as draft September 18, 2020 19:46

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2020

QiWang19 force-pushed the rootless-sign branch 2 times, most recently from 589485c to ef0f44a Compare September 19, 2020 00:59

github-actions bot added the stale-pr label Oct 20, 2020

QiWang19 mentioned this pull request Oct 27, 2020

Sign multi-arch images #7718

Merged

QiWang19 force-pushed the rootless-sign branch 2 times, most recently from a5236b6 to 6103c23 Compare October 27, 2020 21:30

QiWang19 marked this pull request as ready for review October 27, 2020 21:33

QiWang19 force-pushed the rootless-sign branch from 6103c23 to c3831ee Compare October 28, 2020 13:35

rhatdan reviewed Oct 29, 2020

View reviewed changes

pkg/trust/trust.go Show resolved Hide resolved

rhatdan removed the stale-pr label Nov 4, 2020

QiWang19 force-pushed the rootless-sign branch from c3831ee to 6af2958 Compare December 2, 2020 19:28

TomSweeneyRedHat reviewed Dec 2, 2020

View reviewed changes

QiWang19 force-pushed the rootless-sign branch from 6af2958 to beff3f0 Compare December 2, 2020 21:14

mtrmac reviewed Dec 3, 2020

View reviewed changes

mtrmac approved these changes Dec 4, 2020

View reviewed changes

QiWang19 force-pushed the rootless-sign branch from beff3f0 to 6056ec5 Compare December 4, 2020 20:06

image sign using per user registries.d

cf8f234

Support per user ~/.config/containers/registries.d to allow rootless image sign configurations. Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 force-pushed the rootless-sign branch from 6056ec5 to cf8f234 Compare December 7, 2020 14:58

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2020

openshift-ci-robot assigned umohnani8 Dec 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2020

openshift-merge-robot merged commit 51166d0 into containers:master Dec 7, 2020

edsantiago mentioned this pull request Dec 14, 2020

CI flake: sdnotify-conmon test: getting MAINPID=1 instead of READY #8718

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 24, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image sign using per user registries.d #7357

image sign using per user registries.d #7357

QiWang19 commented Aug 18, 2020

rhatdan commented Aug 18, 2020

QiWang19 commented Aug 28, 2020

rhatdan commented Aug 28, 2020

rhatdan commented Sep 3, 2020

vrothberg left a comment

vrothberg Sep 7, 2020

vrothberg Sep 7, 2020

mtrmac left a comment

mtrmac Sep 7, 2020

mtrmac Sep 7, 2020

github-actions bot commented Oct 20, 2020

rhatdan commented Oct 20, 2020

QiWang19 commented Oct 20, 2020

rhatdan commented Oct 27, 2020

QiWang19 commented Oct 28, 2020

QiWang19 commented Nov 3, 2020

QiWang19 commented Dec 2, 2020

TomSweeneyRedHat Dec 2, 2020

TomSweeneyRedHat Dec 2, 2020

mtrmac left a comment •

edited

Loading

mtrmac Dec 3, 2020

QiWang19 commented Dec 4, 2020

mtrmac commented Dec 4, 2020

mtrmac left a comment

openshift-ci-robot commented Dec 4, 2020

umohnani8 commented Dec 7, 2020

QiWang19 commented Dec 7, 2020

umohnani8 commented Dec 7, 2020

mheon commented Dec 7, 2020

	configuration files in /etc/containers/registries.d/ for root user, or $HOME/.config/containers/registries.d for non-root user. When you sign
	configuration files in /etc/containers/registries.d/ for root, or $HOME/.config/containers/registries.d for non-root users. When you sign

	derived from the registry configuration files in `$HOME/.config/containers/registries.d` if exists,
	derived from the registry configuration files in `$HOME/.config/containers/registries.d` if it exists,

image sign using per user registries.d #7357

image sign using per user registries.d #7357

Conversation

QiWang19 commented Aug 18, 2020

rhatdan commented Aug 18, 2020

QiWang19 commented Aug 28, 2020

rhatdan commented Aug 28, 2020

rhatdan commented Sep 3, 2020

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Sep 7, 2020

Choose a reason for hiding this comment

vrothberg Sep 7, 2020

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Sep 7, 2020

Choose a reason for hiding this comment

mtrmac Sep 7, 2020

Choose a reason for hiding this comment

github-actions bot commented Oct 20, 2020

rhatdan commented Oct 20, 2020

QiWang19 commented Oct 20, 2020

rhatdan commented Oct 27, 2020

QiWang19 commented Oct 28, 2020

QiWang19 commented Nov 3, 2020

QiWang19 commented Dec 2, 2020

TomSweeneyRedHat Dec 2, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat Dec 2, 2020

Choose a reason for hiding this comment

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

mtrmac Dec 3, 2020

Choose a reason for hiding this comment

QiWang19 commented Dec 4, 2020

mtrmac commented Dec 4, 2020

mtrmac left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 4, 2020

umohnani8 commented Dec 7, 2020

QiWang19 commented Dec 7, 2020

umohnani8 commented Dec 7, 2020

mheon commented Dec 7, 2020

mtrmac left a comment •

edited

Loading