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

[v4.2.0-rhel] podman image trust overhaul, incl. sigstore #16295

Merged
merged 24 commits into from
Oct 26, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 25, 2022

This is a copy of #15533 = #15466 to v4.2.0-rhel, which seems to be necessary.

git cherry-pick 754ec89a8a185d308ca5ed08afaf34d6cbda08da...68ebf13d48be39b3509601527395f8ec2d56a329

Does this PR introduce a user-facing change?

- BREAKING CHANGE: `podman image trust show` may now show multiple entries for the same scope, to better represent separate requirements. GPG IDs on a single row now always represent alternative keys, only one of which is required; if multiple sets of keys are required, each is represented by a single line.
- Fixed `podman image trust set` not to silently discard unknown fields.
- Added support for `sigstoreSigned` to `podman image trust set`
- Added support for `sigstoreSigned` to `podman image trust show`
- Updated `podman image trust show` to recognize new `lookaside` field names
- Updated `podman image trust show` to recognize `keyPaths` in `signedBy` entries
- Fixed `podman image trust show` to potentially show signature enforcement configuration for the default scope
- Fixed `podman image trust show` to not silently ignore multiple kinds of requirements in a single scope

We can always recover it from git, but it seems to serve
no purpose anyway.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Split the existing code into policy.go and registries.go,
depending on which files it concerns.

Only moves unchanged code, should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Nothing uses it outside the package.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Only process the incoming args[] (which is a single-element array
for some reason) once, and use a semantic variable name for the value
we care about.

Should not change behavior, the only caller already supposedly ensures
that len(args) == 1.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will allow us to write unit tests without setting up the complete Podman runtime
(and without the Linux dependency).

Also, actually add a basic smoke test of the core functionality.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- Also reject public keys with types that don't use them
- Reject unknown trust types
- And add unit tests

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
That way, we don't have to switch over trustType twice.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
NOTE: This does not edit the use-sigstore-attachments value
in registries.d, similarly to how (podman image trust set) didn't
set the lookaside paths for simple signing.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will allow us to write unit tests without setting up the complete Podman runtime
(and without the Linux dependency).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We now have only a few entrypoints that are called externally,
so make the rest private.  This will make it more obvious that
we are not breaking any external users.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sort map keys instead of iterating in the Go-imposed random order.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Add at least a basic unit test for the various entry types.

So that we don't have to actually deal with GPG keys and /usr/bin/gpg*,
parametrize the code with a gpgIDReader , and pass a fake one
in the unit test.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will evetually allow us to use it for the default scope
as well, which currently uses a simplified version.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Now that it is the primary return value of a small function,
the long name only makes reading harder.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Just so that we don't have a boolean-named function returning a struct.
Also reorder the parameters to have the container first, and the lookup
key second.

Shoud not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... instead of taking a shortcut, e.g. not listing any keys if they are required.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Do the registries.d lookup once, separately from building
an entry, so that we can share it across entries.

Also prepare a separate res to allow adding multiple entries.

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

Currently
- the output uses the first entry's type, even if the requirements are different
  (notably signedBy + sigstoreSIgned)
