From 7c9176ae350ee73a1316d0d65722ef433df54477 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Fri, 19 Sep 2025 11:51:22 +0800 Subject: [PATCH 1/3] Update dependencies and refactor crypto imports to use ProtonMail's go-crypto package Signed-off-by: Siew Kam Onn --- go.mod | 2 + go.sum | 4 ++ internal/plugin/signing_info.go | 2 +- pkg/provenance/sign.go | 9 +-- pkg/provenance/sign_test.go | 58 +++++++++++++++++- .../testdata/helm-mixed-keyring.pub | Bin 0 -> 1493 bytes 6 files changed, 69 insertions(+), 6 deletions(-) create mode 100644 pkg/provenance/testdata/helm-mixed-keyring.pub diff --git a/go.mod b/go.mod index 77e761de2..50e736d20 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 9fa40e4d4..6ea802640 100644 --- a/go.sum +++ b/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= diff --git a/internal/plugin/signing_info.go b/internal/plugin/signing_info.go index 43d01c893..61ee9cd15 100644 --- a/internal/plugin/signing_info.go +++ b/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" ) diff --git a/pkg/provenance/sign.go b/pkg/provenance/sign.go index 3ffad2765..57af1ad42 100644 --- a/pkg/provenance/sign.go +++ b/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, ) } diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go index 4f2fc7298..1efa62819 100644 --- a/pkg/provenance/sign_test.go +++ b/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,58 @@ func TestClearSign(t *testing.T) { } } +func TestMixedKeyringRSASigningAndVerification(t *testing.T) { + signer, err := NewFromFiles(testKeyfile, testMixedKeyring) + if err != nil { + t.Fatal(err) + } + + 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{} diff --git a/pkg/provenance/testdata/helm-mixed-keyring.pub b/pkg/provenance/testdata/helm-mixed-keyring.pub new file mode 100644 index 0000000000000000000000000000000000000000..7985bd20f1b2390915c75aff4a24c61c77145fba GIT binary patch literal 1493 zcmYk6c{tQ*9Ke6SF~-c0NopvTab@Hxj3dP)MH9Xny(O5%q&qu zVL#4!o6%*gtn{TBmF0={cdAn0jK>Sr4{lEy7G9I0zFjMQNx$`r^J7Ab33WuKdUQH# zs>lCDLoyt1W*(qrBD=Zu^5ynSdAB|H!@L#Vv@G^Dk2(+^&NNg%GM8R8<<;u6IhD&4 zvfYzlpVR*{FAbFAOwQ);<%J1H1r7hlEN@lav+UD!~|ZVgkii$7#fvC#P|eW#RPq=RKplsW2~$R7=oSsd4ie} z<^=i69Mx}G{LVv-N;XIWb)cdk5|H7C>|7lp!N)I%fD7>PA3!4Dg8XO*9|%CCK!{ir zfJE$#({Im{?o9AXGJyk4YqP^s95Igm_1w%KBq}@TW}C7P8P}c39JZ^3y<}Lucj|~s zq+bH7kK5MjOvs>$JGp2V;a7VUi#>%G-NJfx6%Ph5Ki77{SZk$5swd~+sxWWMGe=bu zmKg&za2MiwpE@kUS*dPCk&diBck+4T0~TaK?)rJSf?7o}TRge1wINiRJa1~fmX1t} zxk+vgT;;FhPUze^Fd~pKWZhOzv7c(mRk564F~>XaG}vLNmJfa$AB$9JYF?CYFlpQ} zZa!ai(vG`gv!MiIOt=SE6l6UX@2srKjde%z{uKE^rOsS|bYD(cXX+DeIoR#=qS^5^Emos61*SEV-0-J;N@NXC$ zXY(r8wP<59GP|HAfznX%FC-zplO*(iV#aWAAaNp9M=yCG9eB^;^e zn7mZi6OcSSJosqYKZS>d$(!~bV$cYW&Wf_jCWoen>u5`Jd%Jpyh&Udu&05&feRK9$ zFZyXP)T_{!$x|GMu2QD@>u2(^(+`eJP%ir&J#?9zso zV)#Ks^A-PR3ha-jB^R$HkHpNNI*L+QaLt^n(eX;UB{iZ@S$sp=`&jKpfa&-24_-Vs ziJ}>!2|vv_YJ5aKx{;)BW7U)OVI(#=&a@~1uYd}L_3w2cz8PL6-HLm6Aw%mGA*}O> zM}3fO$?3a1hc3A?z|Me{LGOTMEC=OjLdnku$Iu967zhQqW+)I8z`GlL!d9?-Vh?F9 zBs8?3+y7e1YpK$Z#TtDI7Mq^vx!;7{_1}c3fx{is+x-W=sZ9DB`d8(|e5fcI37kU9 zKrP#7mI_Wnl!UYyk@a};KDK&4ksq?Nd`EBue|bUFuI3PkECjY|It03-Iz&p~o9W-X zn$WjEHt5U^W$cO!`t~&PZY)|P96ki0GIa68*0yXCKH?nj>gNd_*5Q!n+ V@cIdPI!rgbCMSw~vn_HR_y>WtmqP#m literal 0 HcmV?d00001 From 0b5d8033b98a61675d9820f8fb999ae5bf7ddbc4 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Fri, 19 Sep 2025 13:51:01 +0800 Subject: [PATCH 2/3] Add Ed25519 key presence check in mixed keyring signing test Signed-off-by: Siew Kam Onn --- pkg/provenance/sign_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go index 1efa62819..db43779dc 100644 --- a/pkg/provenance/sign_test.go +++ b/pkg/provenance/sign_test.go @@ -276,6 +276,33 @@ func TestMixedKeyringRSASigningAndVerification(t *testing.T) { 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") } From 1c8d5804e232c338959b151c952f59f1725961a4 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Tue, 23 Sep 2025 16:00:04 +0800 Subject: [PATCH 3/3] Refactor SQL tests to use recentUnixTimestamp for time arguments Signed-off-by: Siew Kam Onn --- pkg/storage/driver/sql_test.go | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/pkg/storage/driver/sql_test.go b/pkg/storage/driver/sql_test.go index bd2918aad..fe147816c 100644 --- a/pkg/storage/driver/sql_test.go +++ b/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 {