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

Build fails if CGO_ENABLED=0 #1634

Closed
paralin opened this issue Aug 22, 2022 · 19 comments · May be fixed by #1638
Closed

Build fails if CGO_ENABLED=0 #1634

paralin opened this issue Aug 22, 2022 · 19 comments · May be fixed by #1638

Comments

@paralin
Copy link

paralin commented Aug 22, 2022

I'm working on a Buildroot package for Podman.

Cgo is not available on many platforms. libgpgme (used for signing) requires cgo.

#1632 (closed) proposed to enable openpgp when cgo is unavailable.

The package should compile when CGO_ENABLED=0 - with some kind of alternative implementation to libgpgme that is go-only.

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 23, 2022

I’m afraid it’s pretty much impossible to get a functionally equivalent alternative without CGo — because in a garbage-collected language, Go code has no control about copies of private key material, and no capability to erase it after use (which was a theoretical concern right until Heartbleed). (The lack of zeroization doesn’t technically prevent users from working with private keys in Go, and many do, of course.) Similarly side-channel-minimizing implementation are pretty hard to do in a higher-level language.

So there are trade-offs. These trade-off manifest in that the containers_image_openpgp implementation doesn’t support signing at all, although that could, in principle, be implemented, and that even the verification side requires opt-in to use this implementation.

Another trade-off is that any extra alternative implementations are not likely to be very well maintained. The openpgp package itself is abandoned upstream.

No amount of automatic magic can make these trade-offs go away so that users just don’t need to care — especially if there really are platforms that plain can’t use CGo; in that case we can’t do a good job with any amount of effort.


Note that the package does compile without CGo: Learn about the trade-off, make a choice, and add the build tag.

Yes, that breaks plain go build.

@paralin
Copy link
Author

paralin commented Aug 23, 2022

@mtrmac thanks for the background information

But this issue is still not solved; building on platforms without cgo fails.

There seems to be two solutions: add stubs that return errors when no implementation is compiled in, and use those for !cgo, or auto enable openpgp if !cgo.

Since the latter was already rejected, are we all good with the former? I'll send a pr...

@rhatdan
Copy link
Member

rhatdan commented Aug 23, 2022

I think the former should be ok. Just functionality will be limited.

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 23, 2022

IMHO the user should just make a choice and add a tag. That’s not impossible, is it?

Hypothetically, I think you wouldn’t accept a containers_image_yes_i_know_signed_image_are_going_to_be_rejected Go tag (which I’m not really proposing) either, because it isn’t automatic with go build.

Silently rejecting to even validate signatures could result in applications that compile OK and just refuse to pull images. If anyone doesn’t have robust testing, that can lead to pointless production outages.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

MHO the user should just make a choice and add a tag. That’s not impossible, is it?

The build fails with an error that doesn't make it clear why the build is failing. At minimum I would expect the build to fail with something like "ImageSigningRequiresCgo is not defined" and not "fooBar() undefined"

Also, this code is being used in non-cgo environments. Maybe not the original intent of the code, but it's the way it is. It should work without cgo, regardless of the security implications. A tag opt-in to avoid a compile error is fine, but the compile error should make it clear why the build is failing.

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 24, 2022

It should work without cgo, regardless of the security implications.

I don’t think we agree on basic values enough to continue this discussion.

@mtrmac mtrmac closed this as completed Aug 24, 2022
@paralin
Copy link
Author

paralin commented Aug 24, 2022

Not sure how it's "basic values" to believe that the code should work without cgo.

I was suggesting to stub the package to fix the build error to be clearer, as an alternative.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

@mtrmac from your readme...

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

How is this different? The other tag you've got also is stubbed.

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 24, 2022

Everyone is busy all the time, and no-one knows everything about everything. so we as engineers are responsible for making code secure by default. The convenient and lazy thing to do must be the sufficiently secure one.

Users have always the freedom to take the extra effort to make the less secure choice — ultimately, by forking, or using another product, sure — but to make the low-effort path the dangerous one is abdicating responsibility and trust placed in us.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

@mtrmac Understood, I'll send a PR which at least changes the build error from:

# github.com/containers/image/v5/signature
signature/mechanism_gpgme.go:18:22: undefined: gpgme.Context
signature/mechanism_gpgme.go:72:50: undefined: gpgme.Context
signature/mechanism_gpgme.go:73:20: undefined: gpgme.New
signature/mechanism_gpgme.go:77:33: undefined: gpgme.ProtocolOpenPGP
signature/mechanism_gpgme.go:81:34: undefined: gpgme.ProtocolOpenPGP
signature/mechanism_gpgme.go:103:26: undefined: gpgme.NewDataBytes
signature/mechanism_gpgme.go:132:26: undefined: gpgme.NewDataBytes
signature/mechanism_gpgme.go:137:24: undefined: gpgme.NewDataWriter
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

To...

# github.com/containers/image/v5/signature
signature/mechanism_nocgo.go: ContainersImageSignatureRequiresCgo is not defined.

... at least that way it's clear that it's intentional and meant to be left that way.

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

Add the buildtag to your error message to tell the user, if they want to disable signing, you can use this flag.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

@rhatdan do you mean add a new build tag which has a stub that returns errors instead of failing the build?

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

Yes if there is a buildtag that states, without_pgp or something like that then the error for no cgo should suggest it.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

@rhatdan there is not currently a tag which builds a stub returning an error - I could add one, maybe containers_image_disable_signing ?

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

I believe that would satisfy @mtrmac desire to make users explicitly disable the security feature.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

@rhatdan The PR I sent: If CGO_ENABLED=0 the build fails with a warning saying Cgo is required.

They would have to explicitly enable openpgp (considered insecure) to make the build pass.

So I think that already satisfies what @mtrmac is saying which is that the "insecure" openpgp implementation should not be compiled unless a build flag is specified. The "default go build" output should always be considered secure.

@paralin
Copy link
Author

paralin commented Aug 24, 2022

If you wanted to add a build tag - say containers_image_disable_signing:

//go:build containers_image_disable_signing
// +build containers_image_disable_signing

package signature

// ErrSigningDisabled is returned if container image signing is disabled.
var ErrSigningDisabled = errors.New("container image signing is disabled in this build")

// newGPGSigningMechanismInDirectory returns an error indicating signing is disabled.
func newGPGSigningMechanismInDirectory(optionalDir string) (SigningMechanism, error) {
	return nil, ErrSigningDisabled
}

// newEphemeralGPGSigningMechanism returns an error indicating signing is disabled.
func newEphemeralGPGSigningMechanism(blobs [][]byte) (SigningMechanism, []string, error) {
	return nil, nil, ErrSigningDisabled
}

@paralin
Copy link
Author

paralin commented Aug 24, 2022

@rhatdan Added to #1638 - the first commit clarifies the build failure error message & adds a comment explaining why. The second commit adds a new tag which compiles cleanly with a stub returning a runtime error instead of a build failure.

Hopefully that clears up all 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

@developer-guy
Copy link

wolfi-dev/os#122

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 a pull request may close this issue.

4 participants