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

Switch to use native go opengpg instead of cgo #274

Closed
mfojtik opened this issue Dec 13, 2016 · 28 comments
Closed

Switch to use native go opengpg instead of cgo #274

mfojtik opened this issue Dec 13, 2016 · 28 comments

Comments

@mfojtik
Copy link

mfojtik commented Dec 13, 2016

Requiring libgpgme and libassuan makes cross-compilation of skopeo (and the library) hard on non-linux OS (linux/mac/etc..)

What is it reason to use CGO dependency instead of native go openpgp?

@mfojtik
Copy link
Author

mfojtik commented Dec 13, 2016

cc @smarterclayton && @pweil-

@runcom
Copy link
Member

runcom commented Dec 13, 2016

/cc @mtrmac (who's on PTO I guess)

@mtrmac
Copy link
Contributor

mtrmac commented Dec 13, 2016

  1. the openpgp module can, from a quick look, only create detached signatures, not a single blob containing both the content and a signature (more similar to the usual X.509 CMS model)
  2. Full integration with the existing ecosystem like passphrase agents and login-unlocked keyrings
  3. Desire to use the primary upstream implementation, with more reviewers and a more chance of long-term maintenance.
  4. … and in particular, use of a FIPS-validated cryptographic module

@mfojtik
Copy link
Author

mfojtik commented Dec 14, 2016

@mtrmac on the other hand, it means the cross-compilation of skopeo and everything that imports containers/image will not work or require CGO dependency.

@smarterclayton FYI

@smarterclayton
Copy link

smarterclayton commented Dec 14, 2016 via email

@smarterclayton
Copy link

smarterclayton commented Dec 14, 2016 via email

@mfojtik
Copy link
Author

mfojtik commented Dec 14, 2016

can we have native opengpg as plugin and similar libgpgme?

IOW. the users won't have nice features like keychain, but they can supply passphrase via readline/--passphrase.
Skopeo can live with libgpgme but native opengpg allows to re-use the library in non-linux environments.

@rhatdan
Copy link
Member

rhatdan commented Dec 14, 2016

Seems like we should be able to support both, opengpg, with less functionality.

@mfojtik
Copy link
Author

mfojtik commented Jan 2, 2017

@mfojtik
Copy link
Author

mfojtik commented Jan 3, 2017

@smarterclayton @pweil- what are the other possibilities when we decide to not add containers/image into openshift binary? IF we decide for that, how we are going to enforce the policy.json file in OpenShift admission?

One option is to go via Kubernetes ImagePolicy and build the web server as external component the Kube admission will send a webhook to (or implement the same inside the OpenShift ImagePolicy controller).

@mtrmac
Copy link
Contributor

mtrmac commented Jan 3, 2017

What is the bare minimum that a Windows user needs to correctly sign an image?

That depends on whether we expect the user to have a $GNUPGHOME or and equivalent existing key store, or whether we are inventing something from scratch.

If we can't come up with a use case, then we just won't add this to the openshift binary.

This seems to somehow mix the concept of use cases and implementations.

@mtrmac
Copy link
Contributor

mtrmac commented Jan 3, 2017

Why are detached signatures not enough for the "sign image" use case? The only thing openshift needs is detached signatures.

The signature blob is not only a cryptographic signature, it also includes image identity (to prevent an attacker substituting a vulnerable 5.9 image for the desired 7.2.3 image), and may include other data (like information about ancestry to allow signing base images and verifying such signatures).

That “signature blob” is separate from the image and detached in that sense; but it is not a “detached signature” in the OpenPGP sense. With an OpenPGP detached signature, we would have

  1. the container image
  2. the information contained in the signature (identity+ancestry+…)
  3. a cryptographic signature

(where “detached” means that 2 and 3 are separate files and 2 is unmodified).

Having an OpenPGP detached signature means having to carry around 2 blobs for every signature, which have to be kept in sync, and that's just a pointless hassle when the OpenPGP RFC provides a perfectly good file format which allows us to use a single blob.

If I had to make a choice, I’d rather implement the single-blob format in Go than change the signature format to use a 2-blob combination: the former inconveniences us, the latter all users. (But I am still hoping that neither will be necessary.)

@mtrmac
Copy link
Contributor

mtrmac commented Jan 3, 2017

can we have native opengpg as plugin and similar libgpgme?

Well, OpenPGP is a RFC and of course there can be separate implementations (if not x/crypto/openpgp, something else). Can we commit to maintaining them at the appropriate level for a security-critical application of cryptographic primitives?

@mtrmac
Copy link
Contributor

mtrmac commented Jan 3, 2017

@mtrmac will https://godoc.org/golang.org/x/crypto/openpgp/clearsign be enough?

No, the current implementation does not use armoring.

But, it prompted me to look into x/crypto/openpgp/packet in more detail; that implements enough of the actual low-level format (per https://tools.ietf.org/html/rfc4880#section-11.3 , Compressed Message (One-Pass Signed Message)) that openpgp.ReadMessage would probably work (which is not obvious from the API documentation).

There still isn't a corresponding signing function but that could be constructed reasonably enough (a subset of openpgp.Encrypt.

(a very naive PoC: https://github.com/containers/image/compare/master...mfojtik:openpgp?expand=1)

This was probably not intended to, but just to make sure, note that this does not do any cryptographic signature verification!

@mfojtik
Copy link
Author

mfojtik commented Jan 3, 2017

@mtrmac I can try to use ReadMessage with the signature fixture and see how far I can get. In theory, we just need to be able to "verify" the signature in OpenShift admission (we don't need to create/sign short term). So if we can have an example where we can just use the openpgp.ReadMessage and some code to verify the signature cryptographically, that might be enough. (it can be part of containers/image library as 'low-level' verification). WDYT?

@mfojtik
Copy link
Author

mfojtik commented Jan 3, 2017

@mtrmac however, I think we will still need to refactor the signature/ package to move the CGO import into a 'upper' package (and use just an interface in signature package so it can be replaced with the native verification and signing as not-implemented).

@mtrmac
Copy link
Contributor

mtrmac commented Jan 3, 2017

So if we can have an example where we can just use the openpgp.ReadMessage and some code to verify the signature cryptographically, that might be enough.

Playing with it now,

	kr, err := openpgp.ReadKeyRing(krFile)
	m, err := openpgp.ReadMessage(input, kr, nil, nil)
	body, err := ioutil.ReadAll(m.UnverifiedBody)
	if m.IsSigned && m.SignedBy != nil && m.SignatureError == nil {
		fmt.Printf("Body: %s", body)
	}

is a start for doing the right thing, at least to the extent that a corrupt blob and an unsigned PGP data are rejected, but it would take a fair amount of review to be sure that everything necessary has been handled (looking at known tests in containers/image/signature/fixtures, this does not handle expiration at the very least).

it can be part of containers/image library as 'low-level' verification)

I have no idea what that “low-level” qualifier means. AFAICT either we commit to implementing and maintaining that, or not. (I categorically refuse to have a “for demonstration only, use at your own risk” code in the repo.)

@mtrmac
Copy link
Contributor

mtrmac commented May 10, 2017

Non-cgo builds of containers/image and skopeo are now possible per containers/image#207 . (This allows verification only, not signing.)

@mtrmac mtrmac closed this as completed May 10, 2017
@monikakatiyar16
Copy link

monikakatiyar16 commented May 26, 2017

Containers have introduced new libraries for adding support of storage drivers, which requires installing btrfs-tools and libdevmapper-dev. How do we build skopeo without installing these C dependencies?

@mtrmac
Copy link
Contributor

mtrmac commented May 26, 2017

How do we build skopeo without installing these C dependencies?

Unless containers/storage has its own build configuration (@nalind?), containers/image/transports/alltransports will probably have to be modified to make containers/storage optional.

@monikakatiyar16
Copy link

monikakatiyar16 commented May 29, 2017

Prior to this enhancement for adding storage in skopeo-0.1.18, the copy on different storage drivers was already working (I have verified it on aufs/vfs/devicemapper storage drivers). Also, since we are already using docker at the backend and it handles these things, why are we registering the storage drivers in the Known transports? Unfortunately, I couldn't find any release notes related to these, which could describe why are we using them. Is this specific to OCI images?

@mtrmac
Copy link
Contributor

mtrmac commented May 29, 2017

The storage drivers within docker, or within containers/storage, and the transport drivers in containers/image, are quite different things.

For transport drivers, think primarily network/IPC protocols, i.e. ways to connect to completely distinct software suites which “deal with” container images, and to allow transferring images between them. The docker daemon image store, with its storage drivers and whatnot, is just one such “software suite”; containers/storage is another; an external docker/distribution server is yet another. All of these are independent, e.g. the docker-daemon: transport does not allow modifying the containers/storage store.

The containers/storage back-end is registered in alltransports because that allows using it in skopeo (or any other containers/image user which uses alltransports.ParseImageName), and that’s quite convenient for containers/storage users.

@monikakatiyar16
Copy link

Okay.. Thanks @mtrmac for the description. That makes it clear to me now..

containers/image/transports/alltransports will probably have to be modified to make containers/storage optional.

If we do this, we would not be able to take advantage of this feature then?

@mtrmac
Copy link
Contributor

mtrmac commented May 29, 2017

containers/image/transports/alltransports will probably have to be modified to make containers/storage optional.

If we do this, we would not be able to take advantage of this feature then?

Sure — optional, not removed. Either using a new build tag, or perhaps depending on the cgo build tag, if that were more convenient for users.

@monikakatiyar16
Copy link

monikakatiyar16 commented May 30, 2017

@mtrmac

The containers/storage back-end is registered in alltransports because that allows using it in skopeo (or any other containers/image user which uses alltransports.ParseImageName), and that’s quite convenient for containers/storage users.

So, finally if I understood it correctly, (I am little ignorant about these)

  1. containers/storage can be treated as an alternative to docker daemon image store (docker client) storing all the images/layers/containers and their metadata?

  2. Who are the users of containers/storage? What are the benefits of using it?

  3. For the container project, there are tags to ignore the build for specific storage drivers eg : exclude_graphdriver_aufs, exclude_graphdriver_devicemapper, exclude_graphdriver_zfs, exclude_graphdriver_overlay, exclude_graphdriver_btrfs.

Since I am getting an error for only btrfs and devicemapper C libraries, using build tag 'exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp' builds and runs successfully. (containers_image_openpgp for gpgme). If we are not using the containers/storage project separately apart from skopeo and if we don't have any containers/storage project users, then is it okay to build and run using these build tags? Or am I still missing anything?

Note: We are using ubuntu/centos systems right now

@mtrmac
Copy link
Contributor

mtrmac commented May 30, 2017

containers/storage can be treated as an alternative to docker daemon image store (docker client) storing all the images/layers/containers and their metadata?

AFAIK the two are not interchangeable. They are similar, but they don’t access the same underlying store, nor can the docker daemon be configured to use the containers/storage store.

Who are the users of containers/storage? What are the benefits of using it?

CRI-O is one notable user AFAIK.

If we are not using the containers/storage project separately apart from skopeo and if we don't have any containers/storage project users, then is it okay to build and run using these build tags?

Sure, whatever works for you. BTW you might be interested in #343 , perhaps there is a major use case you share with @hferentschik which really should be documented?

@hferentschik
Copy link
Contributor

So I am running into compilation issues as well, especially with container/storage. Reading this thread I am wondering whether I need it tough and whether I somehow should be able to skip its use (at compile time).

What I am after is to cache/sync the images of a Docker daemon onto the file system, either in OSTree or OCI. I also need to then at some stage load these images back into a Docker daemon. See also containers/image#155. The important thing for me is that the CLI I am building (Minishift) is cross platform, so I need to be able to compile containers/image (and if needed containers/storage) cross platform.

@mtrmac
Copy link
Contributor

mtrmac commented Jun 26, 2017

(FWIW the way containers/image#290 makes the libostree dependency optional could be equally used to disable the containers/storage dependency.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants