From f17a4eae3ef05a11447b89126e65ce4bcc9a5c43 Mon Sep 17 00:00:00 2001 From: "Quentin McGaw (desktop)" Date: Wed, 18 Aug 2021 20:12:26 +0000 Subject: [PATCH] Maint: rework OpenVPN custom configuration code - Refactor code and errors returned - Add unit tests - Make custom config code independent from loop --- internal/models/openvpn.go | 16 ++ internal/openvpn/custom.go | 210 +++++++++------- internal/openvpn/custom_test.go | 409 ++++++++++++++++++++++++++++++++ internal/openvpn/run.go | 2 +- 4 files changed, 546 insertions(+), 91 deletions(-) create mode 100644 internal/openvpn/custom_test.go diff --git a/internal/models/openvpn.go b/internal/models/openvpn.go index 8dff9071..546d9a1e 100644 --- a/internal/models/openvpn.go +++ b/internal/models/openvpn.go @@ -26,3 +26,19 @@ func (o OpenVPNConnection) RemoteLine() (line string) { func (o OpenVPNConnection) ProtoLine() (line string) { return "proto " + o.Protocol } + +// UpdateEmptyWith updates each field of the connection where the value is not set. +func (o *OpenVPNConnection) UpdateEmptyWith(connection OpenVPNConnection) { + if o.IP == nil { + o.IP = connection.IP + } + if o.Port == 0 { + o.Port = connection.Port + } + if o.Protocol == "" { + o.Protocol = connection.Protocol + } + if o.Hostname == "" { + o.Hostname = connection.Hostname + } +} diff --git a/internal/openvpn/custom.go b/internal/openvpn/custom.go index 2c1ccbdb..e9f29bf6 100644 --- a/internal/openvpn/custom.go +++ b/internal/openvpn/custom.go @@ -15,23 +15,25 @@ import ( "github.com/qdm12/gluetun/internal/provider/utils" ) -var errProcessCustomConfig = errors.New("cannot process custom config") +var ( + errReadCustomConfig = errors.New("cannot read custom configuration file") + errExtractConnection = errors.New("cannot extract connection from custom configuration file") +) -func (l *Loop) processCustomConfig(settings configuration.OpenVPN) ( +func processCustomConfig(settings configuration.OpenVPN) ( lines []string, connection models.OpenVPNConnection, err error) { lines, err = readCustomConfigLines(settings.Config) if err != nil { - return nil, connection, fmt.Errorf("%w: %s", errProcessCustomConfig, err) + return nil, connection, fmt.Errorf("%w: %s", errReadCustomConfig, err) } - lines = modifyCustomConfig(lines, settings) - connection, err = extractConnectionFromLines(lines) if err != nil { - return nil, connection, fmt.Errorf("%w: %s", errProcessCustomConfig, err) + return nil, connection, fmt.Errorf("%w: %s", errExtractConnection, err) } - lines = setConnectionToLines(lines, connection) + lines = modifyCustomConfig(lines, settings, connection) + return lines, connection, nil } @@ -41,10 +43,10 @@ func readCustomConfigLines(filepath string) ( if err != nil { return nil, err } - defer file.Close() b, err := io.ReadAll(file) if err != nil { + _ = file.Close() return nil, err } @@ -55,8 +57,8 @@ func readCustomConfigLines(filepath string) ( return strings.Split(string(b), "\n"), nil } -func modifyCustomConfig(lines []string, - settings configuration.OpenVPN) (modified []string) { +func modifyCustomConfig(lines []string, settings configuration.OpenVPN, + connection models.OpenVPNConnection) (modified []string) { // Remove some lines for _, line := range lines { switch { @@ -65,9 +67,11 @@ func modifyCustomConfig(lines []string, strings.HasPrefix(line, "verb "), strings.HasPrefix(line, "auth-user-pass "), strings.HasPrefix(line, "user "), - len(settings.Cipher) > 0 && strings.HasPrefix(line, "cipher "), - len(settings.Cipher) > 0 && strings.HasPrefix(line, "data-ciphers"), - len(settings.Auth) > 0 && strings.HasPrefix(line, "auth "), + strings.HasPrefix(line, "proto "), + strings.HasPrefix(line, "remote "), + settings.Cipher != "" && strings.HasPrefix(line, "cipher "), + settings.Cipher != "" && strings.HasPrefix(line, "data-ciphers "), + settings.Auth != "" && strings.HasPrefix(line, "auth "), settings.MSSFix > 0 && strings.HasPrefix(line, "mssfix "), !settings.IPv6 && strings.HasPrefix(line, "tun-ipv6"): default: @@ -76,6 +80,8 @@ func modifyCustomConfig(lines []string, } // Add values + modified = append(modified, connection.ProtoLine()) + modified = append(modified, connection.RemoteLine()) modified = append(modified, "mute-replay-warnings") modified = append(modified, "auth-nocache") modified = append(modified, "pull-filter ignore \"auth-token\"") // prevent auth failed loop @@ -85,10 +91,10 @@ func modifyCustomConfig(lines []string, modified = append(modified, "auth-user-pass "+constants.OpenVPNAuthConf) } modified = append(modified, "verb "+strconv.Itoa(settings.Verbosity)) - if len(settings.Cipher) > 0 { + if settings.Cipher != "" { modified = append(modified, utils.CipherLines(settings.Cipher, settings.Version)...) } - if len(settings.Auth) > 0 { + if settings.Auth != "" { modified = append(modified, "auth "+settings.Auth) } if settings.MSSFix > 0 { @@ -105,64 +111,19 @@ func modifyCustomConfig(lines []string, return modified } -var errExtractConnection = errors.New("cannot extract connection") +var ( + errRemoteLineNotFound = errors.New("remote line not found") +) // extractConnectionFromLines always takes the first remote line only. -func extractConnectionFromLines(lines []string) ( //nolint:gocognit +func extractConnectionFromLines(lines []string) ( connection models.OpenVPNConnection, err error) { - for _, line := range lines { - switch { - case strings.HasPrefix(line, "proto "): - fields := strings.Fields(line) - if n := len(fields); n != 2 { //nolint:gomnd - return connection, fmt.Errorf( - "%w: proto line has %d fields instead of 2: %s", - errExtractConnection, n, line) - } - connection.Protocol = fields[1] - - // only take the first remote line - case strings.HasPrefix(line, "remote ") && connection.IP == nil: - fields := strings.Fields(line) - n := len(fields) - //nolint:gomnd - if n < 2 { - return connection, fmt.Errorf( - "%w: remote line has not enough fields: %s", - errExtractConnection, line) - } - - host := fields[1] - if ip := net.ParseIP(host); ip != nil { - connection.IP = ip - } else { - return connection, fmt.Errorf( - "%w: for now, the remote line must contain an IP adddress: %s", - errExtractConnection, line) - // TODO resolve hostname once there is an option to allow it through - // the firewall before the VPN is up. - } - - if n > 2 { //nolint:gomnd - port, err := strconv.Atoi(fields[2]) - if err != nil { - return connection, fmt.Errorf( - "%w: remote line has an invalid port: %s", - errExtractConnection, line) - } - connection.Port = uint16(port) - } - - if n > 3 { //nolint:gomnd - connection.Protocol = strings.ToLower(fields[3]) - } - - if n > 4 { //nolint:gomnd - return connection, fmt.Errorf( - "%w: remote line has too many fields: %s", - errExtractConnection, line) - } + for i, line := range lines { + newConnectionData, err := extractConnectionFromLine(line) + if err != nil { + return connection, fmt.Errorf("on line %d: %w", i+1, err) } + connection.UpdateEmptyWith(newConnectionData) if connection.Protocol != "" && connection.IP != nil { break @@ -170,40 +131,109 @@ func extractConnectionFromLines(lines []string) ( //nolint:gocognit } if connection.IP == nil { - return connection, fmt.Errorf("%w: remote line not found", errExtractConnection) + return connection, errRemoteLineNotFound } - switch connection.Protocol { - case "": - connection.Protocol = "udp" - case "tcp", "udp": - default: - return connection, fmt.Errorf("%w: network protocol not supported: %s", errExtractConnection, connection.Protocol) + if connection.Protocol == "" { + connection.Protocol = constants.UDP } if connection.Port == 0 { - if connection.Protocol == "tcp" { - const defaultPort uint16 = 443 - connection.Port = defaultPort - } else { - const defaultPort uint16 = 1194 - connection.Port = defaultPort + connection.Port = 1194 + if connection.Protocol == constants.TCP { + connection.Port = 443 } } return connection, nil } -func setConnectionToLines(lines []string, connection models.OpenVPNConnection) (modified []string) { - for i, line := range lines { - switch { - case strings.HasPrefix(line, "proto "): - lines[i] = connection.ProtoLine() +var ( + errExtractProto = errors.New("failed extracting protocol from proto line") + errExtractRemote = errors.New("failed extracting protocol from remote line") +) - case strings.HasPrefix(line, "remote "): - lines[i] = connection.RemoteLine() +func extractConnectionFromLine(line string) ( + connection models.OpenVPNConnection, err error) { + switch { + case strings.HasPrefix(line, "proto "): + connection.Protocol, err = extractProto(line) + if err != nil { + return connection, fmt.Errorf("%w: %s", errExtractProto, err) + } + + // only take the first remote line + case strings.HasPrefix(line, "remote ") && connection.IP == nil: + connection.IP, connection.Port, connection.Protocol, err = extractRemote(line) + if err != nil { + return connection, fmt.Errorf("%w: %s", errExtractRemote, err) } } - return lines + return connection, nil +} + +var ( + errProtoLineFieldsCount = errors.New("proto line has not 2 fields as expected") + errProtocolNotSupported = errors.New("network protocol not supported") +) + +func extractProto(line string) (protocol string, err error) { + fields := strings.Fields(line) + if len(fields) != 2 { //nolint:gomnd + return "", fmt.Errorf("%w: %s", errProtoLineFieldsCount, line) + } + + switch fields[1] { + case "tcp", "udp": + default: + return "", fmt.Errorf("%w: %s", errProtocolNotSupported, fields[1]) + } + + return fields[1], nil +} + +var ( + errRemoteLineFieldsCount = errors.New("remote line has not 2 fields as expected") + errHostNotIP = errors.New("host is not an an IP address") + errPortNotValid = errors.New("port is not valid") +) + +func extractRemote(line string) (ip net.IP, port uint16, + protocol string, err error) { + fields := strings.Fields(line) + n := len(fields) + + if n < 2 || n > 4 { + return nil, 0, "", fmt.Errorf("%w: %s", errRemoteLineFieldsCount, line) + } + + host := fields[1] + ip = net.ParseIP(host) + if ip == nil { + return nil, 0, "", fmt.Errorf("%w: %s", errHostNotIP, host) + // TODO resolve hostname once there is an option to allow it through + // the firewall before the VPN is up. + } + + if n > 2 { //nolint:gomnd + portInt, err := strconv.Atoi(fields[2]) + if err != nil { + return nil, 0, "", fmt.Errorf("%w: %s", errPortNotValid, line) + } else if portInt < 1 || portInt > 65535 { + return nil, 0, "", fmt.Errorf("%w: not between 1 and 65535: %d", errPortNotValid, portInt) + } + port = uint16(portInt) + } + + if n > 3 { //nolint:gomnd + switch fields[3] { + case "tcp", "udp": + protocol = fields[3] + default: + return nil, 0, "", fmt.Errorf("%w: %s", errProtocolNotSupported, fields[3]) + } + } + + return ip, port, protocol, nil } diff --git a/internal/openvpn/custom_test.go b/internal/openvpn/custom_test.go new file mode 100644 index 00000000..313e532e --- /dev/null +++ b/internal/openvpn/custom_test.go @@ -0,0 +1,409 @@ +package openvpn + +import ( + "errors" + "net" + "os" + "testing" + + "github.com/qdm12/gluetun/internal/configuration" + "github.com/qdm12/gluetun/internal/constants" + "github.com/qdm12/gluetun/internal/models" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_processCustomConfig(t *testing.T) { + t.Parallel() + + file, err := os.CreateTemp("", "") + require.NoError(t, err) + defer removeFile(t, file.Name()) + defer file.Close() + + _, err = file.WriteString("remote 1.9.8.7\nkeep me\ncipher remove") + require.NoError(t, err) + + err = file.Close() + require.NoError(t, err) + + settings := configuration.OpenVPN{ + Cipher: "cipher", + MSSFix: 999, + Config: file.Name(), + } + + lines, connection, err := processCustomConfig(settings) + assert.NoError(t, err) + + expectedLines := []string{ + "keep me", + "proto udp", + "remote 1.9.8.7 1194", + "mute-replay-warnings", + "auth-nocache", + "pull-filter ignore \"auth-token\"", + "auth-retry nointeract", + "suppress-timestamps", + "verb 0", + "data-ciphers-fallback cipher", + "data-ciphers cipher", + "mssfix 999", + "pull-filter ignore \"route-ipv6\"", + "pull-filter ignore \"ifconfig-ipv6\"", + "user ", + } + assert.Equal(t, expectedLines, lines) + + expectedConnection := models.OpenVPNConnection{ + IP: net.IPv4(1, 9, 8, 7), + Port: 1194, + Protocol: constants.UDP, + } + assert.Equal(t, expectedConnection, connection) +} + +func Test_readCustomConfigLines(t *testing.T) { + t.Parallel() + + file, err := os.CreateTemp("", "") + require.NoError(t, err) + defer removeFile(t, file.Name()) + defer file.Close() + + _, err = file.WriteString("line one\nline two\nline three\n") + require.NoError(t, err) + + err = file.Close() + require.NoError(t, err) + + lines, err := readCustomConfigLines(file.Name()) + assert.NoError(t, err) + + expectedLines := []string{ + "line one", "line two", "line three", "", + } + assert.Equal(t, expectedLines, lines) +} + +func removeFile(t *testing.T, filename string) { + t.Helper() + err := os.RemoveAll(filename) + require.NoError(t, err) +} + +func Test_modifyCustomConfig(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + lines []string + settings configuration.OpenVPN + connection models.OpenVPNConnection + modified []string + }{ + "mixed": { + lines: []string{ + "up bla", + "proto tcp", + "remote 5.5.5.5", + "cipher bla", + "tun-ipv6", + "keep me here", + "auth bla", + }, + settings: configuration.OpenVPN{ + User: "user", + Cipher: "cipher", + Auth: "auth", + MSSFix: 1000, + ProcUser: "procuser", + }, + connection: models.OpenVPNConnection{ + IP: net.IPv4(1, 2, 3, 4), + Port: 1194, + Protocol: constants.UDP, + }, + modified: []string{ + "keep me here", + "proto udp", + "remote 1.2.3.4 1194", + "mute-replay-warnings", + "auth-nocache", + "pull-filter ignore \"auth-token\"", + "auth-retry nointeract", + "suppress-timestamps", + "auth-user-pass /etc/openvpn/auth.conf", + "verb 0", + "data-ciphers-fallback cipher", + "data-ciphers cipher", + "auth auth", + "mssfix 1000", + "pull-filter ignore \"route-ipv6\"", + "pull-filter ignore \"ifconfig-ipv6\"", + "user procuser", + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + modified := modifyCustomConfig(testCase.lines, + testCase.settings, testCase.connection) + + assert.Equal(t, testCase.modified, modified) + }) + } +} + +func Test_extractConnectionFromLines(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + lines []string + connection models.OpenVPNConnection + err error + }{ + "success": { + lines: []string{"bla bla", "proto tcp", "remote 1.2.3.4 1194 tcp"}, + connection: models.OpenVPNConnection{ + IP: net.IPv4(1, 2, 3, 4), + Port: 1194, + Protocol: constants.TCP, + }, + }, + "extraction error": { + lines: []string{"bla bla", "proto bad", "remote 1.2.3.4 1194 tcp"}, + err: errors.New("on line 2: failed extracting protocol from proto line: network protocol not supported: bad"), + }, + "only use first values found": { + lines: []string{"proto udp", "proto tcp", "remote 1.2.3.4 443 tcp", "remote 5.2.3.4 1194 udp"}, + connection: models.OpenVPNConnection{ + IP: net.IPv4(1, 2, 3, 4), + Port: 443, + Protocol: constants.UDP, + }, + }, + "no IP found": { + lines: []string{"proto tcp"}, + connection: models.OpenVPNConnection{ + Protocol: constants.TCP, + }, + err: errRemoteLineNotFound, + }, + "default TCP port": { + lines: []string{"remote 1.2.3.4", "proto tcp"}, + connection: models.OpenVPNConnection{ + IP: net.IPv4(1, 2, 3, 4), + Port: 443, + Protocol: constants.TCP, + }, + }, + "default UDP port": { + lines: []string{"remote 1.2.3.4", "proto udp"}, + connection: models.OpenVPNConnection{ + IP: net.IPv4(1, 2, 3, 4), + Port: 1194, + Protocol: constants.UDP, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + connection, err := extractConnectionFromLines(testCase.lines) + + if testCase.err != nil { + require.Error(t, err) + assert.Equal(t, testCase.err.Error(), err.Error()) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, testCase.connection, connection) + }) + } +} + +func Test_extractConnectionFromLine(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + line string + connection models.OpenVPNConnection + isErr error + }{ + "irrelevant line": { + line: "bla bla", + }, + "extract proto error": { + line: "proto bad", + isErr: errExtractProto, + }, + "extract proto success": { + line: "proto tcp", + connection: models.OpenVPNConnection{ + Protocol: constants.TCP, + }, + }, + "extract remote error": { + line: "remote bad", + isErr: errExtractRemote, + }, + "extract remote success": { + line: "remote 1.2.3.4 1194 udp", + connection: models.OpenVPNConnection{ + IP: net.IPv4(1, 2, 3, 4), + Port: 1194, + Protocol: constants.UDP, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + connection, err := extractConnectionFromLine(testCase.line) + + if testCase.isErr != nil { + assert.ErrorIs(t, err, testCase.isErr) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, testCase.connection, connection) + }) + } +} + +func Test_extractProto(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + line string + protocol string + err error + }{ + "fields error": { + line: "proto one two", + err: errors.New("proto line has not 2 fields as expected: proto one two"), + }, + "bad protocol": { + line: "proto bad", + err: errors.New("network protocol not supported: bad"), + }, + "udp": { + line: "proto udp", + protocol: constants.UDP, + }, + "tcp": { + line: "proto tcp", + protocol: constants.TCP, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + protocol, err := extractProto(testCase.line) + + if testCase.err != nil { + require.Error(t, err) + assert.Equal(t, testCase.err.Error(), err.Error()) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, testCase.protocol, protocol) + }) + } +} + +func Test_extractRemote(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + line string + ip net.IP + port uint16 + protocol string + err error + }{ + "not enough fields": { + line: "remote", + err: errors.New("remote line has not 2 fields as expected: remote"), + }, + "too many fields": { + line: "remote one two three four", + err: errors.New("remote line has not 2 fields as expected: remote one two three four"), + }, + "host is not an IP": { + line: "remote somehost.com", + err: errors.New("host is not an an IP address: somehost.com"), + }, + "only IP host": { + line: "remote 1.2.3.4", + ip: net.IPv4(1, 2, 3, 4), + }, + "port not an integer": { + line: "remote 1.2.3.4 bad", + err: errors.New("port is not valid: remote 1.2.3.4 bad"), + }, + "port is zero": { + line: "remote 1.2.3.4 0", + err: errors.New("port is not valid: not between 1 and 65535: 0"), + }, + "port is minus one": { + line: "remote 1.2.3.4 -1", + err: errors.New("port is not valid: not between 1 and 65535: -1"), + }, + "port is over 65535": { + line: "remote 1.2.3.4 65536", + err: errors.New("port is not valid: not between 1 and 65535: 65536"), + }, + "IP host and port": { + line: "remote 1.2.3.4 8000", + ip: net.IPv4(1, 2, 3, 4), + port: 8000, + }, + "invalid protocol": { + line: "remote 1.2.3.4 8000 bad", + err: errors.New("network protocol not supported: bad"), + }, + "IP host and port and protocol": { + line: "remote 1.2.3.4 8000 udp", + ip: net.IPv4(1, 2, 3, 4), + port: 8000, + protocol: constants.UDP, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + ip, port, protocol, err := extractRemote(testCase.line) + + if testCase.err != nil { + require.Error(t, err) + assert.Equal(t, testCase.err.Error(), err.Error()) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, testCase.ip, ip) + assert.Equal(t, testCase.port, port) + assert.Equal(t, testCase.protocol, protocol) + }) + } +} diff --git a/internal/openvpn/run.go b/internal/openvpn/run.go index 28d2d996..e3215f98 100644 --- a/internal/openvpn/run.go +++ b/internal/openvpn/run.go @@ -36,7 +36,7 @@ func (l *Loop) Run(ctx context.Context, done chan<- struct{}) { lines = providerConf.BuildConf(connection, openVPNSettings) } } else { - lines, connection, err = l.processCustomConfig(openVPNSettings) + lines, connection, err = processCustomConfig(openVPNSettings) } if err != nil { l.crashed(ctx, err)