Merge pull request #31320 from kosiew/sign-31181

provenance: allow RSA signing when ed25519 keys are present (switch to ProtonMail/go-crypto)

go.mod

@@ -10,6 +10,7 @@ require (
 	github.com/Masterminds/sprig/v3 v3.3.0
 	github.com/Masterminds/squirrel v1.5.4
 	github.com/Masterminds/vcs v1.13.3
+	github.com/ProtonMail/go-crypto v1.3.0
 	github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2
 	github.com/cyphar/filepath-securejoin v0.4.1
 	github.com/distribution/distribution/v3 v3.0.0
@@ -64,6 +65,7 @@ require (
 	github.com/cenkalti/backoff/v4 v4.3.0 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/chai2010/gettext-go v1.0.2 // indirect
+	github.com/cloudflare/circl v1.6.0 // indirect
 	github.com/coreos/go-systemd/v22 v22.5.0 // indirect
 	github.com/cpuguy83/go-md2man/v2 v2.0.6 // indirect
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect

go.sum

@@ -22,6 +22,8 @@ github.com/Masterminds/squirrel v1.5.4 h1:uUcX/aBc8O7Fg9kaISIUsHXdKuqehiXAMQTYX8
 github.com/Masterminds/squirrel v1.5.4/go.mod h1:NNaOrjSoIDfDA40n7sr2tPNZRfjzjA400rg+riTZj10=
 github.com/Masterminds/vcs v1.13.3 h1:IIA2aBdXvfbIM+yl/eTnL4hb1XwdpvuQLglAix1gweE=
 github.com/Masterminds/vcs v1.13.3/go.mod h1:TiE7xuEjl1N4j016moRd6vezp6e6Lz23gypeXfzXeW8=
+github.com/ProtonMail/go-crypto v1.3.0 h1:ILq8+Sf5If5DCpHQp4PbZdS1J7HDFRXz/+xKBiRGFrw=
+github.com/ProtonMail/go-crypto v1.3.0/go.mod h1:9whxjD8Rbs29b4XWbB8irEcE8KHMqaR2e7GWU1R+/PE=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
 github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio=
@@ -49,6 +51,8 @@ github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UF
 github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/chai2010/gettext-go v1.0.2 h1:1Lwwip6Q2QGsAdl/ZKPCwTe9fe0CjlUbqj5bFNSjIRk=
 github.com/chai2010/gettext-go v1.0.2/go.mod h1:y+wnP2cHYaVj19NZhYKAwEMH2CI1gNHeQQ+5AjwawxA=
+github.com/cloudflare/circl v1.6.0 h1:cr5JKic4HI+LkINy2lg3W2jF8sHCVTBncJr5gIIq7qk=
+github.com/cloudflare/circl v1.6.0/go.mod h1:uddAzsPgqdMAYatqJ0lsjX1oECcQLIlRpzZh3pJrofs=
 github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8iXXhfZs=
 github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc=
 github.com/cpuguy83/go-md2man/v2 v2.0.6 h1:XJtiaUW6dEEqVuZiMTn1ldk455QWwEIsMIJlo5vtkx0=

internal/plugin/signing_info.go

@@ -23,7 +23,7 @@ import (
 	"path/filepath"
 	"strings"
 
-	"golang.org/x/crypto/openpgp/clearsign" //nolint
+	"github.com/ProtonMail/go-crypto/openpgp/clearsign" //nolint
 
 	"helm.sh/helm/v4/pkg/helmpath"
 )

pkg/provenance/sign.go

@@ -25,9 +25,9 @@ import (
 	"os"
 	"strings"
 
-	"golang.org/x/crypto/openpgp"           //nolint
-	"golang.org/x/crypto/openpgp/clearsign" //nolint
-	"golang.org/x/crypto/openpgp/packet"    //nolint
+	"github.com/ProtonMail/go-crypto/openpgp"           //nolint
+	"github.com/ProtonMail/go-crypto/openpgp/clearsign" //nolint
+	"github.com/ProtonMail/go-crypto/openpgp/packet"    //nolint
 	"sigs.k8s.io/yaml"
 )
 
@@ -281,8 +281,9 @@ func (s *Signatory) Verify(archiveData, provData []byte, filename string) (*Veri
 func (s *Signatory) verifySignature(block *clearsign.Block) (*openpgp.Entity, error) {
 	return openpgp.CheckDetachedSignature(
 		s.KeyRing,
-		bytes.NewBuffer(block.Bytes),
+		bytes.NewReader(block.Bytes),
 		block.ArmoredSignature.Body,
+		&defaultPGPConfig,
 	)
 }
 

pkg/provenance/sign_test.go

@@ -24,7 +24,8 @@ import (
 	"strings"
 	"testing"
 
-	pgperrors "golang.org/x/crypto/openpgp/errors" //nolint
+	pgperrors "github.com/ProtonMail/go-crypto/openpgp/errors" //nolint
+	"github.com/ProtonMail/go-crypto/openpgp/packet"           //nolint
 	"sigs.k8s.io/yaml"
 
 	"helm.sh/helm/v4/pkg/chart/v2/loader"
@@ -59,6 +60,9 @@ const (
 	// testTamperedSigBlock is a tampered copy of msgblock.yaml.asc
 	testTamperedSigBlock = "testdata/msgblock.yaml.tampered"
 
+	// testMixedKeyring points to a keyring containing RSA and ed25519 keys.
+	testMixedKeyring = "testdata/helm-mixed-keyring.pub"
+
 	// testSumfile points to a SHA256 sum generated by an external tool.
 	// We always want to validate against an external tool's representation to
 	// verify that we haven't done something stupid. This file was generated
@@ -266,6 +270,85 @@ func TestClearSign(t *testing.T) {
 	}
 }
 
+func TestMixedKeyringRSASigningAndVerification(t *testing.T) {
+	signer, err := NewFromFiles(testKeyfile, testMixedKeyring)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(signer.KeyRing) == 0 {
+		t.Fatal("expected signer keyring to be loaded")
+	}
+
+	hasEdDSA := false
+	for _, entity := range signer.KeyRing {
+		if entity.PrimaryKey != nil && entity.PrimaryKey.PubKeyAlgo == packet.PubKeyAlgoEdDSA {
+			hasEdDSA = true
+			break
+		}
+
+		for _, subkey := range entity.Subkeys {
+			if subkey.PublicKey != nil && subkey.PublicKey.PubKeyAlgo == packet.PubKeyAlgoEdDSA {
+				hasEdDSA = true
+				break
+			}
+		}
+
+		if hasEdDSA {
+			break
+		}
+	}
+
+	if !hasEdDSA {
+		t.Fatalf("expected %s to include an Ed25519 public key", testMixedKeyring)
+	}
+
+	if signer.Entity == nil {
+		t.Fatal("expected signer entity to be loaded")
+	}
+
+	if signer.Entity.PrivateKey == nil {
+		t.Fatal("expected signer private key to be loaded")
+	}
+
+	if signer.Entity.PrivateKey.PubKeyAlgo != packet.PubKeyAlgoRSA {
+		t.Fatalf("expected RSA key but got %v", signer.Entity.PrivateKey.PubKeyAlgo)
+	}
+
+	metadataBytes := loadChartMetadataForSigning(t, testChartfile)
+
+	archiveData, err := os.ReadFile(testChartfile)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	sig, err := signer.ClearSign(archiveData, filepath.Base(testChartfile), metadataBytes)
+	if err != nil {
+		t.Fatalf("failed to sign chart: %v", err)
+	}
+
+	verification, err := signer.Verify(archiveData, []byte(sig), filepath.Base(testChartfile))
+	if err != nil {
+		t.Fatalf("failed to verify chart signature: %v", err)
+	}
+
+	if verification.SignedBy == nil {
+		t.Fatal("expected verification to include signer")
+	}
+
+	if verification.SignedBy.PrimaryKey == nil {
+		t.Fatal("expected verification to include signer primary key")
+	}
+
+	if verification.SignedBy.PrimaryKey.PubKeyAlgo != packet.PubKeyAlgoRSA {
+		t.Fatalf("expected verification to report RSA key but got %v", verification.SignedBy.PrimaryKey.PubKeyAlgo)
+	}
+
+	if _, ok := verification.SignedBy.Identities[testKeyName]; !ok {
+		t.Fatalf("expected verification to be signed by %q", testKeyName)
+	}
+}
+
 // failSigner always fails to sign and returns an error
 type failSigner struct{}
 

pkg/storage/driver/sql_test.go

@@ -14,6 +14,7 @@ limitations under the License.
 package driver
 
 import (
+	"database/sql/driver"
 	"fmt"
 	"reflect"
 	"regexp"
@@ -26,6 +27,33 @@ import (
 	rspb "helm.sh/helm/v4/pkg/release/v1"
 )
 
+const recentTimestampTolerance = time.Second
+
+func recentUnixTimestamp() sqlmock.Argument {
+	return recentUnixTimestampArgument{}
+}
+
+type recentUnixTimestampArgument struct{}
+
+func (recentUnixTimestampArgument) Match(value driver.Value) bool {
+	var ts int64
+	switch v := value.(type) {
+	case int:
+		ts = int64(v)
+	case int64:
+		ts = v
+	default:
+		return false
+	}
+
+	diff := time.Since(time.Unix(ts, 0))
+	if diff < 0 {
+		diff = -diff
+	}
+
+	return diff <= recentTimestampTolerance
+}
+
 func TestSQLName(t *testing.T) {
 	sqlDriver, _ := newTestFixtureSQL(t)
 	if sqlDriver.Name() != SQLDriverName {
@@ -197,7 +225,7 @@ func TestSqlCreate(t *testing.T) {
 	mock.ExpectBegin()
 	mock.
 		ExpectExec(regexp.QuoteMeta(query)).
-		WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, int(time.Now().Unix())).
+		WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()).
 		WillReturnResult(sqlmock.NewResult(1, 1))
 
 	labelsQuery := fmt.Sprintf(
@@ -255,7 +283,7 @@ func TestSqlCreateAlreadyExists(t *testing.T) {
 	mock.ExpectBegin()
 	mock.
 		ExpectExec(regexp.QuoteMeta(insertQuery)).
-		WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, int(time.Now().Unix())).
+		WithArgs(key, sqlReleaseDefaultType, body, rel.Name, rel.Namespace, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp()).
 		WillReturnError(fmt.Errorf("dialect dependent SQL error"))
 
 	selectQuery := fmt.Sprintf(
@@ -313,7 +341,7 @@ func TestSqlUpdate(t *testing.T) {
 
 	mock.
 		ExpectExec(regexp.QuoteMeta(query)).
-		WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, int(time.Now().Unix()), key, namespace).
+		WithArgs(body, rel.Name, int(rel.Version), rel.Info.Status.String(), sqlReleaseDefaultOwner, recentUnixTimestamp(), key, namespace).
 		WillReturnResult(sqlmock.NewResult(0, 1))
 
 	if err := sqlDriver.Update(key, rel); err != nil {