HTTP listener: default disabled and may only bind to loopback interfaces

Dockerfile

@@ -30,7 +30,7 @@ FROM gcr.io/distroless/static:nonroot@sha256:80c956fb0836a17a565c43a4026c9c80b20
 COPY --from=build-env /usr/local/bin /usr/local/bin
 
 # Document the default server ports for the various server apps
-EXPOSE 8080 8443 8444 10250
+EXPOSE 8443 8444 10250
 
 # Run as non-root for security posture
 # Use the same non-root user as https://github.com/GoogleContainerTools/distroless/blob/fc3c4eaceb0518900f886aae90407c43be0a42d9/base/base.bzl#L9

deploy/supervisor/deployment.yaml

@@ -1,4 +1,4 @@
-#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+#! Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 #! SPDX-License-Identifier: Apache-2.0
 
 #@ load("@ytt:data", "data")
@@ -115,8 +115,6 @@ spec:
               readOnly: false  #! writable to allow for socket use
             #@ end
           ports:
-            - containerPort: 8080
-              protocol: TCP
             - containerPort: 8443
               protocol: TCP
           env:

deploy/supervisor/values.yaml

@@ -1,4 +1,4 @@
-#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+#! Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 #! SPDX-License-Identifier: Apache-2.0
 
 #@data/values
@@ -74,17 +74,17 @@ api_group_suffix: pinniped.dev
 https_proxy: #! e.g. http://proxy.example.com
 no_proxy: "$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost,.svc,.cluster.local" #! do not proxy Kubernetes endpoints
 
-#! Control the https and http listeners of the Supervisor.
+#! Control the HTTP and HTTPS listeners of the Supervisor.
 #!
 #! The schema of this config is as follows:
 #!
 #! endpoints:
 #!   https:
 #!     network: tcp | unix | disabled
-#!     address: interface:port when network=tcp or /pinniped_socket/socketfile.sock when network=unix
+#!     address: host:port when network=tcp or /pinniped_socket/socketfile.sock when network=unix
 #!   http:
 #!     network: same as above
-#!     address: same as above
+#!     address: same as above, except that when network=tcp then the address is only allowed to bind to loopback interfaces
 #!
 #! Setting network to disabled turns off that particular listener.
 #! See https://pkg.go.dev/net#Listen and https://pkg.go.dev/net#Dial for a description of what can be
@@ -98,23 +98,20 @@ no_proxy: "$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost,.svc,.
 #!     network: tcp
 #!     address: :8443
 #!   http:
-#!     network: tcp
+#!     network: disabled
-#!     address: :8080
 #!
-#! These defaults mean: bind to all interfaces using TCP.  Use port 8443 for https and 8080 for http.
+#! These defaults mean: For HTTPS listening, bind to all interfaces using TCP on port 8443.
-#! The defaults will change over time.  Users should explicitly set this value if they wish to avoid
+#! Disable HTTP listening by default.
-#! any changes on upgrade.
 #!
-#! A future version of the Supervisor app may include a breaking change to adjust the default
+#! The HTTP listener can only be bound to loopback interfaces. This allows the listener to accept
-#! behavior of the http listener to only listen on 127.0.0.1 (or perhaps even to be disabled).
+#! traffic from within the pod, e.g. from a service mesh sidecar. The HTTP listener should not be
+#! used to accept traffic from outside the pod, since that would mean that the network traffic could be
+#! transmitted unencrypted. The HTTPS listener should be used instead to accept traffic from outside the pod.
+#! Ingresses and load balancers that terminate TLS connections should re-encrypt the data and route traffic
+#! to the HTTPS listener. Unix domain sockets may also be used for integrations with service meshes.
 #!
-#! Binding the http listener to addresses other than 127.0.0.1 or ::1 is deprecated.
+#! Changing the HTTPS port number must be accompanied by matching changes to the service and deployment
-#!
+#! manifests. Changes to the HTTPS listener must be coordinated with the deployment health checks.
-#! Unix domain sockets are recommended for integrations with service meshes.  Ingresses that terminate
-#! TLS connections at the edge should re-encrypt the data and route traffic to the https listener.
-#!
-#! Changing the port numbers used must be accompanied with matching changes to the service and deployment
-#! manifests.  Changes to the https listener must be coordinated with the deployment health checks.
 #!
 #! Optional.
 endpoints:

internal/config/supervisor/config.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package supervisor contains functionality to load/store Config's from/to
@@ -8,6 +8,7 @@ package supervisor
 import (
 	"fmt"
 	"io/ioutil"
+	"net"
 	"strings"
 
 	"k8s.io/utils/pointer"
@@ -66,8 +67,7 @@ func FromPath(path string) (*Config, error) {
 		Address: ":8443",
 	})
 	maybeSetEndpointDefault(&config.Endpoints.HTTP, Endpoint{
-		Network: NetworkTCP,
+		Network: NetworkDisabled,
-		Address: ":8080",
 	})
 
 	if err := validateEndpoint(*config.Endpoints.HTTPS); err != nil {
@@ -76,6 +76,9 @@ func FromPath(path string) (*Config, error) {
 	if err := validateEndpoint(*config.Endpoints.HTTP); err != nil {
 		return nil, fmt.Errorf("validate http endpoint: %w", err)
 	}
+	if err := validateAdditionalHTTPEndpointRequirements(*config.Endpoints.HTTP); err != nil {
+		return nil, fmt.Errorf("validate http endpoint: %w", err)
+	}
 	if err := validateAtLeastOneEnabledEndpoint(*config.Endpoints.HTTPS, *config.Endpoints.HTTP); err != nil {
 		return nil, fmt.Errorf("validate endpoints: %w", err)
 	}
@@ -128,6 +131,16 @@ func validateEndpoint(endpoint Endpoint) error {
 	}
 }
 
+func validateAdditionalHTTPEndpointRequirements(endpoint Endpoint) error {
+	if endpoint.Network == NetworkTCP && !addrIsOnlyOnLoopback(endpoint.Address) {
+		return fmt.Errorf(
+			"http listener address %q for %q network may only bind to loopback interfaces",
+			endpoint.Address,
+			endpoint.Network)
+	}
+	return nil
+}
+
 func validateAtLeastOneEnabledEndpoint(endpoints ...Endpoint) error {
 	for _, endpoint := range endpoints {
 		if endpoint.Network != NetworkDisabled {
@@ -136,3 +149,36 @@ func validateAtLeastOneEnabledEndpoint(endpoints ...Endpoint) error {
 	}
 	return constable.Error("all endpoints are disabled")
 }
+
+// For tcp networks, the address can be in several formats: host:port, host:, and :port.
+// See address description in https://pkg.go.dev/net#Listen and https://pkg.go.dev/net#Dial.
+// The host may be a literal IP address, or a host name that can be resolved to IP addresses,
+// or a literal unspecified IP address (as in "0.0.0.0:80" or "[::]:80"), or empty.
+// If the host is a literal IPv6 address it must be enclosed in square brackets, as in "[2001:db8::1]:80" or
+// "[fe80::1%zone]:80". The zone specifies the scope of the literal IPv6 address as defined in RFC 4007.
+// The port may be a literal port number or a service name, the value 0, or empty.
+// Returns true if a net.Listen listener at this address would only listen on loopback interfaces.
+// Returns false if the listener would listen on any non-loopback interfaces, or when called with illegal input.
+func addrIsOnlyOnLoopback(addr string) bool {
+	// First try parsing as a `host:port`. net.SplitHostPort allows empty host and empty port.
+	host, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		// Illegal input.
+		return false
+	}
+	if host == "" {
+		// Input was :port. This would bind to all interfaces, so it is not only on loopback.
+		return false
+	}
+	if host == "localhost" {
+		// This is only on loopback.
+		return true
+	}
+	// The host could be a hostname, an IPv4 address, or an IPv6 address.
+	ip := net.ParseIP(host)
+	if ip == nil {
+		// The address was not an IP. It must have been some hostname other than "localhost".
+		return false
+	}
+	return ip.IsLoopback()
+}

internal/config/supervisor/config_test.go

@@ -1,16 +1,16 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisor
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"testing"
 
-	"k8s.io/utils/pointer"
-
 	"github.com/stretchr/testify/require"
+	"k8s.io/utils/pointer"
 
 	"go.pinniped.dev/internal/here"
 )
@@ -37,7 +37,8 @@ func TestFromPath(t *testing.T) {
 				    network: unix
 				    address: :1234
 				  http:
-				    network: disabled
+				    network: tcp
+					address: 127.0.0.1:1234
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: pointer.StringPtr("some.suffix.com"),
@@ -54,7 +55,8 @@ func TestFromPath(t *testing.T) {
 						Address: ":1234",
 					},
 					HTTP: &Endpoint{
-						Network: "disabled",
+						Network: "tcp",
+						Address: "127.0.0.1:1234",
 					},
 				},
 			},
@@ -78,8 +80,7 @@ func TestFromPath(t *testing.T) {
 						Address: ":8443",
 					},
 					HTTP: &Endpoint{
-						Network: "tcp",
+						Network: "disabled",
-						Address: ":8080",
 					},
 				},
 			},
@@ -126,6 +127,21 @@ func TestFromPath(t *testing.T) {
 			`),
 			wantError: `validate http endpoint: unknown network "bar"`,
 		},
+		{
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces",
+			yaml: here.Doc(`
+				---
+				names:
+				  defaultTLSCertificateSecret: my-secret-name
+				endpoints:
+				  https:
+				    network: disabled
+				  http:
+				    network: tcp
+					address: :8080
+			`),
+			wantError: `validate http endpoint: http listener address ":8080" for "tcp" network may only bind to loopback interfaces`,
+		},
 		{
 			name: "endpoint disabled with non-empty address",
 			yaml: here.Doc(`
@@ -208,3 +224,66 @@ func TestFromPath(t *testing.T) {
 		})
 	}
 }
+
+func TestAddrIsOnlyOnLoopback(t *testing.T) {
+	tests := []struct {
+		addr string
+		want bool
+	}{
+		{addr: "localhost:", want: true},
+		{addr: "localhost:0", want: true},
+		{addr: "localhost:80", want: true},
+		{addr: "localhost:http", want: true},
+		{addr: "127.0.0.1:", want: true},
+		{addr: "127.0.0.1:0", want: true},
+		{addr: "127.0.0.1:80", want: true},
+		{addr: "127.0.0.1:http", want: true},
+		{addr: "[::1]:", want: true},
+		{addr: "[::1]:0", want: true},
+		{addr: "[::1]:80", want: true},
+		{addr: "[::1]:http", want: true},
+		{addr: "[0:0:0:0:0:0:0:1]:", want: true},
+		{addr: "[0:0:0:0:0:0:0:1]:0", want: true},
+		{addr: "[0:0:0:0:0:0:0:1]:80", want: true},
+		{addr: "[0:0:0:0:0:0:0:1]:http", want: true},
+		{addr: "", want: false},               // illegal input, can't be empty
+		{addr: "host", want: false},           // illegal input, need colon
+		{addr: "localhost", want: false},      // illegal input, need colon
+		{addr: "127.0.0.1", want: false},      // illegal input, need colon
+		{addr: ":", want: false},              // illegal input, need either host or port
+		{addr: "2001:db8::1:80", want: false}, // illegal input, forgot square brackets
+		{addr: ":0", want: false},
+		{addr: ":80", want: false},
+		{addr: ":http", want: false},
+		{addr: "notlocalhost:", want: false},
+		{addr: "notlocalhost:0", want: false},
+		{addr: "notlocalhost:80", want: false},
+		{addr: "notlocalhost:http", want: false},
+		{addr: "0.0.0.0:", want: false},
+		{addr: "0.0.0.0:0", want: false},
+		{addr: "0.0.0.0:80", want: false},
+		{addr: "0.0.0.0:http", want: false},
+		{addr: "[::]:", want: false},
+		{addr: "[::]:0", want: false},
+		{addr: "[::]:80", want: false},
+		{addr: "[::]:http", want: false},
+		{addr: "42.42.42.42:", want: false},
+		{addr: "42.42.42.42:0", want: false},
+		{addr: "42.42.42.42:80", want: false},
+		{addr: "42.42.42.42:http", want: false},
+		{addr: "[2001:db8::1]:", want: false},
+		{addr: "[2001:db8::1]:0", want: false},
+		{addr: "[2001:db8::1]:80", want: false},
+		{addr: "[2001:db8::1]:http", want: false},
+		{addr: "[fe80::1%zone]:", want: false},
+		{addr: "[fe80::1%zone]:0", want: false},
+		{addr: "[fe80::1%zone]:80", want: false},
+		{addr: "[fe80::1%zone]:http", want: false},
+	}
+	for _, test := range tests {
+		tt := test
+		t.Run(fmt.Sprintf("address %s should be %t", tt.addr, tt.want), func(t *testing.T) {
+			require.Equal(t, tt.want, addrIsOnlyOnLoopback(tt.addr))
+		})
+	}
+}

site/content/docs/howto/configure-supervisor.md

@@ -46,26 +46,26 @@ The most common ways are:
   configured with TLS certificates and will terminate the TLS connection itself (see the section about FederationDomain
   below). The LoadBalancer Service should be configured to use the HTTPS port 443 of the Supervisor pods as its `targetPort`.
 
-  *Warning:* Never expose the Supervisor's HTTP port 8080 to the public. It would not be secure for the OIDC protocol
-  to use HTTP, because the user's secret OIDC tokens would be transmitted across the network without encryption.
-
 - Or, define an [Ingress resource](https://kubernetes.io/docs/concepts/services-networking/ingress/).
 
    In this case, the [Ingress typically terminates TLS](https://kubernetes.io/docs/concepts/services-networking/ingress/#tls)
-   and then talks plain HTTP to its backend,
+   and then talks plain HTTP to its backend.
-   which would be a NodePort or LoadBalancer Service in front of the HTTP port 8080 of the Supervisor pods.
+   However, because the Supervisor's endpoints deal with sensitive credentials, the ingress must be configured to re-encrypt
-   However, because the Supervisor's endpoints deal with sensitive credentials, it is much better if the
+   traffic using TLS on the backend (upstream) into the Supervisor's Pods. It would not be secure for the OIDC protocol
-   traffic is encrypted using TLS all the way into the Supervisor's Pods. Some Ingress implementations
+   to use HTTP, because the user's secret OIDC tokens would be transmitted across the network without encryption.
-   may support re-encrypting the traffic before sending it to the backend. If your Ingress controller does not
+   If your Ingress controller does not support this feature, then consider using one of the other configurations
-   support this, then consider using one of the other configurations described here instead of using an Ingress.
+   described here instead of using an Ingress. The backend of the Ingress would typically point to a NodePort or
+   LoadBalancer Service which exposes the HTTPS port 8443 of the Supervisor pods.
 
    The required configuration of the Ingress is specific to your cluster's Ingress Controller, so please refer to the
    documentation from your Kubernetes provider. If you are using a cluster from a cloud provider, then you'll probably
    want to start with that provider's documentation. For example, if your cluster is a Google GKE cluster, refer to
-   the [GKE documentation for Ingress](https://cloud.google.com/kubernetes-engine/docs/concepts/ingress).
+   the [GKE documentation for Ingress](https://cloud.google.com/kubernetes-engine/docs/concepts/ingress) and the
+   [GKE documentation for enabling TLS on the backend of an Ingress](https://cloud.google.com/kubernetes-engine/docs/concepts/ingress-xlb#https_tls_between_load_balancer_and_your_application).
    Otherwise, the Kubernetes documentation provides a list of popular
    [Ingress Controllers](https://kubernetes.io/docs/concepts/services-networking/ingress-controllers/), including
-   [Contour](https://projectcontour.io/) and many others.
+   [Contour](https://projectcontour.io/) and many others. Contour is an example of an ingress implementation which
+   [supports TLS on the backend](https://projectcontour.io/docs/main/config/upstream-tls/).
 
 - Or, expose the Supervisor app using a Kubernetes service mesh technology (e.g. [Istio](https://istio.io/)).
 
@@ -133,7 +133,7 @@ spec:
   ports:
   - protocol: TCP
     port: 443
-    targetPort: 8443 # 8443 is the TLS port. Do not expose port 8080.
+    targetPort: 8443 # 8443 is the TLS port.
 ```
 
 ### Example: Creating a NodePort Service

site/content/docs/tutorials/concierge-and-supervisor-demo.md

@@ -218,7 +218,7 @@ spec:
   ports:
   - protocol: TCP
     port: 443
-    targetPort: 8443 # 8443 is the TLS port. Do not expose port 8080.
+    targetPort: 8443 # 8443 is the TLS port.
 EOF
 ```