From 631f1deb2e9a8198fb5bfe7bb2edf812a5dc77c4 Mon Sep 17 00:00:00 2001 From: Ali <83188384+testA113@users.noreply.github.com> Date: Mon, 18 Aug 2025 12:07:41 +1200 Subject: [PATCH] fix(helm): support http and custom tls helm registries, give help when misconfigured [r8s-472] (#1032) Co-authored-by: JamesPlayer --- pkg/libhelm/sdk/chartsources.go | 98 ++++++++++++++++++---------- pkg/libhelm/sdk/chartsources_test.go | 40 ++++++++++-- pkg/liboras/registry.go | 17 +++-- pkg/liboras/registry_test.go | 13 +++- pkg/registryhttp/client.go | 48 ++++++++++++++ 5 files changed, 169 insertions(+), 47 deletions(-) create mode 100644 pkg/registryhttp/client.go diff --git a/pkg/libhelm/sdk/chartsources.go b/pkg/libhelm/sdk/chartsources.go index c42196f3d..e37fe55b2 100644 --- a/pkg/libhelm/sdk/chartsources.go +++ b/pkg/libhelm/sdk/chartsources.go @@ -35,10 +35,10 @@ import ( portainer "github.com/portainer/portainer/api" "github.com/portainer/portainer/pkg/libhelm/cache" "github.com/portainer/portainer/pkg/libhelm/options" + "github.com/portainer/portainer/pkg/registryhttp" "github.com/rs/zerolog/log" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/registry" - "oras.land/oras-go/v2/registry/remote/retry" ) // IsOCIRegistry returns true if the registry is an OCI registry (not nil), false if it's an HTTP repository (nil) @@ -140,14 +140,6 @@ func authenticateChartSource(actionConfig *action.Configuration, registry *porta return errors.Wrap(err, "registry credential validation failed") } - // No authentication required - if !registry.Authentication { - log.Debug(). - Str("context", "HelmClient"). - Msg("No OCI registry authentication required") - return nil - } - // Cache Strategy Decision: Use registry ID as cache key // This provides optimal rate limiting protection since each registry only gets // logged into once per Portainer instance, regardless of how many users access it. @@ -180,14 +172,14 @@ func authenticateChartSource(actionConfig *action.Configuration, registry *porta Str("context", "HelmClient"). Msg("Cache miss - creating new registry client") - registryClient, err := loginToOCIRegistry(registry) + registryClient, err := createOCIRegistryClient(registry) if err != nil { log.Error(). Str("context", "HelmClient"). Str("registry_url", registry.URL). Err(err). - Msg("Failed to login to registry") - return errors.Wrap(err, "failed to login to registry") + Msg("Failed to create registry client") + return errors.Wrap(err, "failed to create registry client") } // Cache the client if login was successful (registry ID-based key) @@ -230,11 +222,13 @@ func configureOCIChartPathOptions(chartPathOptions *action.ChartPathOptions, reg } } -// loginToOCIRegistry performs registry login for OCI-based registries using Helm SDK +// createOCIRegistryClient creates and optionally authenticates a registry client for OCI-based registries +// Handles both authenticated and unauthenticated registries with proper TLS configuration // Tries to get a cached registry client if available, otherwise creates and caches a new one -func loginToOCIRegistry(portainerRegistry *portainer.Registry) (*registry.Client, error) { - if IsHTTPRepository(portainerRegistry) || !portainerRegistry.Authentication { - return nil, nil // No authentication needed +func createOCIRegistryClient(portainerRegistry *portainer.Registry) (*registry.Client, error) { + // Handle nil registry (HTTP repository) + if portainerRegistry == nil { + return nil, nil } // Check cache first using registry ID-based key @@ -243,32 +237,70 @@ func loginToOCIRegistry(portainerRegistry *portainer.Registry) (*registry.Client } log.Debug(). - Str("context", "loginToRegistry"). + Str("context", "HelmClient"). Int("registry_id", int(portainerRegistry.ID)). Str("registry_url", portainerRegistry.URL). - Msg("Attempting to login to OCI registry") + Bool("authentication", portainerRegistry.Authentication). + Msg("Creating OCI registry client") - registryClient, err := registry.NewClient(registry.ClientOptHTTPClient(retry.DefaultClient)) + // Create an HTTP client with proper TLS configuration + httpClient, usePlainHTTP, err := registryhttp.CreateClient(portainerRegistry) if err != nil { + log.Error(). + Str("context", "HelmClient"). + Str("registry_url", portainerRegistry.URL). + Err(err). + Msg("Failed to create HTTP client for registry") + return nil, errors.Wrap(err, "failed to create HTTP client for registry") + } + + clientOptions := []registry.ClientOption{ + registry.ClientOptHTTPClient(httpClient), + } + + if usePlainHTTP { + clientOptions = append(clientOptions, registry.ClientOptPlainHTTP()) + } + + registryClient, err := registry.NewClient(clientOptions...) + if err != nil { + log.Error(). + Str("context", "HelmClient"). + Str("registry_url", portainerRegistry.URL). + Err(err). + Msg("Failed to create registry client") return nil, errors.Wrap(err, "failed to create registry client") } - loginOpts := []registry.LoginOption{ - registry.LoginOptBasicAuth(portainerRegistry.Username, portainerRegistry.Password), + // Only perform login if authentication is enabled + if portainerRegistry.Authentication { + loginOpts := []registry.LoginOption{ + registry.LoginOptBasicAuth(portainerRegistry.Username, portainerRegistry.Password), + } + + err = registryClient.Login(portainerRegistry.URL, loginOpts...) + if err != nil { + log.Error(). + Str("context", "HelmClient"). + Str("registry_url", portainerRegistry.URL). + Err(err). + Msg("Failed to login to registry") + return nil, errors.Wrapf(err, "failed to login to registry %s", portainerRegistry.URL) + } + + log.Debug(). + Str("context", "createOCIRegistryClient"). + Int("registry_id", int(portainerRegistry.ID)). + Str("registry_url", portainerRegistry.URL). + Msg("Successfully logged in to OCI registry") + } else { + log.Debug(). + Str("context", "createOCIRegistryClient"). + Int("registry_id", int(portainerRegistry.ID)). + Str("registry_url", portainerRegistry.URL). + Msg("Created unauthenticated OCI registry client") } - err = registryClient.Login(portainerRegistry.URL, loginOpts...) - if err != nil { - return nil, errors.Wrapf(err, "failed to login to registry %s", portainerRegistry.URL) - } - - log.Debug(). - Str("context", "loginToRegistry"). - Int("registry_id", int(portainerRegistry.ID)). - Str("registry_url", portainerRegistry.URL). - Msg("Successfully logged in to OCI registry") - - // Cache using registry ID-based key cache.SetCachedRegistryClientByID(portainerRegistry.ID, registryClient) return registryClient, nil diff --git a/pkg/libhelm/sdk/chartsources_test.go b/pkg/libhelm/sdk/chartsources_test.go index 663601abc..20b6ef397 100644 --- a/pkg/libhelm/sdk/chartsources_test.go +++ b/pkg/libhelm/sdk/chartsources_test.go @@ -6,12 +6,17 @@ import ( "github.com/pkg/errors" portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/pkg/fips" helmregistrycache "github.com/portainer/portainer/pkg/libhelm/cache" "github.com/stretchr/testify/assert" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/registry" ) +func init() { + fips.InitFIPS(false) +} + func TestIsOCIRegistry(t *testing.T) { t.Run("should return false for nil registry (HTTP repo)", func(t *testing.T) { assert.False(t, IsOCIRegistry(nil)) @@ -188,19 +193,40 @@ func TestLoginToOCIRegistry(t *testing.T) { is := assert.New(t) t.Run("should return nil for HTTP repository (nil registry)", func(t *testing.T) { - client, err := loginToOCIRegistry(nil) + client, err := createOCIRegistryClient(nil) is.NoError(err) is.Nil(client) }) - t.Run("should return nil for registry with auth disabled", func(t *testing.T) { + t.Run("should return client for registry with auth disabled (for TLS support)", func(t *testing.T) { registry := &portainer.Registry{ URL: "my-registry.io", Authentication: false, } - client, err := loginToOCIRegistry(registry) + client, err := createOCIRegistryClient(registry) is.NoError(err) - is.Nil(client) + is.NotNil(client) // Now returns a client even without auth for potential TLS configuration + }) + + t.Run("should handle custom TLS configuration without authentication", func(t *testing.T) { + registry := &portainer.Registry{ + URL: "my-registry.io", + Authentication: false, + ManagementConfiguration: &portainer.RegistryManagementConfiguration{ + TLSConfig: portainer.TLSConfiguration{ + TLS: true, + TLSSkipVerify: false, + // In a real scenario, these would point to actual cert files + TLSCACertPath: "", + TLSCertPath: "", + TLSKeyPath: "", + }, + }, + } + client, err := createOCIRegistryClient(registry) + // Should succeed even without cert files when they're empty strings + is.NoError(err) + is.NotNil(client) // Should get a client configured for TLS }) t.Run("should return error for invalid credentials", func(t *testing.T) { @@ -209,7 +235,7 @@ func TestLoginToOCIRegistry(t *testing.T) { Authentication: true, Username: " ", } - client, err := loginToOCIRegistry(registry) + client, err := createOCIRegistryClient(registry) is.Error(err) is.Nil(client) // The error might be a validation error or a login error, both are acceptable @@ -227,7 +253,7 @@ func TestLoginToOCIRegistry(t *testing.T) { } // this will fail because it can't connect to the registry, // but it proves that the loginToOCIRegistry function is calling the login function. - client, err := loginToOCIRegistry(registry) + client, err := createOCIRegistryClient(registry) is.Error(err) is.Nil(client) is.Contains(err.Error(), "failed to login to registry") @@ -249,7 +275,7 @@ func TestLoginToOCIRegistry(t *testing.T) { } // this will fail because it can't connect to the registry, // but it proves that the loginToOCIRegistry function is calling the login function. - client, err := loginToOCIRegistry(registry) + client, err := createOCIRegistryClient(registry) is.Error(err) is.Nil(client) is.Contains(err.Error(), "failed to login to registry") diff --git a/pkg/liboras/registry.go b/pkg/liboras/registry.go index 576f848ac..317619700 100644 --- a/pkg/liboras/registry.go +++ b/pkg/liboras/registry.go @@ -4,10 +4,10 @@ import ( "strings" portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/pkg/registryhttp" "github.com/rs/zerolog/log" "oras.land/oras-go/v2/registry/remote" "oras.land/oras-go/v2/registry/remote/auth" - "oras.land/oras-go/v2/registry/remote/retry" ) func CreateClient(registry portainer.Registry) (*remote.Registry, error) { @@ -16,6 +16,15 @@ func CreateClient(registry portainer.Registry) (*remote.Registry, error) { log.Error().Err(err).Str("registryUrl", registry.URL).Msg("Failed to create registry client") return nil, err } + + // Configure HTTP client based on registry type using the shared utility + httpClient, usePlainHTTP, err := registryhttp.CreateClient(®istry) + if err != nil { + return nil, err + } + + registryClient.PlainHTTP = usePlainHTTP + // By default, oras sends multiple requests to get the full list of repos/tags/referrers. // set a high page size limit for fewer round trips. // e.g. https://github.com/oras-project/oras-go/blob/v2.6.0/registry/remote/registry.go#L129-L142 @@ -29,7 +38,7 @@ func CreateClient(registry portainer.Registry) (*remote.Registry, error) { strings.TrimSpace(registry.Password) != "" { registryClient.Client = &auth.Client{ - Client: retry.DefaultClient, + Client: httpClient, Cache: auth.NewCache(), Credential: auth.StaticCredential(registry.URL, auth.Credential{ Username: registry.Username, @@ -43,8 +52,8 @@ func CreateClient(registry portainer.Registry) (*remote.Registry, error) { Bool("authentication", true). Msg("Created ORAS registry client with authentication") } else { - // Use default client for anonymous access - registryClient.Client = retry.DefaultClient + // Use the configured HTTP client for anonymous access + registryClient.Client = httpClient log.Debug(). Str("registryURL", registry.URL). diff --git a/pkg/liboras/registry_test.go b/pkg/liboras/registry_test.go index 78172f25d..e9b9f8b7c 100644 --- a/pkg/liboras/registry_test.go +++ b/pkg/liboras/registry_test.go @@ -138,9 +138,16 @@ func TestCreateClient_AuthenticationScenarios(t *testing.T) { assert.NotNil(t, authClient, "Auth client should not be nil") assert.NotNil(t, authClient.Credential, "Credential function should be set") } else { - // Should use retry.DefaultClient (no authentication) - assert.Equal(t, retry.DefaultClient, client.Client, - "Expected retry.DefaultClient for anonymous access") + // For anonymous access without custom TLS, all registries should use retry.DefaultClient + // (Only registries with custom TLS configuration use a different retry client) + if tt.registry.ManagementConfiguration == nil || !tt.registry.ManagementConfiguration.TLSConfig.TLS { + assert.Equal(t, retry.DefaultClient, client.Client, + "Expected retry.DefaultClient for anonymous access without custom TLS") + } else { + // Custom TLS configuration means a custom retry client + assert.NotEqual(t, retry.DefaultClient, client.Client, + "Expected custom retry client for registry with custom TLS") + } } }) } diff --git a/pkg/registryhttp/client.go b/pkg/registryhttp/client.go new file mode 100644 index 000000000..ab2b0f6f1 --- /dev/null +++ b/pkg/registryhttp/client.go @@ -0,0 +1,48 @@ +package registryhttp + +import ( + "net/http" + + portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/api/crypto" + "github.com/rs/zerolog/log" + "oras.land/oras-go/v2/registry/remote/retry" +) + +// CreateClient creates an HTTP client with appropriate TLS configuration based on registry type. +// All registries use retry clients for better resilience. +// Returns the HTTP client, whether to use plainHTTP, and any error. +func CreateClient(registry *portainer.Registry) (*http.Client, bool, error) { + switch registry.Type { + case portainer.AzureRegistry, portainer.EcrRegistry, portainer.GithubRegistry, portainer.GitlabRegistry: + // Cloud registries use the default retry client with built-in TLS + return retry.DefaultClient, false, nil + default: + // For all other registry types, check if custom TLS is needed + if registry.ManagementConfiguration != nil && registry.ManagementConfiguration.TLSConfig.TLS { + // Need custom TLS configuration - create a retry client with custom transport + baseTransport := &http.Transport{ + Proxy: http.ProxyFromEnvironment, + } + + tlsConfig, err := crypto.CreateTLSConfigurationFromDisk( + registry.ManagementConfiguration.TLSConfig, + ) + if err != nil { + log.Error().Err(err).Msg("Failed to create TLS configuration") + return nil, false, err + } + baseTransport.TLSClientConfig = tlsConfig + + // Create a retry transport wrapping our custom base transport + retryTransport := retry.NewTransport(baseTransport) + httpClient := &http.Client{ + Transport: retryTransport, + } + return httpClient, false, nil + } + + // Default to HTTP for non-cloud registries without TLS configuration + return retry.DefaultClient, true, nil + } +}