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 + } +} diff --git a/pkg/registryhttp/client_test.go b/pkg/registryhttp/client_test.go new file mode 100644 index 000000000..bf1fa53a1 --- /dev/null +++ b/pkg/registryhttp/client_test.go @@ -0,0 +1,203 @@ +package registryhttp + +import ( + "net/http" + "testing" + + portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/pkg/fips" + "github.com/stretchr/testify/assert" + "oras.land/oras-go/v2/registry/remote/retry" +) + +func init() { + fips.InitFIPS(false) +} + +func TestCreateClient(t *testing.T) { + tests := []struct { + name string + registry *portainer.Registry + expectedUsePlainHTTP bool + expectError bool + }{ + { + name: "Azure Registry should use default client with HTTPS", + registry: &portainer.Registry{ + Type: portainer.AzureRegistry, + URL: "myregistry.azurecr.io", + }, + expectedUsePlainHTTP: false, + expectError: false, + }, + { + name: "ECR Registry should use default client with HTTPS", + registry: &portainer.Registry{ + Type: portainer.EcrRegistry, + URL: "123456789012.dkr.ecr.us-east-1.amazonaws.com", + }, + expectedUsePlainHTTP: false, + expectError: false, + }, + { + name: "GitHub Registry should use default client with HTTPS", + registry: &portainer.Registry{ + Type: portainer.GithubRegistry, + URL: "ghcr.io", + }, + expectedUsePlainHTTP: false, + expectError: false, + }, + { + name: "GitLab Registry should use default client with HTTPS", + registry: &portainer.Registry{ + Type: portainer.GitlabRegistry, + URL: "registry.gitlab.com", + }, + expectedUsePlainHTTP: false, + expectError: false, + }, + { + name: "Custom registry without TLS should use plain HTTP", + registry: &portainer.Registry{ + Type: portainer.CustomRegistry, + URL: "my-custom-registry.local", + }, + expectedUsePlainHTTP: true, + expectError: false, + }, + { + name: "Custom registry with TLS enabled should use HTTPS", + registry: &portainer.Registry{ + Type: portainer.CustomRegistry, + URL: "my-custom-registry.local", + ManagementConfiguration: &portainer.RegistryManagementConfiguration{ + TLSConfig: portainer.TLSConfiguration{ + TLS: true, + TLSSkipVerify: true, // Skip verify to avoid cert file requirements in test + }, + }, + }, + expectedUsePlainHTTP: false, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client, usePlainHTTP, err := CreateClient(tt.registry) + + if tt.expectError { + assert.Error(t, err) + assert.Nil(t, client) + return + } + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.Equal(t, tt.expectedUsePlainHTTP, usePlainHTTP) + + // Verify client type based on registry configuration + switch tt.registry.Type { + case portainer.AzureRegistry, portainer.EcrRegistry, portainer.GithubRegistry, portainer.GitlabRegistry: + // Cloud registries should use the default retry client + assert.Equal(t, retry.DefaultClient, client) + default: + // Custom registries with TLS should get a custom client, others use default + if tt.registry.ManagementConfiguration != nil && tt.registry.ManagementConfiguration.TLSConfig.TLS { + // Custom TLS configuration should create a new client + assert.NotEqual(t, retry.DefaultClient, client) + assert.IsType(t, &http.Client{}, client) + } else { + // No TLS configuration should use default client + assert.Equal(t, retry.DefaultClient, client) + } + } + }) + } +} + +func TestCreateClient_CloudRegistries(t *testing.T) { + cloudRegistryTypes := []struct { + name string + registryType portainer.RegistryType + }{ + {"AzureRegistry", portainer.AzureRegistry}, + {"EcrRegistry", portainer.EcrRegistry}, + {"GithubRegistry", portainer.GithubRegistry}, + {"GitlabRegistry", portainer.GitlabRegistry}, + } + + for _, tt := range cloudRegistryTypes { + t.Run(tt.name, func(t *testing.T) { + registry := &portainer.Registry{ + Type: tt.registryType, + URL: "example.registry.com", + } + + client, usePlainHTTP, err := CreateClient(registry) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.False(t, usePlainHTTP, "Cloud registries should use HTTPS") + assert.Equal(t, retry.DefaultClient, client, "Cloud registries should use default retry client") + }) + } +} + +func TestCreateClient_CustomTLSConfiguration(t *testing.T) { + t.Run("TLS enabled with skip verify", func(t *testing.T) { + registry := &portainer.Registry{ + Type: portainer.CustomRegistry, + URL: "my-registry.local", + ManagementConfiguration: &portainer.RegistryManagementConfiguration{ + TLSConfig: portainer.TLSConfiguration{ + TLS: true, + TLSSkipVerify: true, + }, + }, + } + + client, usePlainHTTP, err := CreateClient(registry) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.False(t, usePlainHTTP, "TLS enabled registries should use HTTPS") + assert.NotEqual(t, retry.DefaultClient, client, "Custom TLS should create new client") + assert.IsType(t, &http.Client{}, client) + }) + + t.Run("TLS disabled should use plain HTTP", func(t *testing.T) { + registry := &portainer.Registry{ + Type: portainer.CustomRegistry, + URL: "my-registry.local", + ManagementConfiguration: &portainer.RegistryManagementConfiguration{ + TLSConfig: portainer.TLSConfiguration{ + TLS: false, + }, + }, + } + + client, usePlainHTTP, err := CreateClient(registry) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.True(t, usePlainHTTP, "TLS disabled should use plain HTTP") + assert.Equal(t, retry.DefaultClient, client, "No TLS should use default client") + }) + + t.Run("No management configuration should use plain HTTP", func(t *testing.T) { + registry := &portainer.Registry{ + Type: portainer.CustomRegistry, + URL: "my-registry.local", + ManagementConfiguration: nil, + } + + client, usePlainHTTP, err := CreateClient(registry) + + assert.NoError(t, err) + assert.NotNil(t, client) + assert.True(t, usePlainHTTP, "No management config should use plain HTTP") + assert.Equal(t, retry.DefaultClient, client, "No management config should use default client") + }) +}