From 0ab44aef702f41b812692e4b3ff5f465dfc7e891 Mon Sep 17 00:00:00 2001
From: Ashley Davis <ashley.davis@jetstack.io>
Date: Tue, 7 Sep 2021 18:20:29 +0100
Subject: [PATCH] add more error checks during the signing process

Before this change, several of the potential errors during the process
of signing a package were skipped.

Crucially, `Close()`ing the ReadCloser from the gpg clearsigner is the call
which actually does the signing, and so has several points of failure
which are ignored; for example, if there's a problem with the format of
the key.

Also changes the error from messageBlock() to be propagated rather than
being swallowed, and adds a test for the case where a signer fails to
sign.

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
---
 pkg/provenance/sign.go      | 21 ++++++++++++++++++---
 pkg/provenance/sign_test.go | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/pkg/provenance/sign.go b/pkg/provenance/sign.go
index 4fefb4ba0..c41f90c61 100644
--- a/pkg/provenance/sign.go
+++ b/pkg/provenance/sign.go
@@ -216,7 +216,7 @@ func (s *Signatory) ClearSign(chartpath string) (string, error) {
 
 	b, err := messageBlock(chartpath)
 	if err != nil {
-		return "", nil
+		return "", err
 	}
 
 	// Sign the buffer
@@ -224,9 +224,24 @@ func (s *Signatory) ClearSign(chartpath string) (string, error) {
 	if err != nil {
 		return "", err
 	}
+
 	_, err = io.Copy(w, b)
-	w.Close()
-	return out.String(), err
+
+	if err != nil {
+		// NB: We intentionally don't call `w.Close()` here! `w.Close()` is the method which
+		// actually does the PGP signing, and therefore is the part which uses the private key.
+		// In other words, if we call Close here, there's a risk that there's an attempt to use the
+		// private key to sign garbage data (since we know that io.Copy failed, `w` won't contain
+		// anything useful).
+		return "", errors.Wrap(err, "failed to write to clearsign encoder")
+	}
+
+	err = w.Close()
+	if err != nil {
+		return "", errors.Wrap(err, "failed to either sign or armor message block")
+	}
+
+	return out.String(), nil
 }
 
 // Verify checks a signature and verifies that it is legit for a chart.
diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go
index c63daebf4..93c169263 100644
--- a/pkg/provenance/sign_test.go
+++ b/pkg/provenance/sign_test.go
@@ -16,6 +16,9 @@ limitations under the License.
 package provenance
 
 import (
+	"crypto"
+	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -230,6 +233,36 @@ func TestClearSign(t *testing.T) {
 	}
 }
 
+// failSigner always fails to sign and returns an error
+type failSigner struct{}
+
+func (s failSigner) Public() crypto.PublicKey {
+	return nil
+}
+
+func (s failSigner) Sign(_ io.Reader, _ []byte, _ crypto.SignerOpts) ([]byte, error) {
+	return nil, fmt.Errorf("always fails")
+}
+
+func TestClearSignError(t *testing.T) {
+	signer, err := NewFromFiles(testKeyfile, testPubfile)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// ensure that signing always fails
+	signer.Entity.PrivateKey.PrivateKey = failSigner{}
+
+	sig, err := signer.ClearSign(testChartfile)
+	if err == nil {
+		t.Fatal("didn't get an error from ClearSign but expected one")
+	}
+
+	if sig != "" {
+		t.Fatalf("expected an empty signature after failed ClearSign but got %q", sig)
+	}
+}
+
 func TestDecodeSignature(t *testing.T) {
 	// Unlike other tests, this does a round-trip test, ensuring that a signature
 	// generated by the library can also be verified by the library.