- all public keys IDs are collected to a single line, even if some of them
  are interchangeable, and some are required (e.g. two signedBy requirements
  could require an image to be signed by (redhatProd OR redhatBeta) AND (vendor1 OR vendor2)

So, stop collapsing the requirements, and return a separate entry for each one. Multiple
GPG IDs on a single line used to mean AND or OR, now they always mean AND.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
sigstoreSigned does not have GPG IDs, so we add N/A in that column.

NOTE: this does not show the use-sigstore-attachments value from
registries.d.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to go from top to bottom.

Should not change behavior.

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

We are unmarshaling and re-marshaling JSON, which can _silently_ drop data
with the Go design decision.data.

Try harder, by using json.RawMessage at least for the data we care about.

Alternatively, this could use json.Decoder.DisallowUnknownFields.

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

openshift-ci bot commented Oct 25, 2022

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2022
@TomSweeneyRedHat
Copy link
Member

The aarch test is breaking in the build testing, I'm not sure why given the changes.....

[+1080s] not ok 270 bud --pull=false --arch test
[+1080s] # (from function `assert' in file tests/helpers.bash, line 467,
[+1080s] #  from function `expect_output' in file tests/helpers.bash, line 494,
[+1080s] #  in test file tests/bud.bats, line 3986)
[+1080s] #   `expect_output --substring arm64' failed
[+1080s] # /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.27.1/tests /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.27.1
[+1080s] # $ podman build --force-rm=false --layers=false -f Containerfile --pull=false -q --arch=amd64 -t image-amd --signature-policy /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.27.1/tests/policy.json /var/tmp/buildah_tests.16a0as/my-dir
[+1080s] # b06aacb2c1abad5048c2ff7431567bc7eec863734aaa12e8e510a4dca1031bbb
[+1080s] # $ buildah inspect --format {{ index .Docker.Config.Labels "architecture" }} image-amd
[+1080s] # x86_64
[+1080s] # $ buildah tag image-amd localhost/ubi8-minimal
[+1080s] # $ podman build --force-rm=false --layers=false -f Containerfile --pull=false -q --arch=arm64 -t image-arm --signature-policy /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.27.1/tests/policy.json /var/tmp/buildah_tests.16a0as/my-dir
[+1080s] # 7b878a1f3e2d8f299b4271c3b7e8680507d7a742aaf48a9612c4ee28f8181e46
[+1080s] # $ buildah inspect --format {{ index .Docker.Config.Labels "architecture" }} image-arm
[+1080s] # aarch64
[+1080s] # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
[+1080s] # #|     FAIL: buildah inspect --format {{ index .Docker.Config.Labels "architecture" }} image-arm
[+1080s] # #| expected: =~ 'arm64'
[+1080s] # #|   actual:    'aarch64'
[+1080s] # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[+1080s] # /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.27.1

@ypu
Copy link
Contributor

ypu commented Oct 26, 2022

Hi Tom
We found the same failed case in our test. But I think this should be a test case problem. As the base image's arch is aarch64:

# buildah inspect --format '{{ index .Docker.Config.Labels "architecture"  }}' registry.access.redhat.com/ubi8-minimal

aarch64

And I test it with an old ubi8 image, it report arm64:

# cat Containerfile 
FROM registry.access.redhat.com/ubi8-minimal:8.6-902
# buildah bud -f Containerfile  --pull=false -q --arch=arm64 -t image-arm-1  .
bd3f49f6af897d34cc4dc5fe2d95a4c2c0213f698dd4b2e9cccb0798b964eccb
# buildah inspect --format '{{ index .Docker.Config.Labels "architecture" }}'  registry.access.redhat.com/ubi8-minimal:8.6-902
arm64
#  buildah inspect --format '{{ index .Docker.Config.Labels "architecture" }}' image-arm-1
arm64

So I think this should caused by the recently change of ubi8-minimal image. We can just ignore it.

@TomSweeneyRedHat
Copy link
Member

@edsantiago any issues with pushing this one through? Looks to be a known issue in the ubi8 image that's not related to these changes.

LGTM, and FWIW, due to timing, we need to get this one decided upon quickly as our window to merge and get into the ZeroDay is closing quickly

@edsantiago
Copy link
Collaborator

Yes, feel free to force-merge. bud-test breakage is unrelated and will take a long, long time (days, well into next week) to fix.

@TomSweeneyRedHat
Copy link
Member

@baude can you take a look and merge if it LGTY please?

@baude
Copy link
Member

baude commented Oct 26, 2022

merging manually ....

@baude baude merged commit 35c0df3 into containers:v4.2.0-rhel Oct 26, 2022
@mtrmac mtrmac deleted the backport-trust-again branch October 26, 2022 17:47
@TomSweeneyRedHat
Copy link
Member

@github-actions 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 github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants