Remove SigningMechanism.ImportKeysFromBytes in favor of ephemeral mec…

…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č <mitr@redhat.com>

copy/copy.go

@@ -204,6 +204,7 @@ func Image(policyContext *signature.PolicyContext, destRef, srcRef types.ImageRe
 		if err != nil {
 			return errors.Wrap(err, "Error initializing GPG")
 		}
+		defer mech.Close()
 		dockerReference := dest.Reference().DockerReference()
 		if dockerReference == nil {
 			return errors.Errorf("Cannot determine canonical Docker reference for destination %s", transports.ImageName(dest.Reference()))

signature/docker_test.go

@@ -11,6 +11,7 @@ import (
 func TestSignDockerManifest(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
+	defer mech.Close()
 	manifest, err := ioutil.ReadFile("fixtures/image.manifest.json")
 	require.NoError(t, err)
 
@@ -41,6 +42,7 @@ func TestSignDockerManifest(t *testing.T) {
 func TestVerifyDockerManifestSignature(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
+	defer mech.Close()
 	manifest, err := ioutil.ReadFile("fixtures/image.manifest.json")
 	require.NoError(t, err)
 	signature, err := ioutil.ReadFile("fixtures/image.signature")

signature/mechanism.go

@@ -5,36 +5,84 @@ package signature
 import (
 	"bytes"
 	"fmt"
+	"io/ioutil"
+	"os"
 
 	"github.com/mtrmac/gpgme"
 )
 
 // SigningMechanism abstracts a way to sign binary blobs and verify their signatures.
+// Each mechanism should eventually be closed by calling Close().
 // FIXME: Eventually expand on keyIdentity (namespace them between mechanisms to
 // eliminate ambiguities, support CA signatures and perhaps other key properties)
 type SigningMechanism interface {
-	// ImportKeysFromBytes imports public keys from the supplied blob and returns their identities.
-	// The blob is assumed to have an appropriate format (the caller is expected to know which one).
-	// NOTE: This may modify long-term state (e.g. key storage in a directory underlying the mechanism).
-	ImportKeysFromBytes(blob []byte) ([]string, error)
-	// Sign creates a (non-detached) signature of input using keyidentity
+	Close() error
+	// Sign creates a (non-detached) signature of input using keyIdentity.
 	Sign(input []byte, keyIdentity string) ([]byte, error)
 	// Verify parses unverifiedSignature and returns the content and the signer's identity
 	Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error)
 }
 
 // A GPG/OpenPGP signing mechanism.
 type gpgSigningMechanism struct {
-	ctx *gpgme.Context
+	ctx          *gpgme.Context
+	ephemeralDir string // If not "", a directory to be removed on Close()
 }
 
-// NewGPGSigningMechanism returns a new GPG/OpenPGP signing mechanism.
+// NewGPGSigningMechanism returns a new GPG/OpenPGP signing mechanism for the user’s default
+// GPG configuration ($GNUPGHOME / ~/.gnupg)
+// The caller must call .Close() on the returned SigningMechanism.
 func NewGPGSigningMechanism() (SigningMechanism, error) {
 	return newGPGSigningMechanismInDirectory("")
 }
 
 // newGPGSigningMechanismInDirectory returns a new GPG/OpenPGP signing mechanism, using optionalDir if not empty.
+// The caller must call .Close() on the returned SigningMechanism.
 func newGPGSigningMechanismInDirectory(optionalDir string) (SigningMechanism, error) {
+	ctx, err := newGPGMEContext(optionalDir)
+	if err != nil {
+		return nil, err
+	}
+	return &gpgSigningMechanism{
+		ctx:          ctx,
+		ephemeralDir: "",
+	}, nil
+}
+
+// NewEphemeralGPGSigningMechanism returns a new GPG/OpenPGP signing mechanism which
+// recognizes _only_ public keys from the supplied blob, and returns the identities
+// of these keys.
+// The caller must call .Close() on the returned SigningMechanism.
+func NewEphemeralGPGSigningMechanism(blob []byte) (SigningMechanism, []string, error) {
+	dir, err := ioutil.TempDir("", "containers-ephemeral-gpg-")
+	if err != nil {
+		return nil, nil, err
+	}
+	removeDir := true
+	defer func() {
+		if removeDir {
+			os.RemoveAll(dir)
+		}
+	}()
+	ctx, err := newGPGMEContext(dir)
+	if err != nil {
+		return nil, nil, err
+	}
+	mech := &gpgSigningMechanism{
+		ctx:          ctx,
+		ephemeralDir: dir,
+	}
+	keyIdentities, err := mech.importKeysFromBytes(blob)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	removeDir = false
+	return mech, keyIdentities, nil
+}
+
+// newGPGMEContext returns a new *gpgme.Context, using optionalDir if not empty.
+func newGPGMEContext(optionalDir string) (*gpgme.Context, error) {
 	ctx, err := gpgme.New()
 	if err != nil {
 		return nil, err
@@ -50,11 +98,21 @@ func newGPGSigningMechanismInDirectory(optionalDir string) (SigningMechanism, er
 	}
 	ctx.SetArmor(false)
 	ctx.SetTextMode(false)
-	return gpgSigningMechanism{ctx: ctx}, nil
+	return ctx, nil
+}
+
+func (m *gpgSigningMechanism) Close() error {
+	if m.ephemeralDir != "" {
+		os.RemoveAll(m.ephemeralDir) // Ignore an error, if any
+	}
+	return nil
 }
 
-// ImportKeysFromBytes implements SigningMechanism.ImportKeysFromBytes
-func (m gpgSigningMechanism) ImportKeysFromBytes(blob []byte) ([]string, error) {
+// importKeysFromBytes imports public keys from the supplied blob and returns their identities.
+// The blob is assumed to have an appropriate format (the caller is expected to know which one).
+// NOTE: This may modify long-term state (e.g. key storage in a directory underlying the mechanism);
+// but we do not make this public, it can only be used through newEphemeralGPGSigningMechanism.
+func (m *gpgSigningMechanism) importKeysFromBytes(blob []byte) ([]string, error) {
 	inputData, err := gpgme.NewDataBytes(blob)
 	if err != nil {
 		return nil, err
@@ -72,7 +130,7 @@ func (m gpgSigningMechanism) ImportKeysFromBytes(blob []byte) ([]string, error)
 	return keyIdentities, nil
 }
 
-// Sign implements SigningMechanism.Sign
+// Sign creates a (non-detached) signature of input using keyIdentity.
 func (m gpgSigningMechanism) Sign(input []byte, keyIdentity string) ([]byte, error) {
 	key, err := m.ctx.GetKey(keyIdentity, true)
 	if err != nil {
@@ -93,7 +151,7 @@ func (m gpgSigningMechanism) Sign(input []byte, keyIdentity string) ([]byte, err
 	return sigBuffer.Bytes(), nil
 }
 
-// Verify implements SigningMechanism.Verify
+// Verify parses unverifiedSignature and returns the content and the signer's identity
 func (m gpgSigningMechanism) Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error) {
 	signedBuffer := bytes.Buffer{}
 	signedData, err := gpgme.NewDataWriter(&signedBuffer)

signature/mechanism_test.go

@@ -3,7 +3,6 @@ package signature
 import (
 	"bytes"
 	"io/ioutil"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -16,26 +15,25 @@ const (
 
 func TestNewGPGSigningMechanism(t *testing.T) {
 	// A dumb test just for code coverage. We test more with newGPGSigningMechanismInDirectory().
-	_, err := NewGPGSigningMechanism()
+	mech, err := NewGPGSigningMechanism()
 	assert.NoError(t, err)
+	mech.Close()
 }
 
 func TestNewGPGSigningMechanismInDirectory(t *testing.T) {
 	// A dumb test just for code coverage.
-	_, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
+	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	assert.NoError(t, err)
+	mech.Close()
 	// The various GPG failure cases are not obviously easy to reach.
 }
 
-func TestGPGSigningMechanismImportKeysFromBytes(t *testing.T) {
-	testDir, err := ioutil.TempDir("", "gpg-import-keys")
-	require.NoError(t, err)
-	defer os.RemoveAll(testDir)
-
-	mech, err := newGPGSigningMechanismInDirectory(testDir)
-	require.NoError(t, err)
-
+func TestNewEphemeralGPGSigningMechanism(t *testing.T) {
 	// Try validating a signature when the key is unknown.
+	mech, keyIdentities, err := NewEphemeralGPGSigningMechanism([]byte{})
+	require.NoError(t, err)
+	defer mech.Close()
+	assert.Empty(t, keyIdentities)
 	signature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature")
 	require.NoError(t, err)
 	content, signingFingerprint, err := mech.Verify(signature)
@@ -44,31 +42,34 @@ func TestGPGSigningMechanismImportKeysFromBytes(t *testing.T) {
 	// Successful import
 	keyBlob, err := ioutil.ReadFile("./fixtures/public-key.gpg")
 	require.NoError(t, err)
-	keyIdentities, err := mech.ImportKeysFromBytes(keyBlob)
+	mech, keyIdentities, err = NewEphemeralGPGSigningMechanism(keyBlob)
 	require.NoError(t, err)
+	defer mech.Close()
 	assert.Equal(t, []string{TestKeyFingerprint}, keyIdentities)
-
 	// After import, the signature should validate.
 	content, signingFingerprint, err = mech.Verify(signature)
 	require.NoError(t, err)
 	assert.Equal(t, []byte("This is not JSON\n"), content)
 	assert.Equal(t, TestKeyFingerprint, signingFingerprint)
 
 	// Two keys: just concatenate the valid input twice.
-	keyIdentities, err = mech.ImportKeysFromBytes(bytes.Join([][]byte{keyBlob, keyBlob}, nil))
+	mech, keyIdentities, err = NewEphemeralGPGSigningMechanism(bytes.Join([][]byte{keyBlob, keyBlob}, nil))
 	require.NoError(t, err)
+	defer mech.Close()
 	assert.Equal(t, []string{TestKeyFingerprint, TestKeyFingerprint}, keyIdentities)
 
 	// Invalid input: This is accepted anyway by GPG, just returns no keys.
-	keyIdentities, err = mech.ImportKeysFromBytes([]byte("This is invalid"))
+	mech, keyIdentities, err = NewEphemeralGPGSigningMechanism([]byte("This is invalid"))
 	require.NoError(t, err)
+	defer mech.Close()
 	assert.Equal(t, []string{}, keyIdentities)
 	// The various GPG/GPGME failures cases are not obviously easy to reach.
 }
 
 func TestGPGSigningMechanismSign(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
+	defer mech.Close()
 
 	// Successful signing
 	content := []byte("content")
@@ -95,6 +96,7 @@ func assertSigningError(t *testing.T, content []byte, fingerprint string, err er
 func TestGPGSigningMechanismVerify(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
+	defer mech.Close()
 
 	// Successful verification
 	signature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature")

signature/policy_eval_signedby.go

@@ -5,7 +5,6 @@ package signature
 import (
 	"fmt"
 	"io/ioutil"
-	"os"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -42,20 +41,11 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(image types.UnparsedImage, sig [
 	}
 
 	// FIXME: move this to per-context initialization
-	dir, err := ioutil.TempDir("", "skopeo-signedBy-")
-	if err != nil {
-		return sarRejected, nil, err
-	}
-	defer os.RemoveAll(dir)
-	mech, err := newGPGSigningMechanismInDirectory(dir)
-	if err != nil {
-		return sarRejected, nil, err
-	}
-
-	trustedIdentities, err := mech.ImportKeysFromBytes(data)
+	mech, trustedIdentities, err := NewEphemeralGPGSigningMechanism(data)
 	if err != nil {
 		return sarRejected, nil, err
 	}
+	defer mech.Close()
 	if len(trustedIdentities) == 0 {
 		return sarRejected, nil, PolicyRequirementError("No public keys imported")
 	}

signature/signature_test.go

@@ -139,6 +139,7 @@ func TestUnmarshalJSON(t *testing.T) {
 func TestSign(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
+	defer mech.Close()
 
 	sig := privateSignature{
 		Signature{
@@ -187,6 +188,7 @@ func TestSign(t *testing.T) {
 func TestVerifyAndExtractSignature(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
+	defer mech.Close()
 
 	type triple struct {
 		keyIdentity                string