From 6c30840cac13f184474654c90b7b0cc314069b25 Mon Sep 17 00:00:00 2001
From: Irbe Krumina <irbe@tailscale.com>
Date: Sun, 19 Jan 2025 19:00:21 +0000
Subject: [PATCH] ipn: [serve] warn that foreground funnel won't work if
 shields are up (#14685)

We throw error early with a warning if users attempt to enable background funnel
for a node that does not allow incoming connections
(shields up), but if it done in foreground mode, we just silently fail
(the funnel command succeeds, but the connections are not allowed).
This change makes sure that we also error early in foreground mode.

Updates tailscale/tailscale#11049

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
---
 ipn/serve.go      | 20 ++++++-----
 ipn/serve_test.go | 85 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/ipn/serve.go b/ipn/serve.go
index b7effa874..176c6d984 100644
--- a/ipn/serve.go
+++ b/ipn/serve.go
@@ -63,12 +63,12 @@ type ServeConfig struct {
 	// traffic is allowed, from trusted ingress peers.
 	AllowFunnel map[HostPort]bool `json:",omitempty"`
 
-	// Foreground is a map of an IPN Bus session ID to an alternate foreground
-	// serve config that's valid for the life of that WatchIPNBus session ID.
-	// This. This allows the config to specify ephemeral configs that are
-	// used in the CLI's foreground mode to ensure ungraceful shutdowns
-	// of either the client or the LocalBackend does not expose ports
-	// that users are not aware of.
+	// Foreground is a map of an IPN Bus session ID to an alternate foreground serve config that's valid for the
+	// life of that WatchIPNBus session ID. This allows the config to specify ephemeral configs that are used
+	// in the CLI's foreground mode to ensure ungraceful shutdowns of either the client or the LocalBackend does not
+	// expose ports that users are not aware of. In practice this contains any serve config set via 'tailscale
+	// serve' command run without the '--bg' flag. ServeConfig contained by Foreground is not expected itself to contain
+	// another Foreground block.
 	Foreground map[string]*ServeConfig `json:",omitempty"`
 
 	// ETag is the checksum of the serve config that's populated
@@ -389,8 +389,7 @@ func (sc *ServeConfig) RemoveTCPForwarding(port uint16) {
 // View version of ServeConfig.IsFunnelOn.
 func (v ServeConfigView) IsFunnelOn() bool { return v.ж.IsFunnelOn() }
 
-// IsFunnelOn reports whether if ServeConfig is currently allowing funnel
-// traffic for any host:port.
+// IsFunnelOn reports whether any funnel endpoint is currently enabled for this node.
 func (sc *ServeConfig) IsFunnelOn() bool {
 	if sc == nil {
 		return false
@@ -400,6 +399,11 @@ func (sc *ServeConfig) IsFunnelOn() bool {
 			return true
 		}
 	}
+	for _, conf := range sc.Foreground {
+		if conf.IsFunnelOn() {
+			return true
+		}
+	}
 	return false
 }
 
diff --git a/ipn/serve_test.go b/ipn/serve_test.go
index e9d8e8f32..ae1d56eef 100644
--- a/ipn/serve_test.go
+++ b/ipn/serve_test.go
@@ -182,3 +182,88 @@ func TestExpandProxyTargetDev(t *testing.T) {
 		})
 	}
 }
+
+func TestIsFunnelOn(t *testing.T) {
+	tests := []struct {
+		name string
+		sc   *ServeConfig
+		want bool
+	}{
+		{
+			name: "nil_config",
+		},
+		{
+			name: "empty_config",
+			sc:   &ServeConfig{},
+		},
+		{
+			name: "funnel_enabled_in_background",
+			sc: &ServeConfig{
+				AllowFunnel: map[HostPort]bool{
+					"tailnet.xyz:443": true,
+				},
+			},
+			want: true,
+		},
+		{
+			name: "funnel_disabled_in_background",
+			sc: &ServeConfig{
+				AllowFunnel: map[HostPort]bool{
+					"tailnet.xyz:443": false,
+				},
+			},
+		},
+		{
+			name: "funnel_enabled_in_foreground",
+			sc: &ServeConfig{
+				Foreground: map[string]*ServeConfig{
+					"abc123": {
+						AllowFunnel: map[HostPort]bool{
+							"tailnet.xyz:443": true,
+						},
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "funnel_disabled_in_both",
+			sc: &ServeConfig{
+				AllowFunnel: map[HostPort]bool{
+					"tailnet.xyz:443": false,
+				},
+				Foreground: map[string]*ServeConfig{
+					"abc123": {
+						AllowFunnel: map[HostPort]bool{
+							"tailnet.xyz:8443": false,
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "funnel_enabled_in_both",
+			sc: &ServeConfig{
+				AllowFunnel: map[HostPort]bool{
+					"tailnet.xyz:443": true,
+				},
+				Foreground: map[string]*ServeConfig{
+					"abc123": {
+						AllowFunnel: map[HostPort]bool{
+							"tailnet.xyz:8443": true,
+						},
+					},
+				},
+			},
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.sc.IsFunnelOn(); got != tt.want {
+				t.Errorf("ServeConfig.IsFunnelOn() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}