Add support for verifying helm charts using provenance file by aryan9600 · Pull Request #605 · fluxcd/source-controller

aryan9600 · 2022-03-08T16:32:55Z

internal/helm/chart/verify.go

internal/helm/chart/builder_local.go

darkowlzz · 2022-03-08T22:43:54Z

internal/helm/chart/builder_local.go

+			}
+		}
+		return nil
+	}


Maybe it'd be better to move this as an independent function. The only thing it's using from the surrounding is the keyring, which can just be passed as another function argument.

internal/helm/chart/builder_remote.go

internal/util/temp.go

controllers/helmchart_controller_test.go

internal/helm/chart/builder.go

api/v1beta2/helmchart_types.go

hiddeco · 2022-03-15T08:24:50Z

controllers/helmchart_controller.go

 	},
 }

+const KeyringFileName = "pubring.gpg"


This does not appear to be actively used (and can be omitted in any case due to the API setting a default).

controllers/helmchart_controller.go

internal/util/file.go

internal/helm/chart/verify.go

hiddeco · 2022-03-15T09:05:11Z

internal/helm/chart/builder_remote.go

+	// Deal with the underlying byte slice to avoid having to read the buffer multiple times.
+	chartBuf := res.Bytes()


Better to deal with err first and only then load the bytes.

internal/helm/chart/builder.go

darkowlzz · 2022-03-16T18:53:48Z

config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml

+                properties:
+                  key:
+                    default: pubring.gpg
+                    description: The key that corresponds to the keyring value.


Looks like old definition.
Running make generate, make manifests and make api-docs seems to result in updates to these and a few other files.

darkowlzz · 2022-03-16T18:58:30Z

api/v1beta2/helmchart_types.go

+
+type VerificationKeyring struct {
+	// +required
+	SecretRef meta.LocalObjectReference `json:"secretRef,omitempty"`


SecretRef and VerificationKeyring can have some description for documentation purposes.

darkowlzz · 2022-03-16T20:17:39Z

controllers/helmchart_controller.go

+			Err:    fmt.Errorf("failed to get public key for chart signature verification: %w", err),
+			Reason: sourcev1.AuthenticationFailedReason,
+		}
+		conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error())


Suggested change

conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error())

conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())

We've been doing it this way in other places with an undocumented agreement that e is an error Event, which just internally calls e.Err.Error() for now, but if the event error string changes in the future, it'd affect the condition message value too.
Event error may not be equal to just the underlying error in the future.

darkowlzz · 2022-03-16T20:27:15Z

controllers/helmchart_controller.go

+	if err != nil {
+		e := &serror.Event{
+			Err:    fmt.Errorf("failed to get public key for chart signature verification: %w", err),
+			Reason: sourcev1.AuthenticationFailedReason,


This reason doesn't quite fit here. It's not an authentication failure, but we couldn't verify because we couldn't get the material needed to perform the verification.
GitRepository reconciler uses VerificationError as the reason, which sounds more accurate. Let's create a new reason constant for it and use the same.

The doc of AuthentiactionFailedReason, states that :

AuthenticationFailedReason signals that a Secret does not have the required fields, or the provided credentials do not match.

This seems like the appropriate reason to have here, because getProvenanceKeyring returns an error only when it can't find a secret or the secret doesn't have the required key.

Yes, but only if the Secret is related to authentication, which provenance isn't.

darkowlzz · 2022-03-16T20:28:16Z

controllers/helmchart_controller.go

 	// 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)
+


don't think this line break is intentional?

darkowlzz · 2022-03-16T21:40:08Z

internal/helm/chart/builder.go

 	// because the list of ValuesFiles has changed.
 	Force bool
+
+	// Keyring can be configured with the bytes of a public kering in legacy


Suggested change

// Keyring can be configured with the bytes of a public kering in legacy

// Keyring can be set to the bytes of a public keyring in legacy

Maybe more consistent with the other descriptions above it?

darkowlzz · 2022-03-16T21:40:54Z

internal/helm/chart/builder.go

 	// 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


Suggested change

// 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

darkowlzz · 2022-03-16T21:41:40Z

internal/helm/chart/builder.go

+	// chart is not verified.
+	ProvFilePath string
+	// VerificationSignature is populated when a chart's signature
+	// is successfully verified using it's provenance file.


Suggested change

// is successfully verified using it's provenance file.

// is successfully verified using its provenance file.

darkowlzz · 2022-03-16T21:45:18Z

internal/helm/chart/builder_local.go

+			}
+		}
+		return nil
+	}


darkowlzz · 2022-03-16T21:47:20Z

internal/helm/chart/verify.go

@@ -0,0 +1,91 @@
+/*
+Copyright 2021 The Flux authors


Suggested change

Copyright 2021 The Flux authors

Copyright 2022 The Flux authors

darkowlzz · 2022-03-16T21:57:08Z

internal/util/file.go

+	"path/filepath"
+)
+
+func writeBytesToFile(bytes []byte, out string, temp bool) (*os.File, error) {


Now that you've two wrapper functions below, how about creating the file in the wrapper functions and don't need to pass a temp variable to this function, pass it os.File that you can just write directly to. Since the wrapper function created the file, it already has the os.File, this function can just return an error.

pjbgf · 2022-03-21T07:34:35Z

controllers/helmchart_controller.go

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


Suggested change

sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: '%s'", build.VerificationSignature.FileHash))

sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: %q", build.VerificationSignature.FileHash))

I think it makes more sense to be consistent, since we are using %s instead of %q in a lot of other places.

We don't use %q because it borks the json we used for logging.

pjbgf · 2022-03-21T07:39:42Z

controllers/helmchart_controller_test.go

 					if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0644); err != nil {
 						return err
 					}
+					if err := testStorage.AtomicWriteFile(provArtifact, strings.NewReader(v), 0644); err != nil {


Let's use octal format for representing os-level octal values:

Suggested change

if err := testStorage.AtomicWriteFile(provArtifact, strings.NewReader(v), 0644); err != nil {

if err := testStorage.AtomicWriteFile(provArtifact, strings.NewReader(v), 0o644); err != nil {

Rather than making a couple of changes in this PR, I think this change should be addressed overall in the PR regarding #603

I do agree on this for existing code, but this line specifically is a new introduction.

Signed-off-by: Sanskar Jaiswal <[email protected]>

aryan9600 · 2022-04-07T18:09:14Z

Converting PR to draft, as this is being put on hold until we reach GA (ref: fluxcd/flux2#2592)

hiddeco self-requested a review March 8, 2022 16:44

hiddeco added area/helm Helm related issues and pull requests enhancement New feature or request labels Mar 8, 2022

darkowlzz reviewed Mar 8, 2022

View reviewed changes

hiddeco mentioned this pull request Mar 11, 2022

controllers: store HelmChart Artifact with suffix #611

Draft

aryan9600 force-pushed the helm-prov-verify branch 2 times, most recently from f3b4a81 to 903d29b Compare March 15, 2022 05:44

aryan9600 marked this pull request as ready for review March 15, 2022 05:48

aryan9600 requested a review from darkowlzz March 15, 2022 06:32

hiddeco reviewed Mar 15, 2022

View reviewed changes

aryan9600 requested a review from hiddeco March 16, 2022 09:53

darkowlzz reviewed Mar 16, 2022

View reviewed changes

aryan9600 force-pushed the helm-prov-verify branch 3 times, most recently from 17d6668 to c3e0c81 Compare March 17, 2022 10:32

pjbgf reviewed Mar 21, 2022

View reviewed changes

aryan9600 force-pushed the helm-prov-verify branch 9 times, most recently from c252e68 to 94abac5 Compare March 25, 2022 06:45

aryan9600 requested a review from darkowlzz March 28, 2022 09:57

aryan9600 added 2 commits March 28, 2022 21:40

update helm chart builder to support prov file verification

fae51ba

Signed-off-by: Sanskar Jaiswal <[email protected]>

update helm builder tests for prov file verification

36f302a

Signed-off-by: Sanskar Jaiswal <[email protected]>

aryan9600 added 8 commits March 28, 2022 21:40

update helmchart reconciliation to verify charts using prov file

d435fe7

Signed-off-by: Sanskar Jaiswal <[email protected]>

update helmchart reconciler tests for prov file verification

4988803

Signed-off-by: Sanskar Jaiswal <[email protected]>

refactor code to be more neat

cef5bee

Signed-off-by: Sanskar Jaiswal <[email protected]>

make doc changes and fix reconilation process

f39ee52

Signed-off-by: Sanskar Jaiswal <[email protected]>

add more docs and fix sig verification error reasons

51a5e77

Signed-off-by: Sanskar Jaiswal <[email protected]>

remove inline function to verify helm charts in builder

a6bb5f0

Signed-off-by: Sanskar Jaiswal <[email protected]>

improve conditions and fix temp file storage

6ee9864

Signed-off-by: Sanskar Jaiswal <[email protected]>

improve verification message and file permissions

fcd34a8

Signed-off-by: Sanskar Jaiswal <[email protected]>

aryan9600 force-pushed the helm-prov-verify branch from 94abac5 to fcd34a8 Compare March 28, 2022 16:11

aryan9600 marked this pull request as draft April 7, 2022 18:08

		// Deal with the underlying byte slice to avoid having to read the buffer multiple times.
		chartBuf := res.Bytes()

	conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Error())
	conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())

	// Keyring can be configured with the bytes of a public kering in legacy
	// Keyring can be set to the bytes of a public keyring in legacy

	// 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

	// is successfully verified using it's provenance file.
	// is successfully verified using its provenance file.

	Copyright 2021 The Flux authors
	Copyright 2022 The Flux authors

	sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: '%s'", build.VerificationSignature.FileHash))
	sigVerMsg.WriteString(fmt.Sprintf(" and hash verified: %q", build.VerificationSignature.FileHash))

	if err := testStorage.AtomicWriteFile(provArtifact, strings.NewReader(v), 0644); err != nil {
	if err := testStorage.AtomicWriteFile(provArtifact, strings.NewReader(v), 0o644); err != nil {

Conversation

aryan9600 commented Mar 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aryan9600 Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkowlzz Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aryan9600 commented Apr 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aryan9600 Mar 17, 2022 •

edited

Loading

darkowlzz Mar 16, 2022 •

edited

Loading