make doc changes and fix reconilation process

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

Author: aryan9600

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.

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
 }

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,
 	}
 }

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

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