From 290a38e424686493465cd10e3af05c0b9e7c4230 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Tue, 5 Jul 2022 14:43:12 +1000 Subject: [PATCH] fix(configuration): address parsing failure (#3653) This fixes an issue with parsing address types from strings. --- config.template.yml | 2 +- .../en/configuration/prologue/common.md | 26 ++++ .../en/configuration/telemetry/metrics.md | 8 +- internal/configuration/config.template.yml | 2 +- internal/configuration/decode_hooks_test.go | 115 ++++++++++++++++++ internal/configuration/schema/const.go | 5 + internal/configuration/schema/telemetry.go | 6 +- internal/configuration/schema/types.go | 98 +++++++-------- internal/configuration/schema/types_test.go | 50 ++++++++ internal/configuration/validator/const.go | 5 + internal/configuration/validator/telemetry.go | 19 ++- .../configuration/validator/telemetry_test.go | 101 +++++++++++++++ internal/suites/Standalone/configuration.yml | 1 + 13 files changed, 371 insertions(+), 67 deletions(-) create mode 100644 internal/configuration/schema/types_test.go create mode 100644 internal/configuration/validator/telemetry_test.go diff --git a/config.template.yml b/config.template.yml index c5ca8f42..70da4ffa 100644 --- a/config.template.yml +++ b/config.template.yml @@ -114,7 +114,7 @@ telemetry: enabled: false ## The address to listen on for metrics. This should be on a different port to the main server.port value. - address: '0.0.0.0:9959' + address: tcp://0.0.0.0:9959 ## ## TOTP Configuration diff --git a/docs/content/en/configuration/prologue/common.md b/docs/content/en/configuration/prologue/common.md index 72ad7f42..aaaabc9b 100644 --- a/docs/content/en/configuration/prologue/common.md +++ b/docs/content/en/configuration/prologue/common.md @@ -57,6 +57,32 @@ While you can use multiple of these blocks in combination, ee suggest keeping it | 1 day | `1d` or `24h` or `86400` or `86400s` | | 10 hours | `10h` or `600m` or `9h60m` or `36000` | +## Address + +The address type is a string that takes the following format: + +```text +[://][:] +``` + +The square brackets indicate optional sections, and the angled brackets indicate required sections. The following +sections elaborate on this. Sections may only be optional for the purposes of parsing, there may be a configuration +requirement that one of these is provided. + +### scheme + +The entire scheme is optional, but if the scheme host delimiter `://` is in the string, the scheme must be present. The +scheme must be one of `tcp://`, or `udp://`. The default scheme is `tcp://`. + +### ip + +The IP is required. If specifying an IPv6 it should be wrapped in square brackets. For example for the IPv6 address +`::1` with the `tcp://` scheme and port `80`: `tcp://[::1]:80`. + +### port + +The entire port is optional, but if the host port delimiter `:` exists it must also include a numeric port. + ## Regular Expressions We have several sections of configuration that utilize regular expressions. It's recommended to validate your regex diff --git a/docs/content/en/configuration/telemetry/metrics.md b/docs/content/en/configuration/telemetry/metrics.md index c417df28..9db3ba24 100644 --- a/docs/content/en/configuration/telemetry/metrics.md +++ b/docs/content/en/configuration/telemetry/metrics.md @@ -20,7 +20,7 @@ toc: true telemetry: metrics: enabled: false - address: "0.0.0.0:9959" + address: "tcp://0.0.0.0:9959" ``` ## Options @@ -33,10 +33,10 @@ Determines if the [Prometheus] HTTP Metrics Exporter is enabled. ### address -{{< confkey type="address" default="0.0.0.0:9959" required="no" >}} +{{< confkey type="address" default="tcp://0.0.0.0:9959" required="no" >}} -Configures the listener address for the [Prometheus] HTTP Metrics Exporter. The address must be a IPv4 or IPv6 address -followed by the port in the `
:` format. +Configures the listener address for the [Prometheus] HTTP Metrics Exporter. This configuration key uses the +[Address](../prologue/common.md#address) format. The scheme must be `tcp://` or empty. ## See More diff --git a/internal/configuration/config.template.yml b/internal/configuration/config.template.yml index c5ca8f42..70da4ffa 100644 --- a/internal/configuration/config.template.yml +++ b/internal/configuration/config.template.yml @@ -114,7 +114,7 @@ telemetry: enabled: false ## The address to listen on for metrics. This should be on a different port to the main server.port value. - address: '0.0.0.0:9959' + address: tcp://0.0.0.0:9959 ## ## TOTP Configuration diff --git a/internal/configuration/decode_hooks_test.go b/internal/configuration/decode_hooks_test.go index 4e815b2e..dc2eca84 100644 --- a/internal/configuration/decode_hooks_test.go +++ b/internal/configuration/decode_hooks_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/authelia/authelia/v4/internal/configuration" + "github.com/authelia/authelia/v4/internal/configuration/schema" ) func TestStringToMailAddressHookFunc(t *testing.T) { @@ -748,6 +749,120 @@ func TestStringToRegexpFuncPointers(t *testing.T) { } } +func TestStringToAddressHookFunc(t *testing.T) { + mustParseAddress := func(a string) (addr schema.Address) { + addrs, err := schema.NewAddressFromString(a) + if err != nil { + panic(err) + } + + return *addrs + } + + mustParseAddressPtr := func(a string) (addr *schema.Address) { + addr, err := schema.NewAddressFromString(a) + if err != nil { + panic(err) + } + + return addr + } + + testCases := []struct { + name string + have interface{} + expected interface{} + err string + decode bool + wantGrps []string + }{ + { + name: "ShouldDecodeNonPtr", + have: "tcp://0.0.0.0:2020", + expected: mustParseAddress("tcp://0.0.0.0:2020"), + decode: true, + }, + { + name: "ShouldDecodePtr", + have: "tcp://0.0.0.0:2020", + expected: mustParseAddressPtr("tcp://0.0.0.0:2020"), + decode: true, + }, + { + name: "ShouldNotDecodeIntegerToCorrectType", + have: 1, + expected: schema.Address{}, + decode: false, + }, + { + name: "ShouldNotDecodeIntegerToCorrectTypePtr", + have: 1, + expected: &schema.Address{}, + decode: false, + }, + { + name: "ShouldNotDecodeIntegerPtrToCorrectType", + have: testInt32Ptr(1), + expected: schema.Address{}, + decode: false, + }, + { + name: "ShouldNotDecodeIntegerPtrToCorrectTypePtr", + have: testInt32Ptr(1), + expected: &schema.Address{}, + decode: false, + }, + { + name: "ShouldNotDecodeToString", + have: "tcp://0.0.0.0:2020", + expected: "", + decode: false, + }, + { + name: "ShouldNotDecodeToIntPtr", + have: "tcp://0.0.0.0:2020", + expected: testInt32Ptr(1), + decode: false, + }, + { + name: "ShouldNotDecodeToIntPtr", + have: "tcp://0.0.0.0:2020", + expected: testInt32Ptr(1), + decode: false, + }, + { + name: "ShouldFailDecode", + have: "tcp://&!@^#*&!@#&*@!:2020", + expected: schema.Address{}, + err: "could not decode 'tcp://&!@^#*&!@#&*@!:2020' to a Address: could not parse string 'tcp://&!@^#*&!@#&*@!:2020' as address: expected format is [://][:]: parse \"tcp://&!@^\": invalid character \"^\" in host name", + decode: false, + }, + } + + hook := configuration.StringToAddressHookFunc() + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual, err := hook(reflect.TypeOf(tc.have), reflect.TypeOf(tc.expected), tc.have) + if tc.err != "" { + assert.EqualError(t, err, tc.err) + + if !tc.decode { + assert.Nil(t, actual) + } + } else { + assert.NoError(t, err) + + if tc.decode { + assert.Equal(t, tc.expected, actual) + } else { + assert.Equal(t, tc.have, actual) + } + } + }) + } +} + func testInt32Ptr(i int32) *int32 { return &i } diff --git a/internal/configuration/schema/const.go b/internal/configuration/schema/const.go index 073712a6..8ee9867b 100644 --- a/internal/configuration/schema/const.go +++ b/internal/configuration/schema/const.go @@ -1,6 +1,7 @@ package schema import ( + "regexp" "time" ) @@ -52,3 +53,7 @@ const ( // TOTPSecretSizeMinimum is the minimum secret size. TOTPSecretSizeMinimum = 20 ) + +// regexpHasScheme checks if a string has a scheme. Valid characters for schemes include alphanumeric, hyphen, +// period, and plus characters. +var regexpHasScheme = regexp.MustCompile(`^[-+.a-zA-Z\d]+://`) diff --git a/internal/configuration/schema/telemetry.go b/internal/configuration/schema/telemetry.go index c37f7bcf..594d193a 100644 --- a/internal/configuration/schema/telemetry.go +++ b/internal/configuration/schema/telemetry.go @@ -11,13 +11,13 @@ type TelemetryConfig struct { // TelemetryMetricsConfig represents the telemetry metrics config. type TelemetryMetricsConfig struct { - Enabled bool `koanf:"enabled"` - Address Address `koanf:"address"` + Enabled bool `koanf:"enabled"` + Address *Address `koanf:"address"` } // DefaultTelemetryConfig is the default telemetry configuration. var DefaultTelemetryConfig = TelemetryConfig{ Metrics: TelemetryMetricsConfig{ - Address: NewAddress("tcp", net.ParseIP("0.0.0.0"), 9959), + Address: &Address{true, "tcp", net.ParseIP("0.0.0.0"), 9959}, }, } diff --git a/internal/configuration/schema/types.go b/internal/configuration/schema/types.go index a6dcb0b9..dc3bc3bb 100644 --- a/internal/configuration/schema/types.go +++ b/internal/configuration/schema/types.go @@ -3,74 +3,64 @@ package schema import ( "fmt" "net" - "regexp" + "net/url" "strconv" + "strings" ) -var regexpAddress = regexp.MustCompile(`^((?P\w+)://)?((?P((((25[0-5]|2[0-4]\d|[01]?\d\d?)(\.)){3})(25[0-5]|2[0-4]\d|[01]?\d\d?)))|(\[(?P([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4})?:)?((25[0-5]|(2[0-4]|1?\d)?\d)\.){3}(25[0-5]|(2[0-4]|1?\d)?\d)|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?\d)?\d)\.){3}(25[0-5]|(2[0-4]|1?\d)?\d))\]):)?(?P\d+)$`) - -const tcp = "tcp" - -// NewAddress produces a valid address from input. -func NewAddress(scheme string, ip net.IP, port int) Address { - return Address{ - valid: true, - Scheme: scheme, - IP: ip, - Port: port, +// NewAddressFromString returns an *Address and error depending on the ability to parse the string as an Address. +func NewAddressFromString(a string) (addr *Address, err error) { + if len(a) == 0 { + return &Address{true, "tcp", net.ParseIP("0.0.0.0"), 0}, nil } + + var u *url.URL + + if regexpHasScheme.MatchString(a) { + u, err = url.Parse(a) + } else { + u, err = url.Parse("tcp://" + a) + } + + if err != nil { + return nil, fmt.Errorf("could not parse string '%s' as address: expected format is [://][:]: %w", a, err) + } + + return NewAddressFromURL(u) } -// NewAddressFromString parses a string and returns an *Address or error. -func NewAddressFromString(addr string) (address *Address, err error) { - if addr == "" { - return &Address{}, nil +// NewAddressFromURL returns an *Address and error depending on the ability to parse the *url.URL as an Address. +func NewAddressFromURL(u *url.URL) (addr *Address, err error) { + addr = &Address{ + Scheme: strings.ToLower(u.Scheme), + IP: net.ParseIP(u.Hostname()), } - if !regexpAddress.MatchString(addr) { - return nil, fmt.Errorf("the string '%s' does not appear to be a valid address", addr) + if addr.IP == nil { + return nil, fmt.Errorf("could not parse ip for address '%s': %s does not appear to be an IP", u.String(), u.Hostname()) } - address = &Address{ - valid: true, - } - - submatches := regexpAddress.FindStringSubmatch(addr) - - var ip, port string - - for i, name := range regexpAddress.SubexpNames() { - switch name { - case "Scheme": - address.Scheme = submatches[i] - case "IPv4": - ip = submatches[i] - - if address.Scheme == "" || address.Scheme == tcp { - address.Scheme = "tcp4" - } - case "IPv6": - ip = submatches[i] - - if address.Scheme == "" || address.Scheme == tcp { - address.Scheme = "tcp6" - } - case "Port": - port = submatches[i] + port := u.Port() + switch port { + case "": + break + default: + addr.Port, err = strconv.Atoi(port) + if err != nil { + return nil, fmt.Errorf("could not parse port for address '%s': %w", u.String(), err) } } - if address.IP = net.ParseIP(ip); address.IP == nil { - return nil, fmt.Errorf("failed to parse '%s' as an IP address", ip) + switch addr.Scheme { + case "tcp", "udp", "http", "https": + break + default: + return nil, fmt.Errorf("could not parse scheme for address '%s': scheme '%s' is not valid, expected to be one of 'tcp://', 'udp://'", u.String(), addr.Scheme) } - address.Port, _ = strconv.Atoi(port) + addr.valid = true - if address.Port <= 0 || address.Port > 65535 { - return nil, fmt.Errorf("failed to parse address port '%d' is invalid: ports must be between 1 and 65535", address.Port) - } - - return address, nil + return addr, nil } // Address represents an address. @@ -78,8 +68,8 @@ type Address struct { valid bool Scheme string - net.IP - Port int + IP net.IP + Port int } // Valid returns true if the Address is valid. diff --git a/internal/configuration/schema/types_test.go b/internal/configuration/schema/types_test.go new file mode 100644 index 00000000..9d5da92c --- /dev/null +++ b/internal/configuration/schema/types_test.go @@ -0,0 +1,50 @@ +package schema + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewAddressFromString(t *testing.T) { + testCases := []struct { + name string + have string + expected *Address + expectedHostPort, expectedString, expectedErr string + }{ + {"ShouldParseBasicAddress", "tcp://0.0.0.0:9091", &Address{true, "tcp", net.ParseIP("0.0.0.0"), 9091}, "0.0.0.0:9091", "tcp://0.0.0.0:9091", ""}, + {"ShouldParseEmptyAddress", "", &Address{true, "tcp", net.ParseIP("0.0.0.0"), 0}, "0.0.0.0:0", "tcp://0.0.0.0:0", ""}, + {"ShouldParseAddressMissingScheme", "0.0.0.0:9091", &Address{true, "tcp", net.ParseIP("0.0.0.0"), 9091}, "0.0.0.0:9091", "tcp://0.0.0.0:9091", ""}, + {"ShouldParseAddressMissingPort", "tcp://0.0.0.0", &Address{true, "tcp", net.ParseIP("0.0.0.0"), 0}, "0.0.0.0:0", "tcp://0.0.0.0:0", ""}, + {"ShouldNotParseUnknownScheme", "a://0.0.0.0", nil, "", "", "could not parse scheme for address 'a://0.0.0.0': scheme 'a' is not valid, expected to be one of 'tcp://', 'udp://'"}, + {"ShouldNotParseInvalidPort", "tcp://0.0.0.0:abc", nil, "", "", "could not parse string 'tcp://0.0.0.0:abc' as address: expected format is [://][:]: parse \"tcp://0.0.0.0:abc\": invalid port \":abc\" after host"}, + {"ShouldNotParseInvalidIP", "tcp://example.com:9091", nil, "", "", "could not parse ip for address 'tcp://example.com:9091': example.com does not appear to be an IP"}, + {"ShouldNotParseInvalidAddress", "@$@#%@#$@@", nil, "", "", "could not parse string '@$@#%@#$@@' as address: expected format is [://][:]: parse \"tcp://@$@#%@#$@@\": invalid URL escape \"%@#\""}, + {"ShouldNotParseInvalidAddressWithScheme", "tcp://@$@#%@#$@@", nil, "", "", "could not parse string 'tcp://@$@#%@#$@@' as address: expected format is [://][:]: parse \"tcp://@$@#%@#$@@\": invalid URL escape \"%@#\""}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual, actualErr := NewAddressFromString(tc.have) + + if len(tc.expectedErr) != 0 { + assert.EqualError(t, actualErr, tc.expectedErr) + } else { + assert.Nil(t, actualErr) + + assert.Equal(t, actual.HostPort(), tc.expectedHostPort) + assert.Equal(t, actual.String(), tc.expectedString) + } + + assert.Equal(t, tc.expected, actual) + + if actual != nil { + assert.True(t, actual.Valid()) + assert.NotEmpty(t, actual.String()) + assert.NotEmpty(t, actual.HostPort()) + } + }) + } +} diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 8656ca0f..bfd6eead 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -123,6 +123,11 @@ const ( errFmtStoragePostgreSQLInvalidSSLMode = "storage: postgres: ssl: option 'mode' must be one of '%s' but it is configured as '%s'" ) +// Telemetry Error constants. +const ( + errFmtTelemetryMetricsScheme = "telemetry: metrics: option 'address' must have a scheme 'tcp://' but it is configured as '%s'" +) + // OpenID Error constants. const ( errFmtOIDCNoClientsConfigured = "identity_providers: oidc: option 'clients' must have one or " + diff --git a/internal/configuration/validator/telemetry.go b/internal/configuration/validator/telemetry.go index 9612d882..d546cc52 100644 --- a/internal/configuration/validator/telemetry.go +++ b/internal/configuration/validator/telemetry.go @@ -1,14 +1,25 @@ package validator import ( + "fmt" + "github.com/authelia/authelia/v4/internal/configuration/schema" ) // ValidateTelemetry validates the telemetry configuration. func ValidateTelemetry(config *schema.Configuration, validator *schema.StructValidator) { - if config.Telemetry.Metrics.Enabled { - if config.Telemetry.Metrics.Address.String() == "" { - config.Telemetry.Metrics.Address = schema.DefaultTelemetryConfig.Metrics.Address - } + if config.Telemetry.Metrics.Address == nil { + config.Telemetry.Metrics.Address = schema.DefaultTelemetryConfig.Metrics.Address + } + + switch config.Telemetry.Metrics.Address.Scheme { + case "tcp": + break + default: + validator.Push(fmt.Errorf(errFmtTelemetryMetricsScheme, config.Telemetry.Metrics.Address.Scheme)) + } + + if config.Telemetry.Metrics.Address.Port == 0 { + config.Telemetry.Metrics.Address.Port = schema.DefaultTelemetryConfig.Metrics.Address.Port } } diff --git a/internal/configuration/validator/telemetry_test.go b/internal/configuration/validator/telemetry_test.go new file mode 100644 index 00000000..aa3e5968 --- /dev/null +++ b/internal/configuration/validator/telemetry_test.go @@ -0,0 +1,101 @@ +package validator + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authelia/authelia/v4/internal/configuration/schema" +) + +func TestValidateTelemetry(t *testing.T) { + mustParseAddress := func(a string) *schema.Address { + addr, err := schema.NewAddressFromString(a) + if err != nil { + panic(err) + } + + return addr + } + + testCases := []struct { + name string + have *schema.Configuration + expected *schema.Configuration + expectedWrns, expectedErrs []string + }{ + { + "ShouldSetDefaults", + &schema.Configuration{}, + &schema.Configuration{Telemetry: schema.DefaultTelemetryConfig}, + nil, + nil, + }, + { + "ShouldSetDefaultPort", + &schema.Configuration{Telemetry: schema.TelemetryConfig{Metrics: schema.TelemetryMetricsConfig{Address: mustParseAddress("tcp://0.0.0.0")}}}, + &schema.Configuration{Telemetry: schema.DefaultTelemetryConfig}, + nil, + nil, + }, + { + "ShouldSetDefaultPortAlt", + &schema.Configuration{Telemetry: schema.TelemetryConfig{Metrics: schema.TelemetryMetricsConfig{Address: mustParseAddress("tcp://0.0.0.0:0")}}}, + &schema.Configuration{Telemetry: schema.DefaultTelemetryConfig}, + nil, + nil, + }, + { + "ShouldSetDefaultPortWithCustomIP", + &schema.Configuration{Telemetry: schema.TelemetryConfig{Metrics: schema.TelemetryMetricsConfig{Address: mustParseAddress("tcp://127.0.0.1")}}}, + &schema.Configuration{Telemetry: schema.TelemetryConfig{Metrics: schema.TelemetryMetricsConfig{Address: mustParseAddress("tcp://127.0.0.1:9959")}}}, + nil, + nil, + }, + { + "ShouldNotValidateUDP", + &schema.Configuration{Telemetry: schema.TelemetryConfig{Metrics: schema.TelemetryMetricsConfig{Address: mustParseAddress("udp://0.0.0.0")}}}, + &schema.Configuration{Telemetry: schema.TelemetryConfig{Metrics: schema.TelemetryMetricsConfig{Address: mustParseAddress("udp://0.0.0.0:9959")}}}, + nil, + []string{"telemetry: metrics: option 'address' must have a scheme 'tcp://' but it is configured as 'udp'"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + v := schema.NewStructValidator() + + ValidateTelemetry(tc.have, v) + + assert.Equal(t, tc.expected.Telemetry.Metrics.Enabled, tc.have.Telemetry.Metrics.Enabled) + assert.Equal(t, tc.expected.Telemetry.Metrics.Address, tc.have.Telemetry.Metrics.Address) + + lenWrns := len(tc.expectedWrns) + wrns := v.Warnings() + + if lenWrns == 0 { + assert.Len(t, wrns, 0) + } else { + require.Len(t, wrns, lenWrns) + + for i, expectedWrn := range tc.expectedWrns { + assert.EqualError(t, wrns[i], expectedWrn) + } + } + + lenErrs := len(tc.expectedErrs) + errs := v.Errors() + + if lenErrs == 0 { + assert.Len(t, errs, 0) + } else { + require.Len(t, errs, lenErrs) + + for i, expectedErr := range tc.expectedErrs { + assert.EqualError(t, errs[i], expectedErr) + } + } + }) + } +} diff --git a/internal/suites/Standalone/configuration.yml b/internal/suites/Standalone/configuration.yml index aaddbd83..5a959c9b 100644 --- a/internal/suites/Standalone/configuration.yml +++ b/internal/suites/Standalone/configuration.yml @@ -14,6 +14,7 @@ server: telemetry: metrics: enabled: true + address: tcp://0.0.0.0:9959 log: level: debug