Refactor groupSigner

pkg/integrity/clearsign.go

@@ -11,6 +11,7 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/clearsign"
@@ -28,32 +29,33 @@ var supportedPGPAlgorithms = []crypto.Hash{
 	crypto.SHA512,
 }
 
-// hashAlgorithmSupported returns whether h is a supported PGP hash function.
-func hashAlgorithmSupported(h crypto.Hash) bool {
-	for _, alg := range supportedPGPAlgorithms {
-		if alg == h {
-			return true
-		}
-	}
-	return false
+type clearsignEncoder struct {
+	e      *openpgp.Entity
+	config *packet.Config
 }
 
-// signAndEncodeJSON encodes v, clear-signs it with privateKey, and writes it to w. If config is
-// nil, sensible defaults are used.
-func signAndEncodeJSON(w io.Writer, v interface{}, privateKey *packet.PrivateKey, config *packet.Config) error {
-	if !hashAlgorithmSupported(config.Hash()) {
-		return errHashUnsupported
+// newClearsignEncoder returns an encoder that signs messages in clear-sign format using entity e. If
+// timeFunc is not nil, it is used to generate signature timestamps.
+func newClearsignEncoder(e *openpgp.Entity, timeFunc func() time.Time) *clearsignEncoder {
+	return &clearsignEncoder{
+		e: e,
+		config: &packet.Config{
+			Time: timeFunc,
+		},
 	}
+}
 
-	// Get clearsign encoder.
-	plaintext, err := clearsign.Encode(w, privateKey, config)
+// signMessage signs the message from r in clear-sign format, and writes the result to w. On
+// success, the hash function and fingerprint of the signing key are returned.
+func (s *clearsignEncoder) signMessage(w io.Writer, r io.Reader) (crypto.Hash, []byte, error) {
+	plaintext, err := clearsign.Encode(w, s.e.PrivateKey, s.config)
 	if err != nil {
-		return err
+		return 0, nil, err
 	}
 	defer plaintext.Close()
 
-	// Wrap clearsign encoder with JSON encoder.
-	return json.NewEncoder(plaintext).Encode(v)
+	_, err = io.Copy(plaintext, r)
+	return s.config.Hash(), s.e.PrimaryKey.Fingerprint, err
 }
 
 // verifyAndDecodeJSON reads the first clearsigned message in data, verifies its signature, and

pkg/integrity/clearsign_test.go

@@ -17,7 +17,6 @@ import (
 	"testing"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
-	"github.com/ProtonMail/go-crypto/openpgp/clearsign"
 	pgperrors "github.com/ProtonMail/go-crypto/openpgp/errors"
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	"github.com/sebdah/goldie/v2"
@@ -28,44 +27,54 @@ type testType struct {
 	Two int
 }
 
-func TestSignAndEncodeJSON(t *testing.T) {
+func Test_clearsignEncoder_signMessage(t *testing.T) {
 	e := getTestEntity(t)
 
-	// Fake an encrypted key.
-	encryptedKey := *e.PrivateKey
-	encryptedKey.Encrypted = true
+	encrypted := getTestEntity(t)
+	encrypted.PrivateKey.Encrypted = true
 
 	tests := []struct {
-		name    string
-		key     *packet.PrivateKey
-		hash    crypto.Hash
-		wantErr bool
+		name     string
+		en       *clearsignEncoder
+		r        io.Reader
+		wantErr  bool
+		wantHash crypto.Hash
+		wantFP   []byte
 	}{
-		{name: "EncryptedKey", key: &encryptedKey, wantErr: true},
-		{name: "DefaultHash", key: e.PrivateKey},
-		{name: "SHA1", key: e.PrivateKey, hash: crypto.SHA1, wantErr: true},
-		{name: "SHA224", key: e.PrivateKey, hash: crypto.SHA224},
-		{name: "SHA256", key: e.PrivateKey, hash: crypto.SHA256},
-		{name: "SHA384", key: e.PrivateKey, hash: crypto.SHA384},
-		{name: "SHA512", key: e.PrivateKey, hash: crypto.SHA512},
+		{
+			name:    "EncryptedKey",
+			en:      newClearsignEncoder(encrypted, fixedTime),
+			r:       strings.NewReader(`{"One":1,"Two":2}`),
+			wantErr: true,
+		},
+		{
+			name:     "OK",
+			en:       newClearsignEncoder(e, fixedTime),
+			r:        strings.NewReader(`{"One":1,"Two":2}`),
+			wantHash: crypto.SHA256,
+			wantFP:   e.PrimaryKey.Fingerprint,
+		},
 	}
 
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			b := bytes.Buffer{}
 
-			config := packet.Config{
-				DefaultHash: tt.hash,
-				Time:        fixedTime,
-			}
-
-			err := signAndEncodeJSON(&b, testType{1, 2}, tt.key, &config)
+			ht, fp, err := tt.en.signMessage(&b, tt.r)
 			if got, want := err, tt.wantErr; (got != nil) != want {
 				t.Fatalf("got error %v, wantErr %v", got, want)
 			}
 
 			if err == nil {
+				if got, want := ht, tt.wantHash; got != want {
+					t.Errorf("got hash %v, want %v", got, want)
+				}
+
+				if got, want := fp, tt.wantFP; !bytes.Equal(got, want) {
+					t.Errorf("got fingerprint %v, want %v", got, want)
+				}
+
 				g := goldie.New(t, goldie.WithTestNameForDir(true))
 				g.Assert(t, tt.name, b.Bytes())
 			}
@@ -78,6 +87,11 @@ func TestVerifyAndDecodeJSON(t *testing.T) {
 
 	testValue := testType{1, 2}
 
+	testMessage, err := json.Marshal(testValue)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	// This is used to corrupt the plaintext.
 	corruptClearsign := func(w io.Writer, s string) error {
 		_, err := strings.NewReplacer(`{"One":1,"Two":2}`, `{"One":2,"Two":4}`).WriteString(w, s)
@@ -135,20 +149,17 @@ func TestVerifyAndDecodeJSON(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			b := bytes.Buffer{}
 
-			config := packet.Config{
-				DefaultHash: tt.hash,
+			cs := clearsignEncoder{
+				e: e,
+				config: &packet.Config{
+					DefaultHash: tt.hash,
+					Time:        fixedTime,
+				},
 			}
-
-			// Manually sign and encode rather than calling signAndEncodeJSON, since we want to
-			// test unsupported hash algorithms.
-			plaintext, err := clearsign.Encode(&b, e.PrivateKey, &config)
+			_, _, err := cs.signMessage(&b, bytes.NewReader(testMessage))
 			if err != nil {
 				t.Fatal(err)
 			}
-			if err := json.NewEncoder(plaintext).Encode(testValue); err != nil {
-				t.Fatal(err)
-			}
-			plaintext.Close()
 
 			// Introduce corruption, if applicable.
 			if tt.corrupter != nil {

pkg/integrity/sign.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2020-2021, Sylabs Inc. All rights reserved.
+// Copyright (c) 2020-2022, Sylabs Inc. All rights reserved.
 // This software is licensed under a 3-clause BSD license. Please consult the LICENSE.md file
 // distributed with the sources of this project regarding your rights to use or distribute this
 // software.
@@ -8,13 +8,14 @@ package integrity
 import (
 	"bytes"
 	"crypto"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"sort"
 	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
-	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	"github.com/sylabs/sif/v2/pkg/sif"
 )
 
@@ -27,12 +28,18 @@ var (
 // ErrNoKeyMaterial is the error returned when no key material was provided.
 var ErrNoKeyMaterial = errors.New("key material not provided")
 
+type encoder interface {
+	// signMessage signs the message from r, and writes the result to w. On success, the hash
+	// function and fingerprint of the signing key are returned.
+	signMessage(w io.Writer, r io.Reader) (ht crypto.Hash, fp []byte, err error)
+}
+
 type groupSigner struct {
-	f         *sif.FileImage   // SIF image to sign.
-	id        uint32           // Group ID.
-	ods       []sif.Descriptor // Descriptors of object(s) to sign.
-	mdHash    crypto.Hash      // Hash type for metadata.
-	sigConfig *packet.Config   // Configuration for signature.
+	en     encoder          // Message encoder.
+	f      *sif.FileImage   // SIF image to sign.
+	id     uint32           // Group ID.
+	ods    []sif.Descriptor // Descriptors of object(s) to sign.
+	mdHash crypto.Hash      // Hash type for metadata.
 }
 
 // groupSignerOpt are used to configure gs.
@@ -68,27 +75,19 @@ func optSignGroupMetadataHash(h crypto.Hash) groupSignerOpt {
 	}
 }
 
-// optSignGroupSignatureConfig sets c as the configuration used for signature generation.
-func optSignGroupSignatureConfig(c *packet.Config) groupSignerOpt {
-	return func(gs *groupSigner) error {
-		gs.sigConfig = c
-		return nil
-	}
-}
-
-// newGroupSigner returns a new groupSigner to add a digital signature for the specified group to
-// f, according to opts.
+// newGroupSigner returns a new groupSigner to add a digital signature using en for the specified
+// group to f, according to opts.
 //
 // By default, all data objects in the group will be signed. To override this behavior, use
 // optSignGroupObjects(). To override the default metadata hash algorithm, use
-// optSignGroupMetadataHash(). To override the default PGP configuration for signature generation,
-// use optSignGroupSignatureConfig().
-func newGroupSigner(f *sif.FileImage, groupID uint32, opts ...groupSignerOpt) (*groupSigner, error) {
+// optSignGroupMetadataHash().
+func newGroupSigner(en encoder, f *sif.FileImage, groupID uint32, opts ...groupSignerOpt) (*groupSigner, error) {
 	if groupID == 0 {
 		return nil, sif.ErrInvalidGroupID
 	}
 
 	gs := groupSigner{
+		en:     en,
 		f:      f,
 		id:     groupID,
 		mdHash: crypto.SHA256,
@@ -136,8 +135,8 @@ func (gs *groupSigner) addObject(od sif.Descriptor) error {
 	return nil
 }
 
-// signWithEntity signs the objects specified by gs with e.
-func (gs *groupSigner) signWithEntity(e *openpgp.Entity) (sif.DescriptorInput, error) {
+// sign creates a digital signature as specified by gs.
+func (gs *groupSigner) sign() (sif.DescriptorInput, error) {
 	// Get minimum object ID in group. Object IDs in the image metadata will be relative to this.
 	minID, err := getGroupMinObjectID(gs.f, gs.id)
 	if err != nil {
@@ -150,17 +149,24 @@ func (gs *groupSigner) signWithEntity(e *openpgp.Entity) (sif.DescriptorInput, e
 		return sif.DescriptorInput{}, fmt.Errorf("failed to get image metadata: %w", err)
 	}
 
-	// Sign and encode image metadata.
+	// Encode image metadata.
+	enc, err := json.Marshal(md)
+	if err != nil {
+		return sif.DescriptorInput{}, fmt.Errorf("failed to encode image metadata: %w", err)
+	}
+
+	// Sign image metadata.
 	b := bytes.Buffer{}
-	if err := signAndEncodeJSON(&b, md, e.PrivateKey, gs.sigConfig); err != nil {
-		return sif.DescriptorInput{}, fmt.Errorf("failed to encode signature: %w", err)
+	ht, fp, err := gs.en.signMessage(&b, bytes.NewReader(enc))
+	if err != nil {
+		return sif.DescriptorInput{}, fmt.Errorf("failed to sign message: %w", err)
 	}
 
 	// Prepare SIF data object descriptor.
 	return sif.NewDescriptorInput(sif.DataSignature, &b,
 		sif.OptNoGroup(),
 		sif.OptLinkedGroupID(gs.id),
-		sif.OptSignatureMetadata(gs.sigConfig.Hash(), e.PrimaryKey.Fingerprint),
+		sif.OptSignatureMetadata(ht, fp),
 	)
 }
 
@@ -264,9 +270,10 @@ type Signer struct {
 	signers []*groupSigner
 }
 
-// NewSigner returns a Signer to add digital signature(s) to f, according to opts.
+// NewSigner returns a Signer to add digital signature(s) to f, according to opts. Key material
+// must be provided, or an error wrapping ErrNoKeyMaterial is returned.
 //
-// Sign requires key material be provided. OptSignWithEntity can be used for this purpose.
+// To use key material from an OpenPGP entity, use OptSignWithEntity.
 //
 // By default, one digital signature is added per object group in f. To override this behavior,
 // consider using OptSignGroup and/or OptSignObjects.
@@ -294,15 +301,18 @@ func NewSigner(f *sif.FileImage, opts ...SignerOpt) (*Signer, error) {
 		opts: so,
 	}
 
-	commonOpts := []groupSignerOpt{
-		optSignGroupSignatureConfig(&packet.Config{
-			Time: so.timeFunc,
-		}),
+	// Get message encoder.
+	var en encoder
+	switch {
+	case so.e != nil:
+		en = newClearsignEncoder(so.e, so.timeFunc)
+	default:
+		return nil, fmt.Errorf("integrity: %w", ErrNoKeyMaterial)
 	}
 
 	// Add signer for each groupID.
 	for _, groupID := range so.groupIDs {
-		gs, err := newGroupSigner(f, groupID, commonOpts...)
+		gs, err := newGroupSigner(en, f, groupID)
 		if err != nil {
 			return nil, fmt.Errorf("integrity: %w", err)
 		}
@@ -312,10 +322,7 @@ func NewSigner(f *sif.FileImage, opts ...SignerOpt) (*Signer, error) {
 	// Add signer(s) for each list of object IDs.
 	for _, ids := range so.objectIDs {
 		err := withGroupedObjects(f, ids, func(groupID uint32, ids []uint32) error {
-			opts := commonOpts
-			opts = append(opts, optSignGroupObjects(ids...))
-
-			gs, err := newGroupSigner(f, groupID, opts...)
+			gs, err := newGroupSigner(en, f, groupID, optSignGroupObjects(ids...))
 			if err != nil {
 				return err
 			}
@@ -336,7 +343,7 @@ func NewSigner(f *sif.FileImage, opts ...SignerOpt) (*Signer, error) {
 		}
 
 		for _, id := range ids {
-			gs, err := newGroupSigner(f, id, commonOpts...)
+			gs, err := newGroupSigner(en, f, id)
 			if err != nil {
 				return nil, fmt.Errorf("integrity: %w", err)
 			}
@@ -348,16 +355,9 @@ func NewSigner(f *sif.FileImage, opts ...SignerOpt) (*Signer, error) {
 }
 
 // Sign adds digital signatures as specified by s.
-//
-// If key material was not provided when s was created, Sign returns an error wrapping
-// ErrNoKeyMaterial.
 func (s *Signer) Sign() error {
-	if s.opts.e == nil {
-		return fmt.Errorf("integrity: %w", ErrNoKeyMaterial)
-	}
-
 	for _, gs := range s.signers {
-		di, err := gs.signWithEntity(s.opts.e)
+		di, err := gs.sign()
 		if err != nil {
 			return fmt.Errorf("integrity: %w", err)
 		}