Merge pull request #6645 from chrischdi/pr-runtimesdk-enforce-https

🌱 RuntimeSDK: enforce https for extensions

Author: k8s-ci-robot

config/crd/bases/runtime.cluster.x-k8s.io_extensionconfigs.yaml

@@ -85,8 +85,7 @@ spec:
                       of `url` or `service` must be specified. \n The `host` should
                       not refer to a service running in the cluster; use the `service`
                       field instead. \n The scheme should be \"https\"; the URL should
-                      begin with \"https://\". \"http\" is supported for insecure
-                      development purposes only. \n A path is optional, and if present
+                      begin with \"https://\". \n A path is optional, and if present
                       may be any string permissible in a URL. If a path is set it
                       will be used as prefix and the hook-specific path will be appended.
                       \n Attempting to use a user or basic auth e.g. \"user:password@\"

exp/runtime/api/v1alpha1/extensionconfig_types.go

@@ -47,7 +47,6 @@ type ClientConfig struct {
 	// the `service` field instead.
 	//
 	// The scheme should be "https"; the URL should begin with "https://".
-	// "http" is supported for insecure development purposes only.
 	//
 	// A path is optional, and if present may be any string permissible in
 	// a URL. If a path is set it will be used as prefix and the hook-specific

exp/runtime/internal/controllers/extensionconfig_controller_test.go

@@ -220,6 +220,7 @@ func TestExtensionReconciler_discoverExtensionConfig(t *testing.T) {
 		})
 
 		extensionConfig := fakeExtensionConfigForURL(ns.Name, extensionName, srv1.URL)
+		extensionConfig.Spec.ClientConfig.CABundle = testcerts.CACert
 
 		discoveredExtensionConfig, err := discoverExtensionConfig(ctx, runtimeClient, extensionConfig)
 		g.Expect(err).To(BeNil())
@@ -252,7 +253,8 @@ func TestExtensionReconciler_discoverExtensionConfig(t *testing.T) {
 			Registry: registry,
 		})
 
-		extensionConfig := fakeExtensionConfigForURL(ns.Name, extensionName, "http://localhost:31239")
+		extensionConfig := fakeExtensionConfigForURL(ns.Name, extensionName, "https://localhost:31239")
+		extensionConfig.Spec.ClientConfig.CABundle = testcerts.CACert
 
 		discoveredExtensionConfig, err := discoverExtensionConfig(ctx, runtimeClient, extensionConfig)
 		g.Expect(err).ToNot(BeNil())

exp/runtime/internal/controllers/warmup_test.go

@@ -123,7 +123,7 @@ func Test_warmupRunnable_Start(t *testing.T) {
 		brokenExtension := "ext2"
 		for _, name := range []string{"ext1", "ext2", "ext3"} {
 			if name == brokenExtension {
-				g.Expect(env.CreateAndWait(ctx, fakeExtensionConfigForURL(ns.Name, name, "http://localhost:1234"))).To(Succeed())
+				g.Expect(env.CreateAndWait(ctx, fakeExtensionConfigForURL(ns.Name, name, "https://localhost:1234"))).To(Succeed())
 				continue
 			}
 			server, err := fakeSecureExtensionServer(discoveryHandler("first", "second", "third"))

internal/runtime/client/client.go

@@ -432,21 +432,20 @@ func httpCall(ctx context.Context, request, response runtime.Object, opts *httpC
 
 	// use client-go's transport.TLSConfigureFor to ensure good defaults for tls
 	client := http.DefaultClient
-	if opts.config.CABundle != nil {
-		tlsConfig, err := transport.TLSConfigFor(&transport.Config{
-			TLS: transport.TLSConfig{
-				CAData:     opts.config.CABundle,
-				ServerName: extensionURL.Hostname(),
-			},
-		})
-		if err != nil {
-			return errors.Wrap(err, "failed to create tls config")
-		}
-		// this also adds http2
-		client.Transport = utilnet.SetTransportDefaults(&http.Transport{
-			TLSClientConfig: tlsConfig,
-		})
+	tlsConfig, err := transport.TLSConfigFor(&transport.Config{
+		TLS: transport.TLSConfig{
+			CAData:     opts.config.CABundle,
+			ServerName: extensionURL.Hostname(),
+		},
+	})
+	if err != nil {
+		return errors.Wrap(err, "failed to create tls config")
 	}
+	// this also adds http2
+	client.Transport = utilnet.SetTransportDefaults(&http.Transport{
+		TLSClientConfig: tlsConfig,
+	})
+
 	resp, err := client.Do(httpRequest)
 	if err != nil {
 		return errCallingExtensionHandler(
@@ -486,12 +485,8 @@ func urlForExtension(config runtimev1.ClientConfig, gvh runtimecatalog.GroupVers
 		if svc.Port != nil {
 			host = net.JoinHostPort(host, strconv.Itoa(int(*svc.Port)))
 		}
-		scheme := "http"
-		if len(config.CABundle) > 0 {
-			scheme = "https"
-		}
 		u = &url.URL{
-			Scheme: scheme,
+			Scheme: "https",
 			Host:   host,
 		}
 		if svc.Path != nil {
@@ -506,6 +501,9 @@ func urlForExtension(config runtimev1.ClientConfig, gvh runtimecatalog.GroupVers
 		if err != nil {
 			return nil, errors.Wrap(err, "URL in ClientConfig is invalid")
 		}
+		if u.Scheme != "https" {
+			return nil, errors.Errorf("expected https scheme, got %s", u.Scheme)
+		}
 	}
 	// Add the subpatch to the ExtensionHandler for the given hook.
 	u.Path = path.Join(u.Path, runtimecatalog.GVHToPath(gvh, name))

internal/runtime/client/client_test.go

@@ -18,6 +18,7 @@ package client
 
 import (
 	"context"
+	"crypto/tls"
 	"encoding/json"
 	"fmt"
 	"net"
@@ -32,6 +33,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apiserver/pkg/admission/plugin/webhook/testcerts"
 	"k8s.io/utils/pointer"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -187,11 +189,14 @@ func TestClient_httpCall(t *testing.T) {
 				// create http server with fakeHookHandler
 				mux := http.NewServeMux()
 				mux.HandleFunc("/", fakeHookHandler)
-				srv := httptest.NewServer(mux)
+
+				srv := newUnstartedTLSServer(mux)
+				srv.StartTLS()
 				defer srv.Close()
 
 				// set url to srv for in tt.opts
 				tt.opts.config.URL = pointer.String(srv.URL)
+				tt.opts.config.CABundle = testcerts.CACert
 			}
 
 			err := httpCall(context.TODO(), tt.request, tt.response, tt.opts)
@@ -260,7 +265,7 @@ func TestURLForExtension(t *testing.T) {
 				extensionHandlerName: "test-handler",
 			},
 			want: want{
-				scheme: "http",
+				scheme: "https",
 				host:   "extension-service.test1.svc:8443",
 				path:   runtimecatalog.GVHToPath(gvh, "test-handler"),
 			},
@@ -525,7 +530,8 @@ func TestClient_CallExtension(t *testing.T) {
 	validExtensionHandlerWithFailPolicy := runtimev1.ExtensionConfig{
 		Spec: runtimev1.ExtensionConfigSpec{
 			ClientConfig: runtimev1.ClientConfig{
-				URL: pointer.String(fmt.Sprintf("http://%s/", testHostPort)),
+				URL:      pointer.String(fmt.Sprintf("https://%s/", testHostPort)),
+				CABundle: testcerts.CACert,
 			},
 			NamespaceSelector: &metav1.LabelSelector{},
 		},
@@ -546,7 +552,8 @@ func TestClient_CallExtension(t *testing.T) {
 	validExtensionHandlerWithIgnorePolicy := runtimev1.ExtensionConfig{
 		Spec: runtimev1.ExtensionConfigSpec{
 			ClientConfig: runtimev1.ClientConfig{
-				URL: pointer.String(fmt.Sprintf("http://%s/", testHostPort)),
+				URL:      pointer.String(fmt.Sprintf("https://%s/", testHostPort)),
+				CABundle: testcerts.CACert,
 			},
 			NamespaceSelector: &metav1.LabelSelector{}},
 		Status: runtimev1.ExtensionConfigStatus{
@@ -722,8 +729,8 @@ func TestClient_CallExtension(t *testing.T) {
 				if tt.testServer.hostPort == "" {
 					tt.testServer.hostPort = testHostPort
 				}
-				srv := createTestServer(g, tt.testServer)
-				srv.Start()
+				srv := createSecureTestServer(g, tt.testServer)
+				srv.StartTLS()
 				defer srv.Close()
 			}
 
@@ -773,7 +780,8 @@ func TestClient_CallAllExtensions(t *testing.T) {
 	extensionConfig := runtimev1.ExtensionConfig{
 		Spec: runtimev1.ExtensionConfigSpec{
 			ClientConfig: runtimev1.ClientConfig{
-				URL: pointer.String(fmt.Sprintf("http://%s/", testHostPort)),
+				URL:      pointer.String(fmt.Sprintf("https://%s/", testHostPort)),
+				CABundle: testcerts.CACert,
 			},
 			NamespaceSelector: &metav1.LabelSelector{},
 		},
@@ -925,8 +933,8 @@ func TestClient_CallAllExtensions(t *testing.T) {
 				if tt.testServer.hostPort == "" {
 					tt.testServer.hostPort = testHostPort
 				}
-				srv := createTestServer(g, tt.testServer)
-				srv.Start()
+				srv := createSecureTestServer(g, tt.testServer)
+				srv.StartTLS()
 				defer srv.Close()
 			}
 
@@ -1142,7 +1150,7 @@ func response(status runtimehooksv1.ResponseStatus) testServerResponse {
 	}
 }
 
-func createTestServer(g *WithT, server testServerConfig) *httptest.Server {
+func createSecureTestServer(g *WithT, server testServerConfig) *httptest.Server {
 	l, err := net.Listen("tcp", server.hostPort)
 	g.Expect(err).NotTo(HaveOccurred())
 
@@ -1166,7 +1174,9 @@ func createTestServer(g *WithT, server testServerConfig) *httptest.Server {
 		// Otherwise write a 404.
 		w.WriteHeader(http.StatusNotFound)
 	})
-	srv := httptest.NewUnstartedServer(mux)
+
+	srv := newUnstartedTLSServer(mux)
+
 	// NewUnstartedServer creates a listener. Close that listener and replace
 	// with the one we created.
 	g.Expect(srv.Listener.Close()).To(Succeed())
@@ -1214,3 +1224,16 @@ func fakeRetryableSuccessResponse(retryAfterSeconds int32) *fakev1alpha1.Retryab
 		},
 	}
 }
+
+func newUnstartedTLSServer(handler http.Handler) *httptest.Server {
+	cert, err := tls.X509KeyPair(testcerts.ServerCert, testcerts.ServerKey)
+	if err != nil {
+		panic(err)
+	}
+	srv := httptest.NewUnstartedServer(handler)
+	srv.TLS = &tls.Config{
+		MinVersion:   tls.VersionTLS13,
+		Certificates: []tls.Certificate{cert},
+	}
+	return srv
+}

internal/webhooks/runtime/extensionconfig_webhook.go

@@ -143,11 +143,11 @@ func validateExtensionConfigSpec(e *runtimev1.ExtensionConfig) field.ErrorList {
 				*e.Spec.ClientConfig.URL,
 				fmt.Sprintf("must be a valid URL, e.g. https://example.com: %v", err),
 			))
-		} else if uri.Scheme != "http" && uri.Scheme != "https" {
+		} else if uri.Scheme != "https" {
 			allErrs = append(allErrs, field.Invalid(
 				specPath.Child("clientConfig", "url"),
 				*e.Spec.ClientConfig.URL,
-				"'https' and 'http' are the only allowed URL schemes, e.g. https://example.com",
+				"'https' is the only allowed URL scheme, e.g. https://example.com",
 			))
 		}
 	}