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 <soule@weave.works>

Author: souleb

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"`

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

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())

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 <url>: no matching signatures:",
-			want:       sreconcile.ResultEmpty,
 			assertConditions: []metav1.Condition{
 				*conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify <url>: no matching signatures:"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify <url>: 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 <url>: no matching signatures:"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify <url>: 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 '<name>' chart with version '<version>'"),
 				*conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of version <version>"),

docs/api/source.md

@@ -684,7 +684,7 @@ OCIRepositoryVerification
 <p>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 &lsquo;oci&rsquo;.
 Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.</p>
 </td>
 </tr>
@@ -2269,7 +2269,7 @@ OCIRepositoryVerification
 <p>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 &lsquo;oci&rsquo;.
 Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.</p>
 </td>
 </tr>

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

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

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) {

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")
 }