fix(helm): support http and custom tls helm registries, give help when misconfigured - develop [r8s-472] (#1050)

Co-authored-by: testA113 <aliharriss1995@gmail.com>

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")
+			Msg("Failed to create registry client")
-		return errors.Wrap(err, "failed to login to registry")
+		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) {
+func createOCIRegistryClient(portainerRegistry *portainer.Registry) (*registry.Client, error) {
-	if IsHTTPRepository(portainerRegistry) || !portainerRegistry.Authentication {
+	// Handle nil registry (HTTP repository)
-		return nil, nil // No authentication needed
+	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{
+	// Only perform login if authentication is enabled
-		registry.LoginOptBasicAuth(portainerRegistry.Username, portainerRegistry.Password),
+	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

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

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(&registry)
+	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
+		// Use the configured HTTP client for anonymous access
-		registryClient.Client = retry.DefaultClient
+		registryClient.Client = httpClient
 
 		log.Debug().
 			Str("registryURL", registry.URL).

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)
+				// For anonymous access without custom TLS, all registries should use retry.DefaultClient
-				assert.Equal(t, retry.DefaultClient, client.Client,
+				// (Only registries with custom TLS configuration use a different retry client)
-					"Expected retry.DefaultClient for anonymous access")
+				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")
+				}
 			}
 		})
 	}

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

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