From 17d666864c903e2c491117cd898e4518170dbcfb Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 17 Mar 2022 14:51:07 +0530 Subject: [PATCH] add more docs and fix error reasons Signed-off-by: Sanskar Jaiswal --- api/v1beta2/condition_types.go | 4 + api/v1beta2/helmchart_types.go | 3 + api/v1beta2/zz_generated.deepcopy.go | 21 ++++++ .../source.toolkit.fluxcd.io_helmcharts.yaml | 9 ++- controllers/gitrepository_controller.go | 2 +- controllers/gitrepository_controller_test.go | 2 +- controllers/helmchart_controller.go | 29 +++---- controllers/helmchart_controller_test.go | 2 +- controllers/storage.go | 8 +- docs/api/source.md | 75 +++++++++++++++++++ internal/helm/chart/builder.go | 6 +- internal/util/file.go | 34 ++++----- 12 files changed, 150 insertions(+), 45 deletions(-) diff --git a/api/v1beta2/condition_types.go b/api/v1beta2/condition_types.go index 2611cf257..bd2f05f62 100644 --- a/api/v1beta2/condition_types.go +++ b/api/v1beta2/condition_types.go @@ -80,4 +80,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 index 5435d42ff..afaa5c1b4 100644 --- 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 index b789d81da..6cfbc3e35 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -442,6 +442,11 @@ func (in *HelmChartSpec) DeepCopyInto(out *HelmChartSpec) { *out = new(acl.AccessFrom) (*in).DeepCopyInto(*out) } + if in.VerificationKeyring != nil { + in, out := &in.VerificationKeyring, &out.VerificationKeyring + *out = new(VerificationKeyring) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HelmChartSpec. @@ -614,3 +619,19 @@ func (in *LocalHelmChartSourceReference) DeepCopy() *LocalHelmChartSourceReferen in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *VerificationKeyring) DeepCopyInto(out *VerificationKeyring) { + *out = *in + out.SecretRef = in.SecretRef +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VerificationKeyring. +func (in *VerificationKeyring) DeepCopy() *VerificationKeyring { + if in == nil { + return nil + } + out := new(VerificationKeyring) + in.DeepCopyInto(out) + return out +} diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index 1671ac596..2026e7efa 100644 --- 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 index 6fa4bc10f..85e16dd3b 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -636,7 +636,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 index 59d17ea16..28b2cb7f4 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -1194,7 +1194,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 index b9a2122aa..b2f40bc3e 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -473,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.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 @@ -483,7 +483,6 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // 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 index 55e0cdfde..51a701431 100644 --- 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 index bd39b66b4..54bf06409 100644 --- 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 index 6f0d1621b..600aa4125 100644 --- a/docs/api/source.md +++ b/docs/api/source.md @@ -668,6 +668,20 @@ references to this object. NOTE: Not implemented, provisional as of https://github.com/fluxcd/flux2/pull/2092

+ + +verificationKeyring
+ + +VerificationKeyring + + + + +(Optional) +

VerificationKeyring for verifying the packaged chart’s signature using a provenance file.

+ + @@ -1850,6 +1864,20 @@ references to this object. NOTE: Not implemented, provisional as of https://github.com/fluxcd/flux2/pull/2092

+ + +verificationKeyring
+ + +VerificationKeyring + + + + +(Optional) +

VerificationKeyring for verifying the packaged chart’s signature using a provenance file.

+ + @@ -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.

+

VerificationKeyring +

+

+(Appears on: +HelmChartSpec) +

+

VerificationKeyring contains enough info to get the public GPG key to be used for verifying +the chart signature using a provenance file.

+
+
+ + + + + + + + + + + + + + + + + +
FieldDescription
+secretRef
+ + +github.com/fluxcd/pkg/apis/meta.LocalObjectReference + + +		 +

SecretRef is a reference to the secret that contains the public GPG key.

+
+key
+ +string + +		 +(Optional) +

Key in the SecretRef that contains the public keyring in legacy GPG format.

+
+
+

This page was automatically generated with gen-crd-api-reference-docs

diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index 6250acad3..aaee045c6 100644 --- 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 index fed8a73f2..403799b60 100644 --- 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) }