From bb83270acc5708b5f03e68100c54087a0411575d Mon Sep 17 00:00:00 2001 From: Soule BA Date: Mon, 26 Sep 2022 16:10:53 +0200 Subject: [PATCH 1/6] Refactor to use authn for authentication as OCIrepository does If implemented the oras registry loginOption will only be used internaly with the specific ChartRepo struct. This will permit reusing more easily feature developped with googlecontainerregistry authn. Signed-off-by: Soule BA --- controllers/helmchart_controller.go | 93 +++++++++++-------- controllers/helmrepository_controller_oci.go | 90 ++++++++++-------- internal/helm/registry/auth.go | 70 +++++++++++--- internal/helm/registry/auth_test.go | 28 ++++-- .../helm/repository/oci_chart_repository.go | 1 + 5 files changed, 185 insertions(+), 97 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 3f6b8504..73662da0 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -56,6 +56,7 @@ import ( "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" "github.com/fluxcd/pkg/untar" + "github.com/google/go-containerregistry/pkg/authn" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" @@ -455,8 +456,9 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1 func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *sourcev1.HelmChart, repo *sourcev1.HelmRepository, b *chart.Build) (sreconcile.Result, error) { var ( - tlsConfig *tls.Config - loginOpts []helmreg.LoginOption + tlsConfig *tls.Config + authenticator authn.Authenticator + keychain authn.Keychain ) // Used to login with the repository declared provider ctxTimeout, cancel := context.WithTimeout(ctx, repo.Spec.Timeout.Duration) @@ -481,10 +483,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * } // Build client options from secret - opts, err := getter.ClientOptionsFromSecret(*secret) + opts, tls, err := r.clientOptionsFromSecret(secret, normalizedURL) if err != nil { e := &serror.Event{ - Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), + Err: err, Reason: sourcev1.AuthenticationFailedReason, } conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) @@ -492,20 +494,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } clientOpts = append(clientOpts, opts...) - - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) - if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create TLS client config with secret data: %w", err), - Reason: sourcev1.AuthenticationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // Requeue as content of secret might change - return sreconcile.ResultEmpty, e - } + tlsConfig = tls // Build registryClient options from secret - loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret) + keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to configure Helm client with secret data: %w", err), @@ -515,10 +507,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // Requeue as content of secret might change return sreconcile.ResultEmpty, e } - - loginOpts = append([]helmreg.LoginOption{}, loginOpt) } else if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI { - auth, authErr := oidcAuthFromAdapter(ctxTimeout, repo.Spec.URL, repo.Spec.Provider) + auth, authErr := oidcAuth(ctxTimeout, repo.Spec.URL, repo.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { e := &serror.Event{ Err: fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr), @@ -528,10 +518,20 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * return sreconcile.ResultEmpty, e } if auth != nil { - loginOpts = append([]helmreg.LoginOption{}, auth) + authenticator = auth } } + loginOpt, err := makeLoginOption(authenticator, keychain, normalizedURL) + if err != nil { + e := &serror.Event{ + Err: err, + Reason: sourcev1.AuthenticationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + // Initialize the chart repository var chartRepo repository.Downloader switch repo.Spec.Type { @@ -545,7 +545,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // this is needed because otherwise the credentials are stored in ~/.docker/config.json. // TODO@souleb: remove this once the registry move to Oras v2 // or rework to enable reusing credentials to avoid the unneccessary handshake operations - registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpt != nil) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -574,8 +574,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if loginOpts != nil { - err = ociChartRepo.Login(loginOpts...) + if keychain != nil { + err = ociChartRepo.Login(loginOpt) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to login to OCI registry: %w", err), @@ -941,8 +941,9 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1. func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartDownloaderCallback { return func(url string) (repository.Downloader, error) { var ( - tlsConfig *tls.Config - loginOpts []helmreg.LoginOption + tlsConfig *tls.Config + authenticator authn.Authenticator + keychain authn.Keychain ) normalizedURL := repository.NormalizeURL(url) repo, err := r.resolveDependencyRepository(ctx, url, namespace) @@ -972,37 +973,39 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont if err != nil { return nil, err } - opts, err := getter.ClientOptionsFromSecret(*secret) + + // Build client options from secret + opts, tls, err := r.clientOptionsFromSecret(secret, normalizedURL) if err != nil { return nil, err } clientOpts = append(clientOpts, opts...) - - tlsConfig, err = getter.TLSClientConfigFromSecret(*secret, normalizedURL) - if err != nil { - return nil, fmt.Errorf("failed to create TLS client config for HelmRepository '%s': %w", repo.Name, err) - } + tlsConfig = tls // Build registryClient options from secret - loginOpt, err := registry.LoginOptionFromSecret(normalizedURL, *secret) + keychain, err = registry.LoginOptionFromSecret(normalizedURL, *secret) if err != nil { return nil, fmt.Errorf("failed to create login options for HelmRepository '%s': %w", repo.Name, err) } - loginOpts = append([]helmreg.LoginOption{}, loginOpt) } else if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI { - auth, authErr := oidcAuthFromAdapter(ctxTimeout, repo.Spec.URL, repo.Spec.Provider) + auth, authErr := oidcAuth(ctxTimeout, repo.Spec.URL, repo.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { return nil, fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr) } if auth != nil { - loginOpts = append([]helmreg.LoginOption{}, auth) + authenticator = auth } } + loginOpt, err := makeLoginOption(authenticator, keychain, normalizedURL) + if err != nil { + return nil, err + } + var chartRepo repository.Downloader if helmreg.IsOCI(normalizedURL) { - registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpts != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(loginOpt != nil) if err != nil { return nil, fmt.Errorf("failed to create registry client for HelmRepository '%s': %w", repo.Name, err) } @@ -1027,8 +1030,8 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if loginOpts != nil { - err = ociChartRepo.Login(loginOpts...) + if keychain != nil { + err = ociChartRepo.Login(loginOpt) if err != nil { errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err)) // clean up the credentialsFile @@ -1078,6 +1081,20 @@ func (r *HelmChartReconciler) resolveDependencyRepository(ctx context.Context, u return nil, fmt.Errorf("no HelmRepository found for '%s' in '%s' namespace", url, namespace) } +func (r *HelmChartReconciler) clientOptionsFromSecret(secret *corev1.Secret, normalizedURL string) ([]helmgetter.Option, *tls.Config, error) { + opts, err := getter.ClientOptionsFromSecret(*secret) + if err != nil { + return nil, nil, fmt.Errorf("failed to configure Helm client with secret data: %w", err) + } + + tlsConfig, err := getter.TLSClientConfigFromSecret(*secret, normalizedURL) + if err != nil { + return nil, nil, fmt.Errorf("failed to create TLS client config with secret data: %w", err) + } + + return opts, tlsConfig, nil +} + func (r *HelmChartReconciler) getHelmRepositorySecret(ctx context.Context, repository *sourcev1.HelmRepository) (*corev1.Secret, error) { if repository.Spec.SecretRef == nil { return nil, nil diff --git a/controllers/helmrepository_controller_oci.go b/controllers/helmrepository_controller_oci.go index d42154d6..7e383e0c 100644 --- a/controllers/helmrepository_controller_oci.go +++ b/controllers/helmrepository_controller_oci.go @@ -45,6 +45,7 @@ import ( helper "github.com/fluxcd/pkg/runtime/controller" "github.com/fluxcd/pkg/runtime/patch" "github.com/fluxcd/pkg/runtime/predicates" + "github.com/google/go-containerregistry/pkg/authn" "github.com/fluxcd/source-controller/api/v1beta2" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" @@ -263,36 +264,21 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta } conditions.Delete(obj, meta.StalledCondition) - var loginOpts []helmreg.LoginOption + var ( + authenticator authn.Authenticator + keychain authn.Keychain + err error + ) // Configure any authentication related options. if obj.Spec.SecretRef != nil { - // Attempt to retrieve secret. - name := types.NamespacedName{ - Namespace: obj.GetNamespace(), - Name: obj.Spec.SecretRef.Name, - } - var secret corev1.Secret - if err := r.Client.Get(ctx, name, &secret); err != nil { - e := fmt.Errorf("failed to get secret '%s': %w", name.String(), err) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) - result, retErr = ctrl.Result{}, e - return - } - - // Construct login options. - loginOpt, err := registry.LoginOptionFromSecret(obj.Spec.URL, secret) + keychain, err = authFromSecret(ctx, r.Client, obj) if err != nil { - e := fmt.Errorf("failed to configure Helm client with secret data: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) - result, retErr = ctrl.Result{}, e + conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) + result, retErr = ctrl.Result{}, err return } - - if loginOpt != nil { - loginOpts = append(loginOpts, loginOpt) - } } else if obj.Spec.Provider != sourcev1.GenericOCIProvider && obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI { - auth, authErr := oidcAuthFromAdapter(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) + auth, authErr := oidcAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { e := fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr) conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) @@ -300,12 +286,19 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta return } if auth != nil { - loginOpts = append(loginOpts, auth) + authenticator = auth } } + loginOpt, err := makeLoginOption(authenticator, keychain, obj.Spec.URL) + if err != nil { + conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) + result, retErr = ctrl.Result{}, err + return + } + // Create registry client and login if needed. - registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil) + registryClient, file, err := r.RegistryClientGenerator(loginOpt != nil) if err != nil { e := fmt.Errorf("failed to create registry client: %w", err) conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, e.Error()) @@ -332,8 +325,8 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta conditions.Delete(obj, meta.StalledCondition) // Attempt to login to the registry if credentials are provided. - if loginOpts != nil { - err = chartRepo.Login(loginOpts...) + if loginOpt != nil { + err = chartRepo.Login(loginOpt) if err != nil { e := fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err) conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error()) @@ -375,16 +368,37 @@ func (r *HelmRepositoryOCIReconciler) eventLogf(ctx context.Context, obj runtime r.Eventf(obj, eventType, reason, msg) } -// oidcAuthFromAdapter generates the OIDC credential authenticator based on the specified cloud provider. -func oidcAuthFromAdapter(ctx context.Context, url, provider string) (helmreg.LoginOption, error) { - auth, err := oidcAuth(ctx, url, provider) +// authFromSecret returns an authn.Keychain for the given HelmRepository. +// If the HelmRepository does not specify a secretRef, an anonymous keychain is returned. +func authFromSecret(ctx context.Context, client client.Client, obj *sourcev1.HelmRepository) (authn.Keychain, error) { + // Attempt to retrieve secret. + name := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.SecretRef.Name, + } + var secret corev1.Secret + if err := client.Get(ctx, name, &secret); err != nil { + return nil, fmt.Errorf("failed to get secret '%s': %w", name.String(), err) + } + + // Construct login options. + keychain, err := registry.LoginOptionFromSecret(obj.Spec.URL, secret) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to configure Helm client with secret data: %w", err) } - - if auth == nil { - return nil, fmt.Errorf("could not validate OCI provider %s with URL %s", provider, url) - } - - return registry.OIDCAdaptHelper(auth) + return keychain, nil +} + +// makeLoginOption returns a registry login option for the given HelmRepository. +// If the HelmRepository does not specify a secretRef, a nil login option is returned. +func makeLoginOption(auth authn.Authenticator, keychain authn.Keychain, registryURL string) (helmreg.LoginOption, error) { + if auth != nil { + return registry.AuthAdaptHelper(auth) + } + + if keychain != nil { + return registry.KeychainAdaptHelper(keychain)(registryURL) + } + + return nil, nil } diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index e45d0517..4914c568 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -23,27 +23,42 @@ import ( "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/credentials" + "github.com/fluxcd/source-controller/internal/oci" "github.com/google/go-containerregistry/pkg/authn" "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" ) +// helper is a subset of the Docker credential helper credentials.Helper interface used by NewKeychainFromHelper. +type helper struct { + registry string + username, password string + err error +} + +func (h helper) Get(serverURL string) (string, string, error) { + if serverURL != h.registry { + return "", "", fmt.Errorf("unexpected serverURL: %s", serverURL) + } + return h.username, h.password, h.err +} + // LoginOptionFromSecret derives authentication data from a Secret to login to an OCI registry. This Secret // may either hold "username" and "password" fields or be of the corev1.SecretTypeDockerConfigJson type and hold // a corev1.DockerConfigJsonKey field with a complete Docker configuration. If both, "username" and "password" are // empty, a nil LoginOption and a nil error will be returned. -func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (registry.LoginOption, error) { +func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (authn.Keychain, error) { var username, password string + parsedURL, err := url.Parse(registryURL) + if err != nil { + return nil, fmt.Errorf("unable to parse registry URL '%s' while reconciling Secret '%s': %w", + registryURL, secret.Name, err) + } if secret.Type == corev1.SecretTypeDockerConfigJson { dockerCfg, err := config.LoadFromReader(bytes.NewReader(secret.Data[corev1.DockerConfigJsonKey])) if err != nil { return nil, fmt.Errorf("unable to load Docker config from Secret '%s': %w", secret.Name, err) } - parsedURL, err := url.Parse(registryURL) - if err != nil { - return nil, fmt.Errorf("unable to parse registry URL '%s' while reconciling Secret '%s': %w", - registryURL, secret.Name, err) - } authConfig, err := dockerCfg.GetAuthConfig(parsedURL.Host) if err != nil { return nil, fmt.Errorf("unable to get authentication data from Secret '%s': %w", secret.Name, err) @@ -63,19 +78,38 @@ func LoginOptionFromSecret(registryURL string, secret corev1.Secret) (registry.L } switch { case username == "" && password == "": - return nil, nil + return oci.Anonymous{}, nil case username == "" || password == "": return nil, fmt.Errorf("invalid '%s' secret data: required fields 'username' and 'password'", secret.Name) } - return registry.LoginOptBasicAuth(username, password), nil + return authn.NewKeychainFromHelper(helper{registry: parsedURL.Host, username: username, password: password}), nil } -// OIDCAdaptHelper returns an ORAS credentials callback configured with the authorization data -// from the given authn authenticator. This allows for example to make use of credential helpers from +// KeyChainAdaptHelper returns an ORAS credentials callback configured with the authorization data +// from the given authn keychain. This allows for example to make use of credential helpers from // cloud providers. // Ref: https://github.com/google/go-containerregistry/tree/main/pkg/authn -func OIDCAdaptHelper(authenticator authn.Authenticator) (registry.LoginOption, error) { - authConfig, err := authenticator.Authorization() +func KeychainAdaptHelper(keyChain authn.Keychain) func(string) (registry.LoginOption, error) { + return func(registryURL string) (registry.LoginOption, error) { + parsedURL, err := url.Parse(registryURL) + if err != nil { + return nil, fmt.Errorf("unable to parse registry URL '%s'", registryURL) + } + authenticator, err := keyChain.Resolve(resource{parsedURL.Host}) + if err != nil { + return nil, fmt.Errorf("unable to resolve credentials for registry '%s': %w", registryURL, err) + } + + return AuthAdaptHelper(authenticator) + } +} + +// AuthAdaptHelper returns an ORAS credentials callback configured with the authorization data +// from the given authn authenticator This allows for example to make use of credential helpers from +// cloud providers. +// Ref: https://github.com/google/go-containerregistry/tree/main/pkg/authn +func AuthAdaptHelper(auth authn.Authenticator) (registry.LoginOption, error) { + authConfig, err := auth.Authorization() if err != nil { return nil, fmt.Errorf("unable to get authentication data from OIDC: %w", err) } @@ -91,3 +125,15 @@ func OIDCAdaptHelper(authenticator authn.Authenticator) (registry.LoginOption, e } return registry.LoginOptBasicAuth(username, password), nil } + +type resource struct { + registry string +} + +func (r resource) String() string { + return r.registry +} + +func (r resource) RegistryStr() string { + return r.registry +} diff --git a/internal/helm/registry/auth_test.go b/internal/helm/registry/auth_test.go index 58dbd04b..14942a5b 100644 --- a/internal/helm/registry/auth_test.go +++ b/internal/helm/registry/auth_test.go @@ -17,6 +17,7 @@ limitations under the License. package registry import ( + "net/url" "testing" "github.com/google/go-containerregistry/pkg/authn" @@ -24,6 +25,8 @@ import ( corev1 "k8s.io/api/core/v1" ) +const repoURL = "https://example.com" + func TestLoginOptionFromSecret(t *testing.T) { testURL := "oci://registry.example.com/foo/bar" testUser := "flux" @@ -131,33 +134,40 @@ func TestLoginOptionFromSecret(t *testing.T) { } } -func TestOIDCAdaptHelper(t *testing.T) { - auth := &authn.Basic{ - Username: "flux", - Password: "flux_password", +func TestKeychainAdaptHelper(t *testing.T) { + g := NewWithT(t) + reg, err := url.Parse(repoURL) + if err != nil { + g.Expect(err).ToNot(HaveOccurred()) + } + + auth := helper{ + username: "flux", + password: "flux_password", + registry: reg.Host, } tests := []struct { name string - auth authn.Authenticator + auth authn.Keychain expectedLogin bool wantErr bool }{ { name: "Login from basic auth with empty auth", - auth: &authn.Basic{}, + auth: authn.NewKeychainFromHelper(helper{}), expectedLogin: false, wantErr: false, }, { name: "Login from basic auth", - auth: auth, + auth: authn.NewKeychainFromHelper(auth), expectedLogin: true, wantErr: false, }, { name: "Login with missing password", - auth: &authn.Basic{Username: "flux"}, + auth: authn.NewKeychainFromHelper(helper{username: "flux", registry: reg.Host}), expectedLogin: false, wantErr: true, }, @@ -166,7 +176,7 @@ func TestOIDCAdaptHelper(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - loginOpt, err := OIDCAdaptHelper(tt.auth) + loginOpt, err := KeychainAdaptHelper(tt.auth)(repoURL) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index 417a5281..a037e6b4 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -215,6 +215,7 @@ func (r *OCIChartRepository) DownloadChart(chart *repo.ChartVersion) (*bytes.Buf // Login attempts to login to the OCI registry. // It returns an error on failure. func (r *OCIChartRepository) Login(opts ...registry.LoginOption) error { + // Get login credentials from keychain err := r.RegistryClient.Login(r.URL.Host, opts...) if err != nil { return err From 55dd799dadc895a1bbfe5093585eb2c1773fbd6c Mon Sep 17 00:00:00 2001 From: Soule BA Date: Mon, 3 Oct 2022 16:07:50 +0200 Subject: [PATCH 2/6] Remove test case on aws This remove test case for contextual login on oci://123456789000.dkr.ecr.us-east-2.amazonaws.com. This is not longer a wrong url since https://github.com/fluxcd/pkg/commit/f7c66eb06aa7810e03f106131306574d192314c3 and we no longer error on nil auth. Signed-off-by: Soule BA --- controllers/helmrepository_controller_oci_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/controllers/helmrepository_controller_oci_test.go b/controllers/helmrepository_controller_oci_test.go index 6a0a6009..de0d51af 100644 --- a/controllers/helmrepository_controller_oci_test.go +++ b/controllers/helmrepository_controller_oci_test.go @@ -220,15 +220,6 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get credential from"), }, }, - { - name: "with contextual login provider and invalid repository URL", - wantErr: true, - provider: "aws", - providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com", - assertConditions: []metav1.Condition{ - *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to get credential from"), - }, - }, { name: "with contextual login provider and secretRef", want: ctrl.Result{RequeueAfter: interval}, From 0e97547eebc1fa7b90bcfdb5a97d41f0766e5c39 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Mon, 3 Oct 2022 17:07:00 +0200 Subject: [PATCH 3/6] implement Cosign verification for HelmCharts If implemented, users will be able to enable chart verification for OCI based helm charts. Signed-off-by: Soule BA --- api/v1beta2/helmchart_types.go | 8 + api/v1beta2/zz_generated.deepcopy.go | 5 + .../source.toolkit.fluxcd.io_helmcharts.yaml | 27 +++ .../testdata/helmchart-from-oci/source.yaml | 14 ++ controllers/helmchart_controller.go | 106 +++++++- controllers/helmchart_controller_test.go | 227 ++++++++++++++++++ controllers/ocirepository_controller.go | 4 +- docs/api/source.md | 37 +++ hack/ci/e2e.sh | 1 + internal/helm/chart/builder.go | 2 + internal/helm/chart/builder_remote.go | 15 +- internal/helm/chart/errors.go | 1 + internal/helm/repository/chart_repository.go | 10 + .../helm/repository/oci_chart_repository.go | 45 +++- internal/helm/repository/repository.go | 3 + internal/oci/verifier.go | 35 ++- 16 files changed, 522 insertions(+), 18 deletions(-) diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index 5b12f1f5..aca993fd 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -86,6 +86,14 @@ type HelmChartSpec struct { // NOTE: Not implemented, provisional as of https://github.com/fluxcd/flux2/pull/2092 // +optional AccessFrom *acl.AccessFrom `json:"accessFrom,omitempty"` + + // Verify contains the secret name containing the trusted public keys + // used to verify the signature and specifies which provider to use to check + // whether OCI image is authentic. + // This field is only supported for OCI sources. + // Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified. + // +optional + Verify *OCIRepositoryVerification `json:"verify,omitempty"` } const ( diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 106a042c..c196f4e5 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -464,6 +464,11 @@ func (in *HelmChartSpec) DeepCopyInto(out *HelmChartSpec) { *out = new(acl.AccessFrom) (*in).DeepCopyInto(*out) } + if in.Verify != nil { + in, out := &in.Verify, &out.Verify + *out = new(OCIRepositoryVerification) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HelmChartSpec. diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index 7ef36829..c6cdeefe 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml @@ -403,6 +403,33 @@ spec: items: type: string type: array + verify: + description: Verify contains the secret name containing the trusted + public keys used to verify the signature and specifies which provider + to use to check whether OCI image is authentic. This field is only + supported for OCI sources. Chart dependencies, which are not bundled + in the umbrella chart artifact, are not verified. + properties: + provider: + default: cosign + description: Provider specifies the technology used to sign the + OCI Artifact. + enum: + - cosign + type: string + secretRef: + description: SecretRef specifies the Kubernetes Secret containing + the trusted public keys. + properties: + name: + description: Name of the referent. + type: string + required: + - name + type: object + required: + - provider + type: object version: default: '*' description: Version is the chart version semver expression, ignored diff --git a/config/testdata/helmchart-from-oci/source.yaml b/config/testdata/helmchart-from-oci/source.yaml index 9d9945ff..354325ef 100644 --- a/config/testdata/helmchart-from-oci/source.yaml +++ b/config/testdata/helmchart-from-oci/source.yaml @@ -19,3 +19,17 @@ spec: name: podinfo version: '6.1.*' interval: 1m +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: HelmChart +metadata: + name: podinfo-keyless +spec: + chart: podinfo + sourceRef: + kind: HelmRepository + name: podinfo + version: '6.2.1' + interval: 1m + verify: + provider: cosign diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 73662da0..773bce46 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -28,6 +28,7 @@ import ( "strings" "time" + soci "github.com/fluxcd/source-controller/internal/oci" helmgetter "helm.sh/helm/v3/pkg/getter" helmreg "helm.sh/helm/v3/pkg/registry" corev1 "k8s.io/api/core/v1" @@ -57,6 +58,7 @@ import ( "github.com/fluxcd/pkg/runtime/predicates" "github.com/fluxcd/pkg/untar" "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/v1/remote" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" @@ -80,6 +82,7 @@ var helmChartReadyCondition = summarize.Conditions{ sourcev1.BuildFailedCondition, sourcev1.ArtifactOutdatedCondition, sourcev1.ArtifactInStorageCondition, + sourcev1.SourceVerifiedCondition, meta.ReadyCondition, meta.ReconcilingCondition, meta.StalledCondition, @@ -90,6 +93,7 @@ var helmChartReadyCondition = summarize.Conditions{ sourcev1.BuildFailedCondition, sourcev1.ArtifactOutdatedCondition, sourcev1.ArtifactInStorageCondition, + sourcev1.SourceVerifiedCondition, meta.StalledCondition, meta.ReconcilingCondition, }, @@ -564,9 +568,30 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * }() } + var verifiers []soci.Verifier + if obj.Spec.Verify != nil { + provider := obj.Spec.Verify.Provider + verifiers, err = r.makeVerifiers(ctx, obj, authenticator, keychain) + if err != nil { + if obj.Spec.Verify.SecretRef == nil { + provider = fmt.Sprintf("%s keyless", provider) + } + e := serror.NewGeneric( + fmt.Errorf("failed to verify the signature using provider '%s': %w", provider, err), + sourcev1.VerificationError, + ) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + } + // Tell the chart repository to use the OCI client with the configured getter clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient)) - ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient)) + ociChartRepo, err := repository.NewOCIChartRepository(normalizedURL, + repository.WithOCIGetter(r.Getters), + repository.WithOCIGetterOptions(clientOpts), + repository.WithOCIRegistryClient(registryClient), + repository.WithVerifiers(verifiers)) if err != nil { return chartRepoConfigErrorReturn(err, obj) } @@ -574,7 +599,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if keychain != nil { + if loginOpt != nil { err = ociChartRepo.Login(loginOpt) if err != nil { e := &serror.Event{ @@ -622,6 +647,17 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * opts := chart.BuildOptions{ ValuesFiles: obj.GetValuesFiles(), Force: obj.Generation != obj.Status.ObservedGeneration, + // The remote builder will not attempt to download the chart if + // an artifact exist with the same name and version and the force is false. + // It will try to verify the chart if: + // - we are on the first reconciliation + // - the HelmChart spec has changed (generation drift) + // - the previous reconciliation resulted in a failed artifact verification + // - there is no artifact in storage + Verify: obj.Spec.Verify != nil && (obj.Generation <= 0 || + conditions.GetObservedGeneration(obj, sourcev1.SourceVerifiedCondition) != obj.Generation || + conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) || + obj.GetArtifact() == nil), } if artifact := obj.GetArtifact(); artifact != nil { opts.CachedChart = r.Storage.LocalPath(*artifact) @@ -1030,7 +1066,7 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if keychain != nil { + if loginOpt != nil { err = ociChartRepo.Login(loginOpt) if err != nil { errs = append(errs, fmt.Errorf("failed to login to OCI chart repository for HelmRepository '%s': %w", repo.Name, err)) @@ -1239,6 +1275,11 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { if build.Complete() { conditions.Delete(obj, sourcev1.FetchFailedCondition) conditions.Delete(obj, sourcev1.BuildFailedCondition) + conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, meta.SucceededReason, fmt.Sprintf("verified signature of version %s", build.Version)) + } + + if obj.Spec.Verify == nil { + conditions.Delete(obj, sourcev1.SourceVerifiedCondition) } if err != nil { @@ -1251,7 +1292,7 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { } switch buildErr.Reason { - case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage: + case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage, chart.ErrChartVerification: conditions.Delete(obj, sourcev1.FetchFailedCondition) conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, buildErr.Reason.Reason, buildErr.Error()) default: @@ -1290,3 +1331,60 @@ func chartRepoConfigErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile. return sreconcile.ResultEmpty, e } } + +// makeVerifiers returns a list of verifiers for the given chart. +func (r *HelmChartReconciler) makeVerifiers(ctx context.Context, obj *sourcev1.HelmChart, auth authn.Authenticator, keychain authn.Keychain) ([]soci.Verifier, error) { + var verifiers []soci.Verifier + verifyOpts := []remote.Option{} + if auth != nil { + verifyOpts = append(verifyOpts, remote.WithAuth(auth)) + } else { + verifyOpts = append(verifyOpts, remote.WithAuthFromKeychain(keychain)) + } + + switch obj.Spec.Verify.Provider { + case "cosign": + defaultCosignOciOpts := []soci.Options{ + soci.WithRemoteOptions(verifyOpts...), + } + + // get the public keys from the given secret + if secretRef := obj.Spec.Verify.SecretRef; secretRef != nil { + certSecretName := types.NamespacedName{ + Namespace: obj.Namespace, + Name: secretRef.Name, + } + + var pubSecret corev1.Secret + if err := r.Get(ctx, certSecretName, &pubSecret); err != nil { + return nil, err + } + + for k, data := range pubSecret.Data { + // search for public keys in the secret + if strings.HasSuffix(k, ".pub") { + verifier, err := soci.NewCosignVerifier(ctx, append(defaultCosignOciOpts, soci.WithPublicKey(data))...) + if err != nil { + return nil, err + } + verifiers = append(verifiers, verifier) + } + } + + if len(verifiers) == 0 { + return nil, fmt.Errorf("no public keys found in secret '%s'", certSecretName) + } + return verifiers, nil + } + + // if no secret is provided, add a keyless verifier + verifier, err := soci.NewCosignVerifier(ctx, defaultCosignOciOpts...) + if err != nil { + return nil, err + } + verifiers = append(verifiers, verifier) + return verifiers, nil + default: + return nil, fmt.Errorf("unsupported verification provider: %s", obj.Spec.Verify.Provider) + } +} diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index 43ddd883..f9aaf2c8 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -26,6 +26,7 @@ import ( "io/ioutil" "net/http" "os" + "path" "path/filepath" "reflect" "strings" @@ -33,6 +34,9 @@ import ( "time" . "github.com/onsi/gomega" + coptions "github.com/sigstore/cosign/cmd/cosign/cli/options" + "github.com/sigstore/cosign/cmd/cosign/cli/sign" + "github.com/sigstore/cosign/pkg/cosign" hchart "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" helmreg "helm.sh/helm/v3/pkg/registry" @@ -57,6 +61,7 @@ import ( serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/helm/chart" "github.com/fluxcd/source-controller/internal/helm/registry" + "github.com/fluxcd/source-controller/internal/oci" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" ) @@ -2213,6 +2218,228 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { } } +func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + server, err := setupRegistryServer(ctx, tmpDir, registryOptions{}) + g.Expect(err).ToNot(HaveOccurred()) + + const ( + chartPath = "testdata/charts/helmchart-0.1.0.tgz" + ) + + // Load a test chart + chartData, err := ioutil.ReadFile(chartPath) + + // Upload the test chart + metadata, err := loadTestChartToOCI(chartData, chartPath, server) + g.Expect(err).NotTo(HaveOccurred()) + + storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords) + g.Expect(err).ToNot(HaveOccurred()) + + cachedArtifact := &sourcev1.Artifact{ + Revision: "0.1.0", + Path: metadata.Name + "-" + metadata.Version + ".tgz", + } + g.Expect(storage.CopyFromPath(cachedArtifact, "testdata/charts/helmchart-0.1.0.tgz")).To(Succeed()) + + pf := func(b bool) ([]byte, error) { + return []byte("cosign-password"), nil + } + + keys, err := cosign.GenerateKeyPair(pf) + g.Expect(err).ToNot(HaveOccurred()) + + err = os.WriteFile(path.Join(tmpDir, "cosign.key"), keys.PrivateBytes, 0600) + g.Expect(err).ToNot(HaveOccurred()) + + defer func() { + err := os.Remove(path.Join(tmpDir, "cosign.key")) + g.Expect(err).ToNot(HaveOccurred()) + }() + + tests := []struct { + name string + want sreconcile.Result + wantErr bool + wantErrMsg string + shouldSign bool + beforeFunc func(obj *sourcev1.HelmChart) + assertConditions []metav1.Condition + cleanFunc func(g *WithT, build *chart.Build) + }{ + { + name: "unsigned charts should not pass verification", + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = metadata.Name + obj.Spec.Version = metadata.Version + obj.Spec.Verify = &sourcev1.OCIRepositoryVerification{ + Provider: "cosign", + SecretRef: &meta.LocalObjectReference{Name: "cosign-key"}, + } + }, + wantErr: true, + wantErrMsg: "chart verification error: failed to verify : no matching signatures:", + want: sreconcile.ResultEmpty, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), + }, + }, + { + name: "unsigned charts should not pass keyless verification", + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = metadata.Name + obj.Spec.Version = metadata.Version + obj.Spec.Verify = &sourcev1.OCIRepositoryVerification{ + Provider: "cosign", + } + }, + wantErr: true, + want: sreconcile.ResultEmpty, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), + }, + }, + { + name: "signed charts should pass verification", + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = metadata.Name + obj.Spec.Version = metadata.Version + obj.Spec.Verify = &sourcev1.OCIRepositoryVerification{ + Provider: "cosign", + SecretRef: &meta.LocalObjectReference{Name: "cosign-key"}, + } + }, + shouldSign: true, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of version "), + }, + cleanFunc: func(g *WithT, build *chart.Build) { + g.Expect(os.Remove(build.Path)).To(Succeed()) + }, + }, + { + name: "verify failed before, removed from spec, remove condition", + beforeFunc: func(obj *sourcev1.HelmChart) { + obj.Spec.Chart = metadata.Name + obj.Spec.Version = metadata.Version + obj.Spec.Verify = nil + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, "VerifyFailed", "fail msg") + obj.Status.Artifact = &sourcev1.Artifact{Path: metadata.Name + "-" + metadata.Version + ".tgz"} + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), + }, + cleanFunc: func(g *WithT, build *chart.Build) { + g.Expect(os.Remove(build.Path)).To(Succeed()) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + clientBuilder := fake.NewClientBuilder() + + repository := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "helmrepository-", + }, + Spec: sourcev1.HelmRepositorySpec{ + URL: fmt.Sprintf("oci://%s/testrepo", server.registryHost), + Timeout: &metav1.Duration{Duration: timeout}, + Provider: sourcev1.GenericOCIProvider, + Type: sourcev1.HelmRepositoryTypeOCI, + }, + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cosign-key", + }, + Data: map[string][]byte{ + "cosign.pub": keys.PublicBytes, + }} + + clientBuilder.WithObjects(repository, secret) + + r := &HelmChartReconciler{ + Client: clientBuilder.Build(), + EventRecorder: record.NewFakeRecorder(32), + Getters: testGetters, + Storage: storage, + RegistryClientGenerator: registry.ClientGenerator, + } + + obj := &sourcev1.HelmChart{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "helmchart-", + }, + Spec: sourcev1.HelmChartSpec{ + SourceRef: sourcev1.LocalHelmChartSourceReference{ + Kind: sourcev1.HelmRepositoryKind, + Name: repository.Name, + }, + }, + } + + chartUrl := fmt.Sprintf("oci://%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version) + + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + + if tt.shouldSign { + ko := coptions.KeyOpts{ + KeyRef: path.Join(tmpDir, "cosign.key"), + PassFunc: pf, + } + + ro := &coptions.RootOptions{ + Timeout: timeout, + } + + err = sign.SignCmd(ro, ko, coptions.RegistryOptions{Keychain: oci.Anonymous{}}, + nil, []string{fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)}, "", + "", true, "", + "", "", false, + false, "", false) + g.Expect(err).ToNot(HaveOccurred()) + } + + assertConditions := tt.assertConditions + for k := range assertConditions { + assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", metadata.Name) + assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", metadata.Version) + assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", chartUrl) + assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "", "cosign") + } + + var b chart.Build + if tt.cleanFunc != nil { + defer tt.cleanFunc(g, &b) + } + + got, err := r.reconcileSource(ctx, obj, &b) + if tt.wantErr { + tt.wantErrMsg = strings.ReplaceAll(tt.wantErrMsg, "", chartUrl) + g.Expect(err).ToNot(BeNil()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErrMsg)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(got).To(Equal(tt.want)) + g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) + }) + } +} + // extractChartMeta is used to extract a chart metadata from a byte array func extractChartMeta(chartData []byte) (*hchart.Metadata, error) { ch, err := loader.LoadArchive(bytes.NewReader(chartData)) diff --git a/controllers/ocirepository_controller.go b/controllers/ocirepository_controller.go index 35aec494..1f3dcffb 100644 --- a/controllers/ocirepository_controller.go +++ b/controllers/ocirepository_controller.go @@ -628,7 +628,7 @@ func (r *OCIRepositoryReconciler) verifySignature(ctx context.Context, obj *sour for k, data := range pubSecret.Data { // search for public keys in the secret if strings.HasSuffix(k, ".pub") { - verifier, err := soci.NewVerifier(ctxTimeout, append(defaultCosignOciOpts, soci.WithPublicKey(data))...) + verifier, err := soci.NewCosignVerifier(ctxTimeout, append(defaultCosignOciOpts, soci.WithPublicKey(data))...) if err != nil { return err } @@ -654,7 +654,7 @@ func (r *OCIRepositoryReconciler) verifySignature(ctx context.Context, obj *sour // if no secret is provided, try keyless verification ctrl.LoggerFrom(ctx).Info("no secret reference is provided, trying to verify the image using keyless method") - verifier, err := soci.NewVerifier(ctxTimeout, defaultCosignOciOpts...) + verifier, err := soci.NewCosignVerifier(ctxTimeout, defaultCosignOciOpts...) if err != nil { return err } diff --git a/docs/api/source.md b/docs/api/source.md index d5762fc3..060197be 100644 --- a/docs/api/source.md +++ b/docs/api/source.md @@ -670,6 +670,24 @@ references to this object. NOTE: Not implemented, provisional as of https://github.com/fluxcd/flux2/pull/2092

+ + +verify
+ + +OCIRepositoryVerification + + + + +(Optional) +

Verify contains the secret name containing the trusted public keys +used to verify the signature and specifies which provider to use to check +whether OCI image is authentic. +This field is only supported for OCI sources. +Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.

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

+ + +verify
+ + +OCIRepositoryVerification + + + + +(Optional) +

Verify contains the secret name containing the trusted public keys +used to verify the signature and specifies which provider to use to check +whether OCI image is authentic. +This field is only supported for OCI sources. +Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.

+ + @@ -3123,6 +3159,7 @@ github.com/fluxcd/pkg/apis/meta.ReconcileRequestStatus

(Appears on: +HelmChartSpec, OCIRepositorySpec)

OCIRepositoryVerification verifies the authenticity of an OCI Artifact

diff --git a/hack/ci/e2e.sh b/hack/ci/e2e.sh index 314eb5b1..3e578de2 100755 --- a/hack/ci/e2e.sh +++ b/hack/ci/e2e.sh @@ -165,6 +165,7 @@ echo "Run HelmChart from OCI registry tests" kubectl -n source-system apply -f "${ROOT_DIR}/config/testdata/helmchart-from-oci/source.yaml" kubectl -n source-system wait helmrepository/podinfo --for=condition=ready --timeout=1m kubectl -n source-system wait helmchart/podinfo --for=condition=ready --timeout=1m +kubectl -n source-system wait helmchart/podinfo-keyless --for=condition=ready --timeout=1m echo "Run OCIRepository verify tests" kubectl -n source-system apply -f "${ROOT_DIR}/config/testdata/ocirepository/signed-with-key.yaml" diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index 76dc517c..5be208d8 100644 --- a/internal/helm/chart/builder.go +++ b/internal/helm/chart/builder.go @@ -113,6 +113,8 @@ 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 + // Verifier can be set to the verification of the chart. + Verify bool } // GetValuesFiles returns BuildOptions.ValuesFiles, except if it equals diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index d15e2429..20589472 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -63,7 +63,7 @@ func NewRemoteBuilder(repository repository.Downloader) Builder { // After downloading the chart, it is only packaged if required due to BuildOptions // modifying the chart, otherwise the exact data as retrieved from the repository // is written to p, after validating it to be a chart. -func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, opts BuildOptions) (*Build, error) { +func (b *remoteChartBuilder) Build(ctx context.Context, ref Reference, p string, opts BuildOptions) (*Build, error) { remoteRef, ok := ref.(RemoteReference) if !ok { err := fmt.Errorf("expected remote chart reference") @@ -74,9 +74,9 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return nil, &BuildError{Reason: ErrChartReference, Err: err} } - res, result, err := b.downloadFromRepository(b.remote, remoteRef, opts) + res, result, err := b.downloadFromRepository(ctx, b.remote, remoteRef, opts) if err != nil { - return nil, &BuildError{Reason: ErrChartPull, Err: err} + return nil, err } if res == nil { return result, nil @@ -124,7 +124,7 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o return result, nil } -func (b *remoteChartBuilder) downloadFromRepository(remote repository.Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) { +func (b *remoteChartBuilder) downloadFromRepository(ctx context.Context, remote repository.Downloader, remoteRef RemoteReference, opts BuildOptions) (*bytes.Buffer, *Build, error) { // Get the current version for the RemoteReference cv, err := remote.GetChartVersion(remoteRef.Name, remoteRef.Version) if err != nil { @@ -132,6 +132,13 @@ func (b *remoteChartBuilder) downloadFromRepository(remote repository.Downloader return nil, nil, &BuildError{Reason: ErrChartReference, Err: err} } + // Verify the chart if necessary + if opts.Verify { + if err := remote.VerifyChart(ctx, cv); err != nil { + return nil, nil, &BuildError{Reason: ErrChartVerification, Err: err} + } + } + result, shouldReturn, err := generateBuildResult(cv, opts) if err != nil { return nil, nil, err diff --git a/internal/helm/chart/errors.go b/internal/helm/chart/errors.go index dedff9e3..7b1b7f3b 100644 --- a/internal/helm/chart/errors.go +++ b/internal/helm/chart/errors.go @@ -84,5 +84,6 @@ var ( ErrValuesFilesMerge = BuildErrorReason{Reason: "ValuesFilesError", Summary: "values files merge error"} ErrDependencyBuild = BuildErrorReason{Reason: "DependencyBuildError", Summary: "dependency build error"} ErrChartPackage = BuildErrorReason{Reason: "ChartPackageError", Summary: "chart package error"} + ErrChartVerification = BuildErrorReason{Reason: "ChartVerificationError", Summary: "chart verification error"} ErrUnknown = BuildErrorReason{Reason: "Unknown", Summary: "unknown build error"} ) diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 282d49a5..3997d5f3 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -18,6 +18,7 @@ package repository import ( "bytes" + "context" "crypto/sha256" "crypto/tls" "encoding/hex" @@ -520,3 +521,12 @@ func (r *ChartRepository) RemoveCache() error { } return nil } + +// VerifyChart verifies the chart against a signature. +// If no signature is provided, a keyless verification is performed. +// It returns an error on failure. +func (r *ChartRepository) VerifyChart(_ context.Context, _ *repo.ChartVersion) error { + // no-op + // this is a no-op because this is not implemented yet. + return nil +} diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index a037e6b4..fe03a0e6 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -18,6 +18,7 @@ package repository import ( "bytes" + "context" "crypto/tls" "fmt" "net/url" @@ -32,7 +33,10 @@ import ( "helm.sh/helm/v3/pkg/repo" "github.com/Masterminds/semver/v3" + "github.com/google/go-containerregistry/pkg/name" + "github.com/fluxcd/pkg/version" + "github.com/fluxcd/source-controller/internal/oci" "github.com/fluxcd/source-controller/internal/transport" ) @@ -63,12 +67,23 @@ type OCIChartRepository struct { RegistryClient RegistryClient // credentialsFile is a temporary credentials file to use while downloading tags or charts from a registry. credentialsFile string + + // verifiers is a list of verifiers to use when verifying a chart. + verifiers []oci.Verifier } // OCIChartRepositoryOption is a function that can be passed to NewOCIChartRepository // to configure an OCIChartRepository. type OCIChartRepositoryOption func(*OCIChartRepository) error +// WithVerifiers returns a ChartRepositoryOption that will set the chart verifiers +func WithVerifiers(verifiers []oci.Verifier) OCIChartRepositoryOption { + return func(r *OCIChartRepository) error { + r.verifiers = verifiers + return nil + } +} + // WithOCIRegistryClient returns a ChartRepositoryOption that will set the registry client func WithOCIRegistryClient(client RegistryClient) OCIChartRepositoryOption { return func(r *OCIChartRepository) error { @@ -215,7 +230,6 @@ func (r *OCIChartRepository) DownloadChart(chart *repo.ChartVersion) (*bytes.Buf // Login attempts to login to the OCI registry. // It returns an error on failure. func (r *OCIChartRepository) Login(opts ...registry.LoginOption) error { - // Get login credentials from keychain err := r.RegistryClient.Login(r.URL.Host, opts...) if err != nil { return err @@ -297,3 +311,32 @@ func getLastMatchingVersionOrConstraint(cvs []string, ver string) (string, error return matchingVersions[0].Original(), nil } + +// VerifyChart verifies the chart against a signature. +// If no signature is provided, a keyless verification is performed. +// It returns an error on failure. +func (r *OCIChartRepository) VerifyChart(ctx context.Context, chart *repo.ChartVersion) error { + if len(r.verifiers) == 0 { + return fmt.Errorf("no verifiers available") + } + + if len(chart.URLs) == 0 { + return fmt.Errorf("chart '%s' has no downloadable URLs", chart.Name) + } + + ref, err := name.ParseReference(strings.TrimPrefix(chart.URLs[0], fmt.Sprintf("%s://", registry.OCIScheme))) + if err != nil { + return fmt.Errorf("invalid chart reference: %s", err) + } + + // verify the chart + for _, verifier := range r.verifiers { + if verified, err := verifier.Verify(ctx, ref); err != nil { + return fmt.Errorf("failed to verify %s: %w", chart.URLs[0], err) + } else if verified { + return nil + } + } + + return fmt.Errorf("no matching signatures were found for '%s'", ref.Name()) +} diff --git a/internal/helm/repository/repository.go b/internal/helm/repository/repository.go index 4c8cb7ff..5fdf62bf 100644 --- a/internal/helm/repository/repository.go +++ b/internal/helm/repository/repository.go @@ -18,6 +18,7 @@ package repository import ( "bytes" + "context" "helm.sh/helm/v3/pkg/repo" ) @@ -29,6 +30,8 @@ type Downloader interface { GetChartVersion(name, version string) (*repo.ChartVersion, error) // DownloadChart downloads a chart from the remote Helm repository or OCI Helm repository. DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error) + // VerifyChart verifies the chart against a signature. + VerifyChart(ctx context.Context, chart *repo.ChartVersion) error // Clear removes all temporary files created by the downloader, caching the files if the cache is configured, // and calling garbage collector to remove unused files. Clear() error diff --git a/internal/oci/verifier.go b/internal/oci/verifier.go index b8d9c5d4..490b3ef4 100644 --- a/internal/oci/verifier.go +++ b/internal/oci/verifier.go @@ -34,13 +34,18 @@ import ( "github.com/sigstore/sigstore/pkg/signature" ) +// Verifier is an interface for verifying the authenticity of an OCI image. +type Verifier interface { + Verify(ctx context.Context, ref name.Reference) (bool, error) +} + // options is a struct that holds options for verifier. type options struct { PublicKey []byte ROpt []remote.Option } -// Options is a function that configures the options applied to a Verifier. +// Options is a function that configures the options applied to a CosignVerifier. type Options func(opts *options) // WithPublicKey sets the public key. @@ -58,13 +63,13 @@ func WithRemoteOptions(opts ...remote.Option) Options { } } -// Verifier is a struct which is responsible for executing verification logic. -type Verifier struct { +// CosignVerifier is a struct which is responsible for executing verification logic. +type CosignVerifier struct { opts *cosign.CheckOpts } -// NewVerifier initializes a new Verifier. -func NewVerifier(ctx context.Context, opts ...Options) (*Verifier, error) { +// NewCosignVerifier initializes a new CosignVerifier. +func NewCosignVerifier(ctx context.Context, opts ...Options) (*CosignVerifier, error) { o := options{} for _, opt := range opts { opt(&o) @@ -117,12 +122,28 @@ func NewVerifier(ctx context.Context, opts ...Options) (*Verifier, error) { checkOpts.RekorClient = rc } - return &Verifier{ + return &CosignVerifier{ opts: checkOpts, }, nil } // VerifyImageSignatures verify the authenticity of the given ref OCI image. -func (v *Verifier) VerifyImageSignatures(ctx context.Context, ref name.Reference) ([]oci.Signature, bool, error) { +func (v *CosignVerifier) VerifyImageSignatures(ctx context.Context, ref name.Reference) ([]oci.Signature, bool, error) { return cosign.VerifyImageSignatures(ctx, ref, v.opts) } + +// Verify verifies the authenticity of the given ref OCI image. +// It returns a boolean indicating if the verification was successful. +// It returns an error if the verification fails, nil otherwise. +func (v *CosignVerifier) Verify(ctx context.Context, ref name.Reference) (bool, error) { + signatures, _, err := v.VerifyImageSignatures(ctx, ref) + if err != nil { + return false, err + } + + if len(signatures) == 0 { + return false, nil + } + + return true, nil +} From 5355fb3142664ff8efc9afba20b5f97287b46d5d Mon Sep 17 00:00:00 2001 From: Soule BA Date: Mon, 17 Oct 2022 23:19:38 +0200 Subject: [PATCH 4/6] adding verfication section to HelmChart api doc Signed-off-by: Soule BA --- docs/spec/v1beta2/helmcharts.md | 96 ++++++++++++++++++++++++++++ docs/spec/v1beta2/ocirepositories.md | 3 + 2 files changed, 99 insertions(+) diff --git a/docs/spec/v1beta2/helmcharts.md b/docs/spec/v1beta2/helmcharts.md index b423dde6..d6e18987 100644 --- a/docs/spec/v1beta2/helmcharts.md +++ b/docs/spec/v1beta2/helmcharts.md @@ -240,6 +240,102 @@ in a new Artifact. When the field is set to `false` or removed, it will resume. For practical information, see [suspending and resuming](#suspending-and-resuming). +### Verification + +**Note:** This feature is available only for Helm charts fetched from an OCI Registry. + +`.spec.verify` is an optional field to enable the verification of [Cosign](https://github.com/sigstore/cosign) +signatures. The field offers two subfields: + +- `.provider`, to specify the verification provider. Only supports `cosign` at present. +- `.secretRef.name`, to specify a reference to a Secret in the same namespace as + the HelmChart, containing the Cosign public keys of trusted authors. + +```yaml +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: HelmChart +metadata: + name: podinfo +spec: + verify: + provider: cosign + secretRef: + name: cosign-public-keys +``` + +When the verification succeeds, the controller adds a Condition with the +following attributes to the HelmChart's `.status.conditions`: + +- `type: SourceVerified` +- `status: "True"` +- `reason: Succeeded` + +#### Public keys verification + +To verify the authenticity of HelmChart hosted in an OCI Registry, create a Kubernetes +secret with the Cosign public keys: + +```yaml +--- +apiVersion: v1 +kind: Secret +metadata: + name: cosign-public-keys +type: Opaque +data: + key1.pub: + key2.pub: +``` + +Note that the keys must have the `.pub` extension for Flux to make use of them. + +Flux will loop over the public keys and use them verify a HelmChart's signature. +This allows for older HelmCharts to be valid as long as the right key is in the secret. + +#### Keyless verification + +For publicly available HelmCharts, which are signed using the +[Cosign Keyless](https://github.com/sigstore/cosign/blob/main/KEYLESS.md) procedure, +you can enable the verification by omitting the `.verify.secretRef` field. + +Example of verifying HelmCharts signed by the +[Cosign GitHub Action](https://github.com/sigstore/cosign-installer) with GitHub OIDC Token: + +```yaml +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: HelmChart +metadata: + name: podinfo +spec: + interval: 5m + reconcileStrategy: ChartVersion + sourceRef: + kind: HelmRepository + name: podinfo + version: ">=6.1.6" + verify: + provider: cosign +``` + +```yaml +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: HelmRepository +metadata: + name: podinfo +spec: + interval: 1m0s + url: oci://ghcr.io/stefanprodan/charts + type: "oci" +``` + +The controller verifies the signatures using the Fulcio root CA and the Rekor +instance hosted at [rekor.sigstore.dev](https://rekor.sigstore.dev/). + +Note that keyless verification is an **experimental feature**, using +custom root CAs or self-hosted Rekor instances are not currently supported. + ## Working with HelmCharts ### Triggering a reconcile diff --git a/docs/spec/v1beta2/ocirepositories.md b/docs/spec/v1beta2/ocirepositories.md index 0320e8e5..a6b22aa9 100644 --- a/docs/spec/v1beta2/ocirepositories.md +++ b/docs/spec/v1beta2/ocirepositories.md @@ -462,6 +462,9 @@ data: Note that the keys must have the `.pub` extension for Flux to make use of them. +Flux will loop over the public keys and use them verify an artifact's signature. +This allows for older artifacts to be valid as long as the right key is in the secret. + #### Keyless verification For publicly available OCI artifacts, which are signed using the From 25673ac5125d62c480098eb778e31fefd83ae74d Mon Sep 17 00:00:00 2001 From: Soule BA Date: Wed, 19 Oct 2022 01:30:47 +0200 Subject: [PATCH 5/6] addressing review comments Signed-off-by: Soule BA --- controllers/helmchart_controller.go | 3 ++- internal/helm/registry/auth.go | 10 ++++++---- internal/helm/repository/chart_repository.go | 2 +- internal/oci/verifier.go | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 773bce46..d74a7ae1 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -214,6 +214,7 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( summarizeHelper := summarize.NewHelper(r.EventRecorder, patchHelper) summarizeOpts := []summarize.Option{ summarize.WithConditions(helmChartReadyCondition), + summarize.WithBiPolarityConditionTypes(sourcev1.SourceVerifiedCondition), summarize.WithReconcileResult(recResult), summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), @@ -648,7 +649,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * ValuesFiles: obj.GetValuesFiles(), Force: obj.Generation != obj.Status.ObservedGeneration, // The remote builder will not attempt to download the chart if - // an artifact exist with the same name and version and the force is false. + // an artifact exists with the same name and version and `Force` is false. // It will try to verify the chart if: // - we are on the first reconciliation // - the HelmChart spec has changed (generation drift) diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index 4914c568..debe87ea 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -95,7 +95,7 @@ func KeychainAdaptHelper(keyChain authn.Keychain) func(string) (registry.LoginOp if err != nil { return nil, fmt.Errorf("unable to parse registry URL '%s'", registryURL) } - authenticator, err := keyChain.Resolve(resource{parsedURL.Host}) + authenticator, err := keyChain.Resolve(stringResource{parsedURL.Host}) if err != nil { return nil, fmt.Errorf("unable to resolve credentials for registry '%s': %w", registryURL, err) } @@ -126,14 +126,16 @@ func AuthAdaptHelper(auth authn.Authenticator) (registry.LoginOption, error) { return registry.LoginOptBasicAuth(username, password), nil } -type resource struct { +// stringResource is there to satisfy the github.com/google/go-containerregistry/pkg/authn.Resource interface. +// It merely wraps a given string and returns it for all of the interface's methods. +type stringResource struct { registry string } -func (r resource) String() string { +func (r stringResource) String() string { return r.registry } -func (r resource) RegistryStr() string { +func (r stringResource) RegistryStr() string { return r.registry } diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 3997d5f3..15e62432 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -528,5 +528,5 @@ func (r *ChartRepository) RemoveCache() error { func (r *ChartRepository) VerifyChart(_ context.Context, _ *repo.ChartVersion) error { // no-op // this is a no-op because this is not implemented yet. - return nil + return fmt.Errorf("not implemented") } diff --git a/internal/oci/verifier.go b/internal/oci/verifier.go index 490b3ef4..23f8f090 100644 --- a/internal/oci/verifier.go +++ b/internal/oci/verifier.go @@ -45,7 +45,7 @@ type options struct { ROpt []remote.Option } -// Options is a function that configures the options applied to a CosignVerifier. +// Options is a function that configures the options applied to a Verifier. type Options func(opts *options) // WithPublicKey sets the public key. From 06a55590a5a0e0bed2c74cd29fdf2f84da13d038 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Thu, 20 Oct 2022 13:48:50 +0200 Subject: [PATCH 6/6] Fix verification condition Delete a failed verification condition at the beginning of the source reconciliation and set `SourceVerifiedCondition` to false approprietly. Set the `BuildOptions.Verify` to true as long as Verify is enabled in the API fields. Signed-off-by: Soule BA --- api/v1beta2/helmchart_types.go | 2 +- .../source.toolkit.fluxcd.io_helmcharts.yaml | 5 +-- controllers/helmchart_controller.go | 31 ++++++++++--------- controllers/helmchart_controller_test.go | 16 +++++----- docs/api/source.md | 4 +-- docs/spec/v1beta2/helmcharts.md | 3 +- docs/spec/v1beta2/ocirepositories.md | 2 +- internal/helm/registry/auth.go | 2 +- internal/helm/repository/chart_repository.go | 2 -- 9 files changed, 36 insertions(+), 31 deletions(-) diff --git a/api/v1beta2/helmchart_types.go b/api/v1beta2/helmchart_types.go index aca993fd..96321a09 100644 --- a/api/v1beta2/helmchart_types.go +++ b/api/v1beta2/helmchart_types.go @@ -90,7 +90,7 @@ type HelmChartSpec struct { // Verify contains the secret name containing the trusted public keys // used to verify the signature and specifies which provider to use to check // whether OCI image is authentic. - // This field is only supported for OCI sources. + // This field is only supported when using HelmRepository source with spec.type 'oci'. // Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified. // +optional Verify *OCIRepositoryVerification `json:"verify,omitempty"` diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml index c6cdeefe..c1ac4b6e 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmcharts.yaml @@ -407,8 +407,9 @@ spec: description: Verify contains the secret name containing the trusted public keys used to verify the signature and specifies which provider to use to check whether OCI image is authentic. This field is only - supported for OCI sources. Chart dependencies, which are not bundled - in the umbrella chart artifact, are not verified. + supported when using HelmRepository source with spec.type 'oci'. + Chart dependencies, which are not bundled in the umbrella chart + artifact, are not verified. properties: provider: default: cosign diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index d74a7ae1..1300db9f 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -372,6 +372,12 @@ func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev } func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (_ sreconcile.Result, retErr error) { + // Remove any failed verification condition. + // The reason is that a failing verification should be recalculated. + if conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) { + conditions.Delete(obj, sourcev1.SourceVerifiedCondition) + } + // Retrieve the source s, err := r.getSource(ctx, obj) if err != nil { @@ -577,10 +583,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if obj.Spec.Verify.SecretRef == nil { provider = fmt.Sprintf("%s keyless", provider) } - e := serror.NewGeneric( - fmt.Errorf("failed to verify the signature using provider '%s': %w", provider, err), - sourcev1.VerificationError, - ) + e := &serror.Event{ + Err: fmt.Errorf("failed to verify the signature using provider '%s': %w", provider, err), + Reason: sourcev1.VerificationError, + } conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -650,15 +656,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * Force: obj.Generation != obj.Status.ObservedGeneration, // The remote builder will not attempt to download the chart if // an artifact exists with the same name and version and `Force` is false. - // It will try to verify the chart if: - // - we are on the first reconciliation - // - the HelmChart spec has changed (generation drift) - // - the previous reconciliation resulted in a failed artifact verification - // - there is no artifact in storage - Verify: obj.Spec.Verify != nil && (obj.Generation <= 0 || - conditions.GetObservedGeneration(obj, sourcev1.SourceVerifiedCondition) != obj.Generation || - conditions.IsFalse(obj, sourcev1.SourceVerifiedCondition) || - obj.GetArtifact() == nil), + // It will however try to verify the chart if `obj.Spec.Verify` is set, at every reconciliation. + Verify: obj.Spec.Verify != nil && obj.Spec.Verify.Provider != "", } if artifact := obj.GetArtifact(); artifact != nil { opts.CachedChart = r.Storage.LocalPath(*artifact) @@ -1293,9 +1292,13 @@ func observeChartBuild(obj *sourcev1.HelmChart, build *chart.Build, err error) { } switch buildErr.Reason { - case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage, chart.ErrChartVerification: + case chart.ErrChartMetadataPatch, chart.ErrValuesFilesMerge, chart.ErrDependencyBuild, chart.ErrChartPackage: conditions.Delete(obj, sourcev1.FetchFailedCondition) conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, buildErr.Reason.Reason, buildErr.Error()) + case chart.ErrChartVerification: + conditions.Delete(obj, sourcev1.FetchFailedCondition) + conditions.MarkTrue(obj, sourcev1.BuildFailedCondition, buildErr.Reason.Reason, buildErr.Error()) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, buildErr.Error()) default: conditions.Delete(obj, sourcev1.BuildFailedCondition) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, buildErr.Reason.Reason, buildErr.Error()) diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go index f9aaf2c8..6f6bb0dd 100644 --- a/controllers/helmchart_controller_test.go +++ b/controllers/helmchart_controller_test.go @@ -2262,11 +2262,11 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T tests := []struct { name string + shouldSign bool + beforeFunc func(obj *sourcev1.HelmChart) want sreconcile.Result wantErr bool wantErrMsg string - shouldSign bool - beforeFunc func(obj *sourcev1.HelmChart) assertConditions []metav1.Condition cleanFunc func(g *WithT, build *chart.Build) }{ @@ -2280,11 +2280,12 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T SecretRef: &meta.LocalObjectReference{Name: "cosign-key"}, } }, + want: sreconcile.ResultEmpty, wantErr: true, wantErrMsg: "chart verification error: failed to verify : no matching signatures:", - want: sreconcile.ResultEmpty, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no matching signatures:"), }, }, { @@ -2296,14 +2297,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T Provider: "cosign", } }, - wantErr: true, want: sreconcile.ResultEmpty, + wantErr: true, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.BuildFailedCondition, "ChartVerificationError", "chart verification error: failed to verify : no matching signatures:"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, sourcev1.VerificationError, "chart verification error: failed to verify : no matching signatures:"), }, }, { - name: "signed charts should pass verification", + name: "signed charts should pass verification", + shouldSign: true, beforeFunc: func(obj *sourcev1.HelmChart) { obj.Spec.Chart = metadata.Name obj.Spec.Version = metadata.Version @@ -2312,8 +2315,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_verifySignature(t *testing.T SecretRef: &meta.LocalObjectReference{Name: "cosign-key"}, } }, - shouldSign: true, - want: sreconcile.ResultSuccess, + want: sreconcile.ResultSuccess, assertConditions: []metav1.Condition{ *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '' chart with version ''"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, meta.SucceededReason, "verified signature of version "), diff --git a/docs/api/source.md b/docs/api/source.md index 060197be..819248f1 100644 --- a/docs/api/source.md +++ b/docs/api/source.md @@ -684,7 +684,7 @@ OCIRepositoryVerification

Verify contains the secret name containing the trusted public keys used to verify the signature and specifies which provider to use to check whether OCI image is authentic. -This field is only supported for OCI sources. +This field is only supported when using HelmRepository source with spec.type ‘oci’. Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.

@@ -2269,7 +2269,7 @@ OCIRepositoryVerification

Verify contains the secret name containing the trusted public keys used to verify the signature and specifies which provider to use to check whether OCI image is authentic. -This field is only supported for OCI sources. +This field is only supported when using HelmRepository source with spec.type ‘oci’. Chart dependencies, which are not bundled in the umbrella chart artifact, are not verified.

diff --git a/docs/spec/v1beta2/helmcharts.md b/docs/spec/v1beta2/helmcharts.md index d6e18987..990ff869 100644 --- a/docs/spec/v1beta2/helmcharts.md +++ b/docs/spec/v1beta2/helmcharts.md @@ -290,7 +290,7 @@ data: Note that the keys must have the `.pub` extension for Flux to make use of them. -Flux will loop over the public keys and use them verify a HelmChart's signature. +Flux will loop over the public keys and use them to verify a HelmChart's signature. This allows for older HelmCharts to be valid as long as the right key is in the secret. #### Keyless verification @@ -309,6 +309,7 @@ metadata: name: podinfo spec: interval: 5m + chart: podinfo reconcileStrategy: ChartVersion sourceRef: kind: HelmRepository diff --git a/docs/spec/v1beta2/ocirepositories.md b/docs/spec/v1beta2/ocirepositories.md index a6b22aa9..39d1decf 100644 --- a/docs/spec/v1beta2/ocirepositories.md +++ b/docs/spec/v1beta2/ocirepositories.md @@ -462,7 +462,7 @@ data: Note that the keys must have the `.pub` extension for Flux to make use of them. -Flux will loop over the public keys and use them verify an artifact's signature. +Flux will loop over the public keys and use them to verify an artifact's signature. This allows for older artifacts to be valid as long as the right key is in the secret. #### Keyless verification diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index debe87ea..d843d7d3 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -105,7 +105,7 @@ func KeychainAdaptHelper(keyChain authn.Keychain) func(string) (registry.LoginOp } // AuthAdaptHelper returns an ORAS credentials callback configured with the authorization data -// from the given authn authenticator This allows for example to make use of credential helpers from +// from the given authn authenticator. This allows for example to make use of credential helpers from // cloud providers. // Ref: https://github.com/google/go-containerregistry/tree/main/pkg/authn func AuthAdaptHelper(auth authn.Authenticator) (registry.LoginOption, error) { diff --git a/internal/helm/repository/chart_repository.go b/internal/helm/repository/chart_repository.go index 15e62432..596bc1a8 100644 --- a/internal/helm/repository/chart_repository.go +++ b/internal/helm/repository/chart_repository.go @@ -523,10 +523,8 @@ func (r *ChartRepository) RemoveCache() error { } // VerifyChart verifies the chart against a signature. -// If no signature is provided, a keyless verification is performed. // It returns an error on failure. func (r *ChartRepository) VerifyChart(_ context.Context, _ *repo.ChartVersion) error { - // no-op // this is a no-op because this is not implemented yet. return fmt.Errorf("not implemented") }