signing: clarify build error when cgo is not available #1638

paralin · 2022-08-24T17:09:54Z

The build fails if CGO_ENABLED=0:

# github.com/containers/image/v5/signature
signature/mechanism_gpgme.go:156:41: undefined: gpgme.PinEntryLoopback
signature/mechanism_gpgme.go:161:31: undefined: gpgme.Key
signature/mechanism_gpgme.go:161:31: too many errors

Make it clear that cgo is required by adding a comment & undefined variable:

# github.com/containers/image/v5/signature
./mechanism_other.go:10:21: undefined: ContainersImageSignatureRequiresCgo

Anyone who encounters this error (like me) will immediately understand the issue and can
read the comment on that line to understand that the alternative build tag is
not actively maintained and considered not secure.

// CgoIsDisabled indicates as a compiler error that this package requires cgo.
//
// You may enable the containers_image_openpgp build tag but beware that this
// implementation is not actively maintained and is considered insecure.
var CgoIsDisabled = ContainersImageSignatureRequiresCgo

Fixes #1634

paralin · 2022-08-24T18:52:35Z

The second commit in this PR adds a new build tag to disable signing:

Possible use cases:

default: use cgo and enable libgpgme
CGO_ENABLED=0 -> build fails
CGO_ENABLED=0 and containers_image_openpgp -> build succeeds
CGO_ENABLED=0 and containers_image_disable_signing -> build succeeds but returns errors at runtime

The default is to compile in the endorsed cgo implementation and fail the build otherwise.

rhatdan · 2022-08-25T20:24:33Z

@vrothberg @mtrmac WDYT?

mtrmac

I’m fine with doing this, overall, though it does seem quite a bit of effort for what I think is a fairly small gain. Only users familiar enough with Go to use build tags are going to benefit, and those might find their way to the README either way.

I’m especially uncertain about the disable_signing part. If that tag is added:

It should be documented in README.md, so that it has some chance of being useful to anyone else
… and then I guess we need it to keep working, i. e. the tests must work (easily quadrupling the size of this PR), and the tests should be run in CI — and maybe we don’t want to pay for a complete separate CI step, so just run unit tests of ./signature/... with that tag?

mtrmac · 2022-08-26T19:35:10Z

signature/mechanism_test.go

@@ -1,3 +1,6 @@
+//go:build cgo || containers_image_openpgp
+// +build cgo containers_image_openpgp


If the goal is to make tests pass with containers_image_disable_signing (and I do think those tests should pass, because parts of c/image/signature are completely independent of the simple signing implementation), this doesn’t do that, and from a quick check it would be a rather larger effort — because individual expected results would have to be made conditional, correctly.

The goal was to make go test pass by excluding the signing tests unless a signing mechanism is compiled in.

I've fixed the build tags so that all three of these succeed:

go test -v CGO_ENABLED=0 go test -v CGO_ENABLED=0 go test -v -tags containers_image_disable_signing CGO_ENABLED=0 go test -v -tags containers_image_openpgp

signature/mechanism_other.go

mtrmac · 2022-08-26T19:40:10Z

signature/mechanism_none.go

@@ -0,0 +1,19 @@
+//go:build containers_image_disable_signing
+// +build containers_image_disable_signing


Something like

Suggested change

// +build containers_image_disable_signing

// +build containers_image_disable_simple_signing

(updated everywhere) because this does not prevent use of sigstore. (Cc: @rhatdan )

@mtrmac I suppose what I meant by disable_signing is "the signing mechanism is disabled so you won't be able to create new signatures" - am I correct that disabling mechanism*.go does this?

c/image/signature does not only do cryptography; it includes policies like “always allow” and “always reject”.

Of the cryptographic mechanisms, it now actually implements two: “simple signing”, which requires GPG, and “sigstore”, which doesn’t. All of the discussion so far was about disabling / choosing a mechanism underlying the “simple signing” part; “sigstore” is unaffected.

So, the tag name should refer (in some way) to simple signing in particular, and not to the concept of signing in general.

signature/mechanism_other.go

paralin · 2022-08-27T00:23:17Z

though it does seem quite a bit of effort for what I think is a fairly small gain

The PR in its current form fixes a number of issues for us downstream in package managers like Buildroot.org.

