add more docs and fix sig verification error reasons

aryan9600 · aryan9600 · commit 51a5e77af6cb · 2022-03-28T21:40:43.000+05:30
Signed-off-by: Sanskar Jaiswal &lt;jaiswalsanskar078@gmail.com&gt;
diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go
@@ -85,4 +85,8 @@ const (
 
 	// SymlinkUpdateFailedReason signals a failure in updating a symlink.
 	SymlinkUpdateFailedReason string = "SymlinkUpdateFailed"
+
+	// VerificationFailedReason signals a failure in verifying the signature of
+	// an artifact instance, such as a git commit or a helm chart.
+	VerificationFailedReason string = "VerificationFailed"
 )
diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go
@@ -90,7 +90,10 @@ type HelmChartSpec struct {
 	VerificationKeyring *VerificationKeyring `json:"verificationKeyring,omitempty"`
 }
 
+// VerificationKeyring contains enough info to get the public GPG key to be used for verifying
+// the chart signature using a provenance file.
 type VerificationKeyring struct {
+	// SecretRef is a reference to the secret that contains the public GPG key.
 	// +required
 	SecretRef meta.LocalObjectReference `json:"secretRef,omitempty"`
 
diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml
@@ -405,16 +405,17 @@ spec:
                   type: string
                 type: array
               verificationKeyring:
-                description: Keyring information for verifying the packaged chart's
+                description: VerificationKeyring for verifying the packaged chart's
                   signature using a provenance file.
                 properties:
                   key:
                     default: pubring.gpg
-                    description: The key that corresponds to the keyring value.
+                    description: Key in the SecretRef that contains the public keyring
+                      in legacy GPG format.
                     type: string
                   secretRef:
-                    description: LocalObjectReference contains enough information
-                      to locate the referenced Kubernetes resource object.
+                    description: SecretRef is a reference to the secret that contains
+                      the public GPG key.
                     properties:
                       name:
                         description: Name of the referent.
diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go
@@ -638,7 +638,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj
 	if err := r.Client.Get(ctx, publicKeySecret, secret); err != nil {
 		e := &serror.Event{
 			Err:    fmt.Errorf("PGP public keys secret error: %w", err),
-			Reason: "VerificationError",
+			Reason: sourcev1.VerificationFailedReason,
 		}
 		conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go
@@ -1209,7 +1209,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) {
 			},
 			wantErr: true,
 			assertConditions: []metav1.Condition{
-				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "VerificationError", "PGP public keys secret error: secrets \"none-existing\" not found"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationFailedReason, "PGP public keys secret error: secrets \"none-existing\" not found"),
 			},
 		},
 		{
diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
@@ -473,17 +473,16 @@ 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.AuthenticationFailedReason,
+			Reason: sourcev1.VerificationFailedReason,
 		}
-		conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error())
+		conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
 	opts.Keyring = keyring
 
 	// Build the chart
 	ref := chart.RemoteReference{Name: obj.Spec.Chart, Version: obj.Spec.Version}
 	build, err := cb.Build(ctx, ref, util.TempPathForObj("", ".tgz", obj), opts)
-
 	if err != nil {
 		return sreconcile.ResultEmpty, err
 	}
@@ -607,9 +606,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.AuthenticationFailedReason,
+			Reason: sourcev1.VerificationFailedReason,
 		}
-		conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error())
+		conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
 		return sreconcile.ResultEmpty, e
 	}
 	opts.Keyring = keyring
@@ -698,10 +697,11 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source
 	if b.ProvFilePath != "" {
 		provArtifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), b.Version, fmt.Sprintf("%s-%s.tgz.prov", b.Name, b.Version))
 		if err = r.Storage.CopyFromPath(&provArtifact, b.ProvFilePath); err != nil {
-			return sreconcile.ResultEmpty, &serror.Event{
+			e := &serror.Event{
 				Err:    fmt.Errorf("unable to copy Helm chart provenance file to storage: %w", err),
-				Reason: sourcev1.StorageOperationFailedCondition,
+				Reason: sourcev1.ArchiveOperationFailedReason,
 			}
+			conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
 		}
 	}
 
@@ -803,14 +803,13 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
 		localPath := r.Storage.LocalPath(*obj.GetArtifact())
 		provFilePath := localPath + ".prov"
 		dir := filepath.Dir(localPath)
-		callbacks := make([]func(path string, info os.FileInfo) bool, 0)
-		callbacks = append(callbacks, func(path string, info os.FileInfo) bool {
+		callback := func(path string, info os.FileInfo) bool {
 			if path != localPath && path != provFilePath && info.Mode()&os.ModeSymlink != os.ModeSymlink {
 				return true
 			}
 			return false
-		})
-		if _, err := r.Storage.RemoveConditionally(dir, callbacks); err != nil {
+		}
+		if _, err := r.Storage.RemoveConditionally(dir, callback); err != nil {
 			return &serror.Event{
 				Err:    fmt.Errorf("garbage collection of old artifacts failed: %w", err),
 				Reason: "GarbageCollectionFailed",
@@ -1036,11 +1035,13 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) {
 
 	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))
+		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())
+	} else {
+		conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
 	}
 
 	if err != nil {
diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go
@@ -551,7 +551,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
 				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"),
+					*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) {
diff --git a/controllers/storage.go b/controllers/storage.go
@@ -53,6 +53,10 @@ type Storage struct {
 	Timeout time.Duration `json:"timeout"`
 }
 
+// removeFileCallback is a function which determines whether the
+// provided file should be removed from the filesystem.
+type removeFileCallback func(path string, info os.FileInfo) bool
+
 // NewStorage creates the storage helper for a given path and hostname.
 func NewStorage(basePath string, hostname string, timeout time.Duration) (*Storage, error) {
 	if f, err := os.Stat(basePath); os.IsNotExist(err) || !f.IsDir() {
@@ -145,7 +149,9 @@ func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) ([]string, err
 	return deletedFiles, nil
 }
 
-func (s *Storage) RemoveConditionally(dir string, callbacks []func(path string, info os.FileInfo) bool) ([]string, error) {
+// RemoveConditionally walks through the provided dir and then deletes all files
+// for which any of the callbacks return true.
+func (s *Storage) RemoveConditionally(dir string, callbacks ...removeFileCallback) ([]string, error) {
 	deletedFiles := []string{}
 	var errors []string
 	_ = filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
diff --git a/docs/api/source.md b/docs/api/source.md
@@ -668,6 +668,20 @@ references to this object.
 NOTE: Not implemented, provisional as of <a href="https://github.com/fluxcd/flux2/pull/2092">https://github.com/fluxcd/flux2/pull/2092</a></p>
 </td>
 </tr>
+<tr>
+<td>
+<code>verificationKeyring</code><br>
+<em>
+<a href="#source.toolkit.fluxcd.io/v1beta2.VerificationKeyring">
+VerificationKeyring
+</a>
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>VerificationKeyring for verifying the packaged chart&rsquo;s signature using a provenance file.</p>
+</td>
+</tr>
 </table>
 </td>
 </tr>
@@ -1850,6 +1864,20 @@ references to this object.
 NOTE: Not implemented, provisional as of <a href="https://github.com/fluxcd/flux2/pull/2092">https://github.com/fluxcd/flux2/pull/2092</a></p>
 </td>
 </tr>
+<tr>
+<td>
+<code>verificationKeyring</code><br>
+<em>
+<a href="#source.toolkit.fluxcd.io/v1beta2.VerificationKeyring">
+VerificationKeyring
+</a>
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>VerificationKeyring for verifying the packaged chart&rsquo;s signature using a provenance file.</p>
+</td>
+</tr>
 </tbody>
 </table>
 </div>
@@ -2251,6 +2279,53 @@ string
 Source is the interface that provides generic access to the Artifact and
 interval. It must be supported by all kinds of the source.toolkit.fluxcd.io
 API group.</p>
+<h3 id="source.toolkit.fluxcd.io/v1beta2.VerificationKeyring">VerificationKeyring
+</h3>
+<p>
+(<em>Appears on:</em>
+<a href="#source.toolkit.fluxcd.io/v1beta2.HelmChartSpec">HelmChartSpec</a>)
+</p>
+<p>VerificationKeyring contains enough info to get the public GPG key to be used for verifying
+the chart signature using a provenance file.</p>
+<div class="md-typeset__scrollwrap">
+<div class="md-typeset__table">
+<table>
+<thead>
+<tr>
+<th>Field</th>
+<th>Description</th>
+</tr>
+</thead>
+<tbody>
+<tr>
+<td>
+<code>secretRef</code><br>
+<em>
+<a href="https://godoc.org/github.com/fluxcd/pkg/apis/meta#LocalObjectReference">
+github.com/fluxcd/pkg/apis/meta.LocalObjectReference
+</a>
+</em>
+</td>
+<td>
+<p>SecretRef is a reference to the secret that contains the public GPG key.</p>
+</td>
+</tr>
+<tr>
+<td>
+<code>key</code><br>
+<em>
+string
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>Key in the SecretRef that contains the public keyring in legacy GPG format.</p>
+</td>
+</tr>
+</tbody>
+</table>
+</div>
+</div>
 <div class="admonition note">
 <p class="last">This page was automatically generated with <code>gen-crd-api-reference-docs</code></p>
 </div>
diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go
@@ -105,7 +105,7 @@ type BuildOptions struct {
 	// because the list of ValuesFiles has changed.
 	Force bool
 
-	// Keyring can be configured with the bytes of a public kering in legacy
+	// Keyring can be set to the bytes of a public kering in legacy
 	// PGP format used for verifying a chart's signature using a provenance file.
 	Keyring []byte
 }
@@ -130,11 +130,11 @@ type Build struct {
 	// Can be empty, in which case a failure should be assumed.
 	Path string
 	// ProvFilePath is the absolute path to a provenance file.
-	// Can be empty, in which case it should be assumed that the packaged
+	// It can be empty, in which case it should be assumed that the packaged
 	// chart is not verified.
 	ProvFilePath string
 	// VerificationSignature is populated when a chart's signature
-	// is successfully verified using it's provenance file.
+	// is successfully verified using its provenance file.
 	VerificationSignature *VerificationSignature
 	// ValuesFiles is the list of files used to compose the chart's
 	// default "values.yaml".
diff --git a/internal/util/file.go b/internal/util/file.go
@@ -22,38 +22,32 @@ import (
 	"path/filepath"
 )
 
-func writeBytesToFile(bytes []byte, out string, temp bool) (*os.File, error) {
-	var file *os.File
-	var err error
-
-	if temp {
-		file, err = os.CreateTemp("", out)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create temporary file %s: %w", filepath.Base(out), err)
-		}
-	} else {
-		file, err = os.Create(out)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create temporary file for chart %s: %w", out, err)
-		}
-	}
+func writeBytesToFile(bytes []byte, file *os.File) error {
 	if _, err := file.Write(bytes); err != nil {
 		_ = file.Close()
-		return nil, fmt.Errorf("failed to write to file %s: %w", file.Name(), err)
+		return fmt.Errorf("failed to write to file %s: %w", file.Name(), err)
 	}
 	if err := file.Close(); err != nil {
-		return nil, err
+		return err
 	}
-	return file, nil
+	return nil
 }
 
 // Writes the provided bytes to a file at the given path and returns the file handle.
 func WriteToFile(bytes []byte, path string) (*os.File, error) {
-	return writeBytesToFile(bytes, path, false)
+	file, err := os.Create(path)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create temporary file for chart %s: %w", path, err)
+	}
+	return file, writeBytesToFile(bytes, file)
 }
 
 // Writes the provided bytes to a temp file with the name provided in the path and
 // returns the file handle.
 func WriteToTempFile(bytes []byte, out string) (*os.File, error) {
-	return writeBytesToFile(bytes, filepath.Base(out), true)
+	file, err := os.CreateTemp("", filepath.Base(out))
+	if err != nil {
+		return nil, fmt.Errorf("failed to create temporary file %s: %w", filepath.Base(out), err)
+	}
+	return file, writeBytesToFile(bytes, file)
 }

Original file line number	Diff line number	Diff line change
`@@ -85,4 +85,8 @@ const (`
`85`	`85`
`86`	`86`	`// SymlinkUpdateFailedReason signals a failure in updating a symlink.`
`87`	`87`	`SymlinkUpdateFailedReason string = "SymlinkUpdateFailed"`
	`88`	`+`
	`89`	`+ // VerificationFailedReason signals a failure in verifying the signature of`
	`90`	`+ // an artifact instance, such as a git commit or a helm chart.`
	`91`	`+ VerificationFailedReason string = "VerificationFailed"`
`88`	`92`	`)`
Original file line number	Diff line number	Diff line change
`@@ -90,7 +90,10 @@ type HelmChartSpec struct {`
`90`	`90`	VerificationKeyring *VerificationKeyring `json:"verificationKeyring,omitempty"`
`91`	`91`	`}`
`92`	`92`
	`93`	`+// VerificationKeyring contains enough info to get the public GPG key to be used for verifying`
	`94`	`+// the chart signature using a provenance file.`
`93`	`95`	`type VerificationKeyring struct {`
	`96`	`+ // SecretRef is a reference to the secret that contains the public GPG key.`
`94`	`97`	`// +required`
`95`	`98`	SecretRef meta.LocalObjectReference `json:"secretRef,omitempty"`
`96`	`99`
Original file line number	Diff line number	Diff line change
`@@ -638,7 +638,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj`
`638`	`638`	`if err := r.Client.Get(ctx, publicKeySecret, secret); err != nil {`
`639`	`639`	`e := &serror.Event{`
`640`	`640`	`Err: fmt.Errorf("PGP public keys secret error: %w", err),`
`641`		`- Reason: "VerificationError",`
	`641`	`+ Reason: sourcev1.VerificationFailedReason,`
`642`	`642`	`}`
`643`	`643`	`conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())`
`644`	`644`	`return sreconcile.ResultEmpty, e`
Original file line number	Diff line number	Diff line change
`@@ -1209,7 +1209,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) {`
`1209`	`1209`	`},`
`1210`	`1210`	`wantErr: true,`
`1211`	`1211`	`assertConditions: []metav1.Condition{`
`1212`		`- *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "VerificationError", "PGP public keys secret error: secrets \"none-existing\" not found"),`
	`1212`	`+ *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationFailedReason, "PGP public keys secret error: secrets \"none-existing\" not found"),`
`1213`	`1213`	`},`
`1214`	`1214`	`},`
`1215`	`1215`	`{`