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

Avoid usage of github.com/mtrmac/gpgme #268

Closed
hferentschik opened this issue May 10, 2017 · 11 comments
Closed

Avoid usage of github.com/mtrmac/gpgme #268

hferentschik opened this issue May 10, 2017 · 11 comments

Comments

@hferentschik
Copy link
Contributor

This library only compiles on Linux, since it uses C extensions which won't compile on OS X and Windows. This makes it for example hard/impossible to use image as a library for a cross compiled CLI applications.

Could one not use Go's golang.org/x/crypto/openpgp?

For now I removed github.com/mtrmac/gpgme and commented its use out from mechanism.go. If there is no way to make without C extensions, one can maybe only build them for Linux, whereas for OS X and Windows it particular feature would not be available (not 100% for what it is actually used).

@runcom
Copy link
Member

runcom commented May 10, 2017

you can use build tag containers_image_openpgp to not use gpgme but getting only a limited signing features, @mtrmac could you chime in here?

@hferentschik
Copy link
Contributor Author

Yes, I've seen

Optionally, you can use the containers_image_openpgp build tag (using go build -tags …, or make … BUILDTAGS=…). This will use a Golang-only OpenPGP implementation for signature verification instead of the default cgo/gpgme-based implementation;

I have not got this to work yet though. Also from a user point of view it would be nice, if the default would work for all OS types.

That said, there is another problem with:

vendor/github.com/containers/storage/storage/lockfile.go:136: st.Mtim undefined (type syscall.Stat_t has no field or method Mtim)

The call to syscall.Stat_t is OS specific and returns different structs depending on the target OS. I guess that's an issue for storage.

@hferentschik
Copy link
Contributor Author

containers_image_openpgp tag aside, I still cannot compile this out of the box. mtrmac/gpgme is a defined dependency which gets downloaded into the vendor directory. When I then try to build, I get:

# github.com/containers/image/vendor/github.com/mtrmac/gpgme
vendor/github.com/mtrmac/gpgme/data.go:4:11: fatal error: 'gpgme.h' file not found
#include <gpgme.h>
          ^
1 error generated.

# github.com/containers/image/vendor/github.com/containers/storage/storage
vendor/github.com/containers/storage/storage/lockfile.go:136: st.Mtim undefined (type syscall.Stat_t has no field or method Mtim)
vendor/github.com/containers/storage/storage/lockfile.go:136: not enough arguments in call to time.Unix

So even if containers/image has some build tags to avoid the usage of gpgme, gpgme will cause compilation errors.

The second error is the mentioned system specific call to syscall.Stat_t in containers/storage.

@mtrmac
Copy link
Collaborator

mtrmac commented May 11, 2017

Also from a user point of view it would be nice, if the default would work for all OS types.

The openpgp backend is limited (verification only, no signing), so there are two dimensions of “work” which may be in conflict.

Though, AFAICT, the situation for macOS and Linux is pretty much the same: GPGME development libraries have to be installed. In neither it is a part of the mandatory base system, so it may need to be installed by the user. I suppose with Linux it may be a little easier when using a container to isolate a prepared build environment, but then a container runtime is not a part of the mandatory base system either…

@mtrmac
Copy link
Collaborator

mtrmac commented May 11, 2017

containers_image_openpgp tag aside, I still cannot compile this out of the box. mtrmac/gpgme is a defined dependency which gets downloaded into the vendor directory. When I then try to build, I get:

# github.com/containers/image/vendor/github.com/mtrmac/gpgme
vendor/github.com/mtrmac/gpgme/data.go:4:11: fatal error: 'gpgme.h' file not found
#include <gpgme.h>
          ^
1 error generated.

What exact command are you using to build this? This should only happen when the build tag is not present. (Alternatively, install a GPGME development library + headers, via brew or whatever cgo can find.)

# github.com/containers/image/vendor/github.com/containers/storage/storage
vendor/github.com/containers/storage/storage/lockfile.go:136: st.Mtim undefined (type syscall.Stat_t has no field or method Mtim)
vendor/github.com/containers/storage/storage/lockfile.go:136: not enough arguments in call to time.Unix