Running the entire test suite for Signatures even when it's not compiled in may be a lot of extra effort for a very small gain.

The way to avoid extra effort here is this: say (in README) that building containers/image with anything other than go build using the default build tags and with Cgo enabled "may work but is not supported" - in other words: let's fix the issues that are preventing compiling the packages in these cases, while the project does not formally support those use cases w/ test suites.

paralin · 2022-08-27T00:55:14Z

@mtrmac I've fixed the PR according to your comments - let me know if you need me to update the name of the tag.

mtrmac · 2022-08-27T14:59:03Z

README.md

+- `containers_image_openpgp`: Use a Golang-only OpenPGP implementation for signature verification instead of the default cgo/gpgme-based implementation;
+the primary downside is that creating new signatures with the Golang-only implementation is not supported.
+- `containers_image_disable_signing`: do not compile in any signing implementation. Compiles a stub which returns an error indicating that signing is disabled.


Both of these should refer to “simple signing signatures” or something like that. (I’m afraid I forgot to update that when adding sigstore.

Note to self: Make sure that is fixed either way.)

mtrmac · 2022-08-27T14:59:21Z

README.md

 - `containers_image_ostree`: Import `ostree:` transport in `github.com/containers/image/transports/alltransports`. This builds the library requiring the `libostree` development libraries. Otherwise a stub which reports that the transport is not supported gets used. The `github.com/containers/image/ostree` package is completely disabled
 and impossible to import when this build tag is not in use.

+Unsupported / untested / not-recommended flags:


This might be a bit too strong — we do test the OpenPGP variant.

mtrmac · 2022-08-27T15:01:38Z

signature/policy_eval_sigstore_test.go

@@ -1,7 +1,10 @@
-// Policy evaluation for prCosignSigned.
+//go:build cgo || containers_image_openpgp


This is the other signing implementation (“sigstore”) and needs to work, so it needs to be tested, even if “simple signing” is disabled.

mtrmac · 2022-08-27T15:02:02Z

signature/policy_eval_simple_test.go

@@ -1,3 +1,6 @@
+//go:build cgo || containers_image_openpgp


This contains things like “reject all images from this repo” and needs to work, so it needs to be tested, even if “simple signing” is disabled.

mtrmac · 2022-08-27T15:02:20Z

signature/policy_eval_test.go

@@ -1,3 +1,6 @@
+//go:build cgo || containers_image_openpgp


This needs to work in all cases.

mtrmac · 2022-08-27T15:04:29Z

signature/mechanism_none.go

@@ -0,0 +1,19 @@
+//go:build containers_image_disable_signing
+// +build containers_image_disable_signing


c/image/signature does not only do cryptography; it includes policies like “always allow” and “always reject”.

Of the cryptographic mechanisms, it now actually implements two: “simple signing”, which requires GPG, and “sigstore”, which doesn’t. All of the discussion so far was about disabling / choosing a mechanism underlying the “simple signing” part; “sigstore” is unaffected.

So, the tag name should refer (in some way) to simple signing in particular, and not to the concept of signing in general.

mtrmac

On second thought (which should have come with the first review already, and I apologize for not thinking this through), do we want the disable_signing tag at all?

If I understand it correctly, you don’t intend to use it.
I don’t know of anyone who wants to use it either.

And making that work without compromising the testing of the rest of the policy.json evaluation would probably be more work than just disabling test files; and some ongoing maintenance cost (if nothing else, the cost of computers running tests). If no-one benefits, is that work worthwhile?

So maybe let’s just add the CGoIsDisabled error reporting mechanism that you truly want, and not the other part.

