diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index 660356535..5435d42ff 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -85,7 +85,7 @@ type HelmChartSpec struct { // +optional AccessFrom *acl.AccessFrom `json:"accessFrom,omitempty"` - // Keyring information for verifying the packaged chart's signature using a provenance file. + // VerificationKeyring for verifying the packaged chart's signature using a provenance file. // +optional VerificationKeyring *VerificationKeyring `json:"verificationKeyring,omitempty"` } @@ -94,7 +94,7 @@ type VerificationKeyring struct { // +required SecretRef meta.LocalObjectReference `json:"secretRef,omitempty"` - // The key that corresponds to the keyring value. + // Key in the SecretRef that contains the public keyring in legacy GPG format. // +kubebuilder:default:=pubring.gpg // +optional Key string `json:"key,omitempty"` @@ -168,6 +168,10 @@ const ( // ChartPackageSucceededReason signals that the package of the Helm // chart succeeded. ChartPackageSucceededReason string = "ChartPackageSucceeded" + + // ChartVerifiedSucceededReason signals that the Helm chart's signature + // has been verified using it's provenance file. + ChartVerifiedSucceededReason string = "ChartVerifiedSucceeded" ) // GetConditions returns the status conditions of the object. diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 00e9693d6..b9a2122aa 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -97,8 +97,6 @@ var helmChartReadyCondition = summarize.Conditions{ }, } -const KeyringFileName = "pubring.gpg" - // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts/status,verbs=get;update;patch // +kubebuilder:rbac:groups=source.toolkit.fluxcd.io,resources=helmcharts/finalizers,verbs=get;create;update;patch;delete @@ -475,9 +473,9 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to get public key for chart signature verification: %w", err), - Reason: sourcev1.SourceVerifiedCondition, + Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkFalse(obj, sourcev1.FetchFailedCondition, sourcev1.SourceVerifiedCondition, e.Error()) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } opts.Keyring = keyring @@ -609,9 +607,9 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to get public key for chart signature verification: %w", err), - Reason: sourcev1.SourceVerifiedCondition, + Reason: sourcev1.AuthenticationFailedReason, } - conditions.MarkFalse(obj, sourcev1.FetchFailedCondition, sourcev1.SourceVerifiedCondition, e.Error()) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error()) return sreconcile.ResultEmpty, e } opts.Keyring = keyring @@ -651,14 +649,6 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) conditions.MarkTrue(obj, meta.ReadyCondition, reasonForBuild(b), b.Summary()) } - if b.VerificationSignature != nil && b.ProvFilePath != "" && obj.GetArtifact() != nil { - var sigVerMsg strings.Builder - sigVerMsg.WriteString(fmt.Sprintf("chart signed by: %v", strings.Join(b.VerificationSignature.Identities[:], ","))) - sigVerMsg.WriteString(fmt.Sprintf(" using key with fingeprint: %X", b.VerificationSignature.KeyFingerprint)) - sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: %s", b.VerificationSignature.FileHash)) - - conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, reasonForBuild(b), sigVerMsg.String()) - } }() // Create artifact from build data @@ -914,22 +904,6 @@ func (r *HelmChartReconciler) getHelmRepositorySecret(ctx context.Context, repos return &secret, nil } -func (r *HelmChartReconciler) getVerificationKeyringSecret(ctx context.Context, chart *sourcev1.HelmChart) (*corev1.Secret, error) { - if chart.Spec.VerificationKeyring == nil { - return nil, nil - } - name := types.NamespacedName{ - Namespace: chart.GetNamespace(), - Name: chart.Spec.VerificationKeyring.SecretRef.Name, - } - var secret corev1.Secret - err := r.Client.Get(ctx, name, &secret) - if err != nil { - return nil, err - } - return &secret, nil -} - func (r *HelmChartReconciler) indexHelmRepositoryByURL(o client.Object) []string { repo, ok := o.(*sourcev1.HelmRepository) if !ok { @@ -1060,6 +1034,15 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { conditions.Delete(obj, sourcev1.BuildFailedCondition) } + if build.VerificationSignature != nil && build.ProvFilePath != "" { + var sigVerMsg strings.Builder + sigVerMsg.WriteString(fmt.Sprintf("chart signed by: %v", strings.Join(build.VerificationSignature.Identities[:], ","))) + sigVerMsg.WriteString(fmt.Sprintf(" using key with fingeprint: %X", build.VerificationSignature.KeyFingerprint)) + sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: %s", build.VerificationSignature.FileHash)) + + conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, sourcev1.ChartVerifiedSucceededReason, sigVerMsg.String()) + } + if err != nil { var buildErr *chart.BuildError if ok := errors.As(err, &buildErr); !ok { @@ -1105,10 +1088,10 @@ func (r *HelmChartReconciler) getProvenanceKeyring(ctx context.Context, chart *s return nil, err } key := chart.Spec.VerificationKeyring.Key - if val, ok := secret.Data[key]; !ok { + val, ok := secret.Data[key] + if !ok { err = fmt.Errorf("secret doesn't contain the advertised verification keyring name %s", key) return nil, err - } else { - return val, nil } + return val, nil } diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 37b08630f..55e0cdfde 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -52,7 +52,7 @@ import ( sreconcile "github.com/fluxcd/source-controller/internal/reconcile" ) -const publicKeyFileName = "pub.pgp" +const publicKeyFileName = "pub.gpg" func TestHelmChartReconciler_Reconcile(t *testing.T) { g := NewWithT(t) @@ -458,14 +458,19 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { } g.Expect(storage.Archive(gitArtifact, "testdata/charts", nil)).To(Succeed()) + keyring, err := os.ReadFile("testdata/charts/pub.gpg") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(keyring).ToNot(BeEmpty()) + tests := []struct { - name string - source sourcev1.Source - beforeFunc func(obj *sourcev1.HelmChart) - want sreconcile.Result - wantErr error - assertFunc func(g *WithT, build chart.Build, obj sourcev1.HelmChart) - cleanFunc func(g *WithT, build *chart.Build) + name string + source sourcev1.Source + keyringSecret *corev1.Secret + beforeFunc func(obj *sourcev1.HelmChart) + want sreconcile.Result + wantErr error + assertFunc func(g *WithT, build chart.Build, obj sourcev1.HelmChart) + cleanFunc func(g *WithT, build *chart.Build) }{ { name: "Observes Artifact revision and build result", @@ -501,6 +506,59 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { g.Expect(os.Remove(build.Path)).To(Succeed()) }, }, + { + name: "Observes Artifact revision and build result with valid signature", + source: &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gitrepository", + Namespace: "default", + }, + Status: sourcev1.GitRepositoryStatus{ + Artifact: gitArtifact, + }, + }, + keyringSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keyring-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + publicKeyFileName: keyring, + }, + }, + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = "testdata/charts/helmchart-0.1.0.tgz" + obj.Spec.SourceRef = sourcev1.LocalHelmChartSourceReference{ + Name: "gitrepository", + Kind: sourcev1.GitRepositoryKind, + } + obj.Spec.VerificationKeyring = &sourcev1.VerificationKeyring{ + SecretRef: meta.LocalObjectReference{ + Name: "keyring-secret", + }, + Key: publicKeyFileName, + } + }, + want: sreconcile.ResultSuccess, + assertFunc: func(g *WithT, build chart.Build, obj sourcev1.HelmChart) { + g.Expect(build.Complete()).To(BeTrue()) + g.Expect(build.Name).To(Equal("helmchart")) + g.Expect(build.Version).To(Equal("0.1.0")) + g.Expect(build.Path).To(BeARegularFile()) + g.Expect(build.VerificationSignature).ToNot(BeNil()) + g.Expect(build.ProvFilePath).To(BeARegularFile()) + + g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision)) + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"), + *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, sourcev1.ChartVerifiedSucceededReason, "chart signed by: TestUser using key with fingeprint: 943CB5929ECDA2B5B5EC88BC7035BA97D32A87C1 and hash verified: sha256:007c7b7446eebcb18caeffe9898a3356ba1795f54df40ad39cfcc7382874a10a"), + })) + }, + cleanFunc: func(g *WithT, build *chart.Build) { + g.Expect(os.Remove(build.Path)).To(Succeed()) + g.Expect(os.Remove(build.ProvFilePath)).To(Succeed()) + }, + }, { name: "Error on unavailable source", beforeFunc: func(obj *sourcev1.HelmChart) { @@ -605,6 +663,9 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) { if tt.source != nil { clientBuilder.WithRuntimeObjects(tt.source) } + if tt.keyringSecret != nil { + clientBuilder.WithRuntimeObjects(tt.keyringSecret) + } r := &HelmChartReconciler{ Client: clientBuilder.Build(), @@ -1129,7 +1190,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, { name: "Copying artifact to storage from build makes Ready=True", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", ""), + build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), beforeFunc: func(obj *sourcev1.HelmChart) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") }, @@ -1145,24 +1206,6 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { *conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), }, }, - { - name: "Build with a verified signature sets SourceVerifiedCondition=Truue", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", "testdata/charts/helmchart-0.1.0.tgz.prov"), - beforeFunc: func(obj *sourcev1.HelmChart) { - obj.Status.Artifact = &sourcev1.Artifact{ - Path: "testdata/charts/helmchart-0.1.0.tgz", - } - }, - want: sreconcile.ResultSuccess, - afterFunc: func(t *WithT, obj *sourcev1.HelmChart) { - provArtifact := testStorage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "0.1.0", "helmchart-0.1.0.tgz.prov") - t.Expect(provArtifact.Path).ToNot(BeEmpty()) - }, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"), - *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, sourcev1.ChartPullSucceededReason, "chart signed by: TestUser1,TestUser2 using key with fingeprint: 0102000000000000000000000000000000000000 and hash verified: 53gntj23r24asnf0"), - }, - }, { name: "Up-to-date chart build does not persist artifact to storage", build: &chart.Build{ @@ -1208,7 +1251,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, { name: "Removes ArtifactOutdatedCondition after creating new artifact", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", ""), + build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), beforeFunc: func(obj *sourcev1.HelmChart) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "Foo", "") }, @@ -1226,7 +1269,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) { }, { name: "Creates latest symlink to the created artifact", - build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz", ""), + build: mockChartBuild("helmchart", "0.1.0", "testdata/charts/helmchart-0.1.0.tgz"), afterFunc: func(t *WithT, obj *sourcev1.HelmChart) { t.Expect(obj.GetArtifact()).ToNot(BeNil()) @@ -1726,10 +1769,8 @@ func TestHelmChartReconciler_reconcileSubRecs(t *testing.T) { } } -func mockChartBuild(name, version, path, provFilePath string) *chart.Build { +func mockChartBuild(name, version, path string) *chart.Build { var copyP string - var copyPP string - var verSig *chart.VerificationSignature if path != "" { f, err := os.Open(path) if err == nil { @@ -1743,29 +1784,9 @@ func mockChartBuild(name, version, path, provFilePath string) *chart.Build { } } } - if provFilePath != "" { - f, err := os.Open(provFilePath) - if err == nil { - defer f.Close() - ff, err := os.CreateTemp("", "chart-mock-*.tgz.prov") - if err == nil { - defer ff.Close() - if _, err = io.Copy(ff, f); err == nil { - copyPP = ff.Name() - } - } - verSig = &chart.VerificationSignature{ - FileHash: "53gntj23r24asnf0", - Identities: []string{"TestUser1", "TestUser2"}, - KeyFingerprint: [20]byte{1, 2}, - } - } - } return &chart.Build{ - Name: name, - Version: version, - Path: copyP, - ProvFilePath: copyPP, - VerificationSignature: verSig, + Name: name, + Version: version, + Path: copyP, } } diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index 50fa9ffc5..6250acad3 100644 --- a/internal/helm/chart/builder.go +++ b/internal/helm/chart/builder.go @@ -105,8 +105,8 @@ type BuildOptions struct { // because the list of ValuesFiles has changed. Force bool - // Keyring can be set to the data of the chart VerificationKeyring secret - // used for verifying a chart's signature using a provenance file. + // Keyring can be configured with the bytes of a public kering in legacy + // PGP format used for verifying a chart's signature using a provenance file. Keyring []byte } @@ -134,7 +134,7 @@ type Build struct { // chart is not verified. ProvFilePath string // VerificationSignature is populated when a chart's signature - // is susccessfully verified using it's provenance file. + // is successfully verified using it's provenance file. VerificationSignature *VerificationSignature // ValuesFiles is the list of files used to compose the chart's // default "values.yaml". diff --git a/internal/helm/chart/builder_local.go b/internal/helm/chart/builder_local.go index 6885e1d40..7e5bdf31f 100644 --- a/internal/helm/chart/builder_local.go +++ b/internal/helm/chart/builder_local.go @@ -137,9 +137,11 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // package the chart ourselves, and instead stored it as is. if !requiresPackaging { provFilePath = provenanceFilePath(opts.CachedChart) - if ver, err := verifyProvFile(opts.CachedChart, provFilePath); err != nil { + ver, err := verifyProvFile(opts.CachedChart, provFilePath) + if err != nil { return nil, err - } else { + } + if ver != nil { result.VerificationSignature = buildVerificationSig(ver) result.ProvFilePath = provFilePath } @@ -163,9 +165,11 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, if err = copyFileToPath(provenanceFilePath(localRef.Path), provFilePath); err != nil { return result, &BuildError{Reason: ErrChartPull, Err: err} } - if ver, err := verifyProvFile(localRef.Path, provFilePath); err != nil { - return result, err - } else { + ver, err := verifyProvFile(localRef.Path, provFilePath) + if err != nil { + return nil, err + } + if ver != nil { result.ProvFilePath = provFilePath result.VerificationSignature = buildVerificationSig(ver) } diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index 910ad102a..90c522812 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -135,9 +135,11 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o // package the chart ourselves, and instead stored it as is. if !requiresPackaging { provFilePath = provenanceFilePath(opts.CachedChart) - if ver, err := verifyProvFile(opts.CachedChart, provFilePath); err != nil { + ver, err := verifyProvFile(opts.CachedChart, provFilePath) + if err != nil { return nil, err - } else { + } + if ver != nil { result.ProvFilePath = provFilePath result.VerificationSignature = buildVerificationSig(ver) } @@ -153,13 +155,13 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o // Download the package for the resolved version res, err := b.remote.DownloadChart(cv) - // Deal with the underlying byte slice to avoid having to read the buffer multiple times. - chartBuf := res.Bytes() - if err != nil { err = fmt.Errorf("failed to download chart for remote reference: %w", err) return result, &BuildError{Reason: ErrChartPull, Err: err} } + // Deal with the underlying byte slice to avoid having to read the buffer multiple times. + chartBuf := res.Bytes() + if opts.Keyring != nil { provFilePath = provenanceFilePath(p) err := b.remote.DownloadProvenanceFile(cv, provFilePath) @@ -176,9 +178,11 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o if err != nil { return nil, err } - if ver, err := verifyProvFile(chart.Name(), provFilePath); err != nil { + ver, err := verifyProvFile(chart.Name(), provFilePath) + if err != nil { return nil, err - } else { + } + if ver != nil { result.ProvFilePath = provFilePath result.VerificationSignature = buildVerificationSig(ver) } diff --git a/internal/helm/chart/verify_test.go b/internal/helm/chart/verify_test.go new file mode 100644 index 000000000..86309714f --- /dev/null +++ b/internal/helm/chart/verify_test.go @@ -0,0 +1,18 @@ +package chart + +import ( + "os" + "testing" + + . "github.com/onsi/gomega" +) + +func Test_verifyChartWithProvFile(t *testing.T) { + g := NewWithT(t) + + keyring, err := os.Open("../testdata/charts/pub.gpg") + g.Expect(err).ToNot(HaveOccurred()) + ver, err := verifyChartWithProvFile(keyring, "../testdata/charts/helmchart-0.1.0.tgz", "../testdata/charts/helmchart-0.1.0.tgz.prov") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(ver).ToNot(BeNil()) +} diff --git a/internal/util/file.go b/internal/util/file.go index dc023d318..fed8a73f2 100644 --- a/internal/util/file.go +++ b/internal/util/file.go @@ -1,3 +1,19 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package util import (