Add second signing mechanism based on native 'openpgp' Go library #198

mfojtik · 2017-01-04T11:42:16Z

Changes made:

Refactored signature/ package to deal with 2 mechanism
Moved SigningMechanism to types.SigningMechanism (to avoid import loops)
Moved default gpgme based signing mechanism to signature/gpgme so it can be opted-out when you don't want to have CGO dependency
The tests inside signature/ package now use openpgp package (this might be a follow up to add tests for gpgme as well, however I wanted to make signature/ package clean so it can be imported without relying on CGO)

NOTE that only verification is currently supported via Golang opengpg. We can work on signing using Encode() but as a follow up.

Also note that this is breaking internal API in some ways (you have to now pass the mechanism you want to use to some functions, so plumbing on skopeo side will be required to use gpgme mechanism).

The unit tests are passing.

@mtrmac @rhatdan @smarterclayton @pweil- PTAL

mfojtik · 2017-01-04T11:43:37Z

signature/openpgp/openpgp_test.go

+		publicKey    string
+	}
+
+	cases := []tc{


@mtrmac I made this test in parity with gpgme test so we can be sure the verification works the same for both.

Yes, a table-driven test is nicer, though it would be nice to keep the comments and ideally to use the same set of tests for both implementations. We can tinker with this after the general structure of the code is fixed, though.

I can add human description as messages in the table test, so when it spits error you can quickly identify the affected case.

Sure, that would be even better.

mfojtik · 2017-01-04T11:44:29Z

signature/policy_eval_signedby_test.go

+	// Pass a nil pointer to, kind of, test that the return value does not depend on the
+	// image parameter..
+	sar, parsedSig, err = pr.isSignatureAuthorAccepted(m, nil, sig)
+	// FIXME: The error is: errors.InvalidArgumentError("no armored data found") but for


@mtrmac I will need some help here :-) I think it still works fine, but for some reason the error is not what you are expecting.

Is it really no armored data found here? I see InvalidSignatureError{msg:"Invalid GPG signature: (*packet.Signature)(nil)"} .

And that, in turn, happens because the public key is unknown, i.e. MessageDetails.SignedByKeyId is set but MessageDetails.SignedBy is nil because we don’t have the key; and when SignedBy is nil, openpgp.readSignedMessage does not even set up a signatureCheckReader, so Signature is never set to non-nil.

And, that ultimately gets us an InvalidSignatureError, not a PolicyRequirementError—but then note that this test case does not require a PolicyRequirementError. So that seems to be perfectly fine as far as this test is concerned at least; though the Invalid GPG signature: (*packet.Signature)(nil) error message could be improved, as could the gpgme handling at this level.

thanks! this trace is really helpful

mtrmac

As a quick first pass, we really need to figure out a different way to customize the mechanism so that prSignedBy can still respect pr.KeyType.

mtrmac · 2017-01-04T19:08:15Z

signature/policy_eval_signedby.go

+		if err != nil {
+			return sarRejected, nil, err
+		}
+	*/


Moving the mechanism selection/creation out of prSignedBy really won’t work:

The mechanism in principle depends on pr.KeyType; a single PolicyContext.IsRunningImageAllowed may involve policies with various prSignedBy instances with different pr.KeyType values. Right now we only support SBKeyTypeGPGKeys but the code above already anticipates other values.

With keys specified inside the policy as pr.KeyPath/pr.KeyData, we really want a temporary instance of the mechanism with its own state; the caller shouldn’t have to deal with creating individual temporary directories.

AFAICT the easiest way to do this is to have the two SigningMechanism implementations available in a single package, chosen by a build tag. Then nobody else needs to care.

Another possibility would be to use types.SystemContext, as we do for other environment overrides (and signature.NewPolicy.Context(ctx *types.SystemContext, policy *Policy) copying the ctx reference into the returned PolicyContext); but types.SystemContext is designed to always have a default, so we would need to build a reference to one of the implementations into signature anyway… and then we are more or less back in the build tag area.

@runcom is there another approach I am missing?

(I’d be happy to work on this, but I do need you to ACK that the chosen approach is usable in OpenShift.)

@mfojtik @runcom Experimentally demonstrating what using build tags would look like in #207 (DO NOT MERGE right now). Would that work for you?

@mtrmac the build tags are the "last resort" to me as the management of them proved cumbersome in openshift (we forgot to add it for registry that resulted in storage driver disabled in release, etc..) They also need to be documented as they are exposed to downstream (origin).

The cleanest (in go way) solution would be to separate the packages and have interface for them. The interface (SigningMechanism) is not really a good interface if an implementation is used instead anyway of it.

The choice of "default" mechanism should be on the user of this library (skopeo vs. origin). We should have specific tests that exercise both implementation properly locked in their packages.

I picked openpgp as a default for signature/ package tests as it does not require any CGO dependency. I'm fine moving some of the test to be package specific as a follow up (even if that means we duplicate tests or create some framework for tests so you can exchange the signing mechanism to prove both implementation produce the same results..)

the build tags are the "last resort" to me as the management of them proved cumbersome in openshift (we forgot to add it for registry that resulted in storage driver disabled in release, etc..)

This seems like a kind of thing which could be once for all ensured using a build-time test.

The cleanest (in go way) solution would be to separate the packages and have interface for them.

Sure, but something somewhere has to make a choice, and forcing callers who do not care (they may not even know that cryptography is involved somewhere in the stack) to make such a choice is not really clean either. (In the worst case, in the future, we could have a caller who provides a X.509 signature, knows that, and is still forced to make a choice between OpenPGP implementations.)

The interface (SigningMechanism) is not really a good interface if an implementation is used instead anyway of it.

Look at #207 - the interface stays, and really is not the point; the point is what and how chooses the which implementation is used.

@mtrmac i think I'm fine with build tags for now although I will prefer the containers_image_gpgme to be the tag (in that way we don't have to add build tag to all build scripts in origin to make sure we don't build anything that requires openpgp).

We have to talk how I'm going to use this library in Origin as I think we are still on the same page (we just going to verify the signature, not policy.json, so probably we will just use the signature initially)

@mtrmac + before I forget, we also need some way to extract the metadata (critical/optional) without us parsing the JSON content (which means that the JSON content should be somehow formalized or have helper function in this library).

mtrmac · 2017-01-04T19:10:42Z

signature/openpgp/openpgp.go

+		m.ctx.keyring = append(m.ctx.keyring, entity)
+	}
+	if len(m.ctx.keyring) == 0 {
+		return nil, errors.New("no public keys found")


This test is imprecise; if there already have been keys imported before, this won’t notice.

… but then see the other conversations about importing 0 keys possibly succeeding… it’s not entirely obvious what needs changing.

agree, this check is necessary.

however I have question about this function... is the intention to always append to keyring when this function is called or should I reset the keyring? What if I have duplicate keys?

Strictly speaking, undefined :)

Right now this is not intended as a generic abstraction over key management systems (import/export/list keys etc.), so that someone could build a key management CLI on top. SigningMechanism is really here only to sign and verify.

The only place where ImportKeysFromBytes is called in production is prSignedBy, which creates a completely separate GPG home directory in the gpgme implementation: the idea is that, regardless of anything in $HOME or $GNUPGHOME, prSignedBy uses only the keys specified in the prSignedBy object.

So, with gpgme, keys are stored in files and keys need to be imported into the keyring before being used; therefore, ImportKeysFromBytes has a persistent effect (adds keys to $optionalDir or so, and they will persist for future uses of that directory); prSignedBy creates a private directory, imports keys exactly once, uses that directory for one (or more, in the future) verifications.

With openpgp, we can use in-memory keys; so, there is no need for ImportKeysFromBytes to have a persistent effect; prSignedBy can create an in-memory object (TODO we should skip creating the directory in that case, and move that into the SigningMechanism as well), import keys into it, and use it for validation.

As it happens, ImportKeysFromBytes is always called only once per directory / object / (in the future) context in prSignedBy, so it does not matter whether the keyring is reset, hence the “strictly speaking, undefined”.

But, per naming of the function, it seems more reasonable to append keys than to override; it is Import, not OverrideWithImport after all. And if SigningMechanism ever grew into the export/import/list key management abstraction, a non-destructive import seems much more useful to me.

As for duplicate keys (note that they may be duplicate both between old keyring contents and the imported data, and within just the imported data), as long as signatures validate using the imported keys and only the imported keys, we don’t really care right now (well, other than one somewhat misguided test in mechanism_test.go, but we can change that one).

Eventually, to implement SBKeyTypeSignedByGPGKeys, if that ever happens, we will need to care about signatures attached to public keys, and the ability to import new signatures to already existing public keys, and this will become more complex. But that’s not a constraint right now.

mtrmac · 2017-01-04T19:11:49Z

signature/openpgp/openpgp.go

+		return nil, errors.New("no public keys found")
+	}
+	keyIdentities := []string{}
+	for _, entity := range m.ctx.keyring {


These are identities of all keys in the keyring, not just those imported right now.

see the question above.

mtrmac · 2017-01-04T19:12:36Z

signature/openpgp/openpgp.go

+
+func (m OpenPGPMechanism) Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error) {
+	if len(m.ctx.keyring) == 0 {
+		return nil, "", errors.New("no public keys imported")


Is this explicit check necessary? Admittedly it results in a nice error message, but it seems to me that “public key with fingerprint XXX not found” should happen anyway?!

agree, removed.

mtrmac · 2017-01-04T19:13:43Z

signature/openpgp/openpgp.go

+	if len(m.ctx.keyring) == 0 {
+		return nil, "", errors.New("no public keys imported")
+	}
+	md, err := openpgp.ReadMessage(bytes.NewReader(unverifiedSignature), m.ctx.keyring, nil, nil)


(Note to self: I didn’t review this in detail yet.)

mtrmac · 2017-01-04T19:28:34Z

signature/policy_eval_signedby_test.go

+	// FIXME: The error is: errors.InvalidArgumentError("no armored data found") but for
+	// some reason it is surfaced here and not translated to PolicyRequirementError.
+	// However the sar is rejected...
+	assertSARRejectedPolicyRequirement(t, sar, parsedSig, PolicyRequirementError(""))


Well, gpgme happens to accept empty bytes (the []byte{} in NewPRSignedByKeyData) as valid input, “correctly importing” it as a set of zero keys. (It also happens to accept completely invalid input, but that’s beside the point).

With gpgme, the code path fails in the if len(trustedIdentities) == 0 { case, which is a PolicyRequirementError; with openpgp, it fails in the … err := mechanism.ImportKeysFromBytes(); if err != nil case, which is an operation error (like ioutil.ReadFile failing). Though arguably the len(trustedIdentities) == 0 is an invalid PolicyRequirement, not a failure to fulfill it… it really isn’t that well defined.

I guess, considering https://tools.ietf.org/html/rfc4880#section-3.6 , it could be argued that an empty keyring is invalid… We will need to nail these things down.

Though arguably the len(trustedIdentities) == 0 is an invalid PolicyRequirement, not a failure to fulfill it… it really isn’t that well defined.

Looking at other cases, that should indeed be a PolicyRequirementError. What to do on corrupt keyring data is unclear.

fixed both.

mtrmac · 2017-01-04T19:41:37Z

signature/policy_eval_signedby_test.go

+	// Pass a nil pointer to, kind of, test that the return value does not depend on the
+	// image parameter..
+	sar, parsedSig, err = pr.isSignatureAuthorAccepted(m, nil, sig)
+	// FIXME: The error is: errors.InvalidArgumentError("no armored data found") but for


Is it really no armored data found here? I see InvalidSignatureError{msg:"Invalid GPG signature: (*packet.Signature)(nil)"} .

And that, in turn, happens because the public key is unknown, i.e. MessageDetails.SignedByKeyId is set but MessageDetails.SignedBy is nil because we don’t have the key; and when SignedBy is nil, openpgp.readSignedMessage does not even set up a signatureCheckReader, so Signature is never set to non-nil.

And, that ultimately gets us an InvalidSignatureError, not a PolicyRequirementError—but then note that this test case does not require a PolicyRequirementError. So that seems to be perfectly fine as far as this test is concerned at least; though the Invalid GPG signature: (*packet.Signature)(nil) error message could be improved, as could the gpgme handling at this level.

mtrmac · 2017-01-04T19:43:43Z

signature/openpgp/openpgp.go

+}
+
+func (m OpenPGPMechanism) ImportKeysFromBytes(blob []byte) ([]string, error) {
+	keyring, err := openpgp.ReadArmoredKeyRing(bytes.NewReader(blob))


Note that this should also accept unarmored data; with keyData inside policy.json containing a base64-encoded representation of keyData, it would be rather silly, and wasting space, to have a base64-encoded of a base64-based armor encoding of the underlying key.

(These kinds of things have so far not been really defined because “whatever GPG does” has worked for us; we will need to gradually nail them down now.)

I can detect if the blob start with --- BEGIN PGP use ReadKeyRing() if data are not armored.

Considering https://github.com/golang/crypto/blob/master/openpgp/armor/armor.go#L165 , such a trivial check may not be sufficient (PGP implementations have frequently been fairly lenient about leading text before armor, e.g. handling an e-mail message with full headers), though it may affect very few cases in practice.

It is, of course, easy enough, to call both ReadKeyRing and ReadArmoredKeyRing, in case either succeeds; if both fail, returning a really good error message may be difficult.

…fication)

mfojtik · 2017-01-10T12:25:45Z

@mtrmac I'm fine closing this PR in favor of #11

mfojtik commented Jan 4, 2017

View reviewed changes

mtrmac requested changes Jan 4, 2017

View reviewed changes

mtrmac mentioned this pull request Jan 9, 2017

Allow choosing between a gpgme and openpgp signature backend using a build tag #207

Merged

Implement second signing mechanism using Go native openpgp (only veri…

c34923c

…fication)

mfojtik force-pushed the openpgp branch from 1823cf0 to c34923c Compare January 9, 2017 19:19

mfojtik added 5 commits January 9, 2017 20:50

return only imported keys

eefa1a6

do not check keys when verifying

7026710

fix golint errors

2a1bc19

fix tests

b25212a

add description to test cases

773201d

mfojtik closed this Jan 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add second signing mechanism based on native 'openpgp' Go library #198

Add second signing mechanism based on native 'openpgp' Go library #198

mfojtik commented Jan 4, 2017 •

edited

Loading

mfojtik Jan 4, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac Jan 9, 2017

mfojtik Jan 4, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac left a comment

mtrmac Jan 4, 2017

mtrmac Jan 9, 2017

mfojtik Jan 9, 2017

mfojtik Jan 9, 2017

mtrmac Jan 9, 2017

mfojtik Jan 9, 2017

mfojtik Jan 9, 2017

mtrmac Jan 4, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac Jan 9, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac Jan 4, 2017

mtrmac Jan 4, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac Jan 4, 2017

mtrmac Jan 4, 2017

mtrmac Jan 4, 2017

mfojtik Jan 9, 2017

mtrmac Jan 9, 2017

mfojtik commented Jan 10, 2017

Add second signing mechanism based on native 'openpgp' Go library #198

Add second signing mechanism based on native 'openpgp' Go library #198

Conversation

mfojtik commented Jan 4, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a 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

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

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

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

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

Choose a reason for hiding this comment

mfojtik commented Jan 10, 2017

mfojtik commented Jan 4, 2017 •

edited

Loading