WRT the tag name, (see #1638 (comment) for background), assuming the tag remains, I think …disable_simple_signing would be an accurate name but I’d prefer a second opinion to confirm.

rhatdan · 2022-10-12T19:41:11Z

@paralin Still working on this?

paralin · 2022-10-12T20:51:45Z

@rhatdan I'll revisit this shortly

The build fails if CGO_ENABLED=0: signature/mechanism_gpgme.go:156:41: undefined: gpgme.PinEntryLoopback signature/mechanism_gpgme.go:161:31: undefined: gpgme.Key signature/mechanism_gpgme.go:161:31: too many errors Make it clear that cgo is required by adding a comment & undefined variable: ./mechanism_other.go:10:21: undefined: ContainersImageSignatureRequiresCgo Anyone who encounters this error will immediately understand the issue and can read the comment on that line to understand that the alternative build tag is not actively maintained and considered not secure. Signed-off-by: Christian Stewart <christian@paral.in>

The build currently fails if CGO_ENABLED=0: ./mechanism_other.go:10:21: undefined: ContainersImageSignatureRequiresCgo Add a new build tag containers_image_disable_signing which, instead of failing to build, compiles in a stub which returns runtime errors when attempting to sign container images. If the user does not have cgo enabled they have two options: - tag containers_image_openpgp enables the insecure OpenPGP implementation. - tag containers_image_disable_signing returns runtime errors when signing. The default is to fail the build w/ an error clarifying why. Signed-off-by: Christian Stewart <christian@paral.in>

paralin force-pushed the stub-if-no-cgo branch from 5d704d5 to f6963dd Compare August 24, 2022 17:13

paralin mentioned this pull request Aug 24, 2022

Build fails if CGO_ENABLED=0 #1634

Closed

mtrmac reviewed Aug 26, 2022

View reviewed changes

paralin force-pushed the stub-if-no-cgo branch from db368e9 to 421a318 Compare August 27, 2022 00:54

mtrmac reviewed Aug 27, 2022

View reviewed changes

paralin added 2 commits November 23, 2022 06:59

paralin force-pushed the stub-if-no-cgo branch from 421a318 to b74fd98 Compare November 23, 2022 14:59

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 8, 2022

mtrmac mentioned this pull request May 9, 2023

fix: Fix the problem that contianers image v5 needs to add tag to bui… #1947

Closed

huyongqii mentioned this pull request Jul 7, 2023

fix: fix goreleaser build apecloud/kubeblocks#4159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signing: clarify build error when cgo is not available #1638

signing: clarify build error when cgo is not available #1638

paralin commented Aug 24, 2022 •

edited

paralin commented Aug 24, 2022

rhatdan commented Aug 25, 2022

mtrmac left a comment

mtrmac Aug 26, 2022

paralin Aug 27, 2022 •

edited

mtrmac Aug 26, 2022

paralin Aug 27, 2022

mtrmac Aug 27, 2022 •

edited

paralin commented Aug 27, 2022 •

edited

paralin commented Aug 27, 2022 •

edited

mtrmac Aug 27, 2022

mtrmac Aug 27, 2022

mtrmac Aug 27, 2022

mtrmac Aug 27, 2022

mtrmac Aug 27, 2022

mtrmac Aug 27, 2022 •

edited

mtrmac left a comment •

edited

rhatdan commented Oct 12, 2022

paralin commented Oct 12, 2022

		@@ -1,3 +1,6 @@
		//go:build cgo \|\| containers_image_openpgp
		// +build cgo containers_image_openpgp

		@@ -0,0 +1,19 @@
		//go:build containers_image_disable_signing
		// +build containers_image_disable_signing

	// +build containers_image_disable_signing
	// +build containers_image_disable_simple_signing

		@@ -1,7 +1,10 @@
		// Policy evaluation for prCosignSigned.
		//go:build cgo \|\| containers_image_openpgp

signing: clarify build error when cgo is not available #1638

Are you sure you want to change the base?

signing: clarify build error when cgo is not available #1638

Conversation

paralin commented Aug 24, 2022 • edited

paralin commented Aug 24, 2022

rhatdan commented Aug 25, 2022

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paralin Aug 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Aug 27, 2022 • edited

Choose a reason for hiding this comment

paralin commented Aug 27, 2022 • edited

paralin commented Aug 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Aug 27, 2022 • edited

Choose a reason for hiding this comment

mtrmac left a comment • edited

Choose a reason for hiding this comment

rhatdan commented Oct 12, 2022

paralin commented Oct 12, 2022

paralin commented Aug 24, 2022 •

edited

paralin Aug 27, 2022 •

edited

mtrmac Aug 27, 2022 •

edited

paralin commented Aug 27, 2022 •

edited

paralin commented Aug 27, 2022 •

edited

mtrmac Aug 27, 2022 •

edited

mtrmac left a comment •

edited