This is the same thing as containers/skopeo#310 , where the supposed fix containers/skopeo#331 lacks a macOS tester. Could you verify that PR works, please? If it does, we can update the vendored version in this repo similarly.

@hferentschik
Copy link
Contributor Author

hferentschik commented May 12, 2017

What exact command are you using to build this? This should only happen when the build tag is not present

Let me double check

This is the same thing as containers/skopeo#310 , where the supposed fix containers/skopeo#331 lacks a macOS tester. Could you verify that PR works, please?

Will do. Btw, I really would need this to cross compile to Windows as well.

@hferentschik
Copy link
Contributor Author

So there is the thing. If I do:

$ make vendor
$ go build -tags="containers_image_openpgp" ./...
# github.com/containers/image/vendor/github.com/containers/storage/storage
vendor/github.com/containers/storage/storage/lockfile.go:136: st.Mtim undefined (type     syscall.Stat_t has no field or method Mtim)
vendor/github.com/containers/storage/storage/lockfile.go:136: not enough arguments in call to time.Unix
# github.com/containers/image/vendor/github.com/mtrmac/
vendor/github.com/mtrmac/gpgme/data.go:4:11: fatal error: 'gpgme.h' file not found
#include <gpgme.h>
      ^
1 error generated.

If I then manually delete the mtrmac:

$ rm -rf vendor/github.com/mtrmac/gpgme
$ go build -tags="containers_image_openpgp" ./...
# github.com/containers/image/vendor/github.com/containers/storage/storage
vendor/github.com/containers/storage/storage/lockfile.go:136: st.Mtim undefined (type syscall.Stat_t has no field or method Mtim)
vendor/github.com/containers/storage/storage/lockfile.go:136: not enough arguments in call to time.Unix

storage issue aside, if I don't want to use gpgme, I not only have to specify the containers_image_openpgp tag, but I also need to make sure not to get the gpgme dependency. Using make vendor I will get it per default though. So at the very least this needs to be documented.

In the REAMDE it says regarding gpgme:

the primary downside is that creating new signatures with the Golang-only implementation is not supported.

So where is that really an issue. What is the use case for creating signatures?

@hferentschik
Copy link
Contributor Author

Updating to the latest version of storage by adding:

github.com/containers/storage d10d8680af74070b362637408a7fe28c4b1f1eff
github.com/tchap/go-patricia/patricia v2.2.6
github.com/opencontainers/selinux ba1aefe8057f1d0cfb8e88d0ec1dc85925ef987d

to vendor.conf, gets me past compilation errors.

@mtrmac
Copy link
Collaborator

mtrmac commented May 12, 2017

If I then manually delete the mtrmac:

Good catch, ./... includes all of the vendor subdirectory. That documentation needs to be fixed.

Also thanks for testing the storage update. I’ll work on on the necessary updates.

What is the use case for creating signatures?

Well, a signature has to exist in order to be verifiable :) If you mean that your intended use of containers/image does not involve signing container images, sure, that’s not inherently required. Much of the library does not actually require the containers/image/signature subpackage and can be used completely independently (but the full copy.Image pipeline does depend on the signature subpackage).

mtrmac added a commit to mtrmac/image that referenced this issue May 12, 2017
The version we use expects Linux-specific struct stat; per
containers#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
wking added a commit to wking/openshift-installer that referenced this issue Mar 10, 2019
And zounds of other dependencies.  Use Gopkg.toml's 'ignored' [1] to
exclude, BoltDB, which comes in via:

  github.com/containers/image/pkg/blobinfocache
    github.com/boltdb/bolt

and github.com/docker/distribution/registry/storage/cache, which comes
in via:

  github.com/containers/image/docker
    github.com/docker/distribution/registry/client
      github.com/docker/distribution/registry/storage/cache

because we don't use either the storage backend or the BoltDB
blob-info cache, but dep isn't checking at that level of granularity.
Runc was being pulled in from a few places for runc/libcontainer/user,
but we apparently don't need that either.  Ideally the upstream
repositories would restructure to split these out into separate
packages (and/or dep will grow support for pruning by build tag [2]),
but until then, we can manually prune via 'ignored'.

Also ignore mtrmac/gpgme, because we only need signature verification
and we don't want to use CGO (because we want to be portable to other
operating systems, and we only need verification support, not signing
support [3]).

There may be more fat we can prune as well, but I got tired of looking
and gave up, even though ~50k lines of new code is pretty embarassing
for what is effectively just a handful of HTTPS requests and an
OpenPGP check.

Generated (after manually editing Gopkg.toml) with:

  $ dep ensure

using:

  $ dep version
  dep:
   version     : v0.5.0-31-g73b3afe
   build date  : 2019-02-08
   git hash    : 73b3afe
   go version  : go1.10.3
   go compiler : gc
   platform    : linux/amd64
   features    : ImportDuringSolve=false

[1]: https://golang.github.io/dep/docs/Gopkg.toml.html#ignored
[2]: golang/dep#291
[3]: containers/image#268
wking added a commit to wking/openshift-installer that referenced this issue Mar 12, 2019
And zounds of other dependencies.  Use Gopkg.toml's 'ignored' [1] to
exclude, BoltDB, which comes in via:

  github.com/containers/image/pkg/blobinfocache
    github.com/boltdb/bolt

and github.com/docker/distribution/registry/storage/cache, which comes
in via:

  github.com/containers/image/docker
    github.com/docker/distribution/registry/client
      github.com/docker/distribution/registry/storage/cache

because we don't use either the storage backend or the BoltDB
blob-info cache, but dep isn't checking at that level of granularity.
Ideally the upstream repositories would restructure to split these out
into separate packages (and/or dep will grow support for pruning by
build tag [2]), but until then, we can manually prune via 'ignored'.

Also ignore mtrmac/gpgme, because we only need signature verification
and we don't want to use CGO (because we want to be portable to other
operating systems, and we only need verification support, not signing
support [3]).

There may be more fat we can prune as well, but I got tired of looking
and gave up, even though ~50k lines of new code is pretty embarassing
for what is effectively just a handful of HTTPS requests and an
OpenPGP check.

Generated (after manually editing Gopkg.toml) with:

  $ dep ensure

using:

  $ dep version
  dep:
   version     : v0.5.0-31-g73b3afe
   build date  : 2019-02-08
   git hash    : 73b3afe
   go version  : go1.10.3
   go compiler : gc
   platform    : linux/amd64
   features    : ImportDuringSolve=false

[1]: https://golang.github.io/dep/docs/Gopkg.toml.html#ignored
[2]: golang/dep#291
[3]: containers/image#268
@rhatdan
Copy link
Member

rhatdan commented Apr 25, 2019

@hferentschik @mtrmac @vrothberg I believe this is fixed, Closing. Reopen if I am mistaken.

@rhatdan rhatdan closed this as completed Apr 25, 2019
@mtrmac
Copy link
Collaborator

mtrmac commented May 7, 2019

Yes, this was fixed by #271 and #272.

Jamesjaj2dc6j added a commit to Jamesjaj2dc6j/lucascalsilvaa that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mrhyperbit23z0d added a commit to mrhyperbit23z0d/thomasdarimont that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
easyriderk0c9g added a commit to easyriderk0c9g/rnin9 that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
oguzturker8ijm8l added a commit to oguzturker8ijm8l/diwir that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
wesnowm added a commit to wesnowm/MatfOOP- that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
gaveen3 added a commit to gaveen3/KimJeongHoon3x that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
gaveen3 added a commit to gaveen3/KimJeongHoon3x that referenced this issue Jun 6, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Manuel-Suarez-Abascal80n9y added a commit to Manuel-Suarez-Abascal80n9y/knimeu that referenced this issue Oct 7, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
straiforos8bsh5n added a commit to straiforos8bsh5n/tokingsq that referenced this issue Oct 7, 2022
The version we use expects Linux-specific struct stat; per
containers/image#268 (comment)
the update fixes this.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
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

No branches or pull requests

4 participants