From c1ee57fa52201d1daec4deae6915a48c72d49a74 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Fri, 20 Feb 2026 14:46:24 +0000 Subject: [PATCH 01/14] First cut at dynamic registry usage --- server/imageproxyserver.go | 356 +++++++++++++++++++++++++++---------- 1 file changed, 264 insertions(+), 92 deletions(-) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index ca8f62e4..9095fc16 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -10,32 +10,231 @@ import ( "net/http" "net/http/httputil" "net/url" + "os" "strings" + "sync" "github.com/go-logr/logr" "sigs.k8s.io/controller-runtime/pkg/client" ) const ( - ghcrIOKey = "ghcr.io/" - keppelKey = "keppel.global.cloud.sap/" imageKey = "imageName" layerDigestKey = "layerDigest" versionKey = "version" MediaTypeUKI = "application/vnd.ironcore.image.uki" ) +type AuthMethod int + +const ( + AuthNone AuthMethod = iota // Anonymous access + AuthBasic // Basic username:password + AuthBearer // Bearer token via /token endpoint +) + +type RegistryInfo struct { + Domain string + AuthMethod AuthMethod + TokenURL string // For bearer token auth + Realm string // For basic auth +} + type TokenResponse struct { Token string `json:"token"` } type ImageDetails struct { OCIImageName string + RegistryDomain string RepositoryName string LayerDigest string Version string } +// Cache registry info to avoid repeated probes +var registryCache = make(map[string]*RegistryInfo) +var registryCacheMutex sync.RWMutex + +// Extract registry domain from OCI image reference +func extractRegistryDomain(imageRef string) string { + parts := strings.SplitN(imageRef, "/", 2) + if len(parts) < 2 { + // No slash means Docker Hub implicit + return "registry-1.docker.io" + } + + // Check if first part looks like a domain (has dot or colon) + if strings.Contains(parts[0], ".") || strings.Contains(parts[0], ":") { + return parts[0] + } + + // Implicit Docker Hub (e.g., "library/ubuntu") + return "registry-1.docker.io" +} + +// Extract repository path without registry +func extractRepository(imageRef, registryDomain string) string { + return strings.TrimPrefix(imageRef, registryDomain+"/") +} + +// Check if registry is in a comma-separated list (exact match only) +func isInList(registry string, list string) bool { + if list == "" { + return false + } + + items := strings.Split(list, ",") + for _, item := range items { + if strings.TrimSpace(item) == registry { + return true + } + } + return false +} + +// Check if registry is allowed based on allow/block lists +// - If ALLOWED_REGISTRIES is set: only those registries are allowed (whitelist) +// - Else if BLOCKED_REGISTRIES is set: all except those are allowed (blacklist) +// - Else: DENY ALL - operator must explicitly configure registry policy +func isRegistryAllowed(registry string) bool { + allowList := os.Getenv("ALLOWED_REGISTRIES") + blockList := os.Getenv("BLOCKED_REGISTRIES") + + // Allow list takes precedence (more restrictive) + if allowList != "" { + return isInList(registry, allowList) + } + + // Block list mode: allow all except blocked + if blockList != "" { + return !isInList(registry, blockList) + } + + // Default: deny all - require explicit configuration + return false +} + +// Parse WWW-Authenticate parameter value +func extractParam(header, param string) string { + start := strings.Index(header, param+"=\"") + if start == -1 { + return "" + } + start += len(param) + 2 + end := strings.Index(header[start:], "\"") + if end == -1 { + return "" + } + return header[start : start+end] +} + +// Parse Bearer token URL from WWW-Authenticate header +func extractTokenURL(authHeader, repository string) string { + realm := extractParam(authHeader, "realm") + service := extractParam(authHeader, "service") + + if realm == "" { + return "" + } + + // Build token URL with repository scope + scope := fmt.Sprintf("repository:%s:pull", repository) + if service != "" { + return fmt.Sprintf("%s?service=%s&scope=%s", realm, service, scope) + } + return fmt.Sprintf("%s?scope=%s", realm, scope) +} + +// Probe registry to determine auth requirements +func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error) { + // Try GET /v2/ - standard registry probe endpoint + targetURL := fmt.Sprintf("https://%s/v2/", registryDomain) + resp, err := http.Get(targetURL) + if err != nil { + return nil, fmt.Errorf("registry unreachable: %w", err) + } + defer resp.Body.Close() + + info := &RegistryInfo{Domain: registryDomain} + + switch resp.StatusCode { + case http.StatusOK: + // Anonymous access allowed + info.AuthMethod = AuthNone + return info, nil + + case http.StatusUnauthorized: + // Parse WWW-Authenticate header + authHeader := resp.Header.Get("WWW-Authenticate") + if authHeader == "" { + return nil, fmt.Errorf("401 without WWW-Authenticate header") + } + + if strings.HasPrefix(authHeader, "Bearer ") { + info.AuthMethod = AuthBearer + info.TokenURL = extractTokenURL(authHeader, repository) + return info, nil + } + + if strings.HasPrefix(authHeader, "Basic ") { + info.AuthMethod = AuthBasic + info.Realm = extractParam(authHeader, "realm") + return info, nil + } + + return nil, fmt.Errorf("unsupported auth: %s", authHeader) + + default: + return nil, fmt.Errorf("unexpected status: %d", resp.StatusCode) + } +} + +// Get or detect registry info with caching +func getOrDetectRegistry(registry, repository string) (*RegistryInfo, error) { + cacheKey := registry + + registryCacheMutex.RLock() + if info, exists := registryCache[cacheKey]; exists { + registryCacheMutex.RUnlock() + return info, nil + } + registryCacheMutex.RUnlock() + + // Detect and cache + info, err := detectRegistryAuth(registry, repository) + if err != nil { + return nil, err + } + + registryCacheMutex.Lock() + registryCache[cacheKey] = info + registryCacheMutex.Unlock() + + return info, nil +} + +// Get bearer token from token URL +func getBearerToken(tokenURL string) (string, error) { + resp, err := http.Get(tokenURL) + if err != nil { + return "", err + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", err + } + + var tokenResponse TokenResponse + if err := json.Unmarshal(body, &tokenResponse); err != nil { + return "", err + } + + return tokenResponse.Token, nil +} + func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, log logr.Logger) { http.HandleFunc("/image", func(w http.ResponseWriter, r *http.Request) { imageDetails, err := parseImageURL(r.URL.Query()) @@ -45,14 +244,7 @@ func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, l return } - if strings.HasPrefix(imageDetails.OCIImageName, ghcrIOKey) { - handleGHCR(w, r, &imageDetails, log) - } else if strings.HasPrefix(imageDetails.OCIImageName, keppelKey) { - handleKeppel(w, r, &imageDetails, log) - } else { - http.Error(w, "Bad Request", http.StatusBadRequest) - log.Info("Unsupported registry") - } + handleDockerRegistry(w, r, &imageDetails, log) }) http.HandleFunc("/httpboot/", func(w http.ResponseWriter, r *http.Request) { @@ -65,14 +257,7 @@ func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, l return } - if strings.HasPrefix(imageDetails.OCIImageName, ghcrIOKey) { - handleGHCR(w, r, &imageDetails, log) - } else if strings.HasPrefix(imageDetails.OCIImageName, keppelKey) { - handleKeppel(w, r, &imageDetails, log) - } else { - http.Error(w, "Bad Request", http.StatusBadRequest) - log.Info("Unsupported registry") - } + handleDockerRegistry(w, r, &imageDetails, log) }) log.Info("Starting image proxy server", "address", imageProxyServerAddr) @@ -92,14 +277,9 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { imageName := strings.Join(segments[:len(segments)-1], "/") digestSegment := segments[len(segments)-1] - var repositoryName string - if strings.HasPrefix(imageName, ghcrIOKey) { - repositoryName = strings.TrimPrefix(imageName, ghcrIOKey) - } else if strings.HasPrefix(imageName, keppelKey) { - repositoryName = strings.TrimPrefix(imageName, keppelKey) - } else { - return ImageDetails{}, fmt.Errorf("unsupported registry key") - } + // Extract registry domain and repository + registryDomain := extractRegistryDomain(imageName) + repositoryName := extractRepository(imageName, registryDomain) digestSegment = strings.TrimSuffix(digestSegment, ".efi") @@ -110,81 +290,87 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { return ImageDetails{ OCIImageName: imageName, - LayerDigest: layerDigest, + RegistryDomain: registryDomain, RepositoryName: repositoryName, + LayerDigest: layerDigest, }, nil } -func handleGHCR(w http.ResponseWriter, r *http.Request, imageDetails *ImageDetails, log logr.Logger) { - log.Info("Processing Image Proxy request", "method", r.Method, "path", r.URL.Path, "clientIP", r.RemoteAddr) +func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails *ImageDetails, log logr.Logger) { + registry := imageDetails.RegistryDomain + repository := imageDetails.RepositoryName - bearerToken, err := imageDetails.getBearerToken() - if err != nil { - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - log.Info("Error: Failed to obtain the bearer token", "error", err) + log.Info("Processing registry request", "registry", registry, "repository", repository, "digest", imageDetails.LayerDigest) + + // Security check + if !isRegistryAllowed(registry) { + http.Error(w, "Forbidden: Registry not allowed", http.StatusForbidden) + log.Info("Registry blocked", "registry", registry, "allowList", os.Getenv("ALLOWED_REGISTRIES"), "blockList", os.Getenv("BLOCKED_REGISTRIES")) return } - digest := imageDetails.LayerDigest - targetURL := fmt.Sprintf("https://ghcr.io/v2/%s/blobs/%s", imageDetails.RepositoryName, digest) - proxyURL, _ := url.Parse(targetURL) - - proxy := &httputil.ReverseProxy{ - Director: imageDetails.modifyDirector(proxyURL, bearerToken, digest), - ModifyResponse: modifyProxyResponse(bearerToken), + // Auto-detect auth method (with caching) + registryInfo, err := getOrDetectRegistry(registry, repository) + if err != nil { + http.Error(w, "Registry detection failed", http.StatusBadGateway) + log.Error(err, "Failed to detect registry", "registry", registry) + return } - r.URL.Host = proxyURL.Host - r.URL.Scheme = proxyURL.Scheme - r.Host = proxyURL.Host - - proxy.ServeHTTP(w, r) -} - -func handleKeppel(w http.ResponseWriter, r *http.Request, imageDetails *ImageDetails, log logr.Logger) { - log.Info("Processing Image Proxy request for Keppel", "method", r.Method, "path", r.URL.Path, "clientIP", r.RemoteAddr) + // Get auth token if needed + var authToken string + switch registryInfo.AuthMethod { + case AuthBearer: + authToken, err = getBearerToken(registryInfo.TokenURL) + if err != nil { + http.Error(w, "Authentication failed", http.StatusUnauthorized) + log.Error(err, "Failed to get bearer token", "tokenURL", registryInfo.TokenURL) + return + } + log.V(1).Info("Obtained bearer token", "registry", registry) + case AuthBasic: + // TODO: Support basic auth with credentials from secrets + http.Error(w, "Basic auth not yet implemented", http.StatusNotImplemented) + log.Info("Basic auth required but not yet supported", "registry", registry) + return + case AuthNone: + log.V(1).Info("Registry allows anonymous access", "registry", registry) + } + // Proxy the blob request digest := imageDetails.LayerDigest - targetURL := fmt.Sprintf("https://%sv2/%s/blobs/%s", keppelKey, imageDetails.RepositoryName, digest) + targetURL := fmt.Sprintf("https://%s/v2/%s/blobs/%s", registry, repository, digest) proxyURL, _ := url.Parse(targetURL) proxy := &httputil.ReverseProxy{ - Director: imageDetails.modifyDirector(proxyURL, "", digest), + Director: buildDirector(proxyURL, authToken, repository, digest), + ModifyResponse: buildModifyResponse(authToken), } r.URL.Host = proxyURL.Host r.URL.Scheme = proxyURL.Scheme r.Host = proxyURL.Host + log.Info("Proxying registry request", "targetURL", targetURL, "authMethod", registryInfo.AuthMethod) proxy.ServeHTTP(w, r) } -func (imageDetails ImageDetails) getBearerToken() (string, error) { - url := fmt.Sprintf("https://ghcr.io/token?scope=repository:%s:pull", imageDetails.RepositoryName) - resp, err := http.Get(url) - if err != nil { - return "", err - } - defer func() { - _ = resp.Body.Close() - }() - - body, err := io.ReadAll(resp.Body) - if err != nil { - return "", err - } - - var tokenResponse TokenResponse - if err := json.Unmarshal(body, &tokenResponse); err != nil { - return "", err +func buildDirector(proxyURL *url.URL, bearerToken string, repository string, digest string) func(*http.Request) { + return func(req *http.Request) { + req.URL.Scheme = proxyURL.Scheme + req.URL.Host = proxyURL.Host + req.URL.Path = fmt.Sprintf("/v2/%s/blobs/%s", repository, digest) + if bearerToken != "" { + req.Header.Set("Authorization", "Bearer "+bearerToken) + } } - - return tokenResponse.Token, nil } -func modifyProxyResponse(bearerToken string) func(*http.Response) error { +func buildModifyResponse(bearerToken string) func(*http.Response) error { return func(resp *http.Response) error { - resp.Header.Set("Authorization", "Bearer "+bearerToken) + if bearerToken != "" { + resp.Header.Set("Authorization", "Bearer "+bearerToken) + } if resp.StatusCode == http.StatusTemporaryRedirect { location, err := resp.Location() @@ -253,30 +439,16 @@ func parseImageURL(queries url.Values) (imageDetails ImageDetails, err error) { } ociImageName = strings.TrimSuffix(ociImageName, ".efi") - var repositoryName string - if strings.HasPrefix(ociImageName, ghcrIOKey) { - repositoryName = strings.TrimPrefix(ociImageName, ghcrIOKey) - } else if strings.HasPrefix(ociImageName, keppelKey) { - repositoryName = strings.TrimPrefix(ociImageName, keppelKey) - } else { - return ImageDetails{}, fmt.Errorf("unsupported registry key") - } + + // Extract registry domain and repository + registryDomain := extractRegistryDomain(ociImageName) + repositoryName := extractRepository(ociImageName, registryDomain) return ImageDetails{ OCIImageName: ociImageName, + RegistryDomain: registryDomain, RepositoryName: repositoryName, LayerDigest: layerDigest, Version: version, }, nil } - -func (ImageDetails ImageDetails) modifyDirector(proxyURL *url.URL, bearerToken string, digest string) func(*http.Request) { - return func(req *http.Request) { - req.URL.Scheme = proxyURL.Scheme - req.URL.Host = proxyURL.Host - req.URL.Path = fmt.Sprintf("/v2/%s/blobs/%s", ImageDetails.RepositoryName, digest) - if bearerToken != "" { - req.Header.Set("Authorization", "Bearer "+bearerToken) - } - } -} From d83197eb21883997a1bea85a7aa90b29256c5597 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Mon, 23 Feb 2026 11:38:25 +0000 Subject: [PATCH 02/14] Add Allow and Block lists for registries. --- Dockerfile | 1 + cmd/main.go | 23 +- ...serverbootconfiguration_http_controller.go | 23 +- .../serverbootconfiguration_pxe_controller.go | 16 +- internal/controller/suite_test.go | 30 +- internal/registry/validation.go | 97 ++++++ internal/registry/validation_test.go | 285 ++++++++++++++++++ server/imageproxyserver.go | 90 ++---- 8 files changed, 470 insertions(+), 95 deletions(-) create mode 100644 internal/registry/validation.go create mode 100644 internal/registry/validation_test.go diff --git a/Dockerfile b/Dockerfile index 3babcf03..7338d43e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -15,6 +15,7 @@ RUN go mod download COPY cmd/main.go cmd/main.go COPY api/ api/ COPY internal/controller/ internal/controller/ +COPY internal/registry/ internal/registry/ COPY server/ server/ COPY templates/ templates/ diff --git a/cmd/main.go b/cmd/main.go index 006439f7..b4fe849f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -34,6 +34,7 @@ import ( bootv1alpha1 "github.com/ironcore-dev/boot-operator/api/v1alpha1" "github.com/ironcore-dev/boot-operator/internal/controller" + "github.com/ironcore-dev/boot-operator/internal/registry" bootserver "github.com/ironcore-dev/boot-operator/server" //+kubebuilder:scaffold:imports ) @@ -227,6 +228,10 @@ func main() { os.Exit(1) } + // Initialize registry validator for OCI image validation + registryValidator := registry.NewValidator() + setupLog.Info("Initialized registry validator", "allowedRegistries", os.Getenv("ALLOWED_REGISTRIES"), "blockedRegistries", os.Getenv("BLOCKED_REGISTRIES")) + if controllers.Enabled(ipxeBootConfigController) { if err = (&controller.IPXEBootConfigReconciler{ Client: mgr.GetClient(), @@ -239,10 +244,11 @@ func main() { if controllers.Enabled(serverBootConfigControllerPxe) { if err = (&controller.ServerBootConfigurationPXEReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - IPXEServiceURL: ipxeServiceURL, - Architecture: architecture, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + IPXEServiceURL: ipxeServiceURL, + Architecture: architecture, + RegistryValidator: registryValidator, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ServerBootConfigPxe") os.Exit(1) @@ -251,10 +257,11 @@ func main() { if controllers.Enabled(serverBootConfigControllerHttp) { if err = (&controller.ServerBootConfigurationHTTPReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - ImageServerURL: imageServerURL, - Architecture: architecture, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + ImageServerURL: imageServerURL, + Architecture: architecture, + RegistryValidator: registryValidator, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ServerBootConfigHttp") os.Exit(1) diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index 5e849d8b..08036454 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -12,6 +12,7 @@ import ( apimeta "k8s.io/apimachinery/pkg/api/meta" "github.com/containerd/containerd/remotes/docker" + "github.com/ironcore-dev/boot-operator/internal/registry" ocispec "github.com/opencontainers/image-spec/specs-go/v1" corev1 "k8s.io/api/core/v1" @@ -36,9 +37,10 @@ const ( type ServerBootConfigurationHTTPReconciler struct { client.Client - Scheme *runtime.Scheme - ImageServerURL string - Architecture string + Scheme *runtime.Scheme + ImageServerURL string + Architecture string + RegistryValidator *registry.Validator } //+kubebuilder:rbac:groups=metal.ironcore.dev,resources=serverbootconfigurations,verbs=get;list;watch @@ -87,6 +89,13 @@ func (r *ServerBootConfigurationHTTPReconciler) reconcile(ctx context.Context, l ukiURL, err := r.constructUKIURL(ctx, config.Spec.Image) if err != nil { + log.Error(err, "Failed to construct UKI URL") + if patchErr := r.patchConfigStateFromHTTPState(ctx, + &bootv1alpha1.HTTPBootConfig{Status: bootv1alpha1.HTTPBootConfigStatus{ + State: bootv1alpha1.HTTPBootConfigStateError}}, + config); patchErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to patch state to error: %w (original error: %w)", patchErr, err) + } return ctrl.Result{}, fmt.Errorf("failed to construct UKI URL: %w", err) } log.V(1).Info("Extracted UKI URL for boot") @@ -203,8 +212,14 @@ func (r *ServerBootConfigurationHTTPReconciler) constructUKIURL(ctx context.Cont } func (r *ServerBootConfigurationHTTPReconciler) getUKIDigestFromNestedManifest(ctx context.Context, imageName, imageVersion string) (string, error) { - resolver := docker.NewResolver(docker.ResolverOptions{}) imageRef := fmt.Sprintf("%s:%s", imageName, imageVersion) + if r.RegistryValidator != nil { + if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { + return "", fmt.Errorf("registry validation failed: %w", err) + } + } + + resolver := docker.NewResolver(docker.ResolverOptions{}) name, desc, err := resolver.Resolve(ctx, imageRef) if err != nil { return "", fmt.Errorf("failed to resolve image reference: %w", err) diff --git a/internal/controller/serverbootconfiguration_pxe_controller.go b/internal/controller/serverbootconfiguration_pxe_controller.go index e3a69c51..6cbe71c1 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller.go +++ b/internal/controller/serverbootconfiguration_pxe_controller.go @@ -29,6 +29,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/ironcore-dev/boot-operator/api/v1alpha1" + "github.com/ironcore-dev/boot-operator/internal/registry" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -56,9 +57,10 @@ const ( type ServerBootConfigurationPXEReconciler struct { client.Client - Scheme *runtime.Scheme - IPXEServiceURL string - Architecture string + Scheme *runtime.Scheme + IPXEServiceURL string + Architecture string + RegistryValidator *registry.Validator } //+kubebuilder:rbac:groups=metal.ironcore.dev,resources=serverbootconfigurations,verbs=get;list;watch @@ -235,8 +237,14 @@ func (r *ServerBootConfigurationPXEReconciler) getImageDetailsFromConfig(ctx con } func (r *ServerBootConfigurationPXEReconciler) getLayerDigestsFromNestedManifest(ctx context.Context, imageName, imageVersion string) (string, string, string, error) { - resolver := docker.NewResolver(docker.ResolverOptions{}) imageRef := fmt.Sprintf("%s:%s", imageName, imageVersion) + if r.RegistryValidator != nil { + if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { + return "", "", "", fmt.Errorf("registry validation failed: %w", err) + } + } + + resolver := docker.NewResolver(docker.ResolverOptions{}) name, desc, err := resolver.Resolve(ctx, imageRef) if err != nil { return "", "", "", fmt.Errorf("failed to resolve image reference: %w", err) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 3bd6d233..614906e8 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -6,6 +6,7 @@ package controller import ( "context" "fmt" + "os" "path/filepath" "runtime" "testing" @@ -13,6 +14,7 @@ import ( "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/config" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "github.com/ironcore-dev/controller-utils/modutils" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" @@ -33,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" bootv1alpha1 "github.com/ironcore-dev/boot-operator/api/v1alpha1" + "github.com/ironcore-dev/boot-operator/internal/registry" //+kubebuilder:scaffold:imports ) @@ -61,6 +64,12 @@ func TestControllers(t *testing.T) { var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + // Set ALLOWED_REGISTRIES for tests to use ghcr.io images + Expect(os.Setenv("ALLOWED_REGISTRIES", "ghcr.io")).To(Succeed()) + DeferCleanup(func() { + Expect(os.Unsetenv("ALLOWED_REGISTRIES")).To(Succeed()) + }) + By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{ @@ -120,6 +129,9 @@ func SetupTest() *corev1.Namespace { k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: scheme.Scheme, + Metrics: metricsserver.Options{ + BindAddress: "0", // Disable metrics server to avoid port 8080 conflicts with Tilt + }, Controller: config.Controller{ // need to skip unique controller name validation // since all tests need a dedicated controller @@ -128,17 +140,21 @@ func SetupTest() *corev1.Namespace { }) Expect(err).ToNot(HaveOccurred()) + registryValidator := registry.NewValidator() + Expect((&ServerBootConfigurationPXEReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - IPXEServiceURL: "http://localhost:5000", - Architecture: "arm64", + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + IPXEServiceURL: "http://localhost:5000", + Architecture: runtime.GOARCH, + RegistryValidator: registryValidator, }).SetupWithManager(k8sManager)).To(Succeed()) Expect((&ServerBootConfigurationHTTPReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - ImageServerURL: "http://localhost:5000/httpboot", + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + ImageServerURL: "http://localhost:5000/httpboot", + RegistryValidator: registryValidator, }).SetupWithManager(k8sManager)).To(Succeed()) go func() { diff --git a/internal/registry/validation.go b/internal/registry/validation.go new file mode 100644 index 00000000..3d2aafbd --- /dev/null +++ b/internal/registry/validation.go @@ -0,0 +1,97 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package registry + +import ( + "fmt" + "os" + "strings" +) + +// Validator provides registry validation with cached environment variables. +type Validator struct { + allowedRegistries string + blockedRegistries string +} + +// NewValidator creates a new Validator with environment variables cached at initialization. +// This should be called once at startup to avoid repeated os.Getenv calls. +func NewValidator() *Validator { + return &Validator{ + allowedRegistries: os.Getenv("ALLOWED_REGISTRIES"), + blockedRegistries: os.Getenv("BLOCKED_REGISTRIES"), + } +} + +// ExtractRegistryDomain extracts the registry domain from an OCI image reference. +func ExtractRegistryDomain(imageRef string) string { + parts := strings.SplitN(imageRef, "/", 2) + if len(parts) < 2 { + return "registry-1.docker.io" + } + + potentialRegistry := parts[0] + + if strings.Contains(potentialRegistry, ".") || strings.Contains(potentialRegistry, ":") || potentialRegistry == "localhost" { + return potentialRegistry + } + + return "registry-1.docker.io" +} + +// isInList checks if a value is in a comma-separated list (exact match only). +func isInList(registry string, list string) bool { + if list == "" { + return false + } + + items := strings.Split(list, ",") + for _, item := range items { + if strings.TrimSpace(item) == registry { + return true + } + } + return false +} + +// IsRegistryAllowed checks if a registry is allowed based on the cached allow/block lists. +func (v *Validator) IsRegistryAllowed(registry string) bool { + if v.allowedRegistries != "" { + return isInList(registry, v.allowedRegistries) + } + + if v.blockedRegistries != "" { + return !isInList(registry, v.blockedRegistries) + } + + return false +} + +// ValidateImageRegistry validates that an image reference uses an allowed registry. +func (v *Validator) ValidateImageRegistry(imageRef string) error { + registry := ExtractRegistryDomain(imageRef) + if !v.IsRegistryAllowed(registry) { + if v.allowedRegistries != "" { + return fmt.Errorf("registry not allowed: %s (allowed registries: %s)", registry, v.allowedRegistries) + } else if v.blockedRegistries != "" { + return fmt.Errorf("registry blocked: %s (blocked registries: %s)", registry, v.blockedRegistries) + } + return fmt.Errorf("registry not allowed: %s (no ALLOWED_REGISTRIES or BLOCKED_REGISTRIES configured, denying all)", registry) + } + return nil +} + +// Package-level functions for backward compatibility (deprecated - use Validator instead) + +// IsRegistryAllowed checks if a registry is allowed (deprecated: use Validator.IsRegistryAllowed). +func IsRegistryAllowed(registry string) bool { + v := NewValidator() + return v.IsRegistryAllowed(registry) +} + +// ValidateImageRegistry validates that an image reference uses an allowed registry (deprecated: use Validator.ValidateImageRegistry). +func ValidateImageRegistry(imageRef string) error { + v := NewValidator() + return v.ValidateImageRegistry(imageRef) +} diff --git a/internal/registry/validation_test.go b/internal/registry/validation_test.go new file mode 100644 index 00000000..7ea738e8 --- /dev/null +++ b/internal/registry/validation_test.go @@ -0,0 +1,285 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package registry + +import ( + "testing" +) + +func TestExtractRegistryDomain(t *testing.T) { + tests := []struct { + name string + imageRef string + want string + }{ + { + name: "ghcr.io with tag", + imageRef: "ghcr.io/ironcore-dev/os-images/gardenlinux:1877.0", + want: "ghcr.io", + }, + { + name: "keppel with tag", + imageRef: "keppel.global.cloud.sap/ironcore/gardenlinux-iso:arm64", + want: "keppel.global.cloud.sap", + }, + { + name: "docker hub explicit", + imageRef: "docker.io/library/ubuntu:latest", + want: "docker.io", + }, + { + name: "docker hub implicit with namespace", + imageRef: "library/ubuntu:latest", + want: "registry-1.docker.io", + }, + { + name: "docker hub implicit without namespace", + imageRef: "ubuntu:latest", + want: "registry-1.docker.io", + }, + { + name: "localhost with port", + imageRef: "localhost:5000/test-image:latest", + want: "localhost:5000", + }, + { + name: "registry with port", + imageRef: "registry.example.com:8080/namespace/image:tag", + want: "registry.example.com:8080", + }, + { + name: "no tag no digest", + imageRef: "ghcr.io/namespace/image", + want: "ghcr.io", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ExtractRegistryDomain(tt.imageRef) + if got != tt.want { + t.Errorf("ExtractRegistryDomain(%q) = %q, want %q", tt.imageRef, got, tt.want) + } + }) + } +} + +func TestIsRegistryAllowed(t *testing.T) { + tests := []struct { + name string + registry string + allowList string + blockList string + want bool + description string + }{ + { + name: "allowed registry - ghcr.io", + registry: "ghcr.io", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + want: true, + description: "ghcr.io is in allow list", + }, + { + name: "allowed registry - keppel", + registry: "keppel.global.cloud.sap", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + want: true, + description: "keppel.global.cloud.sap is in allow list", + }, + { + name: "blocked registry - docker.io with allow list", + registry: "docker.io", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + want: false, + description: "docker.io is NOT in allow list", + }, + { + name: "allowed with block list - ghcr.io", + registry: "ghcr.io", + allowList: "", + blockList: "docker.io,registry-1.docker.io", + want: true, + description: "ghcr.io is NOT in block list", + }, + { + name: "blocked with block list - docker.io", + registry: "docker.io", + allowList: "", + blockList: "docker.io,registry-1.docker.io", + want: false, + description: "docker.io is in block list", + }, + { + name: "blocked with block list - registry-1.docker.io", + registry: "registry-1.docker.io", + allowList: "", + blockList: "docker.io,registry-1.docker.io", + want: false, + description: "registry-1.docker.io is in block list", + }, + { + name: "deny all - no lists configured", + registry: "ghcr.io", + allowList: "", + blockList: "", + want: false, + description: "fail-closed: deny all when neither list is set", + }, + { + name: "allow list takes precedence", + registry: "ghcr.io", + allowList: "ghcr.io", + blockList: "ghcr.io", + want: true, + description: "allow list takes precedence over block list", + }, + { + name: "whitespace handling", + registry: "ghcr.io", + allowList: " ghcr.io , keppel.global.cloud.sap ", + blockList: "", + want: true, + description: "handles whitespace in allow list", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.allowList != "" { + t.Setenv("ALLOWED_REGISTRIES", tt.allowList) + } + if tt.blockList != "" { + t.Setenv("BLOCKED_REGISTRIES", tt.blockList) + } + + got := IsRegistryAllowed(tt.registry) + if got != tt.want { + t.Errorf("IsRegistryAllowed(%q) = %v, want %v (%s)", tt.registry, got, tt.want, tt.description) + } + }) + } +} + +func TestValidateImageRegistry(t *testing.T) { + tests := []struct { + name string + imageRef string + allowList string + blockList string + wantErr bool + errContains string + description string + }{ + { + name: "allowed - ghcr.io image", + imageRef: "ghcr.io/ironcore-dev/os-images/gardenlinux:1877.0", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + wantErr: false, + description: "ghcr.io image should be allowed", + }, + { + name: "allowed - keppel image", + imageRef: "keppel.global.cloud.sap/ironcore/gardenlinux-iso:arm64", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + wantErr: false, + description: "keppel.global.cloud.sap image should be allowed", + }, + { + name: "blocked - docker.io with allow list", + imageRef: "docker.io/library/ubuntu:latest", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + wantErr: true, + errContains: "registry not allowed: docker.io", + description: "docker.io should be blocked when not in allow list", + }, + { + name: "blocked - implicit docker hub", + imageRef: "ubuntu:latest", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + wantErr: true, + errContains: "registry not allowed: registry-1.docker.io", + description: "implicit docker hub should be blocked", + }, + { + name: "error shows allowed registries", + imageRef: "docker.io/library/alpine:latest", + allowList: "ghcr.io,keppel.global.cloud.sap", + blockList: "", + wantErr: true, + errContains: "allowed registries: ghcr.io,keppel.global.cloud.sap", + description: "error message should show the allowed registries", + }, + { + name: "blocked with block list", + imageRef: "docker.io/library/nginx:latest", + allowList: "", + blockList: "docker.io,registry-1.docker.io", + wantErr: true, + errContains: "registry blocked: docker.io", + description: "docker.io should be blocked when in block list", + }, + { + name: "error shows blocked registries", + imageRef: "registry-1.docker.io/library/redis:latest", + allowList: "", + blockList: "docker.io,registry-1.docker.io", + wantErr: true, + errContains: "blocked registries: docker.io,registry-1.docker.io", + description: "error message should show the blocked registries", + }, + { + name: "fail-closed no configuration", + imageRef: "ghcr.io/test/image:latest", + allowList: "", + blockList: "", + wantErr: true, + errContains: "no ALLOWED_REGISTRIES or BLOCKED_REGISTRIES configured", + description: "should deny all when no configuration", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.allowList != "" { + t.Setenv("ALLOWED_REGISTRIES", tt.allowList) + } + if tt.blockList != "" { + t.Setenv("BLOCKED_REGISTRIES", tt.blockList) + } + + err := ValidateImageRegistry(tt.imageRef) + if (err != nil) != tt.wantErr { + t.Errorf("ValidateImageRegistry(%q) error = %v, wantErr %v (%s)", tt.imageRef, err, tt.wantErr, tt.description) + return + } + if tt.wantErr && tt.errContains != "" { + if err == nil || !containsString(err.Error(), tt.errContains) { + t.Errorf("ValidateImageRegistry(%q) error = %v, should contain %q (%s)", tt.imageRef, err, tt.errContains, tt.description) + } + } + }) + } +} + +// Helper function to check if a string contains a substring +func containsString(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && containsSubstring(s, substr)) +} + +func containsSubstring(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index 9095fc16..c9c539ec 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -15,6 +15,7 @@ import ( "sync" "github.com/go-logr/logr" + "github.com/ironcore-dev/boot-operator/internal/registry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -29,8 +30,8 @@ type AuthMethod int const ( AuthNone AuthMethod = iota // Anonymous access - AuthBasic // Basic username:password - AuthBearer // Bearer token via /token endpoint + AuthBasic // Basic username:password + AuthBearer // Bearer token via /token endpoint ) type RegistryInfo struct { @@ -56,65 +57,11 @@ type ImageDetails struct { var registryCache = make(map[string]*RegistryInfo) var registryCacheMutex sync.RWMutex -// Extract registry domain from OCI image reference -func extractRegistryDomain(imageRef string) string { - parts := strings.SplitN(imageRef, "/", 2) - if len(parts) < 2 { - // No slash means Docker Hub implicit - return "registry-1.docker.io" - } - - // Check if first part looks like a domain (has dot or colon) - if strings.Contains(parts[0], ".") || strings.Contains(parts[0], ":") { - return parts[0] - } - - // Implicit Docker Hub (e.g., "library/ubuntu") - return "registry-1.docker.io" -} - // Extract repository path without registry func extractRepository(imageRef, registryDomain string) string { return strings.TrimPrefix(imageRef, registryDomain+"/") } -// Check if registry is in a comma-separated list (exact match only) -func isInList(registry string, list string) bool { - if list == "" { - return false - } - - items := strings.Split(list, ",") - for _, item := range items { - if strings.TrimSpace(item) == registry { - return true - } - } - return false -} - -// Check if registry is allowed based on allow/block lists -// - If ALLOWED_REGISTRIES is set: only those registries are allowed (whitelist) -// - Else if BLOCKED_REGISTRIES is set: all except those are allowed (blacklist) -// - Else: DENY ALL - operator must explicitly configure registry policy -func isRegistryAllowed(registry string) bool { - allowList := os.Getenv("ALLOWED_REGISTRIES") - blockList := os.Getenv("BLOCKED_REGISTRIES") - - // Allow list takes precedence (more restrictive) - if allowList != "" { - return isInList(registry, allowList) - } - - // Block list mode: allow all except blocked - if blockList != "" { - return !isInList(registry, blockList) - } - - // Default: deny all - require explicit configuration - return false -} - // Parse WWW-Authenticate parameter value func extractParam(header, param string) string { start := strings.Index(header, param+"=\"") @@ -154,7 +101,7 @@ func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error if err != nil { return nil, fmt.Errorf("registry unreachable: %w", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() info := &RegistryInfo{Domain: registryDomain} @@ -220,7 +167,7 @@ func getBearerToken(tokenURL string) (string, error) { if err != nil { return "", err } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() body, err := io.ReadAll(resp.Body) if err != nil { @@ -278,7 +225,7 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { digestSegment := segments[len(segments)-1] // Extract registry domain and repository - registryDomain := extractRegistryDomain(imageName) + registryDomain := registry.ExtractRegistryDomain(imageName) repositoryName := extractRepository(imageName, registryDomain) digestSegment = strings.TrimSuffix(digestSegment, ".efi") @@ -297,23 +244,22 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { } func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails *ImageDetails, log logr.Logger) { - registry := imageDetails.RegistryDomain + registryDomain := imageDetails.RegistryDomain repository := imageDetails.RepositoryName - log.Info("Processing registry request", "registry", registry, "repository", repository, "digest", imageDetails.LayerDigest) + log.V(1).Info("Processing registry request", "registry", registryDomain, "repository", repository, "digest", imageDetails.LayerDigest) - // Security check - if !isRegistryAllowed(registry) { + if !registry.IsRegistryAllowed(registryDomain) { http.Error(w, "Forbidden: Registry not allowed", http.StatusForbidden) - log.Info("Registry blocked", "registry", registry, "allowList", os.Getenv("ALLOWED_REGISTRIES"), "blockList", os.Getenv("BLOCKED_REGISTRIES")) + log.Info("Registry blocked", "registry", registryDomain, "allowList", os.Getenv("ALLOWED_REGISTRIES"), "blockList", os.Getenv("BLOCKED_REGISTRIES")) return } // Auto-detect auth method (with caching) - registryInfo, err := getOrDetectRegistry(registry, repository) + registryInfo, err := getOrDetectRegistry(registryDomain, repository) if err != nil { http.Error(w, "Registry detection failed", http.StatusBadGateway) - log.Error(err, "Failed to detect registry", "registry", registry) + log.Error(err, "Failed to detect registry", "registry", registryDomain) return } @@ -327,19 +273,19 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * log.Error(err, "Failed to get bearer token", "tokenURL", registryInfo.TokenURL) return } - log.V(1).Info("Obtained bearer token", "registry", registry) + log.V(1).Info("Obtained bearer token", "registry", registryDomain) case AuthBasic: // TODO: Support basic auth with credentials from secrets http.Error(w, "Basic auth not yet implemented", http.StatusNotImplemented) - log.Info("Basic auth required but not yet supported", "registry", registry) + log.Info("Basic auth required but not yet supported", "registry", registryDomain) return case AuthNone: - log.V(1).Info("Registry allows anonymous access", "registry", registry) + log.V(1).Info("Registry allows anonymous access", "registry", registryDomain) } // Proxy the blob request digest := imageDetails.LayerDigest - targetURL := fmt.Sprintf("https://%s/v2/%s/blobs/%s", registry, repository, digest) + targetURL := fmt.Sprintf("https://%s/v2/%s/blobs/%s", registryDomain, repository, digest) proxyURL, _ := url.Parse(targetURL) proxy := &httputil.ReverseProxy{ @@ -439,9 +385,9 @@ func parseImageURL(queries url.Values) (imageDetails ImageDetails, err error) { } ociImageName = strings.TrimSuffix(ociImageName, ".efi") - + // Extract registry domain and repository - registryDomain := extractRegistryDomain(ociImageName) + registryDomain := registry.ExtractRegistryDomain(ociImageName) repositoryName := extractRepository(ociImageName, registryDomain) return ImageDetails{ From b446367047de99965251f995ca4fd1cb027430f0 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Tue, 24 Feb 2026 20:11:55 +0000 Subject: [PATCH 03/14] Fixes after updates from main --- ...serverbootconfiguration_http_controller.go | 13 +- ...rbootconfiguration_http_controller_test.go | 2 +- .../serverbootconfiguration_pxe_controller.go | 17 +- ...erbootconfiguration_pxe_controller_test.go | 4 +- internal/controller/suite_test.go | 26 +- test/registry/registry.go | 242 ++++++++++++++++++ 6 files changed, 284 insertions(+), 20 deletions(-) create mode 100644 test/registry/registry.go diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index 3601300b..78860307 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -200,18 +200,21 @@ func (r *ServerBootConfigurationHTTPReconciler) getSystemNetworkIDsFromServer(ct } func (r *ServerBootConfigurationHTTPReconciler) constructUKIURL(ctx context.Context, image string) (string, error) { - imageDetails := strings.Split(image, ":") - if len(imageDetails) != 2 { - return "", fmt.Errorf("invalid image format") + // Parse image reference - split on last ':' to handle registry:port/image:tag format + lastColon := strings.LastIndex(image, ":") + if lastColon == -1 { + return "", fmt.Errorf("invalid image format: missing tag") } + imageName := image[:lastColon] + imageVersion := image[lastColon+1:] - ukiDigest, err := r.getUKIDigestFromNestedManifest(ctx, imageDetails[0], imageDetails[1]) + ukiDigest, err := r.getUKIDigestFromNestedManifest(ctx, imageName, imageVersion) if err != nil { return "", fmt.Errorf("failed to fetch UKI layer digest: %w", err) } ukiDigest = strings.TrimPrefix(ukiDigest, "sha256:") - ukiURL := fmt.Sprintf("%s/%s/sha256-%s.efi", r.ImageServerURL, imageDetails[0], ukiDigest) + ukiURL := fmt.Sprintf("%s/%s/sha256-%s.efi", r.ImageServerURL, imageName, ukiDigest) return ukiURL, nil } diff --git a/internal/controller/serverbootconfiguration_http_controller_test.go b/internal/controller/serverbootconfiguration_http_controller_test.go index 1f80c47c..64049023 100644 --- a/internal/controller/serverbootconfiguration_http_controller_test.go +++ b/internal/controller/serverbootconfiguration_http_controller_test.go @@ -65,7 +65,7 @@ var _ = Describe("ServerBootConfiguration Controller", func() { ServerRef: corev1.LocalObjectReference{ Name: server.Name, }, - Image: "ghcr.io/ironcore-dev/os-images/test-image:100.1", + Image: MockImageRef("ironcore-dev/os-images/test-image", "100.1"), IgnitionSecretRef: &corev1.LocalObjectReference{Name: "foo"}, }, } diff --git a/internal/controller/serverbootconfiguration_pxe_controller.go b/internal/controller/serverbootconfiguration_pxe_controller.go index 67cbbca7..00ca7894 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller.go +++ b/internal/controller/serverbootconfiguration_pxe_controller.go @@ -223,19 +223,22 @@ func (r *ServerBootConfigurationPXEReconciler) getSystemIPFromBootConfig(ctx con } func (r *ServerBootConfigurationPXEReconciler) getImageDetailsFromConfig(ctx context.Context, config *metalv1alpha1.ServerBootConfiguration) (string, string, string, error) { - imageDetails := strings.Split(config.Spec.Image, ":") - if len(imageDetails) != 2 { - return "", "", "", fmt.Errorf("invalid image format") + // Parse image reference - split on last ':' to handle registry:port/image:tag format + lastColon := strings.LastIndex(config.Spec.Image, ":") + if lastColon == -1 { + return "", "", "", fmt.Errorf("invalid image format: missing tag") } + imageName := config.Spec.Image[:lastColon] + imageVersion := config.Spec.Image[lastColon+1:] - kernelDigest, initrdDigest, squashFSDigest, err := r.getLayerDigestsFromNestedManifest(ctx, imageDetails[0], imageDetails[1]) + kernelDigest, initrdDigest, squashFSDigest, err := r.getLayerDigestsFromNestedManifest(ctx, imageName, imageVersion) if err != nil { return "", "", "", fmt.Errorf("failed to fetch layer digests: %w", err) } - kernelURL := fmt.Sprintf("%s/image?imageName=%s&version=%s&layerDigest=%s", r.IPXEServiceURL, imageDetails[0], imageDetails[1], kernelDigest) - initrdURL := fmt.Sprintf("%s/image?imageName=%s&version=%s&layerDigest=%s", r.IPXEServiceURL, imageDetails[0], imageDetails[1], initrdDigest) - squashFSURL := fmt.Sprintf("%s/image?imageName=%s&version=%s&layerDigest=%s", r.IPXEServiceURL, imageDetails[0], imageDetails[1], squashFSDigest) + kernelURL := fmt.Sprintf("%s/image?imageName=%s&version=%s&layerDigest=%s", r.IPXEServiceURL, imageName, imageVersion, kernelDigest) + initrdURL := fmt.Sprintf("%s/image?imageName=%s&version=%s&layerDigest=%s", r.IPXEServiceURL, imageName, imageVersion, initrdDigest) + squashFSURL := fmt.Sprintf("%s/image?imageName=%s&version=%s&layerDigest=%s", r.IPXEServiceURL, imageName, imageVersion, squashFSDigest) return kernelURL, initrdURL, squashFSURL, nil } diff --git a/internal/controller/serverbootconfiguration_pxe_controller_test.go b/internal/controller/serverbootconfiguration_pxe_controller_test.go index 604862cd..59daf307 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller_test.go +++ b/internal/controller/serverbootconfiguration_pxe_controller_test.go @@ -64,7 +64,7 @@ var _ = Describe("ServerBootConfiguration Controller", func() { ServerRef: corev1.LocalObjectReference{ Name: server.Name, }, - Image: "ghcr.io/ironcore-dev/os-images/gardenlinux:1877.0", + Image: MockImageRef("ironcore-dev/os-images/gardenlinux", "1877.0"), IgnitionSecretRef: &corev1.LocalObjectReference{Name: "foo"}, }, } @@ -125,7 +125,7 @@ var _ = Describe("ServerBootConfiguration Controller", func() { ServerRef: corev1.LocalObjectReference{ Name: server.Name, }, - Image: "ghcr.io/gardenlinux/gardenlinux:1772.0", + Image: MockImageRef("gardenlinux/gardenlinux", "1772.0"), IgnitionSecretRef: &corev1.LocalObjectReference{Name: "foo"}, }, } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 614906e8..81a82c83 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -36,6 +36,7 @@ import ( bootv1alpha1 "github.com/ironcore-dev/boot-operator/api/v1alpha1" "github.com/ironcore-dev/boot-operator/internal/registry" + testregistry "github.com/ironcore-dev/boot-operator/test/registry" //+kubebuilder:scaffold:imports ) @@ -46,9 +47,10 @@ const ( ) var ( - cfg *rest.Config - k8sClient client.Client - testEnv *envtest.Environment + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + mockRegistry *testregistry.MockRegistry ) func TestControllers(t *testing.T) { @@ -64,8 +66,17 @@ func TestControllers(t *testing.T) { var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - // Set ALLOWED_REGISTRIES for tests to use ghcr.io images - Expect(os.Setenv("ALLOWED_REGISTRIES", "ghcr.io")).To(Succeed()) + By("starting mock OCI registry") + mockRegistry = testregistry.NewMockRegistry() + DeferCleanup(mockRegistry.Close) + + // Push test images to mock registry (using simple paths without localhost prefix) + Expect(mockRegistry.PushPXEImage("ironcore-dev/os-images/gardenlinux", "1877.0", runtime.GOARCH)).To(Succeed()) + Expect(mockRegistry.PushPXEImageOldFormat("gardenlinux/gardenlinux", "1772.0", runtime.GOARCH)).To(Succeed()) + Expect(mockRegistry.PushHTTPImage("ironcore-dev/os-images/test-image", "100.1")).To(Succeed()) + + // Set ALLOWED_REGISTRIES to use mock registry + Expect(os.Setenv("ALLOWED_REGISTRIES", mockRegistry.RegistryAddress())).To(Succeed()) DeferCleanup(func() { Expect(os.Unsetenv("ALLOWED_REGISTRIES")).To(Succeed()) }) @@ -165,3 +176,8 @@ func SetupTest() *corev1.Namespace { return ns } + +// MockImageRef returns a fully qualified image reference for the mock registry +func MockImageRef(name, tag string) string { + return fmt.Sprintf("%s/%s:%s", mockRegistry.RegistryAddress(), name, tag) +} diff --git a/test/registry/registry.go b/test/registry/registry.go new file mode 100644 index 00000000..23f076bd --- /dev/null +++ b/test/registry/registry.go @@ -0,0 +1,242 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package registry + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + + "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" +) + +const ( + MediaTypeKernel = "application/vnd.ironcore.image.kernel" + MediaTypeInitrd = "application/vnd.ironcore.image.initramfs" + MediaTypeSquashfs = "application/vnd.ironcore.image.squashfs" + MediaTypeUKI = "application/vnd.ironcore.image.uki" + MediaTypeKernelOld = "application/io.gardenlinux.kernel" + MediaTypeInitrdOld = "application/io.gardenlinux.initrd" +) + +// MockRegistry provides an in-memory OCI registry for testing +type MockRegistry struct { + mu sync.RWMutex + manifests map[string]ocispec.Manifest // Key: "name:tag" or "name@digest" + manifestsByDigest map[digest.Digest]ocispec.Manifest // For digest lookups + indexes map[string]ocispec.Index + blobs map[digest.Digest][]byte + server *httptest.Server +} + +// NewMockRegistry creates a new mock OCI registry server +func NewMockRegistry() *MockRegistry { + r := &MockRegistry{ + manifests: make(map[string]ocispec.Manifest), + manifestsByDigest: make(map[digest.Digest]ocispec.Manifest), + indexes: make(map[string]ocispec.Index), + blobs: make(map[digest.Digest][]byte), + } + + mux := http.NewServeMux() + + // OCI Distribution API endpoints + mux.HandleFunc("/v2/", func(w http.ResponseWriter, req *http.Request) { + if req.URL.Path == "/v2/" { + // Version check endpoint + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]string{"version": "2.0"}) + return + } + + // Manifest endpoint + if strings.Contains(req.URL.Path, "/manifests/") { + r.handleManifest(w, req) + return + } + + http.NotFound(w, req) + }) + + r.server = httptest.NewServer(mux) + return r +} + +// Close shuts down the mock registry server +func (r *MockRegistry) Close() { + r.server.Close() +} + +// URL returns the base URL of the mock registry +func (r *MockRegistry) URL() string { + return r.server.URL +} + +// RegistryAddress returns the registry address without http:// prefix +func (r *MockRegistry) RegistryAddress() string { + return strings.TrimPrefix(r.URL(), "http://") +} + +// pushPXEManifest is a helper to store PXE manifests with given media types +func (r *MockRegistry) pushPXEManifest(name, tag string, kernelMedia, initrdMedia string) { + kernelDigest := digest.FromString(fmt.Sprintf("kernel-%s-%s", name, tag)) + initrdDigest := digest.FromString(fmt.Sprintf("initrd-%s-%s", name, tag)) + squashfsDigest := digest.FromString(fmt.Sprintf("squashfs-%s-%s", name, tag)) + configDigest := digest.FromString(fmt.Sprintf("config-%s-%s", name, tag)) + + manifest := ocispec.Manifest{ + MediaType: ocispec.MediaTypeImageManifest, + Config: ocispec.Descriptor{ + MediaType: "application/vnd.oci.image.config.v1+json", + Digest: configDigest, + Size: 2, + }, + Layers: []ocispec.Descriptor{ + { + MediaType: kernelMedia, + Digest: kernelDigest, + Size: 1024, + }, + { + MediaType: initrdMedia, + Digest: initrdDigest, + Size: 2048, + }, + { + MediaType: MediaTypeSquashfs, + Digest: squashfsDigest, + Size: 4096, + }, + }, + } + + ref := fmt.Sprintf("%s:%s", name, tag) + r.manifests[ref] = manifest + + // Calculate and store manifest digest + manifestBytes, _ := json.Marshal(manifest) + manifestDigest := digest.FromBytes(manifestBytes) + r.manifestsByDigest[manifestDigest] = manifest + + r.blobs[manifest.Config.Digest] = []byte("{}") + r.blobs[kernelDigest] = []byte("kernel-data") + r.blobs[initrdDigest] = []byte("initrd-data") + r.blobs[squashfsDigest] = []byte("squashfs-data") +} + +// PushPXEImage adds a PXE boot image with kernel, initrd, and squashfs layers +func (r *MockRegistry) PushPXEImage(name, tag, architecture string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.pushPXEManifest(name, tag, MediaTypeKernel, MediaTypeInitrd) + return nil +} + +// PushPXEImageOldFormat adds a PXE boot image using old Gardenlinux media types +func (r *MockRegistry) PushPXEImageOldFormat(name, tag, architecture string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.pushPXEManifest(name, tag, MediaTypeKernelOld, MediaTypeInitrdOld) + return nil +} + +// PushHTTPImage adds an HTTP boot image with UKI layer +func (r *MockRegistry) PushHTTPImage(name, tag string) error { + r.mu.Lock() + defer r.mu.Unlock() + + ukiDigest := digest.FromString(fmt.Sprintf("uki-%s-%s", name, tag)) + configDigest := digest.FromString(fmt.Sprintf("config-%s-%s", name, tag)) + + manifest := ocispec.Manifest{ + MediaType: ocispec.MediaTypeImageManifest, + Config: ocispec.Descriptor{ + MediaType: "application/vnd.oci.image.config.v1+json", + Digest: configDigest, + Size: 2, + }, + Layers: []ocispec.Descriptor{ + { + MediaType: MediaTypeUKI, + Digest: ukiDigest, + Size: 8192, + }, + }, + } + + ref := fmt.Sprintf("%s:%s", name, tag) + r.manifests[ref] = manifest + + // Calculate and store manifest digest + manifestBytes, _ := json.Marshal(manifest) + manifestDigest := digest.FromBytes(manifestBytes) + r.manifestsByDigest[manifestDigest] = manifest + + r.blobs[manifest.Config.Digest] = []byte("{}") + r.blobs[ukiDigest] = []byte("uki-data") + + return nil +} + +func (r *MockRegistry) handleManifest(w http.ResponseWriter, req *http.Request) { + // Match pattern: /v2/{name}/manifests/{reference} + parts := strings.Split(strings.TrimPrefix(req.URL.Path, "/v2/"), "/manifests/") + if len(parts) != 2 { + http.NotFound(w, req) + return + } + + name := parts[0] + reference := parts[1] + + r.mu.RLock() + defer r.mu.RUnlock() + + var manifest ocispec.Manifest + var exists bool + var manifestDigest string + + // Check if reference is a digest (sha256:...) + if strings.HasPrefix(reference, "sha256:") { + // Look up by digest + dgst, err := digest.Parse(reference) + if err != nil { + http.Error(w, "invalid digest", http.StatusBadRequest) + return + } + manifest, exists = r.manifestsByDigest[dgst] + manifestDigest = reference + } else { + // Look up by tag + imageRef := fmt.Sprintf("%s:%s", name, reference) + manifest, exists = r.manifests[imageRef] + // Calculate digest for Content-Digest header + manifestBytes, _ := json.Marshal(manifest) + dgst := digest.FromBytes(manifestBytes) + manifestDigest = dgst.String() + } + + if !exists { + http.Error(w, "manifest not found", http.StatusNotFound) + return + } + + if req.Method == http.MethodHead { + w.Header().Set("Content-Type", ocispec.MediaTypeImageManifest) + w.Header().Set("Docker-Content-Digest", manifestDigest) + w.WriteHeader(http.StatusOK) + return + } + + manifestData, _ := json.Marshal(manifest) + w.Header().Set("Content-Type", ocispec.MediaTypeImageManifest) + w.Header().Set("Docker-Content-Digest", manifestDigest) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(manifestData) +} From bc5bd40560abc945a54ae71d50f728e20806ba8f Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Wed, 25 Feb 2026 14:20:04 +0000 Subject: [PATCH 04/14] Fixes for CodeRabbit comments --- cmd/main.go | 2 +- go.mod | 3 +- go.sum | 2 + .../controller/serverbootconfig_helpers.go | 210 ++++++++++++++++++ ...serverbootconfiguration_http_controller.go | 95 +------- .../serverbootconfiguration_pxe_controller.go | 144 +----------- internal/controller/suite_test.go | 1 + internal/registry/validation.go | 39 ++-- internal/registry/validation_test.go | 22 +- server/imageproxyserver.go | 45 ++-- test/registry/registry.go | 15 +- 11 files changed, 295 insertions(+), 283 deletions(-) create mode 100644 internal/controller/serverbootconfig_helpers.go diff --git a/cmd/main.go b/cmd/main.go index bc34f5c5..dadb0ce7 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -318,7 +318,7 @@ func main() { }() setupLog.Info("starting image-proxy-server") - go bootserver.RunImageProxyServer(imageProxyServerAddr, mgr.GetClient(), serverLog.WithName("imageproxyserver")) + go bootserver.RunImageProxyServer(imageProxyServerAddr, mgr.GetClient(), registryValidator, serverLog.WithName("imageproxyserver")) setupLog.Info("starting manager") if err := mgr.Start(ctx); err != nil { diff --git a/go.mod b/go.mod index dbc37970..7a06664b 100644 --- a/go.mod +++ b/go.mod @@ -5,12 +5,14 @@ go 1.25.6 require ( github.com/containerd/containerd v1.7.30 github.com/coreos/butane v0.26.0 + github.com/distribution/reference v0.6.0 github.com/go-logr/logr v1.4.3 github.com/ironcore-dev/controller-utils v0.11.0 github.com/ironcore-dev/metal v0.0.0-20240624131301-18385f342755 github.com/ironcore-dev/metal-operator v0.3.0 github.com/onsi/ginkgo/v2 v2.28.1 github.com/onsi/gomega v1.39.1 + github.com/opencontainers/go-digest v1.0.0 github.com/opencontainers/image-spec v1.1.1 github.com/spf13/cobra v1.10.2 k8s.io/api v0.35.0 @@ -76,7 +78,6 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect - github.com/opencontainers/go-digest v1.0.0 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/client_golang v1.23.2 // indirect diff --git a/go.sum b/go.sum index 66513e17..f6a076ed 100644 --- a/go.sum +++ b/go.sum @@ -51,6 +51,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= +github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5Y4f/wlDRiLyi3E= github.com/emicklei/go-restful/v3 v3.13.0 h1:C4Bl2xDndpU6nJ4bc1jXd+uTmYPVUwkD6bFY/oTyCes= github.com/emicklei/go-restful/v3 v3.13.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc= github.com/evanphx/json-patch v0.5.2 h1:xVCHIVMUu1wtM/VkR9jVZ45N3FhZfYMMYGorLCR8P3k= diff --git a/internal/controller/serverbootconfig_helpers.go b/internal/controller/serverbootconfig_helpers.go new file mode 100644 index 00000000..0deb7fa8 --- /dev/null +++ b/internal/controller/serverbootconfig_helpers.go @@ -0,0 +1,210 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "encoding/json" + "fmt" + "io" + "strings" + + "github.com/containerd/containerd/remotes" + "github.com/distribution/reference" + metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +// ParseImageReference parses an OCI image reference and returns the image name and version. +// It handles tagged references, digest references, and untagged references (defaulting to "latest"). +func ParseImageReference(image string) (imageName, imageVersion string, err error) { + named, err := reference.ParseNormalizedNamed(image) + if err != nil { + return "", "", fmt.Errorf("invalid image reference: %w", err) + } + + if tagged, ok := named.(reference.Tagged); ok { + imageName = reference.FamiliarName(named) + imageVersion = tagged.Tag() + } else if canonical, ok := named.(reference.Canonical); ok { + imageName = reference.FamiliarName(named) + imageVersion = canonical.Digest().String() + } else { + // No tag or digest, use "latest" as default + imageName = reference.FamiliarName(named) + imageVersion = "latest" + } + + return imageName, imageVersion, nil +} + +// FindManifestByArchitecture navigates an OCI image index to find the manifest for a specific architecture. +// If enableCNAMECompat is true, it first tries to find manifests using the legacy CNAME annotation approach. +// Returns the architecture-specific manifest, or an error if not found. +func FindManifestByArchitecture(ctx context.Context, resolver remotes.Resolver, name string, desc ocispec.Descriptor, architecture string, enableCNAMECompat bool) (ocispec.Manifest, error) { + manifestData, err := fetchContent(ctx, resolver, name, desc) + if err != nil { + return ocispec.Manifest{}, fmt.Errorf("failed to fetch manifest data: %w", err) + } + + var manifest ocispec.Manifest + if err := json.Unmarshal(manifestData, &manifest); err != nil { + return ocispec.Manifest{}, fmt.Errorf("failed to unmarshal manifest: %w", err) + } + + // If not an index, return the manifest directly + if desc.MediaType != ocispec.MediaTypeImageIndex { + return manifest, nil + } + + // Parse as index and find architecture-specific manifest + var indexManifest ocispec.Index + if err := json.Unmarshal(manifestData, &indexManifest); err != nil { + return ocispec.Manifest{}, fmt.Errorf("failed to unmarshal index manifest: %w", err) + } + + var targetManifestDesc ocispec.Descriptor + + // Backward compatibility for CNAME prefix based OCI (PXE only) + if enableCNAMECompat { + for _, m := range indexManifest.Manifests { + if strings.HasPrefix(m.Annotations["cname"], CNAMEPrefixMetalPXE) { + if m.Annotations["architecture"] == architecture { + targetManifestDesc = m + break + } + } + } + } + + // Standard platform-based architecture lookup + if targetManifestDesc.Digest == "" { + for _, m := range indexManifest.Manifests { + if m.Platform != nil && m.Platform.Architecture == architecture { + targetManifestDesc = m + break + } + } + } + + if targetManifestDesc.Digest == "" { + return ocispec.Manifest{}, fmt.Errorf("failed to find target manifest with architecture %s", architecture) + } + + // Fetch the nested manifest + nestedData, err := fetchContent(ctx, resolver, name, targetManifestDesc) + if err != nil { + return ocispec.Manifest{}, fmt.Errorf("failed to fetch nested manifest: %w", err) + } + + var nestedManifest ocispec.Manifest + if err := json.Unmarshal(nestedData, &nestedManifest); err != nil { + return ocispec.Manifest{}, fmt.Errorf("failed to unmarshal nested manifest: %w", err) + } + + return nestedManifest, nil +} + +// ExtractServerNetworkIDs extracts IP addresses (and optionally MAC addresses) from a Server's network interfaces. +// Returns a slice of IP addresses as strings. If includeMACAddresses is true, MAC addresses are also included. +func ExtractServerNetworkIDs(server *metalv1alpha1.Server, includeMACAddresses bool) []string { + ids := make([]string, 0, len(server.Status.NetworkInterfaces)) + + for _, nic := range server.Status.NetworkInterfaces { + // Add IPs + if len(nic.IPs) > 0 { + for _, ip := range nic.IPs { + ids = append(ids, ip.String()) + } + } else if nic.IP != nil && !nic.IP.IsZero() { + ids = append(ids, nic.IP.String()) + } + + // Add MAC address if requested + if includeMACAddresses && nic.MACAddress != "" { + ids = append(ids, nic.MACAddress) + } + } + + return ids +} + +// EnqueueServerBootConfigsReferencingSecret finds all ServerBootConfigurations in the same namespace +// that reference the given Secret via IgnitionSecretRef and returns reconcile requests for them. +func EnqueueServerBootConfigsReferencingSecret(ctx context.Context, c client.Client, secret client.Object) []reconcile.Request { + log := ctrl.LoggerFrom(ctx) + secretObj, ok := secret.(*corev1.Secret) + if !ok { + log.Error(nil, "Failed to decode object into Secret", "object", secret) + return nil + } + + bootConfigList := &metalv1alpha1.ServerBootConfigurationList{} + if err := c.List(ctx, bootConfigList, client.InNamespace(secretObj.Namespace)); err != nil { + log.Error(err, "Failed to list ServerBootConfiguration for Secret", "Secret", client.ObjectKeyFromObject(secretObj)) + return nil + } + + var requests []reconcile.Request + for _, bootConfig := range bootConfigList.Items { + if bootConfig.Spec.IgnitionSecretRef != nil && bootConfig.Spec.IgnitionSecretRef.Name == secretObj.Name { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: bootConfig.Name, + Namespace: bootConfig.Namespace, + }, + }) + } + } + return requests +} + +// fetchContent fetches the content of an OCI descriptor using the provided resolver. +// It validates the content size matches the descriptor and returns the raw bytes. +func fetchContent(ctx context.Context, resolver remotes.Resolver, ref string, desc ocispec.Descriptor) ([]byte, error) { + fetcher, err := resolver.Fetcher(ctx, ref) + if err != nil { + return nil, fmt.Errorf("failed to get fetcher: %w", err) + } + + reader, err := fetcher.Fetch(ctx, desc) + if err != nil { + return nil, fmt.Errorf("failed to fetch content: %w", err) + } + + defer func() { + if cerr := reader.Close(); cerr != nil { + fmt.Printf("failed to close reader: %v\n", cerr) + } + }() + + data, err := io.ReadAll(reader) + if err != nil { + return nil, fmt.Errorf("failed to read content: %w", err) + } + + if int64(len(data)) != desc.Size { + return nil, fmt.Errorf("size mismatch: expected %d, got %d", desc.Size, len(data)) + } + + return data, nil +} diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index 78860307..d67d9a91 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -13,7 +13,6 @@ import ( "github.com/containerd/containerd/remotes/docker" "github.com/ironcore-dev/boot-operator/internal/registry" - ocispec "github.com/opencontainers/image-spec/specs-go/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -177,36 +176,21 @@ func (r *ServerBootConfigurationHTTPReconciler) getSystemUUIDFromServer(ctx cont return server.Spec.UUID, nil } -// getSystemNetworkIDsFromServer fetches the IPs from the network interfaces of the referenced Server object. +// getSystemNetworkIDsFromServer fetches the IPs and MAC addresses from the network interfaces of the referenced Server object. func (r *ServerBootConfigurationHTTPReconciler) getSystemNetworkIDsFromServer(ctx context.Context, config *metalv1alpha1.ServerBootConfiguration) ([]string, error) { server := &metalv1alpha1.Server{} if err := r.Get(ctx, client.ObjectKey{Name: config.Spec.ServerRef.Name}, server); err != nil { return nil, fmt.Errorf("failed to get Server: %w", err) } - nIDs := make([]string, 0, 2*len(server.Status.NetworkInterfaces)) - - for _, nic := range server.Status.NetworkInterfaces { - if len(nic.IPs) > 0 { - for _, ip := range nic.IPs { - nIDs = append(nIDs, ip.String()) - } - } else if nic.IP != nil && !nic.IP.IsZero() { - nIDs = append(nIDs, nic.IP.String()) - } - nIDs = append(nIDs, nic.MACAddress) - } - return nIDs, nil + return ExtractServerNetworkIDs(server, true), nil } func (r *ServerBootConfigurationHTTPReconciler) constructUKIURL(ctx context.Context, image string) (string, error) { - // Parse image reference - split on last ':' to handle registry:port/image:tag format - lastColon := strings.LastIndex(image, ":") - if lastColon == -1 { - return "", fmt.Errorf("invalid image format: missing tag") + imageName, imageVersion, err := ParseImageReference(image) + if err != nil { + return "", err } - imageName := image[:lastColon] - imageVersion := image[lastColon+1:] ukiDigest, err := r.getUKIDigestFromNestedManifest(ctx, imageName, imageVersion) if err != nil { @@ -220,10 +204,8 @@ func (r *ServerBootConfigurationHTTPReconciler) constructUKIURL(ctx context.Cont func (r *ServerBootConfigurationHTTPReconciler) getUKIDigestFromNestedManifest(ctx context.Context, imageName, imageVersion string) (string, error) { imageRef := fmt.Sprintf("%s:%s", imageName, imageVersion) - if r.RegistryValidator != nil { - if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { - return "", fmt.Errorf("registry validation failed: %w", err) - } + if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { + return "", fmt.Errorf("registry validation failed: %w", err) } resolver := docker.NewResolver(docker.ResolverOptions{}) @@ -232,42 +214,9 @@ func (r *ServerBootConfigurationHTTPReconciler) getUKIDigestFromNestedManifest(c return "", fmt.Errorf("failed to resolve image reference: %w", err) } - targetManifestDesc := desc - manifestData, err := fetchContent(ctx, resolver, name, desc) + manifest, err := FindManifestByArchitecture(ctx, resolver, name, desc, r.Architecture, false) if err != nil { - return "", fmt.Errorf("failed to fetch manifest data: %w", err) - } - - var manifest ocispec.Manifest - if desc.MediaType == ocispec.MediaTypeImageIndex { - var indexManifest ocispec.Index - if err := json.Unmarshal(manifestData, &indexManifest); err != nil { - return "", fmt.Errorf("failed to unmarshal index manifest: %w", err) - } - - for _, manifest := range indexManifest.Manifests { - platform := manifest.Platform - if manifest.Platform != nil && platform.Architecture == r.Architecture { - targetManifestDesc = manifest - break - } - } - if targetManifestDesc.Digest == "" { - return "", fmt.Errorf("failed to find target manifest with architecture %s", r.Architecture) - } - - nestedData, err := fetchContent(ctx, resolver, name, targetManifestDesc) - if err != nil { - return "", fmt.Errorf("failed to fetch nested manifest: %w", err) - } - - if err := json.Unmarshal(nestedData, &manifest); err != nil { - return "", fmt.Errorf("failed to unmarshal nested manifest: %w", err) - } - } else { - if err := json.Unmarshal(manifestData, &manifest); err != nil { - return "", fmt.Errorf("failed to unmarshal manifest: %w", err) - } + return "", err } for _, layer := range manifest.Layers { @@ -280,31 +229,7 @@ func (r *ServerBootConfigurationHTTPReconciler) getUKIDigestFromNestedManifest(c } func (r *ServerBootConfigurationHTTPReconciler) enqueueServerBootConfigReferencingIgnitionSecret(ctx context.Context, secret client.Object) []reconcile.Request { - log := ctrl.LoggerFrom(ctx) - secretObj, ok := secret.(*corev1.Secret) - if !ok { - log.Error(nil, "can't decode object into Secret", secret) - return nil - } - - bootConfigList := &metalv1alpha1.ServerBootConfigurationList{} - if err := r.List(ctx, bootConfigList, client.InNamespace(secretObj.Namespace)); err != nil { - log.Error(err, "failed to list ServerBootConfiguration for secret", secret) - return nil - } - - var requests []reconcile.Request - for _, bootConfig := range bootConfigList.Items { - if bootConfig.Spec.IgnitionSecretRef != nil && bootConfig.Spec.IgnitionSecretRef.Name == secretObj.Name { - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: bootConfig.Name, - Namespace: bootConfig.Namespace, - }, - }) - } - } - return requests + return EnqueueServerBootConfigsReferencingSecret(ctx, r.Client, secret) } // SetupWithManager sets up the controller with the Manager. diff --git a/internal/controller/serverbootconfiguration_pxe_controller.go b/internal/controller/serverbootconfiguration_pxe_controller.go index 00ca7894..06d7b4c7 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller.go +++ b/internal/controller/serverbootconfiguration_pxe_controller.go @@ -20,8 +20,6 @@ import ( "context" "encoding/json" "fmt" - "io" - "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -40,9 +38,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/containerd/containerd/remotes" "github.com/containerd/containerd/remotes/docker" - ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) const ( @@ -206,30 +202,14 @@ func (r *ServerBootConfigurationPXEReconciler) getSystemIPFromBootConfig(ctx con return nil, err } - systemIPs := make([]string, 0, len(server.Status.NetworkInterfaces)) - for _, nic := range server.Status.NetworkInterfaces { - if len(nic.IPs) > 0 { - for _, ip := range nic.IPs { - systemIPs = append(systemIPs, ip.String()) - } - continue - } - if nic.IP != nil && !nic.IP.IsZero() { - systemIPs = append(systemIPs, nic.IP.String()) - } - } - - return systemIPs, nil + return ExtractServerNetworkIDs(server, false), nil } func (r *ServerBootConfigurationPXEReconciler) getImageDetailsFromConfig(ctx context.Context, config *metalv1alpha1.ServerBootConfiguration) (string, string, string, error) { - // Parse image reference - split on last ':' to handle registry:port/image:tag format - lastColon := strings.LastIndex(config.Spec.Image, ":") - if lastColon == -1 { - return "", "", "", fmt.Errorf("invalid image format: missing tag") + imageName, imageVersion, err := ParseImageReference(config.Spec.Image) + if err != nil { + return "", "", "", err } - imageName := config.Spec.Image[:lastColon] - imageVersion := config.Spec.Image[lastColon+1:] kernelDigest, initrdDigest, squashFSDigest, err := r.getLayerDigestsFromNestedManifest(ctx, imageName, imageVersion) if err != nil { @@ -245,10 +225,8 @@ func (r *ServerBootConfigurationPXEReconciler) getImageDetailsFromConfig(ctx con func (r *ServerBootConfigurationPXEReconciler) getLayerDigestsFromNestedManifest(ctx context.Context, imageName, imageVersion string) (string, string, string, error) { imageRef := fmt.Sprintf("%s:%s", imageName, imageVersion) - if r.RegistryValidator != nil { - if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { - return "", "", "", fmt.Errorf("registry validation failed: %w", err) - } + if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { + return "", "", "", fmt.Errorf("registry validation failed: %w", err) } resolver := docker.NewResolver(docker.ResolverOptions{}) @@ -257,60 +235,9 @@ func (r *ServerBootConfigurationPXEReconciler) getLayerDigestsFromNestedManifest return "", "", "", fmt.Errorf("failed to resolve image reference: %w", err) } - manifestData, err := fetchContent(ctx, resolver, name, desc) + manifest, err := FindManifestByArchitecture(ctx, resolver, name, desc, r.Architecture, true) if err != nil { - return "", "", "", fmt.Errorf("failed to fetch manifest data: %w", err) - } - - var manifest ocispec.Manifest - if err := json.Unmarshal(manifestData, &manifest); err != nil { - return "", "", "", fmt.Errorf("failed to unmarshal index manifest: %w", err) - } - - if desc.MediaType == ocispec.MediaTypeImageIndex { - var targetManifestDesc ocispec.Descriptor - var indexManifest ocispec.Index - if err := json.Unmarshal(manifestData, &indexManifest); err != nil { - return "", "", "", fmt.Errorf("failed to unmarshal index manifest: %w", err) - } - - // Backward compatibility for CNAME prefix based OCI - // TODO: To be removed later - for _, manifest := range indexManifest.Manifests { - if strings.HasPrefix(manifest.Annotations["cname"], CNAMEPrefixMetalPXE) { - if manifest.Annotations["architecture"] == r.Architecture { - targetManifestDesc = manifest - break - } - } - } - - if targetManifestDesc.Digest == "" { - for _, manifest := range indexManifest.Manifests { - platform := manifest.Platform - if manifest.Platform != nil { - if platform.Architecture == r.Architecture { - targetManifestDesc = manifest - break - } - } - } - } - - if targetManifestDesc.Digest == "" { - return "", "", "", fmt.Errorf("failed to find target manifest with architecture %s", r.Architecture) - } - - nestedData, err := fetchContent(ctx, resolver, name, targetManifestDesc) - if err != nil { - return "", "", "", fmt.Errorf("failed to fetch nested manifest: %w", err) - } - - var nestedManifest ocispec.Manifest - if err := json.Unmarshal(nestedData, &nestedManifest); err != nil { - return "", "", "", fmt.Errorf("failed to unmarshal nested manifest: %w", err) - } - manifest = nestedManifest + return "", "", "", err } var kernelDigest, initrdDigest, squashFSDigest string @@ -338,61 +265,8 @@ func (r *ServerBootConfigurationPXEReconciler) getLayerDigestsFromNestedManifest return kernelDigest, initrdDigest, squashFSDigest, nil } -func fetchContent(ctx context.Context, resolver remotes.Resolver, ref string, desc ocispec.Descriptor) ([]byte, error) { - fetcher, err := resolver.Fetcher(ctx, ref) - if err != nil { - return nil, fmt.Errorf("failed to get fetcher: %w", err) - } - - reader, err := fetcher.Fetch(ctx, desc) - if err != nil { - return nil, fmt.Errorf("failed to fetch content: %w", err) - } - - defer func() { - if cerr := reader.Close(); cerr != nil { - fmt.Printf("failed to close reader: %v\n", cerr) - } - }() - - data, err := io.ReadAll(reader) - if err != nil { - return nil, fmt.Errorf("failed to read content: %w", err) - } - - if int64(len(data)) != desc.Size { - return nil, fmt.Errorf("size mismatch: expected %d, got %d", desc.Size, len(data)) - } - - return data, nil -} - func (r *ServerBootConfigurationPXEReconciler) enqueueServerBootConfigFromIgnitionSecret(ctx context.Context, secret client.Object) []reconcile.Request { - log := ctrl.LoggerFrom(ctx) - secretObj, ok := secret.(*corev1.Secret) - if !ok { - log.Error(nil, "can't decode object into Secret", secret) - return nil - } - - bootConfigList := &metalv1alpha1.ServerBootConfigurationList{} - if err := r.List(ctx, bootConfigList, client.InNamespace(secretObj.Namespace)); err != nil { - log.Error(err, "failed to list ServerBootConfiguration for Secret", "Secret", client.ObjectKeyFromObject(secretObj)) - return nil - } - - var requests []reconcile.Request - for _, bootConfig := range bootConfigList.Items { - if bootConfig.Spec.IgnitionSecretRef != nil && bootConfig.Spec.IgnitionSecretRef.Name == secretObj.Name { - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: bootConfig.Name, - Namespace: bootConfig.Namespace, - }, - }) - } - } - return requests + return EnqueueServerBootConfigsReferencingSecret(ctx, r.Client, secret) } // SetupWithManager sets up the controller with the Manager. diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 81a82c83..132d8c73 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -165,6 +165,7 @@ func SetupTest() *corev1.Namespace { Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), ImageServerURL: "http://localhost:5000/httpboot", + Architecture: runtime.GOARCH, RegistryValidator: registryValidator, }).SetupWithManager(k8sManager)).To(Succeed()) diff --git a/internal/registry/validation.go b/internal/registry/validation.go index 3d2aafbd..f5bb4be4 100644 --- a/internal/registry/validation.go +++ b/internal/registry/validation.go @@ -9,6 +9,11 @@ import ( "strings" ) +const ( + // DefaultRegistry is the default Docker Hub registry domain + DefaultRegistry = "registry-1.docker.io" +) + // Validator provides registry validation with cached environment variables. type Validator struct { allowedRegistries string @@ -28,7 +33,7 @@ func NewValidator() *Validator { func ExtractRegistryDomain(imageRef string) string { parts := strings.SplitN(imageRef, "/", 2) if len(parts) < 2 { - return "registry-1.docker.io" + return DefaultRegistry } potentialRegistry := parts[0] @@ -37,7 +42,17 @@ func ExtractRegistryDomain(imageRef string) string { return potentialRegistry } - return "registry-1.docker.io" + return DefaultRegistry +} + +// normalizeDockerHubDomain normalizes Docker Hub domain variants to canonical form. +func normalizeDockerHubDomain(domain string) string { + switch domain { + case "docker.io", "index.docker.io", DefaultRegistry: + return "docker.io" + default: + return domain + } } // isInList checks if a value is in a comma-separated list (exact match only). @@ -46,9 +61,13 @@ func isInList(registry string, list string) bool { return false } + // Normalize the registry domain for comparison + normalizedRegistry := normalizeDockerHubDomain(registry) + items := strings.Split(list, ",") for _, item := range items { - if strings.TrimSpace(item) == registry { + normalizedItem := normalizeDockerHubDomain(strings.TrimSpace(item)) + if normalizedItem == normalizedRegistry { return true } } @@ -81,17 +100,3 @@ func (v *Validator) ValidateImageRegistry(imageRef string) error { } return nil } - -// Package-level functions for backward compatibility (deprecated - use Validator instead) - -// IsRegistryAllowed checks if a registry is allowed (deprecated: use Validator.IsRegistryAllowed). -func IsRegistryAllowed(registry string) bool { - v := NewValidator() - return v.IsRegistryAllowed(registry) -} - -// ValidateImageRegistry validates that an image reference uses an allowed registry (deprecated: use Validator.ValidateImageRegistry). -func ValidateImageRegistry(imageRef string) error { - v := NewValidator() - return v.ValidateImageRegistry(imageRef) -} diff --git a/internal/registry/validation_test.go b/internal/registry/validation_test.go index 7ea738e8..8d36b01f 100644 --- a/internal/registry/validation_test.go +++ b/internal/registry/validation_test.go @@ -150,14 +150,11 @@ func TestIsRegistryAllowed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.allowList != "" { - t.Setenv("ALLOWED_REGISTRIES", tt.allowList) - } - if tt.blockList != "" { - t.Setenv("BLOCKED_REGISTRIES", tt.blockList) - } + t.Setenv("ALLOWED_REGISTRIES", tt.allowList) + t.Setenv("BLOCKED_REGISTRIES", tt.blockList) - got := IsRegistryAllowed(tt.registry) + v := NewValidator() + got := v.IsRegistryAllowed(tt.registry) if got != tt.want { t.Errorf("IsRegistryAllowed(%q) = %v, want %v (%s)", tt.registry, got, tt.want, tt.description) } @@ -249,14 +246,11 @@ func TestValidateImageRegistry(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.allowList != "" { - t.Setenv("ALLOWED_REGISTRIES", tt.allowList) - } - if tt.blockList != "" { - t.Setenv("BLOCKED_REGISTRIES", tt.blockList) - } + t.Setenv("ALLOWED_REGISTRIES", tt.allowList) + t.Setenv("BLOCKED_REGISTRIES", tt.blockList) - err := ValidateImageRegistry(tt.imageRef) + v := NewValidator() + err := v.ValidateImageRegistry(tt.imageRef) if (err != nil) != tt.wantErr { t.Errorf("ValidateImageRegistry(%q) error = %v, wantErr %v (%s)", tt.imageRef, err, tt.wantErr, tt.description) return diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index c9c539ec..25dd7677 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -13,6 +13,7 @@ import ( "os" "strings" "sync" + "time" "github.com/go-logr/logr" "github.com/ironcore-dev/boot-operator/internal/registry" @@ -30,7 +31,6 @@ type AuthMethod int const ( AuthNone AuthMethod = iota // Anonymous access - AuthBasic // Basic username:password AuthBearer // Bearer token via /token endpoint ) @@ -38,7 +38,6 @@ type RegistryInfo struct { Domain string AuthMethod AuthMethod TokenURL string // For bearer token auth - Realm string // For basic auth } type TokenResponse struct { @@ -97,7 +96,10 @@ func extractTokenURL(authHeader, repository string) string { func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error) { // Try GET /v2/ - standard registry probe endpoint targetURL := fmt.Sprintf("https://%s/v2/", registryDomain) - resp, err := http.Get(targetURL) + client := &http.Client{ + Timeout: 30 * time.Second, + } + resp, err := client.Get(targetURL) if err != nil { return nil, fmt.Errorf("registry unreachable: %w", err) } @@ -124,12 +126,6 @@ func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error return info, nil } - if strings.HasPrefix(authHeader, "Basic ") { - info.AuthMethod = AuthBasic - info.Realm = extractParam(authHeader, "realm") - return info, nil - } - return nil, fmt.Errorf("unsupported auth: %s", authHeader) default: @@ -139,7 +135,8 @@ func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error // Get or detect registry info with caching func getOrDetectRegistry(registry, repository string) (*RegistryInfo, error) { - cacheKey := registry + // Cache key includes repository for per-repository auth granularity + cacheKey := fmt.Sprintf("%s/%s", registry, repository) registryCacheMutex.RLock() if info, exists := registryCache[cacheKey]; exists { @@ -163,7 +160,10 @@ func getOrDetectRegistry(registry, repository string) (*RegistryInfo, error) { // Get bearer token from token URL func getBearerToken(tokenURL string) (string, error) { - resp, err := http.Get(tokenURL) + client := &http.Client{ + Timeout: 30 * time.Second, + } + resp, err := client.Get(tokenURL) if err != nil { return "", err } @@ -182,7 +182,7 @@ func getBearerToken(tokenURL string) (string, error) { return tokenResponse.Token, nil } -func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, log logr.Logger) { +func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, validator *registry.Validator, log logr.Logger) { http.HandleFunc("/image", func(w http.ResponseWriter, r *http.Request) { imageDetails, err := parseImageURL(r.URL.Query()) if err != nil { @@ -191,7 +191,7 @@ func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, l return } - handleDockerRegistry(w, r, &imageDetails, log) + handleDockerRegistry(w, r, &imageDetails, validator, log) }) http.HandleFunc("/httpboot/", func(w http.ResponseWriter, r *http.Request) { @@ -204,7 +204,7 @@ func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, l return } - handleDockerRegistry(w, r, &imageDetails, log) + handleDockerRegistry(w, r, &imageDetails, validator, log) }) log.Info("Starting image proxy server", "address", imageProxyServerAddr) @@ -243,13 +243,13 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { }, nil } -func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails *ImageDetails, log logr.Logger) { +func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails *ImageDetails, validator *registry.Validator, log logr.Logger) { registryDomain := imageDetails.RegistryDomain repository := imageDetails.RepositoryName log.V(1).Info("Processing registry request", "registry", registryDomain, "repository", repository, "digest", imageDetails.LayerDigest) - if !registry.IsRegistryAllowed(registryDomain) { + if !validator.IsRegistryAllowed(registryDomain) { http.Error(w, "Forbidden: Registry not allowed", http.StatusForbidden) log.Info("Registry blocked", "registry", registryDomain, "allowList", os.Getenv("ALLOWED_REGISTRIES"), "blockList", os.Getenv("BLOCKED_REGISTRIES")) return @@ -274,11 +274,6 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * return } log.V(1).Info("Obtained bearer token", "registry", registryDomain) - case AuthBasic: - // TODO: Support basic auth with credentials from secrets - http.Error(w, "Basic auth not yet implemented", http.StatusNotImplemented) - log.Info("Basic auth required but not yet supported", "registry", registryDomain) - return case AuthNone: log.V(1).Info("Registry allows anonymous access", "registry", registryDomain) } @@ -314,17 +309,15 @@ func buildDirector(proxyURL *url.URL, bearerToken string, repository string, dig func buildModifyResponse(bearerToken string) func(*http.Response) error { return func(resp *http.Response) error { - if bearerToken != "" { - resp.Header.Set("Authorization", "Bearer "+bearerToken) - } - if resp.StatusCode == http.StatusTemporaryRedirect { location, err := resp.Location() if err != nil { return err } - client := &http.Client{} + client := &http.Client{ + Timeout: 30 * time.Second, + } redirectReq, err := http.NewRequest("GET", location.String(), nil) if err != nil { return err diff --git a/test/registry/registry.go b/test/registry/registry.go index 23f076bd..68c4e7d4 100644 --- a/test/registry/registry.go +++ b/test/registry/registry.go @@ -15,12 +15,19 @@ import ( ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) +// OCI media types for boot components const ( - MediaTypeKernel = "application/vnd.ironcore.image.kernel" - MediaTypeInitrd = "application/vnd.ironcore.image.initramfs" - MediaTypeSquashfs = "application/vnd.ironcore.image.squashfs" - MediaTypeUKI = "application/vnd.ironcore.image.uki" + // MediaTypeKernel represents the Linux kernel image media type + MediaTypeKernel = "application/vnd.ironcore.image.kernel" + // MediaTypeInitrd represents the initial ramdisk media type + MediaTypeInitrd = "application/vnd.ironcore.image.initramfs" + // MediaTypeSquashfs represents the SquashFS filesystem media type + MediaTypeSquashfs = "application/vnd.ironcore.image.squashfs" + // MediaTypeUKI represents the unified kernel image media type + MediaTypeUKI = "application/vnd.ironcore.image.uki" + // MediaTypeKernelOld represents the legacy kernel media type MediaTypeKernelOld = "application/io.gardenlinux.kernel" + // MediaTypeInitrdOld represents the legacy initrd media type MediaTypeInitrdOld = "application/io.gardenlinux.initrd" ) From 7cf6f7a57711fe7fc951cfb0ab73362ff4998039 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Wed, 25 Feb 2026 15:30:56 +0000 Subject: [PATCH 05/14] Response to further comments --- .../controller/serverbootconfig_helpers.go | 9 ++ .../serverbootconfig_helpers_test.go | 63 ++++++++++ ...serverbootconfiguration_http_controller.go | 2 +- .../serverbootconfiguration_pxe_controller.go | 2 +- internal/registry/validation.go | 7 +- internal/registry/validation_test.go | 113 ++++++++++++++++++ server/imageproxyserver.go | 4 +- 7 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 internal/controller/serverbootconfig_helpers_test.go diff --git a/internal/controller/serverbootconfig_helpers.go b/internal/controller/serverbootconfig_helpers.go index 0deb7fa8..ef43d7ad 100644 --- a/internal/controller/serverbootconfig_helpers.go +++ b/internal/controller/serverbootconfig_helpers.go @@ -57,6 +57,15 @@ func ParseImageReference(image string) (imageName, imageVersion string, err erro return imageName, imageVersion, nil } +// BuildImageReference constructs a properly formatted OCI image reference from name and version. +// Uses @ separator for digest-based references (sha256:..., sha512:...) and : for tags. +func BuildImageReference(imageName, imageVersion string) string { + if strings.HasPrefix(imageVersion, "sha256:") || strings.HasPrefix(imageVersion, "sha512:") { + return fmt.Sprintf("%s@%s", imageName, imageVersion) + } + return fmt.Sprintf("%s:%s", imageName, imageVersion) +} + // FindManifestByArchitecture navigates an OCI image index to find the manifest for a specific architecture. // If enableCNAMECompat is true, it first tries to find manifests using the legacy CNAME annotation approach. // Returns the architecture-specific manifest, or an error if not found. diff --git a/internal/controller/serverbootconfig_helpers_test.go b/internal/controller/serverbootconfig_helpers_test.go new file mode 100644 index 00000000..087639ce --- /dev/null +++ b/internal/controller/serverbootconfig_helpers_test.go @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "testing" +) + +func TestBuildImageReference(t *testing.T) { + tests := []struct { + name string + imageName string + imageVersion string + want string + }{ + { + name: "tagged reference with simple tag", + imageName: "ghcr.io/ironcore-dev/gardenlinux", + imageVersion: "v1.0.0", + want: "ghcr.io/ironcore-dev/gardenlinux:v1.0.0", + }, + { + name: "tagged reference with latest", + imageName: "docker.io/library/ubuntu", + imageVersion: "latest", + want: "docker.io/library/ubuntu:latest", + }, + { + name: "digest reference with sha256", + imageName: "ghcr.io/ironcore-dev/gardenlinux", + imageVersion: "sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef", + want: "ghcr.io/ironcore-dev/gardenlinux@sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef", + }, + { + name: "digest reference with sha512", + imageName: "registry.example.com/myimage", + imageVersion: "sha512:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890", + want: "registry.example.com/myimage@sha512:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890", + }, + { + name: "tagged reference with numeric tag", + imageName: "localhost:5000/testimage", + imageVersion: "1.2.3", + want: "localhost:5000/testimage:1.2.3", + }, + { + name: "tagged reference with complex tag", + imageName: "keppel.global.cloud.sap/ironcore/gardenlinux-iso", + imageVersion: "arm64-v1.0.0-alpha", + want: "keppel.global.cloud.sap/ironcore/gardenlinux-iso:arm64-v1.0.0-alpha", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := BuildImageReference(tt.imageName, tt.imageVersion) + if got != tt.want { + t.Errorf("BuildImageReference(%q, %q) = %q, want %q", tt.imageName, tt.imageVersion, got, tt.want) + } + }) + } +} diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index d67d9a91..c41e9aa3 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -203,7 +203,7 @@ func (r *ServerBootConfigurationHTTPReconciler) constructUKIURL(ctx context.Cont } func (r *ServerBootConfigurationHTTPReconciler) getUKIDigestFromNestedManifest(ctx context.Context, imageName, imageVersion string) (string, error) { - imageRef := fmt.Sprintf("%s:%s", imageName, imageVersion) + imageRef := BuildImageReference(imageName, imageVersion) if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { return "", fmt.Errorf("registry validation failed: %w", err) } diff --git a/internal/controller/serverbootconfiguration_pxe_controller.go b/internal/controller/serverbootconfiguration_pxe_controller.go index 06d7b4c7..2f09dc5d 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller.go +++ b/internal/controller/serverbootconfiguration_pxe_controller.go @@ -224,7 +224,7 @@ func (r *ServerBootConfigurationPXEReconciler) getImageDetailsFromConfig(ctx con } func (r *ServerBootConfigurationPXEReconciler) getLayerDigestsFromNestedManifest(ctx context.Context, imageName, imageVersion string) (string, string, string, error) { - imageRef := fmt.Sprintf("%s:%s", imageName, imageVersion) + imageRef := BuildImageReference(imageName, imageVersion) if err := r.RegistryValidator.ValidateImageRegistry(imageRef); err != nil { return "", "", "", fmt.Errorf("registry validation failed: %w", err) } diff --git a/internal/registry/validation.go b/internal/registry/validation.go index f5bb4be4..b4cabf10 100644 --- a/internal/registry/validation.go +++ b/internal/registry/validation.go @@ -46,12 +46,15 @@ func ExtractRegistryDomain(imageRef string) string { } // normalizeDockerHubDomain normalizes Docker Hub domain variants to canonical form. +// All registry domains are converted to lowercase for case-insensitive comparison, +// as DNS/domain names are case-insensitive by specification. func normalizeDockerHubDomain(domain string) string { - switch domain { + lowerDomain := strings.ToLower(domain) + switch lowerDomain { case "docker.io", "index.docker.io", DefaultRegistry: return "docker.io" default: - return domain + return lowerDomain } } diff --git a/internal/registry/validation_test.go b/internal/registry/validation_test.go index 8d36b01f..4ff7b899 100644 --- a/internal/registry/validation_test.go +++ b/internal/registry/validation_test.go @@ -65,6 +65,79 @@ func TestExtractRegistryDomain(t *testing.T) { } } +func TestNormalizeDockerHubDomain(t *testing.T) { + tests := []struct { + name string + domain string + want string + }{ + { + name: "lowercase docker.io", + domain: "docker.io", + want: "docker.io", + }, + { + name: "uppercase DOCKER.IO", + domain: "DOCKER.IO", + want: "docker.io", + }, + { + name: "mixed case Docker.Io", + domain: "Docker.Io", + want: "docker.io", + }, + { + name: "lowercase index.docker.io", + domain: "index.docker.io", + want: "docker.io", + }, + { + name: "uppercase INDEX.DOCKER.IO", + domain: "INDEX.DOCKER.IO", + want: "docker.io", + }, + { + name: "mixed case Index.Docker.IO", + domain: "Index.Docker.IO", + want: "docker.io", + }, + { + name: "lowercase registry-1.docker.io", + domain: "registry-1.docker.io", + want: "docker.io", + }, + { + name: "uppercase REGISTRY-1.DOCKER.IO", + domain: "REGISTRY-1.DOCKER.IO", + want: "docker.io", + }, + { + name: "non-Docker Hub - ghcr.io preserved", + domain: "ghcr.io", + want: "ghcr.io", + }, + { + name: "non-Docker Hub - GHCR.IO normalized to lowercase", + domain: "GHCR.IO", + want: "ghcr.io", + }, + { + name: "non-Docker Hub - keppel preserved", + domain: "keppel.global.cloud.sap", + want: "keppel.global.cloud.sap", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := normalizeDockerHubDomain(tt.domain) + if got != tt.want { + t.Errorf("normalizeDockerHubDomain(%q) = %q, want %q", tt.domain, got, tt.want) + } + }) + } +} + func TestIsRegistryAllowed(t *testing.T) { tests := []struct { name string @@ -146,6 +219,46 @@ func TestIsRegistryAllowed(t *testing.T) { want: true, description: "handles whitespace in allow list", }, + { + name: "case-insensitive Docker Hub - uppercase blocklist", + registry: "docker.io", + allowList: "", + blockList: "DOCKER.IO", + want: false, + description: "docker.io blocked by uppercase DOCKER.IO in blocklist", + }, + { + name: "case-insensitive Docker Hub - mixed case registry", + registry: "DOCKER.IO", + allowList: "", + blockList: "docker.io", + want: false, + description: "uppercase DOCKER.IO blocked by lowercase docker.io in blocklist", + }, + { + name: "case-insensitive Docker Hub - index variant", + registry: "INDEX.DOCKER.IO", + allowList: "", + blockList: "docker.io", + want: false, + description: "INDEX.DOCKER.IO normalized and blocked by docker.io", + }, + { + name: "case-insensitive Docker Hub - registry-1 variant", + registry: "REGISTRY-1.DOCKER.IO", + allowList: "", + blockList: "docker.io", + want: false, + description: "REGISTRY-1.DOCKER.IO normalized and blocked by docker.io", + }, + { + name: "case-insensitive non-Docker registry matching", + registry: "GHCR.IO", + allowList: "ghcr.io", + blockList: "", + want: true, + description: "all registries are case-insensitive (GHCR.IO matches ghcr.io)", + }, } for _, tt := range tests { diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index 25dd7677..40a6d714 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -285,7 +285,7 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * proxy := &httputil.ReverseProxy{ Director: buildDirector(proxyURL, authToken, repository, digest), - ModifyResponse: buildModifyResponse(authToken), + ModifyResponse: buildModifyResponse(), } r.URL.Host = proxyURL.Host @@ -307,7 +307,7 @@ func buildDirector(proxyURL *url.URL, bearerToken string, repository string, dig } } -func buildModifyResponse(bearerToken string) func(*http.Response) error { +func buildModifyResponse() func(*http.Response) error { return func(resp *http.Response) error { if resp.StatusCode == http.StatusTemporaryRedirect { location, err := resp.Location() From 618e3babe49989416bffb9421e299de41f1fc3ad Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Thu, 26 Feb 2026 08:40:17 +0000 Subject: [PATCH 06/14] Add robustness to http client --- server/imageproxyserver.go | 85 ++++++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index 40a6d714..106757e1 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -56,6 +56,17 @@ type ImageDetails struct { var registryCache = make(map[string]*RegistryInfo) var registryCacheMutex sync.RWMutex +// Shared HTTP client for all registry operations to enable connection reuse +var httpClient = &http.Client{ + Timeout: 30 * time.Second, + Transport: &http.Transport{ + MaxIdleConnsPerHost: 10, // Connection pool per host + IdleConnTimeout: 90 * time.Second, // Keep-alive duration + TLSHandshakeTimeout: 10 * time.Second, // Security timeout + ExpectContinueTimeout: 1 * time.Second, // Reduce latency + }, +} + // Extract repository path without registry func extractRepository(imageRef, registryDomain string) string { return strings.TrimPrefix(imageRef, registryDomain+"/") @@ -96,10 +107,7 @@ func extractTokenURL(authHeader, repository string) string { func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error) { // Try GET /v2/ - standard registry probe endpoint targetURL := fmt.Sprintf("https://%s/v2/", registryDomain) - client := &http.Client{ - Timeout: 30 * time.Second, - } - resp, err := client.Get(targetURL) + resp, err := httpClient.Get(targetURL) if err != nil { return nil, fmt.Errorf("registry unreachable: %w", err) } @@ -160,10 +168,7 @@ func getOrDetectRegistry(registry, repository string) (*RegistryInfo, error) { // Get bearer token from token URL func getBearerToken(tokenURL string) (string, error) { - client := &http.Client{ - Timeout: 30 * time.Second, - } - resp, err := client.Get(tokenURL) + resp, err := httpClient.Get(tokenURL) if err != nil { return "", err } @@ -280,8 +285,11 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * // Proxy the blob request digest := imageDetails.LayerDigest - targetURL := fmt.Sprintf("https://%s/v2/%s/blobs/%s", registryDomain, repository, digest) - proxyURL, _ := url.Parse(targetURL) + proxyURL := &url.URL{ + Scheme: "https", + Host: registryDomain, + Path: fmt.Sprintf("/v2/%s/blobs/%s", repository, digest), + } proxy := &httputil.ReverseProxy{ Director: buildDirector(proxyURL, authToken, repository, digest), @@ -292,7 +300,7 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * r.URL.Scheme = proxyURL.Scheme r.Host = proxyURL.Host - log.Info("Proxying registry request", "targetURL", targetURL, "authMethod", registryInfo.AuthMethod) + log.Info("Proxying registry request", "targetURL", proxyURL.String(), "authMethod", registryInfo.AuthMethod) proxy.ServeHTTP(w, r) } @@ -309,22 +317,33 @@ func buildDirector(proxyURL *url.URL, bearerToken string, repository string, dig func buildModifyResponse() func(*http.Response) error { return func(resp *http.Response) error { - if resp.StatusCode == http.StatusTemporaryRedirect { + // Handle redirects (307, 301, 302, 303) + if resp.StatusCode == http.StatusTemporaryRedirect || + resp.StatusCode == http.StatusMovedPermanently || + resp.StatusCode == http.StatusFound || + resp.StatusCode == http.StatusSeeOther { location, err := resp.Location() if err != nil { return err } - client := &http.Client{ - Timeout: 30 * time.Second, - } redirectReq, err := http.NewRequest("GET", location.String(), nil) if err != nil { return err } - copyHeaders(resp.Request.Header, redirectReq.Header) - redirectResp, err := client.Do(redirectReq) + // Security: Strip sensitive headers on cross-host redirects to prevent + // leaking credentials (e.g., bearer tokens) to third-party CDN/mirrors + if isSameHost(resp.Request.URL, location) { + // Same-host redirect: preserve all headers including Authorization + copyHeaders(resp.Request.Header, redirectReq.Header) + } else { + // Cross-host redirect: exclude sensitive auth headers + copyHeadersExcluding(resp.Request.Header, redirectReq.Header, + []string{"Authorization", "Cookie", "Proxy-Authorization"}) + } + + redirectResp, err := httpClient.Do(redirectReq) if err != nil { return err } @@ -358,10 +377,40 @@ func copyHeaders(source http.Header, destination http.Header) { } } +// copyHeadersExcluding copies headers from source to destination, excluding specified headers. +// Header name comparison is case-insensitive per HTTP specification. +func copyHeadersExcluding(source http.Header, destination http.Header, excludeHeaders []string) { + // Normalize excluded headers to lowercase for case-insensitive comparison + excludeMap := make(map[string]bool, len(excludeHeaders)) + for _, header := range excludeHeaders { + excludeMap[strings.ToLower(header)] = true + } + + for name, values := range source { + if !excludeMap[strings.ToLower(name)] { + for _, value := range values { + destination.Add(name, value) + } + } + } +} + +// isSameHost compares two URLs to determine if they point to the same host. +// Comparison includes both hostname and port to prevent credential leakage across ports. +func isSameHost(url1, url2 *url.URL) bool { + if url1 == nil || url2 == nil { + return false + } + // URL.Host includes port if specified, e.g., "registry.io:443" + return strings.EqualFold(url1.Host, url2.Host) +} + func replaceResponse(originalResp, redirectResp *http.Response) { + // Preserve all values for multi-valued headers (e.g., Set-Cookie) for name, values := range redirectResp.Header { + originalResp.Header.Del(name) // Clear existing values first for _, value := range values { - originalResp.Header.Set(name, value) + originalResp.Header.Add(name, value) // Add all values } } originalResp.Body = redirectResp.Body From 2775aae2332ed677f6ef9064a3c35e95a68a77be Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Thu, 26 Feb 2026 09:51:26 +0000 Subject: [PATCH 07/14] More comment updates --- server/imageproxyserver.go | 116 ++++++++++++++++++++++++++++++------- 1 file changed, 96 insertions(+), 20 deletions(-) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index 106757e1..3467eaec 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "github.com/distribution/reference" "github.com/go-logr/logr" "github.com/ironcore-dev/boot-operator/internal/registry" "sigs.k8s.io/controller-runtime/pkg/client" @@ -40,8 +41,11 @@ type RegistryInfo struct { TokenURL string // For bearer token auth } +// TokenResponse represents the JSON response from an OCI registry token endpoint. +// Supports both Docker registry format (token) and OAuth2 format (access_token). type TokenResponse struct { - Token string `json:"token"` + Token string `json:"token"` // Docker registry format + AccessToken string `json:"access_token"` // OAuth2 format (takes precedence) } type ImageDetails struct { @@ -52,10 +56,22 @@ type ImageDetails struct { Version string } +// registryCacheEntry holds registry info with expiration timestamp +type registryCacheEntry struct { + info *RegistryInfo + expiresAt time.Time +} + // Cache registry info to avoid repeated probes -var registryCache = make(map[string]*RegistryInfo) +var registryCache = make(map[string]*registryCacheEntry) var registryCacheMutex sync.RWMutex +const ( + // registryCacheTTL defines how long registry auth info is cached + // After this duration, auth detection will be re-run to catch policy changes + registryCacheTTL = 15 * time.Minute +) + // Shared HTTP client for all registry operations to enable connection reuse var httpClient = &http.Client{ Timeout: 30 * time.Second, @@ -67,11 +83,6 @@ var httpClient = &http.Client{ }, } -// Extract repository path without registry -func extractRepository(imageRef, registryDomain string) string { - return strings.TrimPrefix(imageRef, registryDomain+"/") -} - // Parse WWW-Authenticate parameter value func extractParam(header, param string) string { start := strings.Index(header, param+"=\"") @@ -128,7 +139,8 @@ func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error return nil, fmt.Errorf("401 without WWW-Authenticate header") } - if strings.HasPrefix(authHeader, "Bearer ") { + // HTTP auth scheme matching is case-insensitive per RFC 7235 + if len(authHeader) > 7 && strings.EqualFold(authHeader[:7], "bearer ") { info.AuthMethod = AuthBearer info.TokenURL = extractTokenURL(authHeader, repository) return info, nil @@ -141,26 +153,32 @@ func detectRegistryAuth(registryDomain, repository string) (*RegistryInfo, error } } -// Get or detect registry info with caching +// Get or detect registry info with caching and TTL-based expiration func getOrDetectRegistry(registry, repository string) (*RegistryInfo, error) { // Cache key includes repository for per-repository auth granularity cacheKey := fmt.Sprintf("%s/%s", registry, repository) registryCacheMutex.RLock() - if info, exists := registryCache[cacheKey]; exists { - registryCacheMutex.RUnlock() - return info, nil + if entry, exists := registryCache[cacheKey]; exists { + // Check if entry has expired + if time.Now().Before(entry.expiresAt) { + registryCacheMutex.RUnlock() + return entry.info, nil + } } registryCacheMutex.RUnlock() - // Detect and cache + // Detect and cache with TTL info, err := detectRegistryAuth(registry, repository) if err != nil { return nil, err } registryCacheMutex.Lock() - registryCache[cacheKey] = info + registryCache[cacheKey] = ®istryCacheEntry{ + info: info, + expiresAt: time.Now().Add(registryCacheTTL), + } registryCacheMutex.Unlock() return info, nil @@ -174,6 +192,12 @@ func getBearerToken(tokenURL string) (string, error) { } defer func() { _ = resp.Body.Close() }() + // Check HTTP status before attempting to parse JSON + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return "", fmt.Errorf("token request failed with status %d: %s", resp.StatusCode, string(body)) + } + body, err := io.ReadAll(resp.Body) if err != nil { return "", err @@ -181,13 +205,53 @@ func getBearerToken(tokenURL string) (string, error) { var tokenResponse TokenResponse if err := json.Unmarshal(body, &tokenResponse); err != nil { - return "", err + return "", fmt.Errorf("failed to parse token response: %w", err) } - return tokenResponse.Token, nil + // Prefer access_token (OAuth2 standard) over token (Docker registry format) + if tokenResponse.AccessToken != "" { + return tokenResponse.AccessToken, nil + } + if tokenResponse.Token != "" { + return tokenResponse.Token, nil + } + + return "", fmt.Errorf("token response missing both 'token' and 'access_token' fields") +} + +// cleanupExpiredCacheEntries periodically removes expired entries from the registry cache +// to prevent unbounded memory growth. Runs every 5 minutes. +func cleanupExpiredCacheEntries(log logr.Logger) { + ticker := time.NewTicker(5 * time.Minute) + defer ticker.Stop() + + for range ticker.C { + now := time.Now() + registryCacheMutex.Lock() + + expiredKeys := make([]string, 0) + for key, entry := range registryCache { + if now.After(entry.expiresAt) { + expiredKeys = append(expiredKeys, key) + } + } + + for _, key := range expiredKeys { + delete(registryCache, key) + } + + registryCacheMutex.Unlock() + + if len(expiredKeys) > 0 { + log.V(1).Info("Cleaned up expired cache entries", "count", len(expiredKeys), "remainingEntries", len(registryCache)) + } + } } func RunImageProxyServer(imageProxyServerAddr string, k8sClient client.Client, validator *registry.Validator, log logr.Logger) { + // Start background cleanup of expired cache entries + go cleanupExpiredCacheEntries(log) + http.HandleFunc("/image", func(w http.ResponseWriter, r *http.Request) { imageDetails, err := parseImageURL(r.URL.Query()) if err != nil { @@ -229,9 +293,13 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { imageName := strings.Join(segments[:len(segments)-1], "/") digestSegment := segments[len(segments)-1] - // Extract registry domain and repository + // Extract registry domain and repository using distribution/reference registryDomain := registry.ExtractRegistryDomain(imageName) - repositoryName := extractRepository(imageName, registryDomain) + named, err := reference.ParseNormalizedNamed(imageName) + if err != nil { + return ImageDetails{}, fmt.Errorf("invalid image reference: %w", err) + } + repositoryName := reference.Path(named) digestSegment = strings.TrimSuffix(digestSegment, ".efi") @@ -413,6 +481,10 @@ func replaceResponse(originalResp, redirectResp *http.Response) { originalResp.Header.Add(name, value) // Add all values } } + // Close the original body to prevent connection leaks + if originalResp.Body != nil { + _ = originalResp.Body.Close() + } originalResp.Body = redirectResp.Body originalResp.StatusCode = redirectResp.StatusCode } @@ -428,9 +500,13 @@ func parseImageURL(queries url.Values) (imageDetails ImageDetails, err error) { ociImageName = strings.TrimSuffix(ociImageName, ".efi") - // Extract registry domain and repository + // Extract registry domain and repository using distribution/reference registryDomain := registry.ExtractRegistryDomain(ociImageName) - repositoryName := extractRepository(ociImageName, registryDomain) + named, err := reference.ParseNormalizedNamed(ociImageName) + if err != nil { + return ImageDetails{}, fmt.Errorf("invalid image reference: %w", err) + } + repositoryName := reference.Path(named) return ImageDetails{ OCIImageName: ociImageName, From 7d5eb281408a5cbd5a19dae1b3520c8a9834d782 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Thu, 26 Feb 2026 10:31:07 +0000 Subject: [PATCH 08/14] Tidy http connection handling --- server/imageproxyserver.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index 3467eaec..fb2dd45e 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -70,6 +70,12 @@ const ( // registryCacheTTL defines how long registry auth info is cached // After this duration, auth detection will be re-run to catch policy changes registryCacheTTL = 15 * time.Minute + + // maxErrorResponseSize limits error response body reads to prevent memory exhaustion + maxErrorResponseSize = 4 * 1024 // 4KB - enough for error details + + // maxTokenResponseSize limits token response body reads to prevent memory exhaustion + maxTokenResponseSize = 64 * 1024 // 64KB - token responses are typically a few hundred bytes ) // Shared HTTP client for all registry operations to enable connection reuse @@ -194,11 +200,15 @@ func getBearerToken(tokenURL string) (string, error) { // Check HTTP status before attempting to parse JSON if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) + // Limit error response body read to prevent memory exhaustion + limitedReader := io.LimitReader(resp.Body, maxErrorResponseSize) + body, _ := io.ReadAll(limitedReader) return "", fmt.Errorf("token request failed with status %d: %s", resp.StatusCode, string(body)) } - body, err := io.ReadAll(resp.Body) + // Limit token response body read to prevent memory exhaustion + limitedReader := io.LimitReader(resp.Body, maxTokenResponseSize) + body, err := io.ReadAll(limitedReader) if err != nil { return "", err } @@ -395,7 +405,8 @@ func buildModifyResponse() func(*http.Response) error { return err } - redirectReq, err := http.NewRequest("GET", location.String(), nil) + // Propagate original request context to enable cancellation on client disconnect + redirectReq, err := http.NewRequestWithContext(resp.Request.Context(), "GET", location.String(), nil) if err != nil { return err } From f8e563b8b09ee9799700ba19fd2251845cd00005 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Wed, 4 Mar 2026 16:38:03 +0000 Subject: [PATCH 09/14] Update docs/README.md, use flags for lists, update ExtractRegistryDomain --- cmd/main.go | 8 +++- docs/README.md | 31 +++++++++++-- go.mod | 2 +- internal/controller/suite_test.go | 19 ++++---- internal/registry/validation.go | 65 +++++++++++++++------------- internal/registry/validation_test.go | 18 +++----- server/imageproxyserver.go | 3 +- 7 files changed, 84 insertions(+), 62 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index dadb0ce7..0faf7028 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -80,6 +80,8 @@ func main() { var ipxeServicePort int var imageServerURL string var architecture string + var allowedRegistries string + var blockedRegistries string flag.StringVar(&architecture, "architecture", "amd64", "Target system architecture (e.g., amd64, arm64)") flag.IntVar(&ipxeServicePort, "ipxe-service-port", 5000, "IPXE Service port to listen on.") @@ -99,6 +101,8 @@ func main() { flag.BoolVar(&secureMetrics, "metrics-secure", true, "If set the metrics endpoint is served securely") flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics and webhook servers") + flag.StringVar(&allowedRegistries, "allowed-registries", "", "Comma-separated list of allowed OCI registries. If set, only these registries are permitted.") + flag.StringVar(&blockedRegistries, "blocked-registries", "", "Comma-separated list of blocked OCI registries. If set, these registries are denied.") controllers := switches.New( // core controllers @@ -229,8 +233,8 @@ func main() { } // Initialize registry validator for OCI image validation - registryValidator := registry.NewValidator() - setupLog.Info("Initialized registry validator", "allowedRegistries", os.Getenv("ALLOWED_REGISTRIES"), "blockedRegistries", os.Getenv("BLOCKED_REGISTRIES")) + registryValidator := registry.NewValidator(allowedRegistries, blockedRegistries) + setupLog.Info("Initialized registry validator", "allowedRegistries", allowedRegistries, "blockedRegistries", blockedRegistries) if controllers.Enabled(ipxeBootConfigController) { if err = (&controller.IPXEBootConfigReconciler{ diff --git a/docs/README.md b/docs/README.md index 88b589fa..4a6f6b8e 100644 --- a/docs/README.md +++ b/docs/README.md @@ -19,16 +19,22 @@ Boot Operator includes the following key components: - Responds with an iPXE script, which the bare metal server uses to download the necessary OS components - This endpoint is typically called directly by the server during boot and is commonly used in PXE boot scenarios + - **HTTP Boot Server** - Handles `/httpboot` requests - Returns a JSON response containing the location of the UKI (Unified Kernel Image) that the server should download - The DHCP server extension typically handles the response and sends the UKI image location to the server - Common in modern cloud-native bare metal setups, especially for containers and minimal OS images + - **Image Proxy Server** - Handles `/image` requests - - Extracts layers from public OCI (Open Container Initiative) images, with current support for GHCR (GitHub Container Registry) only - - Downloads specific layers based on the requested URI and image specifications + - Extracts layers from OCI (Open Container Initiative) images, with support for multiple registries (e.g., GHCR, Docker Hub, Keppel, and any OCI-compliant registry) + - Downloads specific layers based on the requested URI and image specifications + - Registry access is controlled via CLI flags: + - `--allowed-registries`: comma-separated list of permitted registries (allowlist mode) + - `--blocked-registries`: comma-separated list of denied registries (blocklist mode) + - If neither flag is set, all registries are denied (fail-closed) - Example: - `wget http://SERVER_ADDRESS:30007/image?imageName=ghcr.io/ironcore-dev/os-images/gardenlinux&version=1443.10&layerName=application/vnd.ironcore.image.squashfs.v1alpha1.squashfs` @@ -36,4 +42,23 @@ Boot Operator includes the following key components: - Handles `/ignition` requests - Responds with Ignition configuration content tailored to the client machine, identified by its UUID in the request URL. -These servers leverage Kubernetes controllers and API objects to manage the boot process and serve requests from bare metal machines. The architecture and specifics of the controllers and API objects are described in the architecture section of the documentation. \ No newline at end of file +These servers leverage Kubernetes controllers and API objects to manage the boot process and serve requests from bare metal machines. The architecture and specifics of the controllers and API objects are described in the architecture section of the documentation. + +## Registry Validation + +Boot Operator enforces OCI registry restrictions at two levels: + +1. **Controller level (early validation):** The PXE and HTTP boot controllers validate image references against the registry allow/block list during reconciliation. This means misconfigured or disallowed registries are rejected immediately when a `ServerBootConfiguration` is created, providing fast feedback before any machine attempts to boot. + +2. **Image Proxy Server level (runtime enforcement):** The image proxy server also validates registry domains before proxying layer downloads, acting as a second line of defense. + +Registry restrictions are configured via CLI flags on the manager binary: + +| Flag | Description | +|------|-------------| +| `--allowed-registries` | Comma-separated list of permitted registries (allowlist mode). Only these registries are accepted. | +| `--blocked-registries` | Comma-separated list of denied registries (blocklist mode). All registries except these are accepted. | + +- If `--allowed-registries` is set, it takes precedence over `--blocked-registries`. +- If neither flag is set, all registries are **denied** (fail-closed). +- Docker Hub variants (`docker.io`, `index.docker.io`, `registry-1.docker.io`) are normalized for consistent matching. \ No newline at end of file diff --git a/go.mod b/go.mod index 7a06664b..bd9114b3 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.25.6 require ( github.com/containerd/containerd v1.7.30 - github.com/coreos/butane v0.26.0 + github.com/coreos/butane v0.27.0 github.com/distribution/reference v0.6.0 github.com/go-logr/logr v1.4.3 github.com/ironcore-dev/controller-utils v0.11.0 diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 132d8c73..ff9c3bcb 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -6,7 +6,6 @@ package controller import ( "context" "fmt" - "os" "path/filepath" "runtime" "testing" @@ -47,10 +46,11 @@ const ( ) var ( - cfg *rest.Config - k8sClient client.Client - testEnv *envtest.Environment - mockRegistry *testregistry.MockRegistry + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + mockRegistry *testregistry.MockRegistry + allowedRegistries string ) func TestControllers(t *testing.T) { @@ -75,11 +75,8 @@ var _ = BeforeSuite(func() { Expect(mockRegistry.PushPXEImageOldFormat("gardenlinux/gardenlinux", "1772.0", runtime.GOARCH)).To(Succeed()) Expect(mockRegistry.PushHTTPImage("ironcore-dev/os-images/test-image", "100.1")).To(Succeed()) - // Set ALLOWED_REGISTRIES to use mock registry - Expect(os.Setenv("ALLOWED_REGISTRIES", mockRegistry.RegistryAddress())).To(Succeed()) - DeferCleanup(func() { - Expect(os.Unsetenv("ALLOWED_REGISTRIES")).To(Succeed()) - }) + // Set allowed registries to use mock registry + allowedRegistries = mockRegistry.RegistryAddress() By("bootstrapping test environment") testEnv = &envtest.Environment{ @@ -151,7 +148,7 @@ func SetupTest() *corev1.Namespace { }) Expect(err).ToNot(HaveOccurred()) - registryValidator := registry.NewValidator() + registryValidator := registry.NewValidator(allowedRegistries, "") Expect((&ServerBootConfigurationPXEReconciler{ Client: k8sManager.GetClient(), diff --git a/internal/registry/validation.go b/internal/registry/validation.go index b4cabf10..bb9bcf7a 100644 --- a/internal/registry/validation.go +++ b/internal/registry/validation.go @@ -5,44 +5,47 @@ package registry import ( "fmt" - "os" "strings" + + "github.com/distribution/reference" ) const ( // DefaultRegistry is the default Docker Hub registry domain DefaultRegistry = "registry-1.docker.io" + // DockerHubDomain is the canonical short domain for Docker Hub + DockerHubDomain = "docker.io" ) -// Validator provides registry validation with cached environment variables. +// Validator provides registry validation with configurable allow/block lists. type Validator struct { - allowedRegistries string - blockedRegistries string + AllowedRegistries string + BlockedRegistries string } -// NewValidator creates a new Validator with environment variables cached at initialization. -// This should be called once at startup to avoid repeated os.Getenv calls. -func NewValidator() *Validator { +// NewValidator creates a new Validator with the given allowed and blocked registry lists. +// Each list is a comma-separated string of registry domains. +func NewValidator(allowedRegistries, blockedRegistries string) *Validator { return &Validator{ - allowedRegistries: os.Getenv("ALLOWED_REGISTRIES"), - blockedRegistries: os.Getenv("BLOCKED_REGISTRIES"), + AllowedRegistries: allowedRegistries, + BlockedRegistries: blockedRegistries, } } -// ExtractRegistryDomain extracts the registry domain from an OCI image reference. +// ExtractRegistryDomain extracts the registry domain from an OCI image reference +// using the canonical Docker reference parser from github.com/distribution/reference. func ExtractRegistryDomain(imageRef string) string { - parts := strings.SplitN(imageRef, "/", 2) - if len(parts) < 2 { + named, err := reference.ParseNormalizedNamed(imageRef) + if err != nil { return DefaultRegistry } - - potentialRegistry := parts[0] - - if strings.Contains(potentialRegistry, ".") || strings.Contains(potentialRegistry, ":") || potentialRegistry == "localhost" { - return potentialRegistry + domain := reference.Domain(named) + // The reference library normalizes Docker Hub to "docker.io", + // but we use "registry-1.docker.io" as our canonical constant. + if domain == DockerHubDomain { + return DefaultRegistry } - - return DefaultRegistry + return domain } // normalizeDockerHubDomain normalizes Docker Hub domain variants to canonical form. @@ -51,8 +54,8 @@ func ExtractRegistryDomain(imageRef string) string { func normalizeDockerHubDomain(domain string) string { lowerDomain := strings.ToLower(domain) switch lowerDomain { - case "docker.io", "index.docker.io", DefaultRegistry: - return "docker.io" + case DockerHubDomain, "index.docker.io", DefaultRegistry: + return DockerHubDomain default: return lowerDomain } @@ -77,14 +80,14 @@ func isInList(registry string, list string) bool { return false } -// IsRegistryAllowed checks if a registry is allowed based on the cached allow/block lists. +// IsRegistryAllowed checks if a registry is allowed based on the allow/block lists. func (v *Validator) IsRegistryAllowed(registry string) bool { - if v.allowedRegistries != "" { - return isInList(registry, v.allowedRegistries) + if v.AllowedRegistries != "" { + return isInList(registry, v.AllowedRegistries) } - if v.blockedRegistries != "" { - return !isInList(registry, v.blockedRegistries) + if v.BlockedRegistries != "" { + return !isInList(registry, v.BlockedRegistries) } return false @@ -94,12 +97,12 @@ func (v *Validator) IsRegistryAllowed(registry string) bool { func (v *Validator) ValidateImageRegistry(imageRef string) error { registry := ExtractRegistryDomain(imageRef) if !v.IsRegistryAllowed(registry) { - if v.allowedRegistries != "" { - return fmt.Errorf("registry not allowed: %s (allowed registries: %s)", registry, v.allowedRegistries) - } else if v.blockedRegistries != "" { - return fmt.Errorf("registry blocked: %s (blocked registries: %s)", registry, v.blockedRegistries) + if v.AllowedRegistries != "" { + return fmt.Errorf("registry not allowed: %s (allowed registries: %s)", registry, v.AllowedRegistries) + } else if v.BlockedRegistries != "" { + return fmt.Errorf("registry blocked: %s (blocked registries: %s)", registry, v.BlockedRegistries) } - return fmt.Errorf("registry not allowed: %s (no ALLOWED_REGISTRIES or BLOCKED_REGISTRIES configured, denying all)", registry) + return fmt.Errorf("registry not allowed: %s (no --allowed-registries or --blocked-registries configured, denying all)", registry) } return nil } diff --git a/internal/registry/validation_test.go b/internal/registry/validation_test.go index 4ff7b899..686e4c0d 100644 --- a/internal/registry/validation_test.go +++ b/internal/registry/validation_test.go @@ -26,7 +26,7 @@ func TestExtractRegistryDomain(t *testing.T) { { name: "docker hub explicit", imageRef: "docker.io/library/ubuntu:latest", - want: "docker.io", + want: "registry-1.docker.io", }, { name: "docker hub implicit with namespace", @@ -263,10 +263,7 @@ func TestIsRegistryAllowed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Setenv("ALLOWED_REGISTRIES", tt.allowList) - t.Setenv("BLOCKED_REGISTRIES", tt.blockList) - - v := NewValidator() + v := NewValidator(tt.allowList, tt.blockList) got := v.IsRegistryAllowed(tt.registry) if got != tt.want { t.Errorf("IsRegistryAllowed(%q) = %v, want %v (%s)", tt.registry, got, tt.want, tt.description) @@ -307,7 +304,7 @@ func TestValidateImageRegistry(t *testing.T) { allowList: "ghcr.io,keppel.global.cloud.sap", blockList: "", wantErr: true, - errContains: "registry not allowed: docker.io", + errContains: "registry not allowed: registry-1.docker.io", description: "docker.io should be blocked when not in allow list", }, { @@ -334,7 +331,7 @@ func TestValidateImageRegistry(t *testing.T) { allowList: "", blockList: "docker.io,registry-1.docker.io", wantErr: true, - errContains: "registry blocked: docker.io", + errContains: "registry blocked: registry-1.docker.io", description: "docker.io should be blocked when in block list", }, { @@ -352,17 +349,14 @@ func TestValidateImageRegistry(t *testing.T) { allowList: "", blockList: "", wantErr: true, - errContains: "no ALLOWED_REGISTRIES or BLOCKED_REGISTRIES configured", + errContains: "no --allowed-registries or --blocked-registries configured", description: "should deny all when no configuration", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Setenv("ALLOWED_REGISTRIES", tt.allowList) - t.Setenv("BLOCKED_REGISTRIES", tt.blockList) - - v := NewValidator() + v := NewValidator(tt.allowList, tt.blockList) err := v.ValidateImageRegistry(tt.imageRef) if (err != nil) != tt.wantErr { t.Errorf("ValidateImageRegistry(%q) error = %v, wantErr %v (%s)", tt.imageRef, err, tt.wantErr, tt.description) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index fb2dd45e..e7548427 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -10,7 +10,6 @@ import ( "net/http" "net/http/httputil" "net/url" - "os" "strings" "sync" "time" @@ -334,7 +333,7 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * if !validator.IsRegistryAllowed(registryDomain) { http.Error(w, "Forbidden: Registry not allowed", http.StatusForbidden) - log.Info("Registry blocked", "registry", registryDomain, "allowList", os.Getenv("ALLOWED_REGISTRIES"), "blockList", os.Getenv("BLOCKED_REGISTRIES")) + log.Info("Registry blocked", "registry", registryDomain, "allowList", validator.AllowedRegistries, "blockList", validator.BlockedRegistries) return } From 1213ddc27c53aece9b206b00a614bd407a13e2b9 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Tue, 10 Mar 2026 11:58:52 +0000 Subject: [PATCH 10/14] Remove BlockedRegistries and add ghcr.io as default AllowedRegistries --- cmd/main.go | 12 +- cmdutils/suite_test.go | 4 +- docs/README.md | 29 ++- .../serverbootconfig_helpers_test.go | 4 +- internal/controller/suite_test.go | 4 +- internal/registry/validation.go | 42 ++-- internal/registry/validation_test.go | 199 ++++++++---------- server/imageproxyserver.go | 64 ++++-- 8 files changed, 190 insertions(+), 168 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 0faf7028..ab05ab09 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -81,7 +81,6 @@ func main() { var imageServerURL string var architecture string var allowedRegistries string - var blockedRegistries string flag.StringVar(&architecture, "architecture", "amd64", "Target system architecture (e.g., amd64, arm64)") flag.IntVar(&ipxeServicePort, "ipxe-service-port", 5000, "IPXE Service port to listen on.") @@ -101,8 +100,7 @@ func main() { flag.BoolVar(&secureMetrics, "metrics-secure", true, "If set the metrics endpoint is served securely") flag.BoolVar(&enableHTTP2, "enable-http2", false, "If set, HTTP/2 will be enabled for the metrics and webhook servers") - flag.StringVar(&allowedRegistries, "allowed-registries", "", "Comma-separated list of allowed OCI registries. If set, only these registries are permitted.") - flag.StringVar(&blockedRegistries, "blocked-registries", "", "Comma-separated list of blocked OCI registries. If set, these registries are denied.") + flag.StringVar(&allowedRegistries, "allowed-registries", "", "Comma-separated list of allowed OCI registries. Defaults to ghcr.io if not set.") controllers := switches.New( // core controllers @@ -233,8 +231,12 @@ func main() { } // Initialize registry validator for OCI image validation - registryValidator := registry.NewValidator(allowedRegistries, blockedRegistries) - setupLog.Info("Initialized registry validator", "allowedRegistries", allowedRegistries, "blockedRegistries", blockedRegistries) + registryValidator := registry.NewValidator(allowedRegistries) + if allowedRegistries == "" { + setupLog.Info("Initialized registry validator", "allowedRegistries", "ghcr.io (default)") + } else { + setupLog.Info("Initialized registry validator", "allowedRegistries", allowedRegistries) + } if controllers.Enabled(ipxeBootConfigController) { if err = (&controller.IPXEBootConfigReconciler{ diff --git a/cmdutils/suite_test.go b/cmdutils/suite_test.go index 75bd8f74..38f619ad 100644 --- a/cmdutils/suite_test.go +++ b/cmdutils/suite_test.go @@ -59,7 +59,7 @@ var _ = BeforeSuite(func() { // Note that you must have the required binaries setup under the bin directory to perform // the tests directly. When we run make test it will be setup and used automatically. BinaryAssetsDirectory: filepath.Join("..", "bin", "k8s", - fmt.Sprintf("1.34.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + fmt.Sprintf("1.35.0-%s-%s", runtime.GOOS, runtime.GOARCH)), } sourceCfg, err := sourceEnv.Start() @@ -89,7 +89,7 @@ var _ = BeforeSuite(func() { // Note that you must have the required binaries setup under the bin directory to perform // the tests directly. When we run make test it will be setup and used automatically. BinaryAssetsDirectory: filepath.Join("..", "bin", "k8s", - fmt.Sprintf("1.34.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + fmt.Sprintf("1.35.0-%s-%s", runtime.GOOS, runtime.GOARCH)), } // cfg is defined in this file globally. diff --git a/docs/README.md b/docs/README.md index 4a6f6b8e..4244243c 100644 --- a/docs/README.md +++ b/docs/README.md @@ -48,17 +48,28 @@ These servers leverage Kubernetes controllers and API objects to manage the boot Boot Operator enforces OCI registry restrictions at two levels: -1. **Controller level (early validation):** The PXE and HTTP boot controllers validate image references against the registry allow/block list during reconciliation. This means misconfigured or disallowed registries are rejected immediately when a `ServerBootConfiguration` is created, providing fast feedback before any machine attempts to boot. +1. **Controller level (early validation):** The PXE and HTTP boot controllers validate image references against the registry allow list during reconciliation. This means misconfigured or disallowed registries are rejected immediately when a `ServerBootConfiguration` is created, providing fast feedback before any machine attempts to boot. 2. **Image Proxy Server level (runtime enforcement):** The image proxy server also validates registry domains before proxying layer downloads, acting as a second line of defense. -Registry restrictions are configured via CLI flags on the manager binary: +Registry restrictions are configured via the `--allowed-registries` CLI flag on the manager binary. -| Flag | Description | -|------|-------------| -| `--allowed-registries` | Comma-separated list of permitted registries (allowlist mode). Only these registries are accepted. | -| `--blocked-registries` | Comma-separated list of denied registries (blocklist mode). All registries except these are accepted. | +### Default Behavior -- If `--allowed-registries` is set, it takes precedence over `--blocked-registries`. -- If neither flag is set, all registries are **denied** (fail-closed). -- Docker Hub variants (`docker.io`, `index.docker.io`, `registry-1.docker.io`) are normalized for consistent matching. \ No newline at end of file +By default (when `--allowed-registries` is not set), Boot Operator allows only **ghcr.io** registry. This provides a secure-by-default configuration with zero configuration needed for the common case. + +### Custom Configuration + +To allow additional registries or replace the default, use the `--allowed-registries` flag with a comma-separated list: + +```bash +--allowed-registries=ghcr.io,registry.example.com,keppel.global.cloud.sap +``` + +**Important:** When you set `--allowed-registries`, it completely replaces the default. If you want to use ghcr.io along with other registries, you must explicitly include `ghcr.io` in your list. + +### Registry Matching + +- Docker Hub variants (`docker.io`, `index.docker.io`, `registry-1.docker.io`) are normalized to `docker.io` for consistent matching. +- All registry domain matching is case-insensitive. +- Registries not in the allow list are denied. diff --git a/internal/controller/serverbootconfig_helpers_test.go b/internal/controller/serverbootconfig_helpers_test.go index 087639ce..cbcb695a 100644 --- a/internal/controller/serverbootconfig_helpers_test.go +++ b/internal/controller/serverbootconfig_helpers_test.go @@ -46,9 +46,9 @@ func TestBuildImageReference(t *testing.T) { }, { name: "tagged reference with complex tag", - imageName: "keppel.global.cloud.sap/ironcore/gardenlinux-iso", + imageName: "registry.example.com/ironcore/gardenlinux-iso", imageVersion: "arm64-v1.0.0-alpha", - want: "keppel.global.cloud.sap/ironcore/gardenlinux-iso:arm64-v1.0.0-alpha", + want: "registry.example.com/ironcore/gardenlinux-iso:arm64-v1.0.0-alpha", }, } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index ff9c3bcb..e89ba00c 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -92,7 +92,7 @@ var _ = BeforeSuite(func() { // Note that you must have the required binaries setup under the bin directory to perform // the tests directly. When we run make test it will be setup and used automatically. BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s", - fmt.Sprintf("1.34.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + fmt.Sprintf("1.35.0-%s-%s", runtime.GOOS, runtime.GOARCH)), } var err error @@ -148,7 +148,7 @@ func SetupTest() *corev1.Namespace { }) Expect(err).ToNot(HaveOccurred()) - registryValidator := registry.NewValidator(allowedRegistries, "") + registryValidator := registry.NewValidator(allowedRegistries) Expect((&ServerBootConfigurationPXEReconciler{ Client: k8sManager.GetClient(), diff --git a/internal/registry/validation.go b/internal/registry/validation.go index bb9bcf7a..ca185d60 100644 --- a/internal/registry/validation.go +++ b/internal/registry/validation.go @@ -15,37 +15,40 @@ const ( DefaultRegistry = "registry-1.docker.io" // DockerHubDomain is the canonical short domain for Docker Hub DockerHubDomain = "docker.io" + // DefaultAllowedRegistry is the default registry allowed when no configuration is provided + DefaultAllowedRegistry = "ghcr.io" ) -// Validator provides registry validation with configurable allow/block lists. +// Validator provides registry validation with configurable allow list. +// If no allowed registries are configured, it defaults to allowing ghcr.io only. type Validator struct { AllowedRegistries string - BlockedRegistries string } -// NewValidator creates a new Validator with the given allowed and blocked registry lists. -// Each list is a comma-separated string of registry domains. -func NewValidator(allowedRegistries, blockedRegistries string) *Validator { +// NewValidator creates a new Validator with the given allowed registry list. +// The list is a comma-separated string of registry domains. +// If empty, defaults to allowing ghcr.io only. +func NewValidator(allowedRegistries string) *Validator { return &Validator{ AllowedRegistries: allowedRegistries, - BlockedRegistries: blockedRegistries, } } // ExtractRegistryDomain extracts the registry domain from an OCI image reference // using the canonical Docker reference parser from github.com/distribution/reference. -func ExtractRegistryDomain(imageRef string) string { +// Returns an error if the image reference is malformed. +func ExtractRegistryDomain(imageRef string) (string, error) { named, err := reference.ParseNormalizedNamed(imageRef) if err != nil { - return DefaultRegistry + return "", fmt.Errorf("invalid image reference: %w", err) } domain := reference.Domain(named) // The reference library normalizes Docker Hub to "docker.io", // but we use "registry-1.docker.io" as our canonical constant. if domain == DockerHubDomain { - return DefaultRegistry + return DefaultRegistry, nil } - return domain + return domain, nil } // normalizeDockerHubDomain normalizes Docker Hub domain variants to canonical form. @@ -80,29 +83,28 @@ func isInList(registry string, list string) bool { return false } -// IsRegistryAllowed checks if a registry is allowed based on the allow/block lists. +// IsRegistryAllowed checks if a registry is allowed based on the allow list. +// If no allowed registries are configured, it defaults to allowing ghcr.io only. func (v *Validator) IsRegistryAllowed(registry string) bool { if v.AllowedRegistries != "" { return isInList(registry, v.AllowedRegistries) } - if v.BlockedRegistries != "" { - return !isInList(registry, v.BlockedRegistries) - } - - return false + // Default to allowing ghcr.io when no configuration is provided + return normalizeDockerHubDomain(registry) == DefaultAllowedRegistry } // ValidateImageRegistry validates that an image reference uses an allowed registry. func (v *Validator) ValidateImageRegistry(imageRef string) error { - registry := ExtractRegistryDomain(imageRef) + registry, err := ExtractRegistryDomain(imageRef) + if err != nil { + return err + } if !v.IsRegistryAllowed(registry) { if v.AllowedRegistries != "" { return fmt.Errorf("registry not allowed: %s (allowed registries: %s)", registry, v.AllowedRegistries) - } else if v.BlockedRegistries != "" { - return fmt.Errorf("registry blocked: %s (blocked registries: %s)", registry, v.BlockedRegistries) } - return fmt.Errorf("registry not allowed: %s (no --allowed-registries or --blocked-registries configured, denying all)", registry) + return fmt.Errorf("registry not allowed: %s (only %s is allowed by default, use --allowed-registries to configure)", registry, DefaultAllowedRegistry) } return nil } diff --git a/internal/registry/validation_test.go b/internal/registry/validation_test.go index 686e4c0d..40fccfbd 100644 --- a/internal/registry/validation_test.go +++ b/internal/registry/validation_test.go @@ -12,6 +12,7 @@ func TestExtractRegistryDomain(t *testing.T) { name string imageRef string want string + wantErr bool }{ { name: "ghcr.io with tag", @@ -19,9 +20,9 @@ func TestExtractRegistryDomain(t *testing.T) { want: "ghcr.io", }, { - name: "keppel with tag", - imageRef: "keppel.global.cloud.sap/ironcore/gardenlinux-iso:arm64", - want: "keppel.global.cloud.sap", + name: "custom registry with tag", + imageRef: "registry.example.com/ironcore/gardenlinux-iso:arm64", + want: "registry.example.com", }, { name: "docker hub explicit", @@ -53,12 +54,21 @@ func TestExtractRegistryDomain(t *testing.T) { imageRef: "ghcr.io/namespace/image", want: "ghcr.io", }, + { + name: "malformed reference returns error", + imageRef: "not a valid@image:reference", + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := ExtractRegistryDomain(tt.imageRef) - if got != tt.want { + got, err := ExtractRegistryDomain(tt.imageRef) + if (err != nil) != tt.wantErr { + t.Errorf("ExtractRegistryDomain(%q) error = %v, wantErr %v", tt.imageRef, err, tt.wantErr) + return + } + if !tt.wantErr && got != tt.want { t.Errorf("ExtractRegistryDomain(%q) = %q, want %q", tt.imageRef, got, tt.want) } }) @@ -122,9 +132,9 @@ func TestNormalizeDockerHubDomain(t *testing.T) { want: "ghcr.io", }, { - name: "non-Docker Hub - keppel preserved", - domain: "keppel.global.cloud.sap", - want: "keppel.global.cloud.sap", + name: "non-Docker Hub - custom registry preserved", + domain: "registry.example.com", + want: "registry.example.com", }, } @@ -143,127 +153,91 @@ func TestIsRegistryAllowed(t *testing.T) { name string registry string allowList string - blockList string want bool description string }{ { name: "allowed registry - ghcr.io", registry: "ghcr.io", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + allowList: "ghcr.io,registry.example.com", want: true, description: "ghcr.io is in allow list", }, { - name: "allowed registry - keppel", - registry: "keppel.global.cloud.sap", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + name: "allowed registry - custom", + registry: "registry.example.com", + allowList: "ghcr.io,registry.example.com", want: true, - description: "keppel.global.cloud.sap is in allow list", + description: "registry.example.com is in allow list", }, { name: "blocked registry - docker.io with allow list", registry: "docker.io", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + allowList: "ghcr.io,registry.example.com", want: false, description: "docker.io is NOT in allow list", }, { - name: "allowed with block list - ghcr.io", + name: "default - ghcr.io allowed when no config", registry: "ghcr.io", allowList: "", - blockList: "docker.io,registry-1.docker.io", want: true, - description: "ghcr.io is NOT in block list", + description: "ghcr.io is allowed by default when no configuration", }, { - name: "blocked with block list - docker.io", + name: "default - docker.io blocked when no config", registry: "docker.io", allowList: "", - blockList: "docker.io,registry-1.docker.io", want: false, - description: "docker.io is in block list", + description: "docker.io is blocked by default when no configuration", }, { - name: "blocked with block list - registry-1.docker.io", - registry: "registry-1.docker.io", + name: "default - other registry blocked when no config", + registry: "registry.example.com", allowList: "", - blockList: "docker.io,registry-1.docker.io", want: false, - description: "registry-1.docker.io is in block list", - }, - { - name: "deny all - no lists configured", - registry: "ghcr.io", - allowList: "", - blockList: "", - want: false, - description: "fail-closed: deny all when neither list is set", - }, - { - name: "allow list takes precedence", - registry: "ghcr.io", - allowList: "ghcr.io", - blockList: "ghcr.io", - want: true, - description: "allow list takes precedence over block list", + description: "other registries blocked by default when no configuration", }, { name: "whitespace handling", registry: "ghcr.io", - allowList: " ghcr.io , keppel.global.cloud.sap ", - blockList: "", + allowList: " ghcr.io , registry.example.com ", want: true, description: "handles whitespace in allow list", }, { - name: "case-insensitive Docker Hub - uppercase blocklist", - registry: "docker.io", - allowList: "", - blockList: "DOCKER.IO", - want: false, - description: "docker.io blocked by uppercase DOCKER.IO in blocklist", + name: "case-insensitive non-Docker registry matching", + registry: "GHCR.IO", + allowList: "ghcr.io", + want: true, + description: "all registries are case-insensitive (GHCR.IO matches ghcr.io)", }, { - name: "case-insensitive Docker Hub - mixed case registry", - registry: "DOCKER.IO", + name: "case-insensitive default - uppercase GHCR.IO", + registry: "GHCR.IO", allowList: "", - blockList: "docker.io", - want: false, - description: "uppercase DOCKER.IO blocked by lowercase docker.io in blocklist", + want: true, + description: "uppercase GHCR.IO matches default ghcr.io", }, { - name: "case-insensitive Docker Hub - index variant", - registry: "INDEX.DOCKER.IO", + name: "case-insensitive default - mixed case Ghcr.Io", + registry: "Ghcr.Io", allowList: "", - blockList: "docker.io", - want: false, - description: "INDEX.DOCKER.IO normalized and blocked by docker.io", + want: true, + description: "mixed case Ghcr.Io matches default ghcr.io", }, { - name: "case-insensitive Docker Hub - registry-1 variant", - registry: "REGISTRY-1.DOCKER.IO", - allowList: "", - blockList: "docker.io", + name: "custom allow list replaces default", + registry: "ghcr.io", + allowList: "docker.io,registry.example.com", want: false, - description: "REGISTRY-1.DOCKER.IO normalized and blocked by docker.io", - }, - { - name: "case-insensitive non-Docker registry matching", - registry: "GHCR.IO", - allowList: "ghcr.io", - blockList: "", - want: true, - description: "all registries are case-insensitive (GHCR.IO matches ghcr.io)", + description: "ghcr.io blocked when custom allow list doesn't include it", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - v := NewValidator(tt.allowList, tt.blockList) + v := NewValidator(tt.allowList) got := v.IsRegistryAllowed(tt.registry) if got != tt.want { t.Errorf("IsRegistryAllowed(%q) = %v, want %v (%s)", tt.registry, got, tt.want, tt.description) @@ -277,7 +251,6 @@ func TestValidateImageRegistry(t *testing.T) { name string imageRef string allowList string - blockList string wantErr bool errContains string description string @@ -285,24 +258,21 @@ func TestValidateImageRegistry(t *testing.T) { { name: "allowed - ghcr.io image", imageRef: "ghcr.io/ironcore-dev/os-images/gardenlinux:1877.0", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + allowList: "ghcr.io,registry.example.com", wantErr: false, description: "ghcr.io image should be allowed", }, { - name: "allowed - keppel image", - imageRef: "keppel.global.cloud.sap/ironcore/gardenlinux-iso:arm64", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + name: "allowed - custom registry image", + imageRef: "registry.example.com/ironcore/gardenlinux-iso:arm64", + allowList: "ghcr.io,registry.example.com", wantErr: false, - description: "keppel.global.cloud.sap image should be allowed", + description: "registry.example.com image should be allowed", }, { name: "blocked - docker.io with allow list", imageRef: "docker.io/library/ubuntu:latest", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + allowList: "ghcr.io,registry.example.com", wantErr: true, errContains: "registry not allowed: registry-1.docker.io", description: "docker.io should be blocked when not in allow list", @@ -310,8 +280,7 @@ func TestValidateImageRegistry(t *testing.T) { { name: "blocked - implicit docker hub", imageRef: "ubuntu:latest", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + allowList: "ghcr.io,registry.example.com", wantErr: true, errContains: "registry not allowed: registry-1.docker.io", description: "implicit docker hub should be blocked", @@ -319,44 +288,62 @@ func TestValidateImageRegistry(t *testing.T) { { name: "error shows allowed registries", imageRef: "docker.io/library/alpine:latest", - allowList: "ghcr.io,keppel.global.cloud.sap", - blockList: "", + allowList: "ghcr.io,registry.example.com", wantErr: true, - errContains: "allowed registries: ghcr.io,keppel.global.cloud.sap", + errContains: "allowed registries: ghcr.io,registry.example.com", description: "error message should show the allowed registries", }, { - name: "blocked with block list", + name: "default allows ghcr.io", + imageRef: "ghcr.io/test/image:latest", + allowList: "", + wantErr: false, + description: "ghcr.io should be allowed by default", + }, + { + name: "default blocks docker.io", imageRef: "docker.io/library/nginx:latest", allowList: "", - blockList: "docker.io,registry-1.docker.io", wantErr: true, - errContains: "registry blocked: registry-1.docker.io", - description: "docker.io should be blocked when in block list", + errContains: "only ghcr.io is allowed by default", + description: "docker.io should be blocked by default", }, { - name: "error shows blocked registries", - imageRef: "registry-1.docker.io/library/redis:latest", + name: "default blocks other registries", + imageRef: "registry.example.com/test/image:latest", allowList: "", - blockList: "docker.io,registry-1.docker.io", wantErr: true, - errContains: "blocked registries: docker.io,registry-1.docker.io", - description: "error message should show the blocked registries", + errContains: "only ghcr.io is allowed by default", + description: "other registries should be blocked by default", }, { - name: "fail-closed no configuration", + name: "custom list replaces default - ghcr.io blocked", imageRef: "ghcr.io/test/image:latest", - allowList: "", - blockList: "", + allowList: "docker.io,registry.example.com", + wantErr: true, + errContains: "registry not allowed: ghcr.io", + description: "ghcr.io should be blocked when custom list doesn't include it", + }, + { + name: "custom list replaces default - docker.io allowed", + imageRef: "docker.io/library/redis:latest", + allowList: "docker.io,registry.example.com", + wantErr: false, + description: "docker.io should be allowed when in custom list", + }, + { + name: "malformed image reference rejected", + imageRef: "not a valid@image:reference", + allowList: "ghcr.io,docker.io", wantErr: true, - errContains: "no --allowed-registries or --blocked-registries configured", - description: "should deny all when no configuration", + errContains: "invalid image reference", + description: "malformed image references should be rejected during parsing", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - v := NewValidator(tt.allowList, tt.blockList) + v := NewValidator(tt.allowList) err := v.ValidateImageRegistry(tt.imageRef) if (err != nil) != tt.wantErr { t.Errorf("ValidateImageRegistry(%q) error = %v, wantErr %v (%s)", tt.imageRef, err, tt.wantErr, tt.description) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index e7548427..ea94e415 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -80,12 +80,16 @@ const ( // Shared HTTP client for all registry operations to enable connection reuse var httpClient = &http.Client{ Timeout: 30 * time.Second, - Transport: &http.Transport{ - MaxIdleConnsPerHost: 10, // Connection pool per host - IdleConnTimeout: 90 * time.Second, // Keep-alive duration - TLSHandshakeTimeout: 10 * time.Second, // Security timeout - ExpectContinueTimeout: 1 * time.Second, // Reduce latency - }, + Transport: func() *http.Transport { + // Clone http.DefaultTransport to inherit proxy settings from environment + transport := http.DefaultTransport.(*http.Transport).Clone() + // Override specific fields for registry operations + transport.MaxIdleConnsPerHost = 10 // Connection pool per host + transport.IdleConnTimeout = 90 * time.Second // Keep-alive duration + transport.TLSHandshakeTimeout = 10 * time.Second // Security timeout + transport.ExpectContinueTimeout = 1 * time.Second // Reduce latency + return transport + }(), } // Parse WWW-Authenticate parameter value @@ -249,10 +253,11 @@ func cleanupExpiredCacheEntries(log logr.Logger) { delete(registryCache, key) } + remainingCount := len(registryCache) registryCacheMutex.Unlock() if len(expiredKeys) > 0 { - log.V(1).Info("Cleaned up expired cache entries", "count", len(expiredKeys), "remainingEntries", len(registryCache)) + log.V(1).Info("Cleaned up expired cache entries", "count", len(expiredKeys), "remainingEntries", remainingCount) } } } @@ -303,7 +308,10 @@ func parseHttpBootImagePath(path string) (ImageDetails, error) { digestSegment := segments[len(segments)-1] // Extract registry domain and repository using distribution/reference - registryDomain := registry.ExtractRegistryDomain(imageName) + registryDomain, err := registry.ExtractRegistryDomain(imageName) + if err != nil { + return ImageDetails{}, err + } named, err := reference.ParseNormalizedNamed(imageName) if err != nil { return ImageDetails{}, fmt.Errorf("invalid image reference: %w", err) @@ -333,7 +341,7 @@ func handleDockerRegistry(w http.ResponseWriter, r *http.Request, imageDetails * if !validator.IsRegistryAllowed(registryDomain) { http.Error(w, "Forbidden: Registry not allowed", http.StatusForbidden) - log.Info("Registry blocked", "registry", registryDomain, "allowList", validator.AllowedRegistries, "blockList", validator.BlockedRegistries) + log.Info("Registry blocked", "registry", registryDomain, "allowList", validator.AllowedRegistries) return } @@ -411,12 +419,14 @@ func buildModifyResponse() func(*http.Response) error { } // Security: Strip sensitive headers on cross-host redirects to prevent - // leaking credentials (e.g., bearer tokens) to third-party CDN/mirrors - if isSameHost(resp.Request.URL, location) { - // Same-host redirect: preserve all headers including Authorization + // leaking credentials (e.g., bearer tokens) to third-party CDN/mirrors. + // Also strip on HTTPS→HTTP downgrades even if same host. + isSchemeDowngrade := resp.Request.URL.Scheme == "https" && location.Scheme == "http" + if isSameHost(resp.Request.URL, location) && !isSchemeDowngrade { + // Same-host redirect without scheme downgrade: preserve all headers including Authorization copyHeaders(resp.Request.Header, redirectReq.Header) } else { - // Cross-host redirect: exclude sensitive auth headers + // Cross-host redirect or HTTPS→HTTP downgrade: exclude sensitive auth headers copyHeadersExcluding(resp.Request.Header, redirectReq.Header, []string{"Authorization", "Cookie", "Proxy-Authorization"}) } @@ -484,19 +494,26 @@ func isSameHost(url1, url2 *url.URL) bool { } func replaceResponse(originalResp, redirectResp *http.Response) { - // Preserve all values for multi-valued headers (e.g., Set-Cookie) - for name, values := range redirectResp.Header { - originalResp.Header.Del(name) // Clear existing values first - for _, value := range values { - originalResp.Header.Add(name, value) // Add all values - } - } // Close the original body to prevent connection leaks if originalResp.Body != nil { _ = originalResp.Body.Close() } - originalResp.Body = redirectResp.Body + + // Replace all response metadata from redirectResp + originalResp.Status = redirectResp.Status originalResp.StatusCode = redirectResp.StatusCode + originalResp.Proto = redirectResp.Proto + originalResp.ProtoMajor = redirectResp.ProtoMajor + originalResp.ProtoMinor = redirectResp.ProtoMinor + originalResp.Header = redirectResp.Header.Clone() + originalResp.Body = redirectResp.Body + originalResp.ContentLength = redirectResp.ContentLength + originalResp.TransferEncoding = redirectResp.TransferEncoding + originalResp.Close = redirectResp.Close + originalResp.Uncompressed = redirectResp.Uncompressed + originalResp.Trailer = redirectResp.Trailer + originalResp.TLS = redirectResp.TLS + // Preserve originalResp.Request - it must point to the original request } func parseImageURL(queries url.Values) (imageDetails ImageDetails, err error) { @@ -511,7 +528,10 @@ func parseImageURL(queries url.Values) (imageDetails ImageDetails, err error) { ociImageName = strings.TrimSuffix(ociImageName, ".efi") // Extract registry domain and repository using distribution/reference - registryDomain := registry.ExtractRegistryDomain(ociImageName) + registryDomain, err := registry.ExtractRegistryDomain(ociImageName) + if err != nil { + return ImageDetails{}, err + } named, err := reference.ParseNormalizedNamed(ociImageName) if err != nil { return ImageDetails{}, fmt.Errorf("invalid image reference: %w", err) From c4dcd52925ce712c65a21936051cc286443cdd87 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Tue, 10 Mar 2026 12:40:41 +0000 Subject: [PATCH 11/14] Fixes re comments --- docs/README.md | 10 ++++------ server/imageproxyserver.go | 14 ++++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/README.md b/docs/README.md index 4244243c..d7190162 100644 --- a/docs/README.md +++ b/docs/README.md @@ -29,12 +29,10 @@ Boot Operator includes the following key components: - **Image Proxy Server** - Handles `/image` requests - - Extracts layers from OCI (Open Container Initiative) images, with support for multiple registries (e.g., GHCR, Docker Hub, Keppel, and any OCI-compliant registry) + - Extracts layers from OCI (Open Container Initiative) images, with support for multiple registries (e.g., GHCR, Docker Hub, and any OCI-compliant registry) - Downloads specific layers based on the requested URI and image specifications - - Registry access is controlled via CLI flags: - - `--allowed-registries`: comma-separated list of permitted registries (allowlist mode) - - `--blocked-registries`: comma-separated list of denied registries (blocklist mode) - - If neither flag is set, all registries are denied (fail-closed) + - Registry access is controlled via the `--allowed-registries` CLI flag (comma-separated list) + - By default (when not specified), only **ghcr.io** is allowed - Example: - `wget http://SERVER_ADDRESS:30007/image?imageName=ghcr.io/ironcore-dev/os-images/gardenlinux&version=1443.10&layerName=application/vnd.ironcore.image.squashfs.v1alpha1.squashfs` @@ -63,7 +61,7 @@ By default (when `--allowed-registries` is not set), Boot Operator allows only * To allow additional registries or replace the default, use the `--allowed-registries` flag with a comma-separated list: ```bash ---allowed-registries=ghcr.io,registry.example.com,keppel.global.cloud.sap +--allowed-registries=ghcr.io,registry.example.com,quay.io ``` **Important:** When you set `--allowed-registries`, it completely replaces the default. If you want to use ghcr.io along with other registries, you must explicitly include `ghcr.io` in your list. diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index ea94e415..bb3e3302 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -79,17 +79,18 @@ const ( // Shared HTTP client for all registry operations to enable connection reuse var httpClient = &http.Client{ - Timeout: 30 * time.Second, Transport: func() *http.Transport { // Clone http.DefaultTransport to inherit proxy settings from environment transport := http.DefaultTransport.(*http.Transport).Clone() // Override specific fields for registry operations - transport.MaxIdleConnsPerHost = 10 // Connection pool per host - transport.IdleConnTimeout = 90 * time.Second // Keep-alive duration - transport.TLSHandshakeTimeout = 10 * time.Second // Security timeout - transport.ExpectContinueTimeout = 1 * time.Second // Reduce latency + transport.MaxIdleConnsPerHost = 10 // Connection pool per host + transport.IdleConnTimeout = 90 * time.Second // Keep-alive duration + transport.TLSHandshakeTimeout = 10 * time.Second // Security timeout + transport.ExpectContinueTimeout = 1 * time.Second // Reduce latency + transport.ResponseHeaderTimeout = 30 * time.Second // Timeout for response headers only return transport }(), + // No Timeout at client level - allows unlimited body streaming for large image layers } // Parse WWW-Authenticate parameter value @@ -402,8 +403,9 @@ func buildDirector(proxyURL *url.URL, bearerToken string, repository string, dig func buildModifyResponse() func(*http.Response) error { return func(resp *http.Response) error { - // Handle redirects (307, 301, 302, 303) + // Handle redirects (307, 308, 301, 302, 303) if resp.StatusCode == http.StatusTemporaryRedirect || + resp.StatusCode == http.StatusPermanentRedirect || resp.StatusCode == http.StatusMovedPermanently || resp.StatusCode == http.StatusFound || resp.StatusCode == http.StatusSeeOther { From 450a32be64af0da8555c58fbad22cd8de405ab58 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Tue, 10 Mar 2026 12:59:29 +0000 Subject: [PATCH 12/14] lint fix --- server/imageproxyserver.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server/imageproxyserver.go b/server/imageproxyserver.go index bb3e3302..92d10b88 100644 --- a/server/imageproxyserver.go +++ b/server/imageproxyserver.go @@ -77,20 +77,20 @@ const ( maxTokenResponseSize = 64 * 1024 // 64KB - token responses are typically a few hundred bytes ) -// Shared HTTP client for all registry operations to enable connection reuse +// Shared HTTP client for all registry operations to enable connection reuse. +// No Timeout at client level - allows unlimited body streaming for large image layers. var httpClient = &http.Client{ Transport: func() *http.Transport { // Clone http.DefaultTransport to inherit proxy settings from environment transport := http.DefaultTransport.(*http.Transport).Clone() // Override specific fields for registry operations - transport.MaxIdleConnsPerHost = 10 // Connection pool per host - transport.IdleConnTimeout = 90 * time.Second // Keep-alive duration - transport.TLSHandshakeTimeout = 10 * time.Second // Security timeout - transport.ExpectContinueTimeout = 1 * time.Second // Reduce latency - transport.ResponseHeaderTimeout = 30 * time.Second // Timeout for response headers only + transport.MaxIdleConnsPerHost = 10 // Connection pool per host + transport.IdleConnTimeout = 90 * time.Second // Keep-alive duration + transport.TLSHandshakeTimeout = 10 * time.Second // Security timeout + transport.ExpectContinueTimeout = 1 * time.Second // Reduce latency + transport.ResponseHeaderTimeout = 30 * time.Second // Timeout for response headers only return transport }(), - // No Timeout at client level - allows unlimited body streaming for large image layers } // Parse WWW-Authenticate parameter value From b0a030d6ea81ef65d94f36875a52cde5327c8870 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Wed, 11 Mar 2026 10:26:51 +0000 Subject: [PATCH 13/14] Add Validation failure to ServerBootConfiguration conditions --- .../controller/serverbootconfig_helpers.go | 28 +++++ .../serverbootconfig_helpers_test.go | 115 ++++++++++++++++++ ...serverbootconfiguration_http_controller.go | 8 +- .../serverbootconfiguration_pxe_controller.go | 16 +-- 4 files changed, 150 insertions(+), 17 deletions(-) diff --git a/internal/controller/serverbootconfig_helpers.go b/internal/controller/serverbootconfig_helpers.go index ef43d7ad..af57c608 100644 --- a/internal/controller/serverbootconfig_helpers.go +++ b/internal/controller/serverbootconfig_helpers.go @@ -28,6 +28,8 @@ import ( metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" ocispec "github.com/opencontainers/image-spec/specs-go/v1" corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -217,3 +219,29 @@ func fetchContent(ctx context.Context, resolver remotes.Resolver, ref string, de return data, nil } + +// PatchServerBootConfigWithError updates the ServerBootConfiguration state to Error +// and sets an ImageValidation condition with the error details. +func PatchServerBootConfigWithError( + ctx context.Context, + c client.Client, + namespacedName types.NamespacedName, + err error, +) error { + var cur metalv1alpha1.ServerBootConfiguration + if fetchErr := c.Get(ctx, namespacedName, &cur); fetchErr != nil { + return fmt.Errorf("failed to fetch ServerBootConfiguration: %w", fetchErr) + } + base := cur.DeepCopy() + + cur.Status.State = metalv1alpha1.ServerBootConfigurationStateError + apimeta.SetStatusCondition(&cur.Status.Conditions, metav1.Condition{ + Type: "ImageValidation", + Status: metav1.ConditionFalse, + Reason: "ValidationFailed", + Message: err.Error(), + ObservedGeneration: cur.Generation, + }) + + return c.Status().Patch(ctx, &cur, client.MergeFrom(base)) +} diff --git a/internal/controller/serverbootconfig_helpers_test.go b/internal/controller/serverbootconfig_helpers_test.go index cbcb695a..9806d2eb 100644 --- a/internal/controller/serverbootconfig_helpers_test.go +++ b/internal/controller/serverbootconfig_helpers_test.go @@ -4,7 +4,17 @@ package controller import ( + "errors" "testing" + + metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" ) func TestBuildImageReference(t *testing.T) { @@ -61,3 +71,108 @@ func TestBuildImageReference(t *testing.T) { }) } } + +var _ = Describe("PatchServerBootConfigWithError", func() { + var ns *corev1.Namespace + + BeforeEach(func(ctx SpecContext) { + By("creating a test namespace") + ns = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + } + Expect(k8sClient.Create(ctx, ns)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ns) + }) + + It("should patch ServerBootConfiguration with error state and condition", func(ctx SpecContext) { + By("creating a ServerBootConfiguration") + config := &metalv1alpha1.ServerBootConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: ns.Name, + }, + Spec: metalv1alpha1.ServerBootConfigurationSpec{ + ServerRef: corev1.LocalObjectReference{Name: "test-server"}, + Image: "test-image:latest", + }, + } + Expect(k8sClient.Create(ctx, config)).To(Succeed()) + + By("patching with error") + testErr := errors.New("registry validation failed: registry docker.io is not in the allowed list") + err := PatchServerBootConfigWithError(ctx, k8sClient, + types.NamespacedName{Name: config.Name, Namespace: config.Namespace}, testErr) + Expect(err).NotTo(HaveOccurred()) + + By("verifying the state is set to Error") + Eventually(Object(config)).Should(SatisfyAll( + HaveField("Status.State", metalv1alpha1.ServerBootConfigurationStateError), + )) + + By("verifying the ImageValidation condition is set") + var updated metalv1alpha1.ServerBootConfiguration + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: config.Name, Namespace: config.Namespace}, &updated)).To(Succeed()) + + condition := apimeta.FindStatusCondition(updated.Status.Conditions, "ImageValidation") + Expect(condition).NotTo(BeNil()) + Expect(condition.Type).To(Equal("ImageValidation")) + Expect(condition.Status).To(Equal(metav1.ConditionFalse)) + Expect(condition.Reason).To(Equal("ValidationFailed")) + Expect(condition.Message).To(Equal(testErr.Error())) + Expect(condition.ObservedGeneration).To(Equal(updated.Generation)) + }) + + It("should return error when ServerBootConfiguration does not exist", func(ctx SpecContext) { + By("attempting to patch non-existent config") + testErr := errors.New("some error") + err := PatchServerBootConfigWithError(ctx, k8sClient, + types.NamespacedName{Name: "non-existent", Namespace: ns.Name}, testErr) + + By("verifying error is returned") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to fetch ServerBootConfiguration")) + }) + + It("should update existing condition if called multiple times", func(ctx SpecContext) { + By("creating a ServerBootConfiguration") + config := &metalv1alpha1.ServerBootConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config-update", + Namespace: ns.Name, + }, + Spec: metalv1alpha1.ServerBootConfigurationSpec{ + ServerRef: corev1.LocalObjectReference{Name: "test-server"}, + Image: "test-image:latest", + }, + } + Expect(k8sClient.Create(ctx, config)).To(Succeed()) + + By("patching with first error") + firstErr := errors.New("first error message") + err := PatchServerBootConfigWithError(ctx, k8sClient, + types.NamespacedName{Name: config.Name, Namespace: config.Namespace}, firstErr) + Expect(err).NotTo(HaveOccurred()) + + By("patching with second error") + secondErr := errors.New("second error message") + err = PatchServerBootConfigWithError(ctx, k8sClient, + types.NamespacedName{Name: config.Name, Namespace: config.Namespace}, secondErr) + Expect(err).NotTo(HaveOccurred()) + + By("verifying only one condition exists with latest message") + var updated metalv1alpha1.ServerBootConfiguration + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: config.Name, Namespace: config.Namespace}, &updated)).To(Succeed()) + + conditions := updated.Status.Conditions + imageValidationConditions := 0 + for _, c := range conditions { + if c.Type == "ImageValidation" { + imageValidationConditions++ + Expect(c.Message).To(Equal(secondErr.Error())) + } + } + Expect(imageValidationConditions).To(Equal(1)) + }) +}) diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index c41e9aa3..d2061fc8 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -89,13 +89,11 @@ func (r *ServerBootConfigurationHTTPReconciler) reconcile(ctx context.Context, l ukiURL, err := r.constructUKIURL(ctx, config.Spec.Image) if err != nil { log.Error(err, "Failed to construct UKI URL") - if patchErr := r.patchConfigStateFromHTTPState(ctx, - &bootv1alpha1.HTTPBootConfig{Status: bootv1alpha1.HTTPBootConfigStatus{ - State: bootv1alpha1.HTTPBootConfigStateError}}, - config); patchErr != nil { + if patchErr := PatchServerBootConfigWithError(ctx, r.Client, + types.NamespacedName{Name: config.Name, Namespace: config.Namespace}, err); patchErr != nil { return ctrl.Result{}, fmt.Errorf("failed to patch state to error: %w (original error: %w)", patchErr, err) } - return ctrl.Result{}, fmt.Errorf("failed to construct UKI URL: %w", err) + return ctrl.Result{}, err } log.V(1).Info("Extracted UKI URL for boot") diff --git a/internal/controller/serverbootconfiguration_pxe_controller.go b/internal/controller/serverbootconfiguration_pxe_controller.go index 2f09dc5d..aa924f95 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller.go +++ b/internal/controller/serverbootconfiguration_pxe_controller.go @@ -106,10 +106,11 @@ func (r *ServerBootConfigurationPXEReconciler) reconcile(ctx context.Context, lo kernelURL, initrdURL, squashFSURL, err := r.getImageDetailsFromConfig(ctx, bootConfig) if err != nil { - if err := r.patchState(ctx, bootConfig, metalv1alpha1.ServerBootConfigurationStateError); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to patch server boot config state to %s: %w", metalv1alpha1.ServerBootConfigurationStateError, err) + if patchErr := PatchServerBootConfigWithError(ctx, r.Client, + types.NamespacedName{Name: bootConfig.Name, Namespace: bootConfig.Namespace}, err); patchErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to patch server boot config state: %w (original error: %w)", patchErr, err) } - return ctrl.Result{}, fmt.Errorf("failed to get image details from BootConfig: %w", err) + return ctrl.Result{}, err } log.V(1).Info("Extracted OS image layer details") @@ -178,15 +179,6 @@ func (r *ServerBootConfigurationPXEReconciler) patchConfigStateFromIPXEState(ctx return r.Status().Patch(ctx, bootConfig, client.MergeFrom(bootConfigBase)) } -func (r *ServerBootConfigurationPXEReconciler) patchState(ctx context.Context, config *metalv1alpha1.ServerBootConfiguration, state metalv1alpha1.ServerBootConfigurationState) error { - configBase := config.DeepCopy() - config.Status.State = state - if err := r.Status().Patch(ctx, config, client.MergeFrom(configBase)); err != nil { - return err - } - return nil -} - func (r *ServerBootConfigurationPXEReconciler) getSystemUUIDFromBootConfig(ctx context.Context, config *metalv1alpha1.ServerBootConfiguration) (string, error) { server := &metalv1alpha1.Server{} if err := r.Get(ctx, client.ObjectKey{Name: config.Spec.ServerRef.Name}, server); err != nil { From f547bf16c4de6174fc873ee889a0f6ef72e7e7ed Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Thu, 12 Mar 2026 09:58:57 +0000 Subject: [PATCH 14/14] Remove conditions when setting ready state --- internal/controller/serverbootconfiguration_http_controller.go | 2 ++ internal/controller/serverbootconfiguration_pxe_controller.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index d2061fc8..7f80112b 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -154,6 +154,8 @@ func (r *ServerBootConfigurationHTTPReconciler) patchConfigStateFromHTTPState(ct switch httpBootConfig.Status.State { case bootv1alpha1.HTTPBootConfigStateReady: cur.Status.State = metalv1alpha1.ServerBootConfigurationStateReady + // Remove ImageValidation condition when transitioning to Ready + apimeta.RemoveStatusCondition(&cur.Status.Conditions, "ImageValidation") case bootv1alpha1.HTTPBootConfigStateError: cur.Status.State = metalv1alpha1.ServerBootConfigurationStateError } diff --git a/internal/controller/serverbootconfiguration_pxe_controller.go b/internal/controller/serverbootconfiguration_pxe_controller.go index aa924f95..30ef100b 100644 --- a/internal/controller/serverbootconfiguration_pxe_controller.go +++ b/internal/controller/serverbootconfiguration_pxe_controller.go @@ -168,6 +168,8 @@ func (r *ServerBootConfigurationPXEReconciler) patchConfigStateFromIPXEState(ctx switch config.Status.State { case v1alpha1.IPXEBootConfigStateReady: bootConfig.Status.State = metalv1alpha1.ServerBootConfigurationStateReady + // Remove ImageValidation condition when transitioning to Ready + apimeta.RemoveStatusCondition(&bootConfig.Status.Conditions, "ImageValidation") case v1alpha1.IPXEBootConfigStateError: bootConfig.Status.State = metalv1alpha1.ServerBootConfigurationStateError }