From 2a40a734aa30c732d8ee6b4bcf59b11680824024 Mon Sep 17 00:00:00 2001
From: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Date: Fri, 8 Oct 2021 21:02:29 -0400
Subject: [PATCH] Update preferred endpoint preference order (#1583)

Rather than pick addresses totally at random, this changes the
preference to be private IP4 > public IP4 > DNS. (Ordering is not
guaranteed, but in many cases there will only be one of each type of IP
anyways.) All three of these are preferred over IP6, which also goes
public > private.

This seems likely to match the common case where you're getting into a
machine on the private side from the public side. It's harder to know if
DNS is private or not, so IPs are preferred in this case.
---
 internal/libs/endpoint/preferencer.go      | 53 +++++++++++++++--
 internal/libs/endpoint/preferencer_test.go | 68 +++++++++++++++++-----
 2 files changed, 102 insertions(+), 19 deletions(-)

diff --git a/internal/libs/endpoint/preferencer.go b/internal/libs/endpoint/preferencer.go
index 44619fd80..ab838855a 100644
--- a/internal/libs/endpoint/preferencer.go
+++ b/internal/libs/endpoint/preferencer.go
@@ -2,8 +2,7 @@ package endpoint
 
 import (
 	"context"
-	"math/rand"
-	"time"
+	"net"
 
 	"github.com/hashicorp/boundary/internal/errors"
 )
@@ -47,10 +46,52 @@ func (p *preferencer) Choose(ctx context.Context, opt ...Option) (string, error)
 
 	switch len(p.matchers) {
 	case 0:
-		// We have no matchers, so pick one at random
-		allAddrs := append(opts.withIpAddrs, opts.withDnsNames...)
-		rng := rand.New(rand.NewSource(time.Now().UnixNano()))
-		return allAddrs[rng.Intn(len(allAddrs))], nil
+		// We have no matchers, so pick a private address if we have one (since
+		// those are most likely to be what the user is trying to gain access to
+		// via a worker). If we don't have one, pick any IP address, as that's
+		// also more likely desired than a DNS name. Otherwise, pick a DNS name.
+		// IPv6 follows same rules as 4, but least preferenced.
+		nonPrivateIp4s := make([]string, 0, len(opts.withIpAddrs))
+		nonPrivateIp6s := make([]string, 0, len(opts.withIpAddrs))
+		privateIp6s := make([]string, 0, len(opts.withIpAddrs))
+
+		for _, ipStr := range opts.withIpAddrs {
+			ipVal := net.ParseIP(ipStr)
+			if ipVal != nil {
+				switch ipVal.To4() {
+				case nil: // it's v6
+					if ipVal.IsPrivate() {
+						privateIp6s = append(privateIp6s, ipStr)
+					} else {
+						nonPrivateIp6s = append(nonPrivateIp6s, ipStr)
+					}
+				default:
+					if ipVal.IsPrivate() {
+						// private IPv4 is most highly preferenced, so return it directly
+						return ipStr, nil
+					}
+					nonPrivateIp4s = append(nonPrivateIp4s, ipStr)
+				}
+			}
+		}
+
+		switch {
+		case len(nonPrivateIp4s) > 0:
+			return nonPrivateIp4s[0], nil
+
+		case len(opts.withDnsNames) > 0:
+			return opts.withDnsNames[0], nil
+
+		case len(privateIp6s) > 0:
+			return privateIp6s[0], nil
+
+		case len(nonPrivateIp6s) > 0:
+			return nonPrivateIp6s[0], nil
+
+		default:
+			// We literally have nothing if we get here, so return nothing
+			return "", nil
+		}
 
 	default:
 		for _, m := range p.matchers {
diff --git a/internal/libs/endpoint/preferencer_test.go b/internal/libs/endpoint/preferencer_test.go
index 917a75aa8..0fd904a60 100644
--- a/internal/libs/endpoint/preferencer_test.go
+++ b/internal/libs/endpoint/preferencer_test.go
@@ -91,20 +91,62 @@ func TestPreferencer(t *testing.T) {
 			})
 		}
 	})
-	t.Run("noPrefRandom", func(t *testing.T) {
+	t.Run("noPrefReturnsPrivate", func(t *testing.T) {
+		const privAddr = "192.168.4.3"
 		p, err := NewPreferencer(ctx)
 		require.NoError(t, err)
-		checkMap := map[string]int{}
-		for i := 0; i < 200; i++ {
-			out, err := p.Choose(
-				ctx,
-				WithIpAddrs([]string{"48.134.5.1", "1.2.7.56"}),
-				WithDnsNames([]string{"foo.bar.com", "bar.baz.com"}),
-			)
-			require.NoError(t, err)
-			checkMap[out] = checkMap[out] + 1
-		}
-		// Ensure that we've inserted all four keys
-		assert.Len(t, checkMap, 4)
+		out, err := p.Choose(
+			ctx,
+			WithIpAddrs([]string{"48.134.5.1", "2001::1", privAddr}),
+			WithDnsNames([]string{"foo.bar.com", "bar.baz.com"}),
+		)
+		require.NoError(t, err)
+		assert.Equal(t, privAddr, out)
+	})
+	t.Run("noPrefNoPrivateReturnsIp4", func(t *testing.T) {
+		const exp = "48.134.5.1"
+		p, err := NewPreferencer(ctx)
+		require.NoError(t, err)
+		out, err := p.Choose(
+			ctx,
+			WithIpAddrs([]string{"2001::1", exp}),
+			WithDnsNames([]string{"foo.bar.com", "bar.baz.com"}),
+		)
+		require.NoError(t, err)
+		assert.Equal(t, exp, out)
+	})
+	t.Run("noPrefNoIp4ReturnsDns", func(t *testing.T) {
+		const exp = "foo.bar.com"
+		p, err := NewPreferencer(ctx)
+		require.NoError(t, err)
+		out, err := p.Choose(
+			ctx,
+			WithIpAddrs([]string{"2001::1"}),
+			WithDnsNames([]string{exp}),
+		)
+		require.NoError(t, err)
+		assert.Equal(t, exp, out)
+	})
+	t.Run("ip6OnlyReturnsPrivate", func(t *testing.T) {
+		const exp = "fc00::1"
+		p, err := NewPreferencer(ctx)
+		require.NoError(t, err)
+		out, err := p.Choose(
+			ctx,
+			WithIpAddrs([]string{"2001::1", exp}),
+		)
+		require.NoError(t, err)
+		assert.Equal(t, exp, out)
+	})
+	t.Run("ip6OnlyPublic", func(t *testing.T) {
+		const exp = "2001::1"
+		p, err := NewPreferencer(ctx)
+		require.NoError(t, err)
+		out, err := p.Choose(
+			ctx,
+			WithIpAddrs([]string{exp}),
+		)
+		require.NoError(t, err)
+		assert.Equal(t, exp, out)
 	})
 }