Allow choosing between a gpgme and openpgp signature backend using a build tag #207
Conversation
|
Note to self: this would also need a |
|
I'm all for build tags, it looks really nice 👍 |
| } | ||
| } | ||
|
|
||
| pubring, err := ioutil.ReadFile(path.Join(gpgHome, "pubring.gpg")) |
There was a problem hiding this comment.
Do we want to do this? (I guess this mimic what gpgme does).
There was a problem hiding this comment.
(Repeating a personal conversation) this allows skopeo standalone-verify to work. Otherwise I agree that it is not likely to be that useful in common use (verifying against arbitrary keys including keys collected from people who have ever sent me an PGP-signed email)
signature/mechanism_openpgp.go
Outdated
| } | ||
|
|
||
| func (m *openpgpSigningMechanism) Sign(input []byte, keyIdentity string) ([]byte, error) { | ||
| return nil, errors.New("signing is not supported in github.com/containers/image built with the containers_image_openpgp build tag") |
There was a problem hiding this comment.
why not just return SupportSigning() here as an error?
There was a problem hiding this comment.
I was considering that, but it felt dirty. Perhaps the SupportsSigning() error interface itself is just too ugly; it could just return a bool as well. Or we can define the shared error string as a constant. I don’t have a strong opinion.
signature/mechanism_openpgp.go
Outdated
| } | ||
|
|
||
| func (m *openpgpSigningMechanism) Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error) { | ||
| if len(m.keyring) == 0 { |
|
Updated, see the first commit for an API change eliminating a publc |
signature/mechanism.go
Outdated
| // Sign creates a (non-detached) signature of input using keyidentity | ||
| Close() error | ||
| // SupportsSigning returns nil if the mechanism supports signing, or an error message. | ||
| SupportsSigning() error |
There was a problem hiding this comment.
Can we get rid of this method on the interface and instead Sign returns a ErrSigningNotSupported error? I don't really like this additional method where Sign could already say not supported by returning a typed error (I also believe it's more clean if this can be done). Are there any downsides with my approach?
There was a problem hiding this comment.
Having an extra check which does not require you to get a key ID etc. is convenient for things like
if err := mech.SupportsSigning(); err != nil {
t.Skipf("Signing not supported: %v", err)
}which this PR adds to tests, or perhaps to modify the UI of a tool (replace signing description in --help with a message saying that signing is not supported in this build).
We could, of course, also define a SigningNotSupported error type, and return that from Sign(), for those who don’t care to check in advance but still want to detect the situation.
There was a problem hiding this comment.
alright, fine with that if that's used mainly to present stuff in the UI, I would rather use the typed error internally though.
There was a problem hiding this comment.
Added a SigningNotSupportedError; in openpgpSigningMechanism both SupportsSigning and Sign now return that type.
| type gpgSigningMechanism struct { | ||
| ctx *gpgme.Context | ||
| // SigningNotSupportedError is returned when trying to sign using a mechanism which does not support that. | ||
| type SigningNotSupportedError string |
There was a problem hiding this comment.
sorry for nit-picking, this should be called ErrSigningNotSupported according to golang's standards
There was a problem hiding this comment.
Pointer to the standards?
Both https://blog.golang.org/error-handling-and-go and https://golang.org/doc/effective_go.html#errors and e.g. types in https://golang.org/pkg/os/ use the ...Error form.
There was a problem hiding this comment.
forget this, https://github.com/golang/go/wiki/Errors#naming (I swapped types and variables)
There was a problem hiding this comment.
Hum, interesting. (And confusing that both types and values are recommended.)
|
LGTM |
|
@mtrmac is there anything left to be done here? Otherwise let's pull this in. |
|
LGTM |
A bit of test coverage for the new logic (e.g. $GNUPGHOME), test automation, build tag documentation, at least. And a corresponding skopeo update. |
|
@mtrmac can those be done as a follow up? |
|
@mtrmac @runcom i'm taking the last comment back... I tried to pick up This is because For OpenShift, all I need is the OpenPGP signing mechanism, that I can use to call WDYT? |
|
What I propose here is to break the
Then glue these together using the Go interfaces. |
+1 to this. |
|
@runcom also the openshift dependency (AKA openshift transport) have to go away because:
|
I see, I understand those points. Let's see what @mtrmac thinks about this (since he wrote the openshift transport at the beginning). |
No, that really comes back to Go being utterly unhelpful for maintaining stable APIs and sharing referenced libraries across projects. This kind of breakage could just as well have happened in any other dependency like (@runcom is working on allowing to use
Getting back “true/false” really won’t work that way, with future signatures of base images etc. “Does the signature verify” and “does the signature apply to this image” will still not have a sufficient semantic meaning, and something like that policy will be necessary (perhaps not as a
For the above reason, it is fairly likely that the fairly low-level |
We do need to have that conversation, but it can’t go away right now, while registries which only support the OpenShift-like API are deployed. This will need a deprecation plan, compatibility transition mechanism, and so on.
Really there is another, perhaps pretty good alternative: #188 , just depend on the OpenShift-provided client implementation (if that has a stable API). Right now the size of the added dependency seems to make it prohibitive. Right now, the simplest way to clean this up is to stop using the |
I’ll get rid of But given @runcom ’s LGTM, I think you can safely rely on the API and build tag name not changing. |
|
*sigh* What other alternative is there for 1) not having a mandatory It seems that 1) is critical for OpenShift, 2) is critical for containers/image; and both 3 and 4 seem very desirable. We can give up on 3 by defaulting to |
|
The I see the build tags as a band-aid to make this work now, but what I think should really happen is a Then whoever is importing the @smarterclayton FYI |
mtrmac
force-pushed
the
openpgp
branch
2 times, most recently
from
474e19a to
0eeca14
Compare
mtrmac
force-pushed
the
openpgp
branch
2 times, most recently
from
108e23d to
bc7933d
Compare
mtrmac
force-pushed
the
openpgp
branch
3 times, most recently
from
c2c51e0 to
4683181
Compare
I am not sure what you mean by drawing this distinction. Signing happens by callers of the library invoking code within the library.
AFAICT it is a complete non-issue:
Or do you mean some other CRI project?
I still can’t see any way to refactor this to individual Lego bricks while doing at least 1) and 2) from #207 (comment) . For 2),
(BTW exposing the mechanism and making them pluggable is fairly undesirable. Every time the interface allows it, “helpful” recipes like http://www.rgagnon.com/javadetails/java-fix-certificate-problem-in-HTTPS.html start appearing on the internet. I am very tempted to make the Sure, in the ideal world, and all that… But anyway, 2) above makes this interface design/nudge aspect secondary.) FWIW https://github.com/mtrmac/origin/tree/c-i-verify-image-signature , and mtrmac/origin@e45af45 in particular, seems to work well enough as a proof of concept for using the build tag in OpenShift: at least both (Disclaimers:
) |
|
@mtrmac i think I'm ok with the current state as long as we can move forward with the image verification in openshift. |
|
@runcom PTAL again |
|
seems like tests are failing with the new build tag https://travis-ci.org/containers/image/jobs/212314846 |
Yes, because A manual |
…hanism objects Instead of SigningMechanism.ImportKeysFromBytes, which may modify long-term state in the user’s home directory, provide NewEphemeralGPGSigningMechanism, which combines creation of a temporary directory and key import; users of NewGPGSigningMechanism (using $GNUPGHOME etc.) and (test-suite) users of newGPGSigningMechanismInDirectory can no longer import keys. To support this, all users of SigningMechanism must call .Close() on the object. Signed-off-by: Miloslav Trmač <[email protected]>
…build tag The default is gpgme; a containers_image_openpgp build tag can be used to use openpgp instead. openpgp does not currently support signing, and is based on mfojtik's implementation (adding GPG home directory support, parsing of unarmored keys, and fixing ImportKeysFromBytes semantics). Also adds build documentation, including the new containers_image_openpgp build tag, to README.md. This does not yet hook this into Travis, because that needs a corresponding skopeo build infrastructure and test update to make it possible. That is a separate commit (perhaps a separate PR)? Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
This uses the Travis test matrix support; arguably the total time might be smaller if setting up the VM only once and running both variants within the shell script. Signed-off-by: Miloslav Trmač <[email protected]>
|
@mfojtik containers/skopeo#298 passes tests with this PR vendored so I'd say it's ok to merge this if @mtrmac agrees. |
|
👍 |
This demonstrates an alternative to #198 , which percolates the mechanism choice through the API and does not permit file-format-dependent mechanism selection.
In here, the default mechanism is gpgme; a
containers_image_openpgpbuild tag can be used to use openpgp instead.openpgp does not currently support signing, and is based on mfojtik's implementation (adding GPG home directory support, parsing of unarmored keys, and fixing
ImportKeysFromBytessemantics).NOTE: The openpgp backend is not really fleshed out yet. Some of the
mechanism_test.gotests may be better mechanism-specific, and openpgp definitely needs more tests e.g. foroptionalDir.