Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for verifying helm charts using provenance file #605

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

aryan9600
Copy link
Member

Ref: #541

@hiddeco hiddeco self-requested a review March 8, 2022 16:44
@hiddeco hiddeco added area/helm Helm related issues and pull requests enhancement New feature or request labels Mar 8, 2022
internal/helm/chart/verify.go Outdated Show resolved Hide resolved
internal/helm/chart/verify.go Outdated Show resolved Hide resolved
internal/helm/chart/verify.go Show resolved Hide resolved
internal/helm/chart/builder_local.go Show resolved Hide resolved
}
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No?

internal/helm/chart/builder_remote.go Outdated Show resolved Hide resolved
internal/util/temp.go Outdated Show resolved Hide resolved
controllers/helmchart_controller_test.go Show resolved Hide resolved
internal/helm/chart/builder.go Outdated Show resolved Hide resolved
@aryan9600 aryan9600 force-pushed the helm-prov-verify branch 2 times, most recently from f3b4a81 to 903d29b Compare March 15, 2022 05:44
@aryan9600 aryan9600 marked this pull request as ready for review March 15, 2022 05:48
api/v1beta2/helmchart_types.go Outdated Show resolved Hide resolved
api/v1beta2/helmchart_types.go Outdated Show resolved Hide resolved
@@ -95,6 +97,8 @@ var helmChartReadyCondition = summarize.Conditions{
},
}

const KeyringFileName = "pubring.gpg"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
internal/util/file.go Show resolved Hide resolved
internal/helm/chart/verify.go Show resolved Hide resolved
Comment on lines 156 to 157
// Deal with the underlying byte slice to avoid having to read the buffer multiple times.
chartBuf := res.Bytes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

internal/helm/chart/builder.go Outdated Show resolved Hide resolved
internal/helm/chart/builder.go Outdated Show resolved Hide resolved
@aryan9600 aryan9600 requested a review from hiddeco March 16, 2022 09:53
properties:
key:
default: pubring.gpg
description: The key that corresponds to the keyring value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SecretRef and VerificationKeyring can have some description for documentation purposes.

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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@aryan9600 aryan9600 Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think this line break is intentional?

@@ -104,6 +104,10 @@ type BuildOptions struct {
// Force can be set to force the build of the chart, for example
// because the list of ValuesFiles has changed.
Force bool

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -125,6 +129,13 @@ type Build struct {
// Path is the absolute path to the packaged chart.
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Copyright 2021 The Flux authors
Copyright 2022 The Flux authors

"path/filepath"
)

func writeBytesToFile(bytes []byte, out string, temp bool) (*os.File, error) {
Copy link
Contributor

@darkowlzz darkowlzz Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aryan9600 aryan9600 force-pushed the helm-prov-verify branch 3 times, most recently from 17d6668 to c3e0c81 Compare March 17, 2022 10:32
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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
return err
}
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aryan9600 aryan9600 force-pushed the helm-prov-verify branch 9 times, most recently from c252e68 to 94abac5 Compare March 25, 2022 06:45
@aryan9600 aryan9600 marked this pull request as draft April 7, 2022 18:08
@aryan9600
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests enhancement New feature or request
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

5 participants