fix issue #2499 - nil dereference while using capi whitelists (#2501)

pkg/csconfig/api.go

@@ -3,7 +3,9 @@ package csconfig
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"errors"
 	"fmt"
+	"io"
 	"net"
 	"os"
 	"strings"
@@ -331,43 +333,59 @@ type capiWhitelists struct {
 	Cidrs []string `yaml:"cidrs"`
 }
 
-func (s *LocalApiServerCfg) LoadCapiWhitelists() error {
-	if s.CapiWhitelistsPath == "" {
-		return nil
-	}
-	if _, err := os.Stat(s.CapiWhitelistsPath); os.IsNotExist(err) {
-		return fmt.Errorf("capi whitelist file '%s' does not exist", s.CapiWhitelistsPath)
-	}
-	fd, err := os.Open(s.CapiWhitelistsPath)
-	if err != nil {
-		return fmt.Errorf("unable to open capi whitelist file '%s': %s", s.CapiWhitelistsPath, err)
-	}
-
-	var fromCfg capiWhitelists
+func parseCapiWhitelists(fd io.Reader) (*CapiWhitelist, error) {
+	fromCfg := capiWhitelists{}
 
-	defer fd.Close()
 	decoder := yaml.NewDecoder(fd)
 	if err := decoder.Decode(&fromCfg); err != nil {
-		return fmt.Errorf("while parsing capi whitelist file '%s': %s", s.CapiWhitelistsPath, err)
+		if errors.Is(err, io.EOF) {
+			return nil, fmt.Errorf("empty file")
+		}
+		return nil, err
 	}
-	s.CapiWhitelists = &CapiWhitelist{
+	ret := &CapiWhitelist{
 		Ips:   make([]net.IP, len(fromCfg.Ips)),
 		Cidrs: make([]*net.IPNet, len(fromCfg.Cidrs)),
 	}
-	for _, v := range fromCfg.Ips {
+	for idx, v := range fromCfg.Ips {
 		ip := net.ParseIP(v)
 		if ip == nil {
-			return fmt.Errorf("unable to parse ip whitelist '%s'", v)
+			return nil, fmt.Errorf("invalid IP address: %s", v)
 		}
-		s.CapiWhitelists.Ips = append(s.CapiWhitelists.Ips, ip)
+		ret.Ips[idx] = ip
 	}
-	for _, v := range fromCfg.Cidrs {
+	for idx, v := range fromCfg.Cidrs {
 		_, tnet, err := net.ParseCIDR(v)
 		if err != nil {
-			return fmt.Errorf("unable to parse cidr whitelist '%s' : %v", v, err)
+			return nil, err
 		}
-		s.CapiWhitelists.Cidrs = append(s.CapiWhitelists.Cidrs, tnet)
+		ret.Cidrs[idx] = tnet
 	}
+
+	return ret, nil
+}
+
+func (s *LocalApiServerCfg) LoadCapiWhitelists() error {
+	if s.CapiWhitelistsPath == "" {
+		return nil
+	}
+
+	if _, err := os.Stat(s.CapiWhitelistsPath); os.IsNotExist(err) {
+		return fmt.Errorf("capi whitelist file '%s' does not exist", s.CapiWhitelistsPath)
+	}
+
+	fd, err := os.Open(s.CapiWhitelistsPath)
+	if err != nil {
+		return fmt.Errorf("while opening capi whitelist file: %s", err)
+	}
+
+	defer fd.Close()
+
+	s.CapiWhitelists, err = parseCapiWhitelists(fd)
+	if err != nil {
+		return fmt.Errorf("while parsing capi whitelist file '%s': %w", s.CapiWhitelistsPath, err)
+	}
+
 	return nil
 }
 

pkg/csconfig/api_test.go

@@ -2,6 +2,7 @@ package csconfig
 
 import (
 	"fmt"
+	"net"
 	"os"
 	"path/filepath"
 	"strings"
@@ -234,7 +235,7 @@ func TestLoadAPIServer(t *testing.T) {
 				DisableAPI: false,
 			},
 			expected: &LocalApiServerCfg{
-				Enable:    ptr.Of(true),
+				Enable:       ptr.Of(true),
 				PapiLogLevel: &logLevel,
 			},
 			expectedErr: "no database configuration provided",
@@ -259,3 +260,67 @@ func TestLoadAPIServer(t *testing.T) {
 		}
 	}
 }
+
+func mustParseCIDRNet(s string) *net.IPNet {
+	_, ipNet, err := net.ParseCIDR(s)
+	if err != nil {
+		panic(fmt.Sprintf("MustParseCIDR: parsing %q: %v", s, err))
+	}
+	return ipNet
+}
+
+func TestParseCapiWhitelists(t *testing.T) {
+	tests := []struct {
+		name        string
+		input       string
+		expected    *CapiWhitelist
+		expectedErr string
+	}{
+		{
+			name:  "empty file",
+			input: "",
+			expected: &CapiWhitelist{
+				Ips:   []net.IP{},
+				Cidrs: []*net.IPNet{},
+			},
+			expectedErr: "empty file",
+		},
+		{
+			name:  "empty ip and cidr",
+			input: `{"ips": [], "cidrs": []}`,
+			expected: &CapiWhitelist{
+				Ips:   []net.IP{},
+				Cidrs: []*net.IPNet{},
+			},
+		},
+		{
+			name:  "some ip",
+			input: `{"ips": ["1.2.3.4"]}`,
+			expected: &CapiWhitelist{
+				Ips:   []net.IP{net.IPv4(1, 2, 3, 4)},
+				Cidrs: []*net.IPNet{},
+			},
+		},
+		{
+			name:  "some cidr",
+			input: `{"cidrs": ["1.2.3.0/24"]}`,
+			expected: &CapiWhitelist{
+				Ips:   []net.IP{},
+				Cidrs: []*net.IPNet{mustParseCIDRNet("1.2.3.0/24")},
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			wl, err := parseCapiWhitelists(strings.NewReader(tc.input))
+			cstest.RequireErrorContains(t, err, tc.expectedErr)
+			if tc.expectedErr != "" {
+				return
+			}
+
+			assert.Equal(t, tc.expected, wl)
+		})
+	}
+}

test/bats/13_capi_whitelists.bats

@@ -0,0 +1,76 @@
+#!/usr/bin/env bats
+# vim: ft=bats:list:ts=8:sts=4:sw=4:et:ai:si:
+
+set -u
+
+setup_file() {
+    load "../lib/setup_file.sh"
+    CONFIG_DIR=$(dirname "$CONFIG_YAML")
+    CAPI_WHITELISTS_YAML="$CONFIG_DIR/capi-whitelists.yaml"
+    export CAPI_WHITELISTS_YAML
+}
+
+teardown_file() {
+    load "../lib/teardown_file.sh"
+}
+
+setup() {
+    load "../lib/setup.sh"
+    load "../lib/bats-file/load.bash"
+    ./instance-data load
+    config_set '.api.server.capi_whitelists_path=strenv(CAPI_WHITELISTS_YAML)'
+}
+
+teardown() {
+    ./instance-crowdsec stop
+}
+
+#----------
+
+@test "capi_whitelists: file missing" {
+    rune -1 timeout 1s "${CROWDSEC}"
+    assert_stderr --partial "capi whitelist file '$CAPI_WHITELISTS_YAML' does not exist"
+}
+
+@test "capi_whitelists: error on open" {
+    echo > "$CAPI_WHITELISTS_YAML"
+    chmod 000 "$CAPI_WHITELISTS_YAML"
+    rune -1 timeout 1s "${CROWDSEC}"
+    assert_stderr --partial "while opening capi whitelist file: open $CAPI_WHITELISTS_YAML: permission denied"
+}
+
+@test "capi_whitelists: empty file" {
+    echo > "$CAPI_WHITELISTS_YAML"
+    rune -1 timeout 1s "${CROWDSEC}"
+    assert_stderr --partial "while parsing capi whitelist file '$CAPI_WHITELISTS_YAML': empty file"
+}
+
+@test "capi_whitelists: empty lists" {
+    echo '{"ips": [], "cidrs": []}' > "$CAPI_WHITELISTS_YAML"
+    rune -124 timeout 1s "${CROWDSEC}"
+}
+
+@test "capi_whitelists: bad ip" {
+    echo '{"ips": ["blahblah"], "cidrs": []}' > "$CAPI_WHITELISTS_YAML"
+    rune -1 timeout 1s "${CROWDSEC}"
+    assert_stderr --partial "while parsing capi whitelist file '$CAPI_WHITELISTS_YAML': invalid IP address: blahblah"
+}
+
+@test "capi_whitelists: bad cidr" {
+    echo '{"ips": [], "cidrs": ["blahblah"]}' > "$CAPI_WHITELISTS_YAML"
+    rune -1 timeout 1s "${CROWDSEC}"
+    assert_stderr --partial "while parsing capi whitelist file '$CAPI_WHITELISTS_YAML': invalid CIDR address: blahblah"
+}
+
+@test "capi_whitelists: file with ip and cidr values" {
+    cat <<-EOT > "$CAPI_WHITELISTS_YAML"
+	ips:
+	- 1.2.3.4
+	- 2.3.4.5
+	cidrs:
+	- 1.2.3.0/24
+	EOT
+
+    config_set '.common.log_level="trace"'
+    rune -0 ./instance-crowdsec start
+}