From a794e282ad010632a802830b038bd415fa72f950 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Thu, 20 Oct 2022 13:48:50 +0200 Subject: [PATCH] Fix verification condition Delete a failed verification condition at the beginning of the source reconciliation and set `SourceVerifiedCondition` to false approprietly. Set the `BuildOptions.Verify` to true as long as Verify is enabled in the API fields. Signed-off-by: Soule BA --- api/v1beta2/helmchart_types.go | 2 +- .../source.toolkit.fluxcd.io_helmcharts.yaml | 5 ++-- controllers/helmchart_controller.go | 23 +++++++++++-------- controllers/helmchart_controller_test.go | 16 +++++++------ docs/api/source.md | 4 ++-- docs/spec/v1beta2/helmcharts.md | 2 +- docs/spec/v1beta2/ocirepositories.md | 2 +- internal/helm/registry/auth.go | 2 +- internal/helm/repository/chart_repository.go | 2 -- 9 files changed, 31 insertions(+), 27 deletions(-) diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index aca993fde..96321a091 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -90,7 +90,7 @@ type HelmChartSpec struct { // Verify contains the secret name containing the trusted public keys // used to verify the signature and specifies which provider to use to check // whether OCI image is authentic. - // This field is only supported for OCI sources. + // This field is only supported when using HelmRepository source with spec.type 'oci'. // Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified. // +optional Verify *OCIRepositoryVerification `json:"verify,omitempty"` diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index c6cdeefeb..c1ac4b6e4 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml @@ -407,8 +407,9 @@ spec: description: Verify contains the secret name containing the trusted public keys used to verify the signature and specifies which provider to use to check whether OCI image is authentic. This field is only - supported for OCI sources. Chart dependencies, which are not bundled - in the umbrella chart artifact, are not verified. + supported when using HelmRepository source with spec.type 'oci'. + Chart dependencies, which are not bundled in the umbrella chart + artifact, are not verified. properties: provider: default: cosign diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index d74a7ae11..a78c7020e 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -372,6 +372,12 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev } func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) { + // Remove any failed verification condition. + // The reason is that a failing verification should be recalculated. + if conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) { + conditions.Delete(obj, sourcev1.SourceVerifiedCondition) + } + // Retrieve the source s, err := r.getSource(ctx, obj) if err != nil { @@ -650,15 +656,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * Force: obj.Generation != obj.Status.ObservedGeneration, // The remote builder will not attempt to download the chart if // an artifact exists with the same name and version and `Force` is false. - // It will try to verify the chart if: - // - we are on the first reconciliation - // - the HelmChart spec has changed (generation drift) - // - the previous reconciliation resulted in a failed artifact verification - // - there is no artifact in storage - Verify: obj.Spec.Verify != nil && (obj.Generation <= 0 || - conditions.GetObservedGeneration(obj, sourcev1.SourceVerifiedCondition) != obj.Generation || - conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) || - obj.GetArtifact() == nil), + // It will however try to verify the chart if `obj.Spec.Verify` is set, at every reconciliation. + Verify: obj.Spec.Verify != nil && obj.Spec.Verify.Provider != "", } if artifact := obj.GetArtifact(); artifact != nil { opts.CachedChart = r.Storage.LocalPath(*artifact) @@ -1293,9 +1292,13 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { } switch buildErr.Reason { - case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage, chart.ErrChartVerification: + case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage: + conditions.Delete(obj, sourcev1.FetchFailedCondition) + conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, buildErr.Reason.Reason, buildErr.Error()) + case chart.ErrChartVerification: conditions.Delete(obj, sourcev1.FetchFailedCondition) conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, buildErr.Reason.Reason, buildErr.Error()) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, buildErr.Error()) default: conditions.Delete(obj, sourcev1.BuildFailedCondition) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, buildErr.Reason.Reason, buildErr.Error()) diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index f9aaf2c89..6f6bb0ddb 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -2262,11 +2262,11 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T tests := []struct { name string + shouldSign bool + beforeFunc func(obj *sourcev1.HelmChart) want sreconcile.Result wantErr bool wantErrMsg string - shouldSign bool - beforeFunc func(obj *sourcev1.HelmChart) assertConditions []metav1.Condition cleanFunc func(g *WithT, build *chart.Build) }{ @@ -2280,11 +2280,12 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T SecretRef: &meta.LocalObjectReference{Name: "cosign-key"}, } }, + want: sreconcile.ResultEmpty, wantErr: true, wantErrMsg: "chart verification error: failed to verify : no matching signatures:", - want: sreconcile.ResultEmpty, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no matching signatures:"), }, }, { @@ -2296,14 +2297,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T Provider: "cosign", } }, - wantErr: true, want: sreconcile.ResultEmpty, + wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no matching signatures:"), }, }, { - name: "signed charts should pass verification", + name: "signed charts should pass verification", + shouldSign: true, beforeFunc: func(obj *sourcev1.HelmChart) { obj.Spec.Chart = metadata.Name obj.Spec.Version = metadata.Version @@ -2312,8 +2315,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T SecretRef: &meta.LocalObjectReference{Name: "cosign-key"}, } }, - shouldSign: true, - want: sreconcile.ResultSuccess, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of version "), diff --git a/docs/api/source.md b/docs/api/source.md index 060197bea..819248f1b 100644 --- a/docs/api/source.md +++ b/docs/api/source.md @@ -684,7 +684,7 @@ OCIRepositoryVerification

Verify contains the secret name containing the trusted public keys used to verify the signature and specifies which provider to use to check whether OCI image is authentic. -This field is only supported for OCI sources. +This field is only supported when using HelmRepository source with spec.type ‘oci’. Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.

@@ -2269,7 +2269,7 @@ OCIRepositoryVerification

Verify contains the secret name containing the trusted public keys used to verify the signature and specifies which provider to use to check whether OCI image is authentic. -This field is only supported for OCI sources. +This field is only supported when using HelmRepository source with spec.type ‘oci’. Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.

diff --git a/docs/spec/v1beta2/helmcharts.md b/docs/spec/v1beta2/helmcharts.md index d6e189870..9dec95d65 100644 --- a/docs/spec/v1beta2/helmcharts.md +++ b/docs/spec/v1beta2/helmcharts.md @@ -290,7 +290,7 @@ data: Note that the keys must have the `.pub` extension for Flux to make use of them. -Flux will loop over the public keys and use them verify a HelmChart's signature. +Flux will loop over the public keys and use them to verify a HelmChart's signature. This allows for older HelmCharts to be valid as long as the right key is in the secret. #### Keyless verification diff --git a/docs/spec/v1beta2/ocirepositories.md b/docs/spec/v1beta2/ocirepositories.md index a6b22aa9d..39d1decf7 100644 --- a/docs/spec/v1beta2/ocirepositories.md +++ b/docs/spec/v1beta2/ocirepositories.md @@ -462,7 +462,7 @@ data: Note that the keys must have the `.pub` extension for Flux to make use of them. -Flux will loop over the public keys and use them verify an artifact's signature. +Flux will loop over the public keys and use them to verify an artifact's signature. This allows for older artifacts to be valid as long as the right key is in the secret. #### Keyless verification diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index debe87eaf..d843d7d3b 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -105,7 +105,7 @@ func KeychainAdaptHelper(keyChain authn.Keychain) func(string) (registry.LoginOp } // AuthAdaptHelper returns an ORAS credentials callback configured with the authorization data -// from the given authn authenticator This allows for example to make use of credential helpers from +// from the given authn authenticator. This allows for example to make use of credential helpers from // cloud providers. // Ref: https://github.com/google/go-containerregistry/tree/main/pkg/authn func AuthAdaptHelper(auth authn.Authenticator) (registry.LoginOption, error) { diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 15e62432a..596bc1a82 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -523,10 +523,8 @@ func (r *ChartRepository) RemoveCache() error { } // VerifyChart verifies the chart against a signature. -// If no signature is provided, a keyless verification is performed. // It returns an error on failure. func (r *ChartRepository) VerifyChart(_ context.Context, _ *repo.ChartVersion) error { - // no-op // this is a no-op because this is not implemented yet. return fmt.Errorf("not implemented") }