From c4b75fbbcf24ed12ddc96b6e71726a0d4e54c643 Mon Sep 17 00:00:00 2001 From: pratap0007 Date: Thu, 10 Sep 2026 12:04:08 +0530 Subject: [PATCH] fix(hub): add HTTPS enforcement for manifest downloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `tkn hub install` command fetched resource manifests by calling httpGet() on a URL returned verbatim from the Hub API with no security validation. This allowed supply-chain attacks where an attacker controlling the network path or Hub API response could inject arbitrary Kubernetes manifests. Security improvements: - Enforce HTTPS-only for manifest downloads (blocks HTTP MITM) - Block localhost URLs (prevents local SSRF attacks) - Require TLS 1.2+ with strong cipher suites (ECDHE-AES-GCM) - Enforce 10MB size limit (prevents DoS via large files) - Validate all redirects are HTTPS (prevents downgrade attacks) - Add SHA256 digest verification infrastructure (ready for use) Changes: - Modified: pkg/cmd/hub/hub/get_resource.go (httpGet→secureHTTPGet) - Added: pkg/cmd/hub/hub/secure_fetch.go (security layer) - Added: pkg/cmd/hub/hub/secure_fetch_test.go (35+ test cases) Signed-off-by: pratap0007 Assisted-by: Claude Sonnet 4.5 (via Claude Code) --- pkg/cmd/hub/hub/get_resource.go | 3 +- pkg/cmd/hub/hub/secure_fetch.go | 223 +++++++++++++++ pkg/cmd/hub/hub/secure_fetch_test.go | 390 +++++++++++++++++++++++++++ 3 files changed, 615 insertions(+), 1 deletion(-) create mode 100644 pkg/cmd/hub/hub/secure_fetch.go create mode 100644 pkg/cmd/hub/hub/secure_fetch_test.go diff --git a/pkg/cmd/hub/hub/get_resource.go b/pkg/cmd/hub/hub/get_resource.go index bc7e747fd4..fa7dde9d34 100644 --- a/pkg/cmd/hub/hub/get_resource.go +++ b/pkg/cmd/hub/hub/get_resource.go @@ -315,7 +315,8 @@ func (rr *TektonHubResourceResult) Manifest() ([]byte, error) { return nil, err } - data, status, err := httpGet(rawURL) + // Use secure fetch with URL validation and TLS enforcement + data, status, err := secureHTTPGet(rawURL) if err != nil { return nil, err diff --git a/pkg/cmd/hub/hub/secure_fetch.go b/pkg/cmd/hub/hub/secure_fetch.go new file mode 100644 index 0000000000..5bd643a435 --- /dev/null +++ b/pkg/cmd/hub/hub/secure_fetch.go @@ -0,0 +1,223 @@ +// Copyright © 2026 The Tekton Authors. +// +// 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 hub + +import ( + "crypto/sha256" + "crypto/tls" + "encoding/hex" + "fmt" + "io" + "net" + "net/http" + "net/url" + "strings" + "time" +) + +const ( + // maxManifestSize limits the size of downloaded manifests to prevent memory exhaustion + maxManifestSize = 10 * 1024 * 1024 // 10 MB +) + +var ( + // secureHTTPClient is a singleton HTTP client configured with secure defaults + secureHTTPClient *http.Client +) + +func init() { + secureHTTPClient = createSecureHTTPClient() +} + +// createSecureHTTPClient creates an HTTP client with secure TLS configuration +func createSecureHTTPClient() *http.Client { + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.TLSClientConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + // Use strong cipher suites only + CipherSuites: []uint16{ + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + }, + } + transport.TLSHandshakeTimeout = 10 * time.Second + + return &http.Client{ + Timeout: 30 * time.Second, + Transport: transport, + // Prevent following redirects to arbitrary locations + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return fmt.Errorf("stopped after 10 redirects") + } + // Validate redirect URL is also HTTPS and resolves to public IP + if err := validateManifestURL(req.URL.String()); err != nil { + return fmt.Errorf("redirect to insecure URL blocked: %w", err) + } + return nil + }, + } +} + +// validateManifestURL validates that a manifest URL meets security requirements +// It resolves the hostname and checks that all resolved IPs are public to prevent SSRF +func validateManifestURL(rawURL string) error { + if rawURL == "" { + return fmt.Errorf("manifest URL cannot be empty") + } + + parsedURL, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("invalid manifest URL: %w", err) + } + + // Enforce HTTPS-only for manifest downloads + if parsedURL.Scheme != "https" { + return fmt.Errorf("insecure manifest URL scheme '%s' not allowed, only HTTPS is permitted", parsedURL.Scheme) + } + + // Validate hostname is present + if parsedURL.Host == "" { + return fmt.Errorf("manifest URL missing hostname") + } + + hostname := parsedURL.Hostname() + + // Resolve hostname to IP addresses to prevent DNS rebinding and hostname-based bypasses + ips, err := net.LookupIP(hostname) + if err != nil { + return fmt.Errorf("failed to resolve manifest hostname %s: %w", hostname, err) + } + + if len(ips) == 0 { + return fmt.Errorf("manifest hostname %s resolves to no addresses", hostname) + } + + // Check that all resolved IPs are public (not private, loopback, or link-local) + for _, ip := range ips { + if !isPublicIP(ip) { + return fmt.Errorf("manifest URL hostname %s resolves to non-public address %s", hostname, ip.String()) + } + } + + return nil +} + +// isPublicIP checks if an IP address is publicly routable +// Returns false for loopback, private, link-local, and multicast addresses +func isPublicIP(ip net.IP) bool { + // Reject loopback addresses (127.0.0.0/8 for IPv4, ::1 for IPv6) + if ip.IsLoopback() { + return false + } + + // Reject private addresses (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7) + if ip.IsPrivate() { + return false + } + + // Reject link-local addresses (169.254.0.0/16 for IPv4, fe80::/10 for IPv6) + if ip.IsLinkLocalUnicast() { + return false + } + + // Reject link-local multicast (224.0.0.0/24 for IPv4, ff02::/16 for IPv6) + if ip.IsLinkLocalMulticast() { + return false + } + + // Reject multicast addresses + if ip.IsMulticast() { + return false + } + + // Reject unspecified addresses (0.0.0.0, ::) + if ip.IsUnspecified() { + return false + } + + return true +} + +// verifyDigest verifies the SHA256 digest of data against expected digest +func verifyDigest(data []byte, expectedDigest string) error { + if expectedDigest == "" { + // No digest provided - skip verification + return nil + } + + // Compute SHA256 hash of the data + hash := sha256.Sum256(data) + actualDigest := hex.EncodeToString(hash[:]) + + // Compare digests (case-insensitive) + if !strings.EqualFold(actualDigest, expectedDigest) { + return fmt.Errorf("manifest digest mismatch: expected %s, got %s", expectedDigest, actualDigest) + } + + return nil +} + +// secureHTTPGet fetches data from a URL with security validations +// This replaces the insecure httpGet function for manifest downloads +func secureHTTPGet(rawURL string) ([]byte, int, error) { + return secureHTTPGetWithDigest(rawURL, "") +} + +// secureHTTPGetWithDigest fetches data from a URL with security validations and optional digest verification +// expectedDigest should be a hex-encoded SHA256 hash, or empty string to skip digest verification +func secureHTTPGetWithDigest(rawURL string, expectedDigest string) ([]byte, int, error) { + // Validate URL before making request + if err := validateManifestURL(rawURL); err != nil { + return nil, 0, fmt.Errorf("manifest URL validation failed: %w", err) + } + + if err := loadConfigFile(); err != nil { + return nil, 0, err + } + + resp, err := secureHTTPClient.Get(rawURL) + if err != nil { + return nil, 0, fmt.Errorf("failed to fetch manifest: %w", err) + } + defer resp.Body.Close() + + // Check for HTTP error status codes + if resp.StatusCode != http.StatusOK { + return nil, resp.StatusCode, fmt.Errorf("HTTP %d from manifest URL", resp.StatusCode) + } + + // Enforce maximum size to prevent memory exhaustion + limitedReader := io.LimitReader(resp.Body, maxManifestSize+1) + data, err := io.ReadAll(limitedReader) + if err != nil { + return nil, 0, fmt.Errorf("failed to read manifest: %w", err) + } + + if len(data) > maxManifestSize { + return nil, 0, fmt.Errorf("manifest exceeds maximum size of %d bytes", maxManifestSize) + } + + // Verify digest if provided + if err := verifyDigest(data, expectedDigest); err != nil { + return nil, 0, err + } + + return data, resp.StatusCode, nil +} diff --git a/pkg/cmd/hub/hub/secure_fetch_test.go b/pkg/cmd/hub/hub/secure_fetch_test.go new file mode 100644 index 0000000000..ec03d6b5b0 --- /dev/null +++ b/pkg/cmd/hub/hub/secure_fetch_test.go @@ -0,0 +1,390 @@ +// Copyright © 2026 The Tekton Authors. +// +// 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 hub + +import ( + "crypto/sha256" + "crypto/tls" + "encoding/hex" + "fmt" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + rclient "github.com/tektoncd/cli/pkg/cmd/hub/gen/http/resource/client" +) + +func TestValidateManifestURL(t *testing.T) { + tests := []struct { + name string + url string + wantErr bool + errMsg string + }{ + { + name: "valid HTTPS URL with public IP", + url: "https://8.8.8.8/manifest.yaml", + wantErr: false, + }, + { + name: "valid HTTPS URL with public IPv6", + url: "https://[2001:4860:4860::8888]/manifest.yaml", + wantErr: false, + }, + { + name: "HTTP URL rejected", + url: "http://example.com/manifest.yaml", + wantErr: true, + errMsg: "insecure manifest URL scheme", + }, + { + name: "FTP URL rejected", + url: "ftp://example.com/manifest.yaml", + wantErr: true, + errMsg: "insecure manifest URL scheme", + }, + { + name: "empty URL rejected", + url: "", + wantErr: true, + errMsg: "manifest URL cannot be empty", + }, + { + name: "malformed URL rejected", + url: "ht!tp://invalid", + wantErr: true, + errMsg: "invalid manifest URL", + }, + { + name: "localhost rejected", + url: "https://localhost/manifest.yaml", + wantErr: true, + errMsg: "non-public address", + }, + { + name: "127.0.0.1 rejected", + url: "https://127.0.0.1/manifest.yaml", + wantErr: true, + errMsg: "non-public address", + }, + { + name: "IPv6 localhost rejected", + url: "https://[::1]/manifest.yaml", + wantErr: true, + errMsg: "non-public address", + }, + { + name: "URL without hostname rejected", + url: "https:///manifest.yaml", + wantErr: true, + errMsg: "missing hostname", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateManifestURL(tt.url) + if tt.wantErr { + if err == nil { + t.Errorf("validateManifestURL() expected error containing %q, got nil", tt.errMsg) + return + } + if !strings.Contains(err.Error(), tt.errMsg) { + t.Errorf("validateManifestURL() error = %v, want error containing %q", err, tt.errMsg) + } + } else if err != nil { + t.Errorf("validateManifestURL() unexpected error = %v", err) + } + }) + } +} + +func TestIsPublicIP(t *testing.T) { + tests := []struct { + name string + ip string + want bool + }{ + // Loopback addresses - should be rejected + {"IPv4 loopback", "127.0.0.1", false}, + {"IPv6 loopback", "::1", false}, + + // Private IPv4 ranges - should be rejected + {"private 10.x", "10.0.0.1", false}, + {"private 192.168.x", "192.168.1.1", false}, + {"private 172.16.x", "172.16.0.1", false}, + {"private 172.31.x", "172.31.255.255", false}, + + // Link-local addresses - should be rejected + {"IPv4 link-local", "169.254.1.1", false}, + {"IPv6 link-local", "fe80::1", false}, + + // IPv6 private ranges - should be rejected + {"IPv6 ULA fc00", "fc00::1", false}, + {"IPv6 ULA fd00", "fd00::1", false}, + + // Multicast - should be rejected + {"IPv4 multicast", "224.0.0.1", false}, + {"IPv6 multicast", "ff02::1", false}, + + // Unspecified - should be rejected + {"IPv4 unspecified", "0.0.0.0", false}, + {"IPv6 unspecified", "::", false}, + + // Public addresses - should be accepted + {"public IPv4", "8.8.8.8", true}, + {"public IPv4 alt", "1.1.1.1", true}, + {"public IPv6", "2001:4860:4860::8888", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ip := net.ParseIP(tt.ip) + if ip == nil { + t.Fatalf("Failed to parse IP %s", tt.ip) + } + if got := isPublicIP(ip); got != tt.want { + t.Errorf("isPublicIP(%q) = %v, want %v", tt.ip, got, tt.want) + } + }) + } +} + +func TestVerifyDigest(t *testing.T) { + testData := []byte("test manifest content") + hash := sha256.Sum256(testData) + validDigest := hex.EncodeToString(hash[:]) + invalidDigest := "0000000000000000000000000000000000000000000000000000000000000000" + + tests := []struct { + name string + data []byte + expectedDigest string + wantErr bool + }{ + { + name: "valid digest", + data: testData, + expectedDigest: validDigest, + wantErr: false, + }, + { + name: "valid digest uppercase", + data: testData, + expectedDigest: strings.ToUpper(validDigest), + wantErr: false, + }, + { + name: "invalid digest", + data: testData, + expectedDigest: invalidDigest, + wantErr: true, + }, + { + name: "empty digest skips verification", + data: testData, + expectedDigest: "", + wantErr: false, + }, + { + name: "different data fails verification", + data: []byte("different content"), + expectedDigest: validDigest, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := verifyDigest(tt.data, tt.expectedDigest) + if tt.wantErr { + if err == nil { + t.Errorf("verifyDigest() expected error, got nil") + } + } else { + if err != nil { + t.Errorf("verifyDigest() unexpected error = %v", err) + } + } + }) + } +} + +func TestSecureHTTPGet(t *testing.T) { + t.Run("rejects HTTP server", func(t *testing.T) { + // Create HTTP server (not HTTPS) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("test")) + })) + defer server.Close() + + // Attempt to fetch - should fail due to HTTP + _, _, err := secureHTTPGet(server.URL) + if err == nil { + t.Error("secureHTTPGet() should reject HTTP URLs") + } + if !strings.Contains(err.Error(), "insecure manifest URL scheme") { + t.Errorf("secureHTTPGet() error = %v, want error about insecure scheme", err) + } + }) + + t.Run("rejects localhost URLs", func(t *testing.T) { + _, _, err := secureHTTPGet("https://localhost/manifest.yaml") + if err == nil { + t.Error("secureHTTPGet() should reject localhost URLs") + } + if !strings.Contains(err.Error(), "non-public address") { + t.Errorf("secureHTTPGet() error = %v, want error about non-public address", err) + } + }) +} + +func TestSecureHTTPGetWithDigest(t *testing.T) { + t.Run("validates URL before digest", func(t *testing.T) { + // Should reject HTTP URLs even with valid digest + _, _, err := secureHTTPGetWithDigest("http://example.com/manifest.yaml", "abc123") + if err == nil { + t.Error("secureHTTPGetWithDigest() should reject HTTP URLs") + } + if !strings.Contains(err.Error(), "insecure manifest URL scheme") { + t.Errorf("secureHTTPGetWithDigest() error = %v, want error about insecure scheme", err) + } + }) + +} + +func TestSecureHTTPClient(t *testing.T) { + t.Run("has secure TLS configuration", func(t *testing.T) { + transport, ok := secureHTTPClient.Transport.(*http.Transport) + if !ok { + t.Fatal("secureHTTPClient.Transport is not *http.Transport") + } + + if transport.TLSClientConfig == nil { + t.Fatal("TLSClientConfig is nil") + } + + if transport.TLSClientConfig.MinVersion < tls.VersionTLS12 { + t.Errorf("MinVersion = %d, want >= %d (TLS 1.2)", transport.TLSClientConfig.MinVersion, tls.VersionTLS12) + } + + if len(transport.TLSClientConfig.CipherSuites) == 0 { + t.Error("CipherSuites is empty, should have secure ciphers configured") + } + }) + + t.Run("has timeout configured", func(t *testing.T) { + if secureHTTPClient.Timeout == 0 { + t.Error("secureHTTPClient.Timeout is 0, should have timeout configured") + } + }) + + t.Run("has redirect validation", func(t *testing.T) { + if secureHTTPClient.CheckRedirect == nil { + t.Error("CheckRedirect is nil, should validate redirects") + } + }) + + t.Run("validates redirect URLs", func(t *testing.T) { + // Test that CheckRedirect validates HTTPS + req := &http.Request{ + URL: mustParseURL("http://insecure.example.com/redirect"), + } + + err := secureHTTPClient.CheckRedirect(req, nil) + if err == nil { + t.Error("CheckRedirect should reject HTTP redirects") + } + }) +} + +func mustParseURL(rawURL string) *url.URL { + u, err := url.Parse(rawURL) + if err != nil { + panic(fmt.Sprintf("mustParseURL: %v", err)) + } + return u +} + +// Integration tests for Manifest() security with TektonHubResourceResult + +func TestManifestSecurityHTTPRejection(t *testing.T) { + httpURL := "http://evil.example.com/malicious.yaml" + + resourceData := &ResourceData{ + LatestVersion: &rclient.ResourceVersionDataResponseBody{ + RawURL: &httpURL, + }, + } + + result := &TektonHubResourceResult{ + data: []byte(`{}`), + status: http.StatusOK, + err: nil, + set: true, + resourceData: resourceData, + } + + _, err := result.Manifest() + if err == nil { + t.Fatal("Manifest() should reject HTTP URLs") + } + + if !strings.Contains(err.Error(), "insecure manifest URL scheme") { + t.Errorf("Expected error about insecure scheme, got: %v", err) + } +} + +func TestManifestSecurityNonPublicRejection(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"localhost", "https://localhost/manifest.yaml"}, + {"127.0.0.1", "https://127.0.0.1/manifest.yaml"}, + {"::1", "https://[::1]/manifest.yaml"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resourceData := &ResourceData{ + LatestVersion: &rclient.ResourceVersionDataResponseBody{ + RawURL: &tt.url, + }, + } + + result := &TektonHubResourceResult{ + data: []byte(`{}`), + status: http.StatusOK, + err: nil, + set: true, + resourceData: resourceData, + } + + _, err := result.Manifest() + if err == nil { + t.Fatalf("Manifest() should reject non-public address %s", tt.url) + } + + if !strings.Contains(err.Error(), "non-public address") { + t.Errorf("Expected error about non-public address, got: %v", err) + } + }) + } +}