Merge pull request #1952 from vmware-tanzu/jtc/issue-1605-limit-tls-c…

…iphers-for-tls1.2-v2

Allow admin user to further limit TLS ciphers used for TLS1.2 client requests and server ports (not including CLI)

deploy/concierge/deployment.yaml

@@ -100,6 +100,9 @@ data:
     log:
       level: (@= getAndValidateLogLevel() @)
     (@ end @)
+    tls:
+      onedottwo:
+        allowedCiphers: (@= str(data.values.allowed_ciphers_for_tls_onedottwo) @)
 ---
 #@ if data.values.image_pull_dockerconfigjson and data.values.image_pull_dockerconfigjson != "":
 apiVersion: v1

deploy/concierge/values.yaml

@@ -214,3 +214,20 @@ https_proxy: ""
 #@ localhost endpoints, and the known instance metadata IP address for public cloud providers."
 #@schema/desc no_proxy_desc
 no_proxy: "$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost,.svc,.cluster.local"
+
+#@schema/title "Allowed Ciphers for TLS 1.2"
+#@ allowed_ciphers_for_tls_onedottwo_desc = "When specified, only the ciphers listed will be used for TLS 1.2. \
+#@ This includes both server-side and client-side TLS connections. \
+#@ This list must only include cipher suites that Pinniped is configured to accept \
+#@ (see internal/crypto/ptls/profiles.go and internal/crypto/ptls/profiles_fips_strict.go). \
+#@ Allowing too few ciphers may cause critical parts of Pinniped to be unable to function. For example, \
+#@ Kubernetes pod readiness checks, Pinniped pods acting as a client to the Kubernetes API server, \
+#@ Pinniped pods acting as a client to external identity providers, or Pinniped pods acting as an APIService server \
+#@ all need to be able to function with the allowed TLS cipher suites. \
+#@ An empty array means accept Pinniped's defaults."
+#@schema/desc allowed_ciphers_for_tls_onedottwo_desc
+#@schema/examples ("Example with a few secure ciphers", ["TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256"])
+#! No type, default, or validation is required here.
+#! An empty array is perfectly valid, as is any array of strings.
+allowed_ciphers_for_tls_onedottwo:
+- ""

deploy/supervisor/helpers.lib.yaml

@@ -53,6 +53,11 @@ _: #@ template.replace(data.values.custom_labels)
 #@       "apiService": defaultResourceNameWithSuffix("api"),
 #@     },
 #@     "labels": labels(),
+#@     "tls": {
+#@       "onedottwo": {
+#@         "allowedCiphers": data.values.allowed_ciphers_for_tls_onedottwo
+#@       }
+#@     }
 #@   }
 #@   if data.values.log_level:
 #@     config["log"] = {}

deploy/supervisor/values.yaml

@@ -203,3 +203,20 @@ no_proxy: "$(KUBERNETES_SERVICE_HOST),169.254.169.254,127.0.0.1,localhost,.svc,.
 #@schema/nullable
 #@schema/validation ("a map with keys 'http' and 'https', whose values are either the string 'disabled' or a map having keys 'network' and 'address', and the value of 'network' must be one of the allowed values", validate_endpoints)
 endpoints: { }
+
+#@schema/title "Allowed Ciphers for TLS 1.2"
+#@ allowed_ciphers_for_tls_onedottwo_desc = "When specified, only the ciphers listed will be used for TLS 1.2. \
+#@ This includes both server-side and client-side TLS connections. \
+#@ This list must only include cipher suites that Pinniped is configured to accept \
+#@ (see internal/crypto/ptls/profiles.go and internal/crypto/ptls/profiles_fips_strict.go). \
+#@ Allowing too few ciphers may cause critical parts of Pinniped to be unable to function. For example, \
+#@ Kubernetes pod readiness checks, Pinniped pods acting as a client to the Kubernetes API server, \
+#@ Pinniped pods acting as a client to external identity providers, or Pinniped pods acting as an APIService server \
+#@ all need to be able to function with the allowed TLS cipher suites. \
+#@ An empty array means accept Pinniped's defaults."
+#@schema/desc allowed_ciphers_for_tls_onedottwo_desc
+#@schema/examples ("Example with a few secure ciphers", ["TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256"])
+#! No type, default, or validation is required here.
+#! An empty array is perfectly valid, as is any array of strings.
+allowed_ciphers_for_tls_onedottwo:
+- ""

internal/concierge/server/server.go

@@ -112,11 +112,14 @@ func (a *App) runServer(ctx context.Context) error {
 	featuregates.DisableKubeFeatureGate(features.UnauthenticatedHTTP2DOSMitigation)
 
 	// Read the server config file.
-	cfg, err := concierge.FromPath(ctx, a.configPath)
+	cfg, err := concierge.FromPath(ctx, a.configPath, ptls.SetUserConfiguredAllowedCipherSuitesForTLSOneDotTwo)
 	if err != nil {
 		return fmt.Errorf("could not load config: %w", err)
 	}
 
+	// The above server config should have set the allowed ciphers global, so now log the ciphers for all profiles.
+	ptls.LogAllProfiles(plog.New())
+
 	// Discover in which namespace we are installed.
 	podInfo, err := downward.Load(a.downwardAPIPath)
 	if err != nil {

internal/config/concierge/config.go

@@ -15,6 +15,7 @@ import (
 	"sigs.k8s.io/yaml"
 
 	"go.pinniped.dev/internal/constable"
+	"go.pinniped.dev/internal/crypto/ptls"
 	"go.pinniped.dev/internal/groupsuffix"
 	"go.pinniped.dev/internal/plog"
 )
@@ -42,7 +43,7 @@ const (
 // Note! The Config file should contain base64-encoded WebhookCABundle data.
 // This function will decode that base64-encoded data to PEM bytes to be stored
 // in the Config.
-func FromPath(ctx context.Context, path string) (*Config, error) {
+func FromPath(ctx context.Context, path string, setAllowedCiphers ptls.SetAllowedCiphersFunc) (*Config, error) {
 	data, err := os.ReadFile(path)
 	if err != nil {
 		return nil, fmt.Errorf("read file: %w", err)
@@ -83,6 +84,10 @@ func FromPath(ctx context.Context, path string) (*Config, error) {
 		return nil, fmt.Errorf("validate log level: %w", err)
 	}
 
+	if err := setAllowedCiphers(config.TLS.OneDotTwo.AllowedCiphers); err != nil {
+		return nil, fmt.Errorf("validate tls: %w", err)
+	}
+
 	if config.Labels == nil {
 		config.Labels = make(map[string]string)
 	}

internal/config/concierge/config_test.go

@@ -5,6 +5,7 @@ package concierge
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"testing"
 
@@ -17,10 +18,11 @@ import (
 
 func TestFromPath(t *testing.T) {
 	tests := []struct {
-		name       string
-		yaml       string
-		wantConfig *Config
-		wantError  string
+		name                string
+		yaml                string
+		allowedCiphersError error
+		wantConfig          *Config
+		wantError           string
 	}{
 		{
 			name: "Fully filled out",
@@ -59,6 +61,12 @@ func TestFromPath(t *testing.T) {
 				  imagePullSecrets: [kube-cert-agent-image-pull-secret]
 				log:
 				  level: debug
+				tls:
+				  onedottwo:
+				    allowedCiphers:
+				    - foo
+				    - bar
+					- TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
 			`),
 			wantConfig: &Config{
 				DiscoveryInfo: DiscoveryInfoSpec{
@@ -98,6 +106,15 @@ func TestFromPath(t *testing.T) {
 				Log: plog.LogSpec{
 					Level: plog.LevelDebug,
 				},
+				TLS: TLSSpec{
+					OneDotTwo: TLSProtocolSpec{
+						AllowedCiphers: []string{
+							"foo",
+							"bar",
+							"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305",
+						},
+					},
+				},
 			},
 		},
 		{
@@ -587,6 +604,31 @@ func TestFromPath(t *testing.T) {
 			`),
 			wantError: "validate apiGroupSuffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')",
 		},
+		{
+			name: "returns setAllowedCiphers errors",
+			yaml: here.Doc(`
+				---
+				names:
+				  servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate
+				  credentialIssuer: pinniped-config
+				  apiService: pinniped-api
+				  impersonationLoadBalancerService: impersonationLoadBalancerService-value
+				  impersonationClusterIPService: impersonationClusterIPService-value
+				  impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value
+				  impersonationCACertificateSecret: impersonationCACertificateSecret-value
+				  impersonationSignerSecret: impersonationSignerSecret-value
+				  impersonationSignerSecret: impersonationSignerSecret-value
+				  agentServiceAccount: agentServiceAccount-value
+				  impersonationProxyServiceAccount: impersonationProxyServiceAccount-value
+				  impersonationProxyLegacySecret: impersonationProxyLegacySecret-value
+				tls:
+				  onedottwo:
+				    allowedCiphers:
+				    - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
+			`),
+			allowedCiphersError: fmt.Errorf("some error from setAllowedCiphers"),
+			wantError:           "validate tls: some error from setAllowedCiphers",
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -595,10 +637,10 @@ func TestFromPath(t *testing.T) {
 			// Write yaml to temp file
 			f, err := os.CreateTemp("", "pinniped-test-config-yaml-*")
 			require.NoError(t, err)
-			defer func() {
+			t.Cleanup(func() {
 				err := os.Remove(f.Name())
 				require.NoError(t, err)
-			}()
+			})
 			_, err = f.WriteString(test.yaml)
 			require.NoError(t, err)
 			err = f.Close()
@@ -607,14 +649,23 @@ func TestFromPath(t *testing.T) {
 			// Test FromPath()
 			ctx, cancel := context.WithCancel(context.Background())
 			t.Cleanup(cancel)
-			config, err := FromPath(ctx, f.Name())
+
+			var actualCiphers []string
+			setAllowedCiphers := func(ciphers []string) error {
+				actualCiphers = ciphers
+				return test.allowedCiphersError
+			}
+
+			config, err := FromPath(ctx, f.Name(), setAllowedCiphers)
 
 			if test.wantError != "" {
 				require.EqualError(t, err, test.wantError)
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, test.wantConfig, config)
+				return
 			}
+
+			require.NoError(t, err)
+			require.Equal(t, test.wantConfig, config)
+			require.Equal(t, test.wantConfig.TLS.OneDotTwo.AllowedCiphers, actualCiphers)
 		})
 	}
 }

internal/config/concierge/types.go

@@ -16,6 +16,18 @@ type Config struct {
 	KubeCertAgentConfig          KubeCertAgentSpec `json:"kubeCertAgent"`
 	Labels                       map[string]string `json:"labels"`
 	Log                          plog.LogSpec      `json:"log"`
+	TLS                          TLSSpec           `json:"tls"`
+}
+
+type TLSSpec struct {
+	OneDotTwo TLSProtocolSpec `json:"onedottwo"`
+}
+
+type TLSProtocolSpec struct {
+	// AllowedCiphers will permit Pinniped to use only the listed ciphers.
+	// This affects Pinniped both when it acts as a client and as a server.
+	// If empty, Pinniped will use a built-in list of ciphers.
+	AllowedCiphers []string `json:"allowedCiphers"`
 }
 
 // DiscoveryInfoSpec contains configuration knobs specific to

internal/config/supervisor/config.go

@@ -16,6 +16,7 @@ import (
 	"sigs.k8s.io/yaml"
 
 	"go.pinniped.dev/internal/constable"
+	"go.pinniped.dev/internal/crypto/ptls"
 	"go.pinniped.dev/internal/groupsuffix"
 	"go.pinniped.dev/internal/plog"
 )
@@ -35,7 +36,7 @@ const (
 // FromPath loads an Config from a provided local file path, inserts any
 // defaults (from the Config documentation), and verifies that the config is
 // valid (Config documentation).
-func FromPath(ctx context.Context, path string) (*Config, error) {
+func FromPath(ctx context.Context, path string, setAllowedCiphers ptls.SetAllowedCiphersFunc) (*Config, error) {
 	data, err := os.ReadFile(path)
 	if err != nil {
 		return nil, fmt.Errorf("read file: %w", err)
@@ -95,6 +96,9 @@ func FromPath(ctx context.Context, path string) (*Config, error) {
 	if err := validateAtLeastOneEnabledEndpoint(*config.Endpoints.HTTPS, *config.Endpoints.HTTP); err != nil {
 		return nil, fmt.Errorf("validate endpoints: %w", err)
 	}
+	if err := setAllowedCiphers(config.TLS.OneDotTwo.AllowedCiphers); err != nil {
+		return nil, fmt.Errorf("validate tls: %w", err)
+	}
 
 	return &config, nil
 }

internal/config/supervisor/config_test.go

@@ -18,10 +18,11 @@ import (
 
 func TestFromPath(t *testing.T) {
 	tests := []struct {
-		name       string
-		yaml       string
-		wantConfig *Config
-		wantError  string
+		name                string
+		yaml                string
+		allowedCiphersError error
+		wantConfig          *Config
+		wantError           string
 	}{
 		{
 			name: "Happy",
@@ -45,6 +46,12 @@ func TestFromPath(t *testing.T) {
 				  level: info
 				  format: json
 				aggregatedAPIServerPort: 12345
+				tls:
+				  onedottwo:
+				    allowedCiphers:
+				    - foo
+				    - bar
+				    - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: ptr.To("some.suffix.com"),
@@ -70,6 +77,15 @@ func TestFromPath(t *testing.T) {
 					Format: plog.FormatJSON,
 				},
 				AggregatedAPIServerPort: ptr.To[int64](12345),
+				TLS: TLSSpec{
+					OneDotTwo: TLSProtocolSpec{
+						AllowedCiphers: []string{
+							"foo",
+							"bar",
+							"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305",
+						},
+					},
+				},
 			},
 		},
 		{
@@ -251,6 +267,20 @@ func TestFromPath(t *testing.T) {
 			`),
 			wantError: "validate aggregatedAPIServerPort: must be within range 1024 to 65535",
 		},
+		{
+			name: "returns setAllowedCiphers errors",
+			yaml: here.Doc(`
+				---
+				names:
+				  defaultTLSCertificateSecret: my-secret-name
+				tls:
+				  onedottwo:
+				    allowedCiphers:
+				    - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
+			`),
+			allowedCiphersError: fmt.Errorf("some error from setAllowedCiphers"),
+			wantError:           "validate tls: some error from setAllowedCiphers",
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -259,10 +289,10 @@ func TestFromPath(t *testing.T) {
 			// Write yaml to temp file
 			f, err := os.CreateTemp("", "pinniped-test-config-yaml-*")
 			require.NoError(t, err)
-			defer func() {
+			t.Cleanup(func() {
 				err := os.Remove(f.Name())
 				require.NoError(t, err)
-			}()
+			})
 			_, err = f.WriteString(test.yaml)
 			require.NoError(t, err)
 			err = f.Close()
@@ -271,14 +301,23 @@ func TestFromPath(t *testing.T) {
 			// Test FromPath()
 			ctx, cancel := context.WithCancel(context.Background())
 			t.Cleanup(cancel)
-			config, err := FromPath(ctx, f.Name())
+
+			var actualCiphers []string
+			setAllowedCiphers := func(ciphers []string) error {
+				actualCiphers = ciphers
+				return test.allowedCiphersError
+			}
+
+			config, err := FromPath(ctx, f.Name(), setAllowedCiphers)
 
 			if test.wantError != "" {
 				require.EqualError(t, err, test.wantError)
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, test.wantConfig, config)
+				return
 			}
+
+			require.NoError(t, err)
+			require.Equal(t, test.wantConfig, config)
+			require.Equal(t, test.wantConfig.TLS.OneDotTwo.AllowedCiphers, actualCiphers)
 		})
 	}
 }