From 764d8a979fb7e6b3477b449caaf731a035f8f36d Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Tue, 10 Oct 2023 20:10:01 +0100 Subject: [PATCH 1/3] redaction should only work for TCP listeners, also fix bug that allowed custom response headers for unix listeners --- command/server.go | 4 +- command/server/config_test_helpers.go | 33 ++++++++++---- command/server/listener.go | 2 +- internalshared/configutil/config.go | 2 +- internalshared/configutil/listener.go | 51 +++++++++++++++++----- internalshared/configutil/listener_test.go | 40 ++++++++++++++++- 6 files changed, 108 insertions(+), 24 deletions(-) diff --git a/command/server.go b/command/server.go index 6b91557ac2b1..fa078f975c40 100644 --- a/command/server.go +++ b/command/server.go @@ -886,9 +886,9 @@ func (c *ServerCommand) InitListeners(config *server.Config, disableClustering b } if reloadFunc != nil { - relSlice := (*c.reloadFuncs)["listener|"+lnConfig.Type] + relSlice := (*c.reloadFuncs)["listener|"+lnConfig.Type.String()] relSlice = append(relSlice, reloadFunc) - (*c.reloadFuncs)["listener|"+lnConfig.Type] = relSlice + (*c.reloadFuncs)["listener|"+lnConfig.Type.String()] = relSlice } if !disableClustering && lnConfig.Type == "tcp" { diff --git a/command/server/config_test_helpers.go b/command/server/config_test_helpers.go index cb10655a7e0c..5e5378d23c37 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -890,6 +890,15 @@ listener "tcp" { redact_addresses = true redact_cluster_name = true redact_version = true +} +listener "unix" { + address = "/var/run/vault.sock" + socket_mode = "644" + socket_user = "1000" + socket_group = "1000" + redact_addresses = true + redact_cluster_name = true + redact_version = true }`)) config := Config{ @@ -903,16 +912,14 @@ listener "tcp" { config.Listeners = listeners // Track which types of listener were found. for _, l := range config.Listeners { - config.found(l.Type, l.Type) + config.found(l.Type.String(), l.Type.String()) } - if len(config.Listeners) == 0 { - t.Fatalf("expected at least one listener in the config") - } - listener := config.Listeners[0] - if listener.Type != "tcp" { - t.Fatalf("expected tcp listener in the config") - } + require.Len(t, config.Listeners, 2) + tcpListener := config.Listeners[0] + require.Equal(t, configutil.TCP, tcpListener.Type) + unixListner := config.Listeners[1] + require.Equal(t, configutil.Unix, unixListner.Type) expected := &Config{ SharedConfig: &configutil.SharedConfig{ @@ -946,6 +953,16 @@ listener "tcp" { RedactClusterName: true, RedactVersion: true, }, + { + Type: "unix", + Address: "/var/run/vault.sock", + SocketMode: "644", + SocketUser: "1000", + SocketGroup: "1000", + RedactAddresses: false, + RedactClusterName: false, + RedactVersion: false, + }, }, }, } diff --git a/command/server/listener.go b/command/server/listener.go index 1e2133dc395b..2f2e058a6b84 100644 --- a/command/server/listener.go +++ b/command/server/listener.go @@ -30,7 +30,7 @@ var BuiltinListeners = map[string]ListenerFactory{ // NewListener creates a new listener of the given type with the given // configuration. The type is looked up in the BuiltinListeners map. func NewListener(l *configutil.Listener, logger io.Writer, ui cli.Ui) (net.Listener, map[string]string, reloadutil.ReloadFunc, error) { - f, ok := BuiltinListeners[l.Type] + f, ok := BuiltinListeners[l.Type.String()] if !ok { return nil, nil, nil, fmt.Errorf("unknown listener type: %q", l.Type) } diff --git a/internalshared/configutil/config.go b/internalshared/configutil/config.go index f5e8d6fa0122..2cd7058d01d8 100644 --- a/internalshared/configutil/config.go +++ b/internalshared/configutil/config.go @@ -141,7 +141,7 @@ func ParseConfig(d string) (*SharedConfig, error) { // Track which types of listener were found. for _, l := range result.Listeners { - result.found(l.Type, l.Type) + result.found(l.Type.String(), l.Type.String()) } } diff --git a/internalshared/configutil/listener.go b/internalshared/configutil/listener.go index 20cc7bdfc52b..4459f144fb67 100644 --- a/internalshared/configutil/listener.go +++ b/internalshared/configutil/listener.go @@ -22,6 +22,14 @@ import ( "github.com/hashicorp/vault/helper/namespace" ) +const ( + TCP listenerType = "tcp" + Unix listenerType = "unix" +) + +// listenerType represents the supported types of listener. +type listenerType string + type ListenerTelemetry struct { UnusedKeys UnusedKeyMap `hcl:",unusedKeyPositions"` UnauthenticatedMetricsAccess bool `hcl:"-"` @@ -45,7 +53,7 @@ type Listener struct { UnusedKeys UnusedKeyMap `hcl:",unusedKeyPositions"` RawConfig map[string]interface{} - Type string + Type listenerType Purpose []string `hcl:"-"` PurposeRaw interface{} `hcl:"purpose"` Role string `hcl:"role"` @@ -254,6 +262,11 @@ func parseListener(item *ast.ObjectItem) (*Listener, error) { return l, nil } +// String returns the string version of a listener type. +func (t listenerType) String() string { + return string(t) +} + // parseChrootNamespace attempts to parse the raw listener chroot namespace settings. // The state of the listener will be modified, raw data will be cleared upon // successful parsing. @@ -279,27 +292,28 @@ func (l *Listener) parseChrootNamespaceSettings() error { // The state of the listener will be modified. func (l *Listener) parseType(fallback string) error { switch { - case l.Type != "": + case l.Type.String() != "": case fallback != "": default: return errors.New("listener type must be specified") } // Use type if available, otherwise fall back. - result := l.Type - if result == "" { - result = fallback + rawType := l.Type + if rawType == "" { + rawType = listenerType(fallback) } - result = strings.ToLower(result) + + parsedType := listenerType(strings.ToLower(rawType.String())) // Sanity check the values - switch result { - case "tcp", "unix": + switch parsedType { + case TCP, Unix: default: - return fmt.Errorf("unsupported listener type %q", result) + return fmt.Errorf("unsupported listener type %q", parsedType.String()) } - l.Type = result + l.Type = parsedType return nil } @@ -396,6 +410,13 @@ func (l *Listener) parseTLSSettings() error { // The state of the listener will be modified, raw data will be cleared upon // successful parsing. func (l *Listener) parseHTTPHeaderSettings() error { + // Custom response headers are only supported by TCP listeners. + // Clear raw data and return early if it was something else. + if l.Type != TCP { + l.CustomResponseHeadersRaw = nil + return nil + } + // if CustomResponseHeadersRaw is nil, we still need to set the default headers customHeadersMap, err := ParseCustomResponseHeaders(l.CustomResponseHeadersRaw) if err != nil { @@ -604,6 +625,16 @@ func (l *Listener) parseCORSSettings() error { // The state of the listener will be modified, raw data will be cleared upon // successful parsing. func (l *Listener) parseRedactionSettings() error { + // Redaction is only supported on TCP listeners. + // Clear raw data and return early if it was something else. + if l.Type != TCP { + l.RedactAddressesRaw = nil + l.RedactClusterNameRaw = nil + l.RedactVersionRaw = nil + + return nil + } + var err error if l.RedactAddressesRaw != nil { diff --git a/internalshared/configutil/listener_test.go b/internalshared/configutil/listener_test.go index ac4239efc1ff..bfa5a3382266 100644 --- a/internalshared/configutil/listener_test.go +++ b/internalshared/configutil/listener_test.go @@ -159,7 +159,7 @@ func TestListener_parseType(t *testing.T) { tc := tc t.Run(name, func(t *testing.T) { t.Parallel() - l := &Listener{Type: tc.inputType} + l := &Listener{Type: listenerType(tc.inputType)} err := l.parseType(tc.inputFallback) switch { case tc.isErrorExpected: @@ -167,7 +167,7 @@ func TestListener_parseType(t *testing.T) { require.ErrorContains(t, err, tc.errorMessage) default: require.NoError(t, err) - require.Equal(t, tc.expectedValue, l.Type) + require.Equal(t, tc.expectedValue, l.Type.String()) } }) } @@ -861,16 +861,19 @@ func TestListener_parseCORSSettings(t *testing.T) { // assign the relevant value on the SharedConfig struct. func TestListener_parseHTTPHeaderSettings(t *testing.T) { tests := map[string]struct { + listenerType listenerType rawCustomResponseHeaders []map[string]any expectedNumCustomResponseHeaders int isErrorExpected bool errorMessage string }{ "nil": { + listenerType: TCP, isErrorExpected: false, expectedNumCustomResponseHeaders: 1, // default: Strict-Transport-Security }, "custom-headers-bad": { + listenerType: TCP, rawCustomResponseHeaders: []map[string]any{ {"juan": false}, }, @@ -878,6 +881,7 @@ func TestListener_parseHTTPHeaderSettings(t *testing.T) { errorMessage: "failed to parse custom_response_headers", }, "custom-headers-good": { + listenerType: TCP, rawCustomResponseHeaders: []map[string]any{ { "2xx": []map[string]any{ @@ -888,6 +892,18 @@ func TestListener_parseHTTPHeaderSettings(t *testing.T) { expectedNumCustomResponseHeaders: 2, isErrorExpected: false, }, + "unix-no-headers": { + listenerType: Unix, + rawCustomResponseHeaders: []map[string]any{ + { + "2xx": []map[string]any{ + {"X-Custom-Header": []any{"Custom Header Value 1", "Custom Header Value 2"}}, + }, + }, + }, + expectedNumCustomResponseHeaders: 0, + isErrorExpected: false, + }, } for name, tc := range tests { @@ -898,6 +914,7 @@ func TestListener_parseHTTPHeaderSettings(t *testing.T) { // Configure listener with raw values l := &Listener{ + Type: tc.listenerType, CustomResponseHeadersRaw: tc.rawCustomResponseHeaders, } @@ -978,6 +995,7 @@ func TestListener_parseChrootNamespaceSettings(t *testing.T) { // assign the relevant value on the SharedConfig struct. func TestListener_parseRedactionSettings(t *testing.T) { tests := map[string]struct { + listenerType listenerType rawRedactAddresses any expectedRedactAddresses bool rawRedactClusterName any @@ -988,41 +1006,58 @@ func TestListener_parseRedactionSettings(t *testing.T) { errorMessage string }{ "missing": { + listenerType: TCP, isErrorExpected: false, expectedRedactAddresses: false, expectedRedactClusterName: false, expectedRedactVersion: false, }, "redact-addresses-bad": { + listenerType: TCP, rawRedactAddresses: "juan", isErrorExpected: true, errorMessage: "invalid value for redact_addresses", }, "redact-addresses-good": { + listenerType: TCP, rawRedactAddresses: "true", expectedRedactAddresses: true, isErrorExpected: false, }, "redact-cluster-name-bad": { + listenerType: TCP, rawRedactClusterName: "juan", isErrorExpected: true, errorMessage: "invalid value for redact_cluster_name", }, "redact-cluster-name-good": { + listenerType: TCP, rawRedactClusterName: "true", expectedRedactClusterName: true, isErrorExpected: false, }, "redact-version-bad": { + listenerType: TCP, rawRedactVersion: "juan", isErrorExpected: true, errorMessage: "invalid value for redact_version", }, "redact-version-good": { + listenerType: TCP, rawRedactVersion: "true", expectedRedactVersion: true, isErrorExpected: false, }, + "redact-unix-na": { + listenerType: Unix, + rawRedactAddresses: "true", + expectedRedactAddresses: false, + rawRedactClusterName: "true", + expectedRedactClusterName: false, + rawRedactVersion: "true", + expectedRedactVersion: false, + isErrorExpected: false, + }, } for name, tc := range tests { @@ -1033,6 +1068,7 @@ func TestListener_parseRedactionSettings(t *testing.T) { // Configure listener with raw values l := &Listener{ + Type: tc.listenerType, RedactAddressesRaw: tc.rawRedactAddresses, RedactClusterNameRaw: tc.rawRedactClusterName, RedactVersionRaw: tc.rawRedactVersion, From 434133f273a62899c5fb703e70f36b5f332fa87b Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Tue, 10 Oct 2023 20:36:59 +0100 Subject: [PATCH 2/3] fix failing test --- command/server/config_test_helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/server/config_test_helpers.go b/command/server/config_test_helpers.go index 5e5378d23c37..bfeec3ca0de7 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -793,7 +793,7 @@ func testConfig_Sanitized(t *testing.T) { "address": "127.0.0.1:443", "chroot_namespace": "admin/", }, - "type": "tcp", + "type": configutil.TCP, }, }, "log_format": "", From 0929124e379206b061104cec8f72fc15f149fc29 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Wed, 11 Oct 2023 09:32:13 +0100 Subject: [PATCH 3/3] updates from PR feedback --- command/server.go | 4 ++-- command/server/listener.go | 4 ++-- internalshared/configutil/listener.go | 27 +++++++++++++--------- internalshared/configutil/listener_test.go | 6 ++--- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/command/server.go b/command/server.go index fa078f975c40..870871e11bfb 100644 --- a/command/server.go +++ b/command/server.go @@ -886,9 +886,9 @@ func (c *ServerCommand) InitListeners(config *server.Config, disableClustering b } if reloadFunc != nil { - relSlice := (*c.reloadFuncs)["listener|"+lnConfig.Type.String()] + relSlice := (*c.reloadFuncs)[fmt.Sprintf("listener|%s", lnConfig.Type)] relSlice = append(relSlice, reloadFunc) - (*c.reloadFuncs)["listener|"+lnConfig.Type.String()] = relSlice + (*c.reloadFuncs)[fmt.Sprintf("listener|%s", lnConfig.Type)] = relSlice } if !disableClustering && lnConfig.Type == "tcp" { diff --git a/command/server/listener.go b/command/server/listener.go index 2f2e058a6b84..7e308a0ccb79 100644 --- a/command/server/listener.go +++ b/command/server/listener.go @@ -22,7 +22,7 @@ import ( type ListenerFactory func(*configutil.Listener, io.Writer, cli.Ui) (net.Listener, map[string]string, reloadutil.ReloadFunc, error) // BuiltinListeners is the list of built-in listener types. -var BuiltinListeners = map[string]ListenerFactory{ +var BuiltinListeners = map[configutil.ListenerType]ListenerFactory{ "tcp": tcpListenerFactory, "unix": unixListenerFactory, } @@ -30,7 +30,7 @@ var BuiltinListeners = map[string]ListenerFactory{ // NewListener creates a new listener of the given type with the given // configuration. The type is looked up in the BuiltinListeners map. func NewListener(l *configutil.Listener, logger io.Writer, ui cli.Ui) (net.Listener, map[string]string, reloadutil.ReloadFunc, error) { - f, ok := BuiltinListeners[l.Type.String()] + f, ok := BuiltinListeners[l.Type] if !ok { return nil, nil, nil, fmt.Errorf("unknown listener type: %q", l.Type) } diff --git a/internalshared/configutil/listener.go b/internalshared/configutil/listener.go index 4459f144fb67..b040ef9246bb 100644 --- a/internalshared/configutil/listener.go +++ b/internalshared/configutil/listener.go @@ -23,12 +23,12 @@ import ( ) const ( - TCP listenerType = "tcp" - Unix listenerType = "unix" + TCP ListenerType = "tcp" + Unix ListenerType = "unix" ) -// listenerType represents the supported types of listener. -type listenerType string +// ListenerType represents the supported types of listener. +type ListenerType string type ListenerTelemetry struct { UnusedKeys UnusedKeyMap `hcl:",unusedKeyPositions"` @@ -53,7 +53,7 @@ type Listener struct { UnusedKeys UnusedKeyMap `hcl:",unusedKeyPositions"` RawConfig map[string]interface{} - Type listenerType + Type ListenerType Purpose []string `hcl:"-"` PurposeRaw interface{} `hcl:"purpose"` Role string `hcl:"role"` @@ -262,9 +262,14 @@ func parseListener(item *ast.ObjectItem) (*Listener, error) { return l, nil } +// Normalize returns the lower case string version of a listener type. +func (t ListenerType) Normalize() ListenerType { + return ListenerType(strings.ToLower(string(t))) +} + // String returns the string version of a listener type. -func (t listenerType) String() string { - return string(t) +func (t ListenerType) String() string { + return string(t.Normalize()) } // parseChrootNamespace attempts to parse the raw listener chroot namespace settings. @@ -292,7 +297,7 @@ func (l *Listener) parseChrootNamespaceSettings() error { // The state of the listener will be modified. func (l *Listener) parseType(fallback string) error { switch { - case l.Type.String() != "": + case l.Type != "": case fallback != "": default: return errors.New("listener type must be specified") @@ -301,16 +306,16 @@ func (l *Listener) parseType(fallback string) error { // Use type if available, otherwise fall back. rawType := l.Type if rawType == "" { - rawType = listenerType(fallback) + rawType = ListenerType(fallback) } - parsedType := listenerType(strings.ToLower(rawType.String())) + parsedType := rawType.Normalize() // Sanity check the values switch parsedType { case TCP, Unix: default: - return fmt.Errorf("unsupported listener type %q", parsedType.String()) + return fmt.Errorf("unsupported listener type %q", parsedType) } l.Type = parsedType diff --git a/internalshared/configutil/listener_test.go b/internalshared/configutil/listener_test.go index bfa5a3382266..4ea11d57cc60 100644 --- a/internalshared/configutil/listener_test.go +++ b/internalshared/configutil/listener_test.go @@ -159,7 +159,7 @@ func TestListener_parseType(t *testing.T) { tc := tc t.Run(name, func(t *testing.T) { t.Parallel() - l := &Listener{Type: listenerType(tc.inputType)} + l := &Listener{Type: ListenerType(tc.inputType)} err := l.parseType(tc.inputFallback) switch { case tc.isErrorExpected: @@ -861,7 +861,7 @@ func TestListener_parseCORSSettings(t *testing.T) { // assign the relevant value on the SharedConfig struct. func TestListener_parseHTTPHeaderSettings(t *testing.T) { tests := map[string]struct { - listenerType listenerType + listenerType ListenerType rawCustomResponseHeaders []map[string]any expectedNumCustomResponseHeaders int isErrorExpected bool @@ -995,7 +995,7 @@ func TestListener_parseChrootNamespaceSettings(t *testing.T) { // assign the relevant value on the SharedConfig struct. func TestListener_parseRedactionSettings(t *testing.T) { tests := map[string]struct { - listenerType listenerType + listenerType ListenerType rawRedactAddresses any expectedRedactAddresses bool rawRedactClusterName any