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

Allow accepting multiple GPG keyrings via signedBy.keyPaths #1609

Merged
merged 8 commits into from
Jul 13, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 13, 2022

See individual commit messages for details.

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 PTAL

keySources++
}
if keySources != 1 {
return nil, InvalidPolicyFormatError("exactly one of keyPath and keyData can be specified")
Copy link
Member

Choose a reason for hiding this comment

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

Should be
"exactly one of keyPath and keyData must be specified"
Since this will fail if neither is specified.

if keyData != nil {
keySources++
}
if keySources != 1 {
return nil, InvalidPolicyFormatError("exactly one of keyPath and keyData can be specified")
return nil, InvalidPolicyFormatError("exactly one of keyPath, keyPaths and keyData can be specified")
Copy link
Member

Choose a reason for hiding this comment

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

	return nil, InvalidPolicyFormatError("exactly one of keyPath, keyPaths and keyData must be specified")

@rhatdan
Copy link
Member

rhatdan commented Jul 13, 2022

A couple of wording nits, then LGTM

Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
... to make it easier to figure out which of breakFns failed.

Signed-off-by: Miloslav Trmač <[email protected]>
... to make sure we are exercising exactly the failure pathss we are
trying to test.

Signed-off-by: Miloslav Trmač <[email protected]>
... to make it a bit easier to add one more variant.

Should not change behavior (apart from error messages).

Signed-off-by: Miloslav Trmač <[email protected]>
... to make it easier to add more cases.

Should not change (test) behavior.

Signed-off-by: Miloslav Trmač <[email protected]>
A user will be added momentarily.

For now, should not change behavior.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 13, 2022

Updated, also fixed one more typo.

@rhatdan
Copy link
Member

rhatdan commented Jul 13, 2022

LGTM

@rhatdan rhatdan merged commit cf6ccb9 into containers:main Jul 13, 2022
@mtrmac mtrmac deleted the keyFiles branch July 14, 2022 09:16
ondrejbudai added a commit to ondrejbudai/osbuild-composer that referenced this pull request Aug 28, 2022
Version 5.22 introduced a new option to /etc/containers/policy.json called
keyPaths, see

containers/image#1609

EL9 immediately took advantage of this new feature and started using it, see
https://gitlab.com/redhat/centos-stream/rpms/containers-common/-/commit/04645c4a84442da3324eea8f6538a5768e69919a

This quickly became an issue in our code: The go library (containers/image)
parses the configuration file very strictly and refuses to create a client
when policy.json with an unknown key is present on the filesystem. As we
used 5.21.1 that doesn't know the new key, our unit tests started to
failing when containers-common was present.

Reproducer:
podman run --pull=always --rm -it centos:stream9
dnf install -y dnf-plugins-core
dnf config-manager --set-enabled crb
dnf install -y gpgme-devel libassuan-devel krb5-devel golang git-core
git clone https://github.com/osbuild/osbuild-composer
cd osbuild-composer

# install the new containers-common and run the test
dnf install -y https://kojihub.stream.centos.org/kojifiles/packages/containers-common/1/44.el9/x86_64/containers-common-1-44.el9.x86_64.rpm
go test -count 1 ./...

# this returns:
--- FAIL: TestClientResolve (0.00s)
    client_test.go:31:
        	Error Trace:	client_test.go:31
        	Error:      	Received unexpected error:
        	            	Unknown key "keyPaths"
        	            	invalid policy in "/etc/containers/policy.json"
        	            	github.com/containers/image/v5/signature.NewPolicyFromFile
        	            		/osbuild-composer/vendor/github.com/containers/image/v5/signature/policy_config.go:88
        	            	github.com/osbuild/osbuild-composer/internal/container.NewClient
        	            		/osbuild-composer/internal/container/client.go:123
        	            	github.com/osbuild/osbuild-composer/internal/container_test.TestClientResolve
        	            		/osbuild-composer/internal/container/client_test.go:29
        	            	testing.tRunner
        	            		/usr/lib/golang/src/testing/testing.go:1439
        	            	runtime.goexit
        	            		/usr/lib/golang/src/runtime/asm_amd64.s:1571
        	Test:       	TestClientResolve
    client_test.go:32:
        	Error Trace:	client_test.go:32
        	Error:      	Expected value not to be nil.
        	Test:       	TestClientResolve

 When run with an older containers-common, it succeeds:
 dnf install -y https://kojihub.stream.centos.org/kojifiles/packages/containers-common/1/40.el9/x86_64/containers-common-1-40.el9.x86_64.rpm
 go test -count 1 ./...
 PASS

To sum it up, I had to upgrade github.com/containers/image/v5 to v5.22.0.
Unfortunately, this wasn't so simple, see

go get github.com/containers/image/v5@latest
go: github.com/containers/image/[email protected] requires
	github.com/letsencrypt/[email protected] requires
	github.com/honeycombio/[email protected] requires
	github.com/gobuffalo/pop/[email protected] requires
	github.com/mattn/[email protected]+incompatible: reading github.com/mattn/go-sqlite3/go.mod at revision v2.0.3: unknown revision v2.0.3

It turns out that github.com/mattn/[email protected]+incompatible has been
recently retracted mattn/go-sqlite3#998 and this
broke a ton of packages depending on it. I was able to fix it by adding

exclude github.com/mattn/go-sqlite3 v2.0.3+incompatible

to our go.mod, see
mattn/go-sqlite3#975 (comment)

After adding it,
go get github.com/containers/image/v5@latest
succeeded and tools/prepare-source.sh took care of the rest.

Signed-off-by: Ondřej Budai <[email protected]>
ondrejbudai added a commit to ondrejbudai/osbuild-composer that referenced this pull request Aug 28, 2022
Version 5.22 introduced a new option to /etc/containers/policy.json called
keyPaths, see

containers/image#1609

EL9 immediately took advantage of this new feature and started using it, see
https://gitlab.com/redhat/centos-stream/rpms/containers-common/-/commit/04645c4a84442da3324eea8f6538a5768e69919a

This quickly became an issue in our code: The go library (containers/image)
parses the configuration file very strictly and refuses to create a client
when policy.json with an unknown key is present on the filesystem. As we
used 5.21.1 that doesn't know the new key, our unit tests started to
failing when containers-common was present.

Reproducer:
podman run --pull=always --rm -it centos:stream9
dnf install -y dnf-plugins-core
dnf config-manager --set-enabled crb
dnf install -y gpgme-devel libassuan-devel krb5-devel golang git-core
git clone https://github.com/osbuild/osbuild-composer
cd osbuild-composer

# install the new containers-common and run the test
dnf install -y https://kojihub.stream.centos.org/kojifiles/packages/containers-common/1/44.el9/x86_64/containers-common-1-44.el9.x86_64.rpm
go test -count 1 ./...

# this returns:
--- FAIL: TestClientResolve (0.00s)
    client_test.go:31:
        	Error Trace:	client_test.go:31
        	Error:      	Received unexpected error:
        	            	Unknown key "keyPaths"
        	            	invalid policy in "/etc/containers/policy.json"
        	            	github.com/containers/image/v5/signature.NewPolicyFromFile
        	            		/osbuild-composer/vendor/github.com/containers/image/v5/signature/policy_config.go:88
        	            	github.com/osbuild/osbuild-composer/internal/container.NewClient
        	            		/osbuild-composer/internal/container/client.go:123
        	            	github.com/osbuild/osbuild-composer/internal/container_test.TestClientResolve
        	            		/osbuild-composer/internal/container/client_test.go:29
        	            	testing.tRunner
        	            		/usr/lib/golang/src/testing/testing.go:1439
        	            	runtime.goexit
        	            		/usr/lib/golang/src/runtime/asm_amd64.s:1571
        	Test:       	TestClientResolve
    client_test.go:32:
        	Error Trace:	client_test.go:32
        	Error:      	Expected value not to be nil.
        	Test:       	TestClientResolve

 When run with an older containers-common, it succeeds:
 dnf install -y https://kojihub.stream.centos.org/kojifiles/packages/containers-common/1/40.el9/x86_64/containers-common-1-40.el9.x86_64.rpm
 go test -count 1 ./...
 PASS

To sum it up, I had to upgrade github.com/containers/image/v5 to v5.22.0.
Unfortunately, this wasn't so simple, see

go get github.com/containers/image/v5@latest
go: github.com/containers/image/[email protected] requires
	github.com/letsencrypt/[email protected] requires
	github.com/honeycombio/[email protected] requires
	github.com/gobuffalo/pop/[email protected] requires
	github.com/mattn/[email protected]+incompatible: reading github.com/mattn/go-sqlite3/go.mod at revision v2.0.3: unknown revision v2.0.3

It turns out that github.com/mattn/[email protected]+incompatible has been
recently retracted mattn/go-sqlite3#998 and this
broke a ton of packages depending on it. I was able to fix it by adding

exclude github.com/mattn/go-sqlite3 v2.0.3+incompatible

to our go.mod, see
mattn/go-sqlite3#975 (comment)

After adding it,
go get github.com/containers/image/v5@latest
succeeded and tools/prepare-source.sh took care of the rest.

Signed-off-by: Ondřej Budai <[email protected]>
thozza pushed a commit to osbuild/osbuild-composer that referenced this pull request Aug 29, 2022
Version 5.22 introduced a new option to /etc/containers/policy.json called
keyPaths, see

containers/image#1609

EL9 immediately took advantage of this new feature and started using it, see
https://gitlab.com/redhat/centos-stream/rpms/containers-common/-/commit/04645c4a84442da3324eea8f6538a5768e69919a

This quickly became an issue in our code: The go library (containers/image)
parses the configuration file very strictly and refuses to create a client
when policy.json with an unknown key is present on the filesystem. As we
used 5.21.1 that doesn't know the new key, our unit tests started to
failing when containers-common was present.

Reproducer:
podman run --pull=always --rm -it centos:stream9
dnf install -y dnf-plugins-core
dnf config-manager --set-enabled crb
dnf install -y gpgme-devel libassuan-devel krb5-devel golang git-core
git clone https://github.com/osbuild/osbuild-composer
cd osbuild-composer

# install the new containers-common and run the test
dnf install -y https://kojihub.stream.centos.org/kojifiles/packages/containers-common/1/44.el9/x86_64/containers-common-1-44.el9.x86_64.rpm
go test -count 1 ./...

# this returns:
--- FAIL: TestClientResolve (0.00s)
    client_test.go:31:
        	Error Trace:	client_test.go:31
        	Error:      	Received unexpected error:
        	            	Unknown key "keyPaths"
        	            	invalid policy in "/etc/containers/policy.json"
        	            	github.com/containers/image/v5/signature.NewPolicyFromFile
        	            		/osbuild-composer/vendor/github.com/containers/image/v5/signature/policy_config.go:88
        	            	github.com/osbuild/osbuild-composer/internal/container.NewClient
        	            		/osbuild-composer/internal/container/client.go:123
        	            	github.com/osbuild/osbuild-composer/internal/container_test.TestClientResolve
        	            		/osbuild-composer/internal/container/client_test.go:29
        	            	testing.tRunner
        	            		/usr/lib/golang/src/testing/testing.go:1439
        	            	runtime.goexit
        	            		/usr/lib/golang/src/runtime/asm_amd64.s:1571
        	Test:       	TestClientResolve
    client_test.go:32:
        	Error Trace:	client_test.go:32
        	Error:      	Expected value not to be nil.
        	Test:       	TestClientResolve

 When run with an older containers-common, it succeeds:
 dnf install -y https://kojihub.stream.centos.org/kojifiles/packages/containers-common/1/40.el9/x86_64/containers-common-1-40.el9.x86_64.rpm
 go test -count 1 ./...
 PASS

To sum it up, I had to upgrade github.com/containers/image/v5 to v5.22.0.
Unfortunately, this wasn't so simple, see

go get github.com/containers/image/v5@latest
go: github.com/containers/image/[email protected] requires
	github.com/letsencrypt/[email protected] requires
	github.com/honeycombio/[email protected] requires
	github.com/gobuffalo/pop/[email protected] requires
	github.com/mattn/[email protected]+incompatible: reading github.com/mattn/go-sqlite3/go.mod at revision v2.0.3: unknown revision v2.0.3

It turns out that github.com/mattn/[email protected]+incompatible has been
recently retracted mattn/go-sqlite3#998 and this
broke a ton of packages depending on it. I was able to fix it by adding

exclude github.com/mattn/go-sqlite3 v2.0.3+incompatible

to our go.mod, see
mattn/go-sqlite3#975 (comment)

After adding it,
go get github.com/containers/image/v5@latest
succeeded and tools/prepare-source.sh took care of the rest.

Signed-off-by: Ondřej Budai <[email protected]>
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.

3 participants