Repository: helm/helm
Pull request #10201 (2021-09-30 13:42:49)

Add more error checks during the signing process
**What this PR does / why we need it**: 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 unit test which fails when run against the `main` branch currently. 

**Special notes for your reviewer**: `io.Copy` is documented to not return EOF, so there's no need to check for that case when checking its error.

I guess returning these errors is, in a way, backwards-incompatible - a workflow today could be silently failing to sign and returning `"", nil` which wouldn't be reported as an error. I'm not sure how useful such a workflow would be though since the generated "signature" would be an empty file. In such a case I'd want `helm` to start loudly failing to sign. 


Patch:
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.


Test patch:
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.