transport/tcp: support for CIDR-mask based ACLs + client-identities

Co-authored-by: Christian Schwarz <me@cschwarz.com>

fixes #235
close #265

config/samples/source.yml

@@ -5,7 +5,11 @@ jobs:
     type: tcp
     listen: "0.0.0.0:8888"
     clients: {
-      "192.168.122.123" : "client1"
+      "192.168.122.123" :               "mysql01",
+      "192.168.122.42" :                "mx01",
+      "2001:0db8:85a3::8a2e:0370:7334": "gateway",
+      "10.23.42.0/24":       "cluster-*",
+      "fde4:8dba:82e1::/64": "san-*",
     }
   filesystems: {
     "<": true,

docs/configuration/transports.rst

@@ -52,8 +52,15 @@ Serve
         listen: ":8888"
         listen_freebind: true # optional, default false
         clients: {
-          "192.168.122.123" : "mysql01"
+          "192.168.122.123" :               "mysql01",
-          "192.168.122.123" : "mx01"
+          "192.168.122.42" :                "mx01",
+          "2001:0db8:85a3::8a2e:0370:7334": "gateway",
+
+          # CIDR masks require a '*' in the client identity string
+          # that is expanded to the client's IP address
+
+          "10.23.42.0/24":       "cluster-*"
+          "fde4:8dba:82e1::/64": "san-*"
         }
       ...
 

go.sum

@@ -263,6 +263,7 @@ github.com/spf13/viper v1.0.2/go.mod h1:A8kyI5cUJhb8N+3pkfONlcEcZbueH6nhAm0Fq7Sr
 github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=

transport/tcp/serve_tcp.go

@@ -11,39 +11,6 @@ import (
 	"github.com/zrepl/zrepl/util/tcpsock"
 )
 
-type ipMapEntry struct {
-	ip    net.IP
-	ident string
-}
-
-type ipMap struct {
-	entries []ipMapEntry
-}
-
-func ipMapFromConfig(clients map[string]string) (*ipMap, error) {
-	entries := make([]ipMapEntry, 0, len(clients))
-	for clientIPString, clientIdent := range clients {
-		clientIP := net.ParseIP(clientIPString)
-		if clientIP == nil {
-			return nil, errors.Errorf("cannot parse client IP %q", clientIPString)
-		}
-		if err := transport.ValidateClientIdentity(clientIdent); err != nil {
-			return nil, errors.Wrapf(err, "invalid client identity for IP %q", clientIPString)
-		}
-		entries = append(entries, ipMapEntry{clientIP, clientIdent})
-	}
-	return &ipMap{entries: entries}, nil
-}
-
-func (m *ipMap) Get(ip net.IP) (string, error) {
-	for _, e := range m.entries {
-		if e.ip.Equal(ip) {
-			return e.ident, nil
-		}
-	}
-	return "", errors.Errorf("no identity mapping for client IP %s", ip)
-}
-
 func TCPListenerFactoryFromConfig(c *config.Global, in *config.TCPServe) (transport.AuthenticatedListenerFactory, error) {
 	clientMap, err := ipMapFromConfig(in.Clients)
 	if err != nil {
@@ -69,10 +36,13 @@ func (f *TCPAuthListener) Accept(ctx context.Context) (*transport.AuthConn, erro
 	if err != nil {
 		return nil, err
 	}
-	clientIP := nc.RemoteAddr().(*net.TCPAddr).IP
+	clientAddr := &net.IPAddr{
-	clientIdent, err := f.clientMap.Get(clientIP)
+		IP:   nc.RemoteAddr().(*net.TCPAddr).IP,
+		Zone: nc.RemoteAddr().(*net.TCPAddr).Zone,
+	}
+	clientIdent, err := f.clientMap.Get(clientAddr)
 	if err != nil {
-		transport.GetLogger(ctx).WithField("ip", clientIP).Error("client IP not in client map")
+		transport.GetLogger(ctx).WithField("ipaddr", clientAddr).Error("client IP not in client map")
 		nc.Close()
 		return nil, err
 	}

transport/tcp/serve_tcp_ipmap.go

@@ -0,0 +1,166 @@
+package tcp
+
+import (
+	"bytes"
+	"fmt"
+	"net"
+	"sort"
+	"strings"
+
+	"github.com/pkg/errors"
+	"github.com/zrepl/zrepl/transport"
+	"golang.org/x/sys/unix"
+)
+
+type ipMapEntry struct {
+	subnet *net.IPNet
+	// ident is always not empty
+	ident string
+	// zone may be empty (e.g. for IPv4)
+	zone string
+}
+
+type ipMap struct {
+	entries []*ipMapEntry
+}
+
+func ipMapFromConfig(clients map[string]string) (*ipMap, error) {
+
+	entries := make([]*ipMapEntry, 0, len(clients))
+	for clientInput, clientIdent := range clients {
+		userIPMapEntry, err := newIPMapEntry(clientInput, clientIdent)
+		if err != nil {
+			return nil, errors.Wrapf(err, "cannot not parse %q:%q", clientInput, clientIdent)
+		}
+
+		entries = append(entries, userIPMapEntry)
+	}
+
+	sort.Sort(byPrefixlen(entries))
+
+	return &ipMap{entries: entries}, nil
+}
+
+func (m *ipMap) Get(ipAddr *net.IPAddr) (string, error) {
+	for _, e := range m.entries {
+		if e.zone != ipAddr.Zone {
+			continue
+		}
+		if e.subnet.Contains(ipAddr.IP) {
+			return zfsDatasetPathComponentCompatibleRepresentation(e.ident, ipAddr), nil
+		}
+	}
+	return "", errors.Errorf("no identity mapping for client IP: %s%%%s", ipAddr.IP, ipAddr.Zone)
+}
+
+var ipv6FullySpecifiedMask = bytes.Repeat([]byte{0xff}, net.IPv6len)
+
+func newIPMapEntry(input string, ident string) (*ipMapEntry, error) {
+	ip := input
+	var zone string
+
+	ipZoneSplit := strings.SplitN(input, "%", 2)
+	if len(ipZoneSplit) > 1 {
+		ip = ipZoneSplit[0]
+		zone = ipZoneSplit[1]
+	}
+
+	_, subnet, err := net.ParseCIDR(ip)
+	if err != nil {
+		// expect full IP, no '*' placeholder expansion
+
+		if strings.Count(ident, "*") != 0 {
+			return nil, fmt.Errorf("non-CIDR matches must not contain '*' placeholder")
+		}
+
+		if err := transport.ValidateClientIdentity(ident); err != nil {
+			return nil, errors.Wrapf(err, "invalid client identity %q for IP %q", ident, ip)
+		}
+
+		parsedIP := net.ParseIP(ip)
+		if parsedIP == nil {
+			return nil, errors.Wrapf(err, "invalid client address %q", ip)
+		}
+		parsedIP = parsedIP.To16()
+
+		return &ipMapEntry{
+			subnet: &net.IPNet{
+				IP:   parsedIP,
+				Mask: ipv6FullySpecifiedMask,
+			},
+			zone:  zone,
+			ident: ident,
+		}, nil
+	}
+
+	// expect CIDR and '*' placeholder expansion
+
+	if strings.Count(ident, "*") != 1 {
+		err = fmt.Errorf("CIDRs require 1 IP placeholder")
+		return nil, errors.Wrapf(err, "invalid client identity %q for IP %q", ident, ip)
+	}
+
+	longestIPAddr := net.IPAddr{
+		IP:   net.IP(bytes.Repeat([]byte{0xff}, net.IPv6len)),
+		Zone: strings.Repeat("i", unix.IFNAMSIZ),
+	}
+	longestIdent := zfsDatasetPathComponentCompatibleRepresentation(ident, &longestIPAddr)
+
+	if err := transport.ValidateClientIdentity(longestIdent); err != nil {
+		return nil, errors.Wrapf(err, "invalid client identity for IP %q", ip)
+	}
+
+	ones, _ := subnet.Mask.Size()
+	preExpansionAddrlen := len(subnet.IP) * 8
+	expanded := subnet.IP.To16()
+	postExpansionAddrlen := len(expanded) * 8
+	return &ipMapEntry{
+		subnet: &net.IPNet{
+			IP:   expanded,
+			Mask: net.CIDRMask(postExpansionAddrlen-preExpansionAddrlen+ones, postExpansionAddrlen),
+		},
+		zone:  zone,
+		ident: ident,
+	}, nil
+}
+
+func zfsDatasetPathComponentCompatibleRepresentation(identityWithWildcard string, addr *net.IPAddr) string {
+	// If a Zone exists we append it after the IP using a "-" because "%"
+	// is a zfs dataset forbidden char.
+	if addr.Zone != "" {
+		return strings.Replace(identityWithWildcard, "*", addr.IP.String()+"-"+addr.Zone, 1)
+	}
+	// newIPMapEntry validates that the line contains exactly one "*"
+	return strings.Replace(identityWithWildcard, "*", addr.IP.String(), 1)
+}
+
+func (e *ipMapEntry) String() string {
+	return fmt.Sprintf("&ipMapEntry{subnet=%q, ident=%q, zone=%q}", e.subnet.String(), e.ident, e.zone)
+}
+
+func (e *ipMapEntry) PrefixLen() int {
+	ones, bits := e.subnet.Mask.Size()
+	if bits != net.IPv6len*8 {
+		panic(fmt.Sprintf("impl error: we represent all addresses as 16byte internally ones=%v bits=%v: %s", ones, bits, e))
+	}
+	return ones
+}
+
+// newtype to support sorting by prefixlength
+type byPrefixlen []*ipMapEntry
+
+func (m byPrefixlen) Len() int { return len(m) }
+func (m byPrefixlen) Less(i, j int) bool {
+
+	if m[i].PrefixLen() != m[j].PrefixLen() {
+		return m[i].PrefixLen() > m[j].PrefixLen()
+	}
+
+	addrCmp := bytes.Compare(m[i].subnet.IP.To16(), m[j].subnet.IP.To16())
+	if addrCmp != 0 {
+		return addrCmp < 0
+	}
+
+	return strings.Compare(m[i].zone, m[j].zone) < 0
+}
+func (m byPrefixlen) Swap(i, j int) { m[i], m[j] = m[j], m[i] }

transport/tcp/serve_tcp_ipmap_test.go

@@ -0,0 +1,204 @@
+package tcp
+
+import (
+	"net"
+	"os"
+	"testing"
+
+	"github.com/kr/pretty"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIPMap(t *testing.T) {
+	type testCaseExpect struct {
+		expectNoMapping bool
+		expectIdent     string
+	}
+	type testCase struct {
+		name          string
+		config        map[string]string
+		expectInitErr bool
+		expect        map[string]testCaseExpect
+	}
+
+	cases := []testCase{
+		{
+			name:          "regular ips",
+			expectInitErr: false,
+			config: map[string]string{
+				"192.168.123.234": "ident1",
+				"192.168.20.23":   "ident2",
+			},
+			expect: map[string]testCaseExpect{
+				"192.168.123.234": {expectIdent: "ident1"},
+				"192.168.20.23":   {expectIdent: "ident2"},
+				"192.168.20.10":   {expectNoMapping: true},
+			},
+		},
+		{
+			name:          "full wildcard",
+			expectInitErr: false,
+			config: map[string]string{
+				"0.0.0.0/0": "*",
+				"0::/0":     "*",
+			},
+			expect: map[string]testCaseExpect{
+				"10.123.234.24": {expectIdent: "10.123.234.24"},
+				// '[' and ']' are forbiddenin dataset names
+				"fe80::23:42": {expectIdent: "fe80::23:42"},
+			},
+		},
+		{
+			name:          "longest prefix matching",
+			expectInitErr: false,
+			config: map[string]string{
+				"10.1.2.3":              "specific-host",
+				"10.1.2.0/24":           "subnet-one-two-*",
+				"10.1.1.0/24":           "subnet-one-one-*",
+				"10.1.0.0/24":           "subnet-one-zero-*",
+				"10.1.0.0/16":           "subnet-one-*",
+				"fde4:8dba:82e1:1::1":   "v6-48-1-specialhost",
+				"fde4:8dba:82e1::/48":   "v6-48-*",
+				"fde4:8dba:82e1:1::/64": "v6-64-1-*",
+				"fde4:8dba:82e1:2::/64": "v6-64-2-*",
+			},
+			expect: map[string]testCaseExpect{
+				"10.1.2.3":             {expectIdent: "specific-host"},
+				"10.1.2.1":             {expectIdent: "subnet-one-two-10.1.2.1"},
+				"10.1.1.1":             {expectIdent: "subnet-one-one-10.1.1.1"},
+				"10.1.0.23":            {expectIdent: "subnet-one-zero-10.1.0.23"},
+				"10.1.3.1":             {expectIdent: "subnet-one-10.1.3.1"},
+				"10.2.1.1":             {expectNoMapping: true},
+				"fde4:8dba:82e1:1::1":  {expectIdent: "v6-48-1-specialhost"},
+				"fde4:8dba:82e1:23::1": {expectIdent: "v6-48-fde4:8dba:82e1:23::1"},
+				"fde4:8dba:82e1:1::2":  {expectIdent: "v6-64-1-fde4:8dba:82e1:1::2"},
+				"fde4:8dba:82e1:2::1":  {expectIdent: "v6-64-2-fde4:8dba:82e1:2::1"},
+				"fde4:8dba:82e2::1":    {expectNoMapping: true},
+			},
+		},
+		{
+			name:          "different prefixes, mixed ipv4 ipv6, with interface ids",
+			expectInitErr: false,
+			config: map[string]string{
+				"192.168.23.0/24": "db-*",
+				"192.168.23.23":   "db-twentythree",
+				"192.168.42.0/24": "web-*",
+				"10.1.4.0/24":     "my-*-server",
+				"2001:0db8:85a3:0000:0000:8a2e:0370:7334":      "aspecifichost",
+				"2001:0db8:85a3:0000:0000:8a2e:0370:7334%eth1": "aspecifichost",
+				"fe80::/16%eth1":      "san-*",
+				"fde4:8dba:82e1::/64": "sub64-*",
+			},
+			expect: map[string]testCaseExpect{
+				"10.1.2.3":        {expectNoMapping: true},
+				"192.168.23.1":    {expectIdent: "db-192.168.23.1"},
+				"192.168.23.23":   {expectIdent: "db-twentythree"},
+				"192.168.023.001": {expectIdent: "db-192.168.23.1"},
+				"10.1.4.5":        {expectIdent: "my-10.1.4.5-server"},
+
+				// normalization
+				"192.168.42.1":    {expectIdent: "web-192.168.42.1"},
+				"192.168.042.001": {expectIdent: "web-192.168.42.1"},
+				// v6 matching
+				"fe80::23:42%eth1": {expectIdent: "san-fe80::23:42-eth1"},
+				"fe80::23:42%eth2": {expectNoMapping: true},
+
+				// v6 subnet matching
+				"fde4:8dba:82e1::1": {expectIdent: "sub64-fde4:8dba:82e1::1"},
+				// v6 subnet matching with suffix that matches another allowed IPv4
+				"fde4:8dba:82e1::c0a8:1717": {expectIdent: "sub64-fde4:8dba:82e1::c0a8:1717"},
+
+				"2001:0db8:85a3:0000:0000:8a2e:0370:7334":      {expectIdent: "aspecifichost"},
+				"2001:0db8:85a3:0000:0000:8a2e:0370:7334%eth1": {expectIdent: "aspecifichost"},
+				"2001:0db8:85a3:0000:0000:8a2e:0370:7334%eth2": {expectNoMapping: true},
+			},
+		},
+		{
+			name:          "invalid user input: non ip or cidr",
+			expectInitErr: true,
+			config: map[string]string{
+				"jimmy": "db",
+			},
+		},
+		{
+			name:          "invalid user input: v4 ip with an identity containing *",
+			expectInitErr: true,
+			config: map[string]string{
+				"192.168.1.2": "db-*",
+			},
+		},
+		{
+			name:          "invalid user input: v4 subnet without an identity containing *",
+			expectInitErr: true,
+			config: map[string]string{
+				"192.168.1.0/24": "db-",
+			},
+		},
+		{
+			name:          "invalid user input: v6 ip with an identity containing *",
+			expectInitErr: true,
+			config: map[string]string{
+				"2001:0db8:85a3:0000:0000:8a2e:0370:7334": "aspecifichost*",
+			},
+		},
+		{
+			name:          "invalid user input: v6 subnet without identity containing *",
+			expectInitErr: true,
+			config: map[string]string{
+				"fe80::/16%eth1": "db-",
+			},
+		},
+		{
+			name:          "invalid user input with subnet match: client identity with forbidden zfs dataset name char @",
+			expectInitErr: true,
+			config: map[string]string{
+				"fe80::/16": "db@-*",
+			},
+		},
+		{
+			name:          "invalid user input with IP match: client identity with forbidden zfs dataset name char @",
+			expectInitErr: true,
+			config: map[string]string{
+				"fe80::1": "db@foo",
+			},
+		},
+	}
+
+	for i := range cases {
+		c := cases[i]
+		t.Run(c.name, func(t *testing.T) {
+			pretty.Fprintf(os.Stderr, "running %#v\n", c)
+			m, err := ipMapFromConfig(c.config)
+			if c.expectInitErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, m)
+			}
+			for input, expect := range c.expect {
+				// reuse newIPMapEntry to parse test case input
+				// "test" is not used during testing but must not be empty.
+				ipMapEntry, _ := newIPMapEntry(input, "test")
+				ones, bits := ipMapEntry.subnet.Mask.Size()
+				require.Equal(t, bits, net.IPv6len*8, "and we know ipMapEntry always expands its IPs to 16bytes")
+				require.Equal(t, ones, net.IPv6len*8, "test case addresses must be fully specified")
+				require.NotNil(t, ipMapEntry)
+				ident, err := m.Get(&net.IPAddr{
+					IP:   ipMapEntry.subnet.IP,
+					Zone: ipMapEntry.zone,
+				})
+				if expect.expectNoMapping {
+					assert.Empty(t, ident)
+				} else {
+					assert.NoError(t, err)
+					assert.Equal(t, expect.expectIdent, ident)
+				}
+			}
+		})
+	}
+}
+
+func TestPackageNetAssumptions(t *testing.T) {
+
+}

transport/transport.go

@@ -4,10 +4,10 @@ package transport
 
 import (
 	"context"
-	"errors"
 	"net"
 	"syscall"
 
+	"github.com/pkg/errors"
 	"github.com/zrepl/zrepl/logger"
 	"github.com/zrepl/zrepl/rpc/dataconn/timeoutconn"
 	"github.com/zrepl/zrepl/zfs"
@@ -55,13 +55,10 @@ type Connecter interface {
 }
 
 // A client identity must be a single component in a ZFS filesystem path
-func ValidateClientIdentity(in string) (err error) {
+func ValidateClientIdentity(in string) error {
-	path, err := zfs.NewDatasetPath(in)
+	err := zfs.ComponentNamecheck(in)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "client identity must be usable as a single dataset path component")
-	}
-	if path.Length() != 1 {
-		return errors.New("client identity must be a single path component (not empty, no '/')")
 	}
 	return nil
 }