tailscale/cli: add interactive flow for enabling Funnel

Updates tailscale/corp#10577

Signed-off-by: Sonia Appasamy <sonia@tailscale.com>

client/tailscale/localclient.go

@@ -1144,18 +1144,18 @@ func (lc *LocalClient) DeleteProfile(ctx context.Context, profile ipn.ProfileID)
 	return err
 }
 
-// QueryFeature makes a request for instructions on how to enable a
+// QueryFeature makes a request for instructions on how to enable
-// feature, such as Funnel, for the node's tailnet.
+// a feature, such as Funnel, for the node's tailnet. If relevant,
+// this includes a control server URL the user can visit to enable
+// the feature.
 //
-// This request itself does not directly enable the feature on behalf
+// If you are looking to use QueryFeature, you'll likely want to
-// of the node, but rather returns information that can be presented
+// use cli.enableFeatureInteractive instead, which handles the logic
-// to the acting user about where/how to enable the feature.
+// of wraping QueryFeature and translating its response into an
+// interactive flow for the user, including using the IPN notify bus
+// to block until the feature has been enabled.
 //
-// If relevant, this includes a control URL the user can visit to
+// 2023-08-09: Valid feature values are "serve" and "funnel".
-// explicitly consent to using the feature. LocalClient.WatchIPNBus
-// can be used to block on the feature being enabled.
-//
-// 2023-08-02: Valid feature values are "serve" and "funnel".
 func (lc *LocalClient) QueryFeature(ctx context.Context, feature string) (*tailcfg.QueryFeatureResponse, error) {
 	v := url.Values{"feature": {feature}}
 	body, err := lc.send(ctx, "POST", "/localapi/v0/query-feature?"+v.Encode(), 200, nil)

cmd/tailscale/cli/funnel.go

@@ -13,7 +13,10 @@ import (
 	"strings"
 
 	"github.com/peterbourgon/ff/v3/ffcli"
+	"golang.org/x/exp/slices"
 	"tailscale.com/ipn"
+	"tailscale.com/ipn/ipnstate"
+	"tailscale.com/tailcfg"
 	"tailscale.com/util/mak"
 )
 
@@ -91,9 +94,10 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 	}
 	port := uint16(port64)
 
-	if err := ipn.CheckFunnelAccess(port, st.Self.Capabilities); err != nil {
+	if err := e.verifyFunnelEnabled(ctx, st, port); err != nil {
 		return err
 	}
+
 	dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
 	hp := ipn.HostPort(dnsName + ":" + strconv.Itoa(int(port)))
 	if on == sc.AllowFunnel[hp] {
@@ -117,6 +121,49 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 	return nil
 }
 
+// verifyFunnelEnabled verifies that the self node is allowed to use Funnel.
+//
+// If Funnel is not yet enabled by the current node capabilities,
+// the user is sent through an interactive flow to enable the feature.
+// Once enabled, verifyFunnelEnabled checks that the given port is allowed
+// with Funnel.
+//
+// If an error is reported, the CLI should stop execution and return the error.
+//
+// verifyFunnelEnabled may refresh the local state and modify the st input.
+func (e *serveEnv) verifyFunnelEnabled(ctx context.Context, st *ipnstate.Status, port uint16) error {
+	hasFunnelAttrs := func(attrs []string) bool {
+		hasHTTPS := slices.Contains(attrs, tailcfg.CapabilityHTTPS)
+		hasFunnel := slices.Contains(attrs, tailcfg.NodeAttrFunnel)
+		return hasHTTPS && hasFunnel
+	}
+	if hasFunnelAttrs(st.Self.Capabilities) {
+		return nil // already enabled
+	}
+	enableErr := e.enableFeatureInteractive(ctx, "funnel", hasFunnelAttrs)
+	st, statusErr := e.getLocalClientStatus(ctx) // get updated status; interactive flow may block
+	switch {
+	case statusErr != nil:
+		return fmt.Errorf("getting client status: %w", statusErr)
+	case enableErr != nil:
+		// enableFeatureInteractive is a new flow behind a control server
+		// feature flag. If anything caused it to error, fallback to using
+		// the old CheckFunnelAccess call. Likely this domain does not have
+		// the feature flag on.
+		// TODO(sonia,tailscale/corp#10577): Remove this fallback once the
+		// control flag is turned on for all domains.
+		if err := ipn.CheckFunnelAccess(port, st.Self.Capabilities); err != nil {
+			return err
+		}
+	default:
+		// Done with enablement, make sure the requested port is allowed.
+		if err := ipn.CheckFunnelPort(port, st.Self.Capabilities); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // printFunnelWarning prints a warning if the Funnel is on but there is no serve
 // config for its host:port.
 func printFunnelWarning(sc *ipn.ServeConfig) {

cmd/tailscale/cli/serve.go

@@ -10,6 +10,7 @@ import (
 	"flag"
 	"fmt"
 	"io"
+	"log"
 	"net"
 	"net/url"
 	"os"
@@ -22,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/peterbourgon/ff/v3/ffcli"
+	"tailscale.com/client/tailscale"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
@@ -129,7 +131,8 @@ type localServeClient interface {
 	Status(context.Context) (*ipnstate.Status, error)
 	GetServeConfig(context.Context) (*ipn.ServeConfig, error)
 	SetServeConfig(context.Context, *ipn.ServeConfig) error
-	QueryFeature(context.Context, string) (*tailcfg.QueryFeatureResponse, error)
+	QueryFeature(ctx context.Context, feature string) (*tailcfg.QueryFeatureResponse, error)
+	WatchIPNBus(ctx context.Context, mask ipn.NotifyWatchOpt) (*tailscale.IPNBusWatcher, error)
 }
 
 // serveEnv is the environment the serve command runs within. All I/O should be
@@ -766,3 +769,70 @@ func parseServePort(s string) (uint16, error) {
 	}
 	return uint16(p), nil
 }
+
+// enableFeatureInteractive sends the node's user through an interactive
+// flow to enable a feature, such as Funnel, on their tailnet.
+//
+// hasRequiredCapabilities should be provided as a function that checks
+// whether a slice of node capabilities encloses the necessary values
+// needed to use the feature.
+//
+// If err is returned empty, the feature has been successfully enabled.
+//
+// If err is returned non-empty, the client failed to query the control
+// server for information about how to enable the feature.
+//
+// If the feature cannot be enabled, enableFeatureInteractive terminates
+// the CLI process.
+//
+// 2023-08-09: The only valid feature values are "serve" and "funnel".
+// This can be moved to some CLI lib when expanded past serve/funnel.
+func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string, hasRequiredCapabilities func(caps []string) bool) (err error) {
+	info, err := e.lc.QueryFeature(ctx, feature)
+	if err != nil {
+		return err
+	}
+	if info.Complete {
+		return nil // already enabled
+	}
+	if info.Text != "" {
+		fmt.Fprintln(os.Stdout, info.Text)
+	}
+	if info.URL != "" {
+		fmt.Fprintln(os.Stdout, "\n         "+info.URL)
+	}
+	if !info.ShouldWait {
+		// The feature has not been enabled yet,
+		// but the CLI should not block on user action.
+		// Once info.Text is printed, exit the CLI.
+		os.Exit(0)
+	}
+	// Block until feature is enabled.
+	watchCtx, cancelWatch := context.WithCancel(ctx)
+	defer cancelWatch()
+	watcher, err := e.lc.WatchIPNBus(watchCtx, 0)
+	if err != nil {
+		// If we fail to connect to the IPN notification bus,
+		// don't block. We still present the URL in the CLI,
+		// then close the process. Swallow the error.
+		log.Fatalf("lost connection to tailscaled: %v", err)
+		return err
+	}
+	defer watcher.Close()
+	for {
+		n, err := watcher.Next()
+		if err != nil {
+			// Stop blocking if we error.
+			// Let the user finish enablement then rerun their
+			// command themselves.
+			log.Fatalf("lost connection to tailscaled: %v", err)
+			return err
+		}
+		if nm := n.NetMap; nm != nil && nm.SelfNode != nil {
+			if hasRequiredCapabilities(nm.SelfNode.Capabilities) {
+				fmt.Fprintln(os.Stdout, "\nSuccess.")
+				return nil
+			}
+		}
+	}
+}

cmd/tailscale/cli/serve_test.go

@@ -6,6 +6,7 @@ package cli
 import (
 	"bytes"
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"os"
@@ -16,6 +17,7 @@ import (
 	"testing"
 
 	"github.com/peterbourgon/ff/v3/ffcli"
+	"tailscale.com/client/tailscale"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
@@ -745,14 +747,105 @@ func TestServeConfigMutations(t *testing.T) {
 	}
 }
 
+func TestVerifyFunnelEnabled(t *testing.T) {
+	lc := &fakeLocalServeClient{}
+	var stdout bytes.Buffer
+	var flagOut bytes.Buffer
+	e := &serveEnv{
+		lc:          lc,
+		testFlagOut: &flagOut,
+		testStdout:  &stdout,
+	}
+
+	tests := []struct {
+		name string
+		// queryFeatureResponse is the mock response desired from the
+		// call made to lc.QueryFeature by verifyFunnelEnabled.
+		queryFeatureResponse mockQueryFeatureResponse
+		caps                 []string // optionally set at fakeStatus.Capabilities
+		wantErr              string
+		wantPanic            string
+	}{
+		{
+			name:                 "enabled",
+			queryFeatureResponse: mockQueryFeatureResponse{resp: &tailcfg.QueryFeatureResponse{Complete: true}, err: nil},
+			wantErr:              "", // no error, success
+		},
+		{
+			name:                 "fallback-to-non-interactive-flow",
+			queryFeatureResponse: mockQueryFeatureResponse{resp: nil, err: errors.New("not-allowed")},
+			wantErr:              "Funnel not available; HTTPS must be enabled. See https://tailscale.com/s/https.",
+		},
+		{
+			name:                 "fallback-flow-missing-acl-rule",
+			queryFeatureResponse: mockQueryFeatureResponse{resp: nil, err: errors.New("not-allowed")},
+			caps:                 []string{tailcfg.CapabilityHTTPS},
+			wantErr:              `Funnel not available; "funnel" node attribute not set. See https://tailscale.com/s/no-funnel.`,
+		},
+		{
+			name:                 "fallback-flow-enabled",
+			queryFeatureResponse: mockQueryFeatureResponse{resp: nil, err: errors.New("not-allowed")},
+			caps:                 []string{tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel},
+			wantErr:              "", // no error, success
+		},
+		{
+			name: "not-allowed-to-enable",
+			queryFeatureResponse: mockQueryFeatureResponse{resp: &tailcfg.QueryFeatureResponse{
+				Complete:   false,
+				Text:       "You don't have permission to enable this feature.",
+				ShouldWait: false,
+			}, err: nil},
+			wantErr:   "",
+			wantPanic: "unexpected call to os.Exit(0) during test", // os.Exit(0) should be called to end process
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			lc.setQueryFeatureResponse(tt.queryFeatureResponse)
+
+			if tt.caps != nil {
+				oldCaps := fakeStatus.Self.Capabilities
+				defer func() { fakeStatus.Self.Capabilities = oldCaps }() // reset after test
+				fakeStatus.Self.Capabilities = tt.caps
+			}
+			st, err := e.getLocalClientStatus(ctx)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			defer func() {
+				r := recover()
+				var gotPanic string
+				if r != nil {
+					gotPanic = fmt.Sprint(r)
+				}
+				if gotPanic != tt.wantPanic {
+					t.Errorf("wrong panic; got=%s, want=%s", gotPanic, tt.wantPanic)
+				}
+			}()
+			gotErr := e.verifyFunnelEnabled(ctx, st, 443)
+			var got string
+			if gotErr != nil {
+				got = gotErr.Error()
+			}
+			if got != tt.wantErr {
+				t.Errorf("wrong error; got=%s, want=%s", gotErr, tt.wantErr)
+			}
+		})
+	}
+}
+
 // fakeLocalServeClient is a fake tailscale.LocalClient for tests.
 // It's not a full implementation, just enough to test the serve command.
 //
 // The fake client is stateful, and is used to test manipulating
 // ServeConfig state. This implementation cannot be used concurrently.
 type fakeLocalServeClient struct {
-	config   *ipn.ServeConfig
+	config               *ipn.ServeConfig
-	setCount int // counts calls to SetServeConfig
+	setCount             int                       // counts calls to SetServeConfig
+	queryFeatureResponse *mockQueryFeatureResponse // mock response to QueryFeature calls
 }
 
 // fakeStatus is a fake ipnstate.Status value for tests.
@@ -782,7 +875,24 @@ func (lc *fakeLocalServeClient) SetServeConfig(ctx context.Context, config *ipn.
 	return nil
 }
 
-func (lc *fakeLocalServeClient) QueryFeature(context.Context, string) (*tailcfg.QueryFeatureResponse, error) {
+type mockQueryFeatureResponse struct {
+	resp *tailcfg.QueryFeatureResponse
+	err  error
+}
+
+func (lc *fakeLocalServeClient) setQueryFeatureResponse(resp mockQueryFeatureResponse) {
+	lc.queryFeatureResponse = &resp
+}
+
+func (lc *fakeLocalServeClient) QueryFeature(ctx context.Context, feature string) (*tailcfg.QueryFeatureResponse, error) {
+	if resp := lc.queryFeatureResponse; resp != nil {
+		// If we're testing QueryFeature, use the response value set for the test.
+		return resp.resp, resp.err
+	}
+	return &tailcfg.QueryFeatureResponse{Complete: true}, nil // fallback to already enabled
+}
+
+func (lc *fakeLocalServeClient) WatchIPNBus(ctx context.Context, mask ipn.NotifyWatchOpt) (*tailscale.IPNBusWatcher, error) {
 	return nil, nil
 }
 

ipn/serve.go

@@ -204,31 +204,27 @@ func (sc *ServeConfig) IsFunnelOn() bool {
 // CheckFunnelAccess checks whether Funnel access is allowed for the given node
 // and port.
 // It checks:
-//  1. Funnel is enabled on the Tailnet
+//  1. HTTPS is enabled on the Tailnet
-//  2. HTTPS is enabled on the Tailnet
+//  2. the node has the "funnel" nodeAttr
-//  3. the node has the "funnel" nodeAttr
+//  3. the port is allowed for Funnel
-//  4. the port is allowed for Funnel
 //
 // The nodeAttrs arg should be the node's Self.Capabilities which should contain
 // the attribute we're checking for and possibly warning-capabilities for
 // Funnel.
 func CheckFunnelAccess(port uint16, nodeAttrs []string) error {
-	if slices.Contains(nodeAttrs, tailcfg.CapabilityWarnFunnelNoInvite) {
+	if !slices.Contains(nodeAttrs, tailcfg.CapabilityHTTPS) {
-		return errors.New("Funnel not enabled; See https://tailscale.com/s/no-funnel.")
-	}
-	if slices.Contains(nodeAttrs, tailcfg.CapabilityWarnFunnelNoHTTPS) {
 		return errors.New("Funnel not available; HTTPS must be enabled. See https://tailscale.com/s/https.")
 	}
 	if !slices.Contains(nodeAttrs, tailcfg.NodeAttrFunnel) {
 		return errors.New("Funnel not available; \"funnel\" node attribute not set. See https://tailscale.com/s/no-funnel.")
 	}
-	return checkFunnelPort(port, nodeAttrs)
+	return CheckFunnelPort(port, nodeAttrs)
 }
 
-// checkFunnelPort checks whether the given port is allowed for Funnel.
+// CheckFunnelPort checks whether the given port is allowed for Funnel.
 // It uses the tailcfg.CapabilityFunnelPorts nodeAttr to determine the allowed
 // ports.
-func checkFunnelPort(wantedPort uint16, nodeAttrs []string) error {
+func CheckFunnelPort(wantedPort uint16, nodeAttrs []string) error {
 	deny := func(allowedPorts string) error {
 		if allowedPorts == "" {
 			return fmt.Errorf("port %d is not allowed for funnel", wantedPort)

ipn/serve_test.go

@@ -16,14 +16,13 @@ func TestCheckFunnelAccess(t *testing.T) {
 		wantErr bool
 	}{
 		{443, []string{portAttr}, true}, // No "funnel" attribute
-		{443, []string{portAttr, tailcfg.CapabilityWarnFunnelNoInvite}, true},
+		{443, []string{portAttr, tailcfg.NodeAttrFunnel}, true},
-		{443, []string{portAttr, tailcfg.CapabilityWarnFunnelNoHTTPS}, true},
+		{443, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, false},
-		{443, []string{portAttr, tailcfg.NodeAttrFunnel}, false},
+		{8443, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, false},
-		{8443, []string{portAttr, tailcfg.NodeAttrFunnel}, false},
+		{8321, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, true},
-		{8321, []string{portAttr, tailcfg.NodeAttrFunnel}, true},
+		{8083, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, false},
-		{8083, []string{portAttr, tailcfg.NodeAttrFunnel}, false},
+		{8091, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, true},
-		{8091, []string{portAttr, tailcfg.NodeAttrFunnel}, true},
+		{3000, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, true},
-		{3000, []string{portAttr, tailcfg.NodeAttrFunnel}, true},
 	}
 	for _, tt := range tests {
 		err := CheckFunnelAccess(tt.port, tt.caps)

tailcfg/tailcfg.go

@@ -1964,10 +1964,11 @@ const (
 	// Funnel warning capabilities used for reporting errors to the user.
 
 	// CapabilityWarnFunnelNoInvite indicates whether Funnel is enabled for the tailnet.
-	// NOTE: In transition from Alpha to Beta, this capability is being reused as the enablement.
+	// This cap is no longer used 2023-08-09 onwards.
 	CapabilityWarnFunnelNoInvite = "https://tailscale.com/cap/warn-funnel-no-invite"
 
 	// CapabilityWarnFunnelNoHTTPS indicates HTTPS has not been enabled for the tailnet.
+	// This cap is no longer used 2023-08-09 onwards.
 	CapabilityWarnFunnelNoHTTPS = "https://tailscale.com/cap/warn-funnel-no-https"
 
 	// Debug logging capabilities
@@ -2270,6 +2271,7 @@ type QueryFeatureRequest struct {
 }
 
 // QueryFeatureResponse is the response to an QueryFeatureRequest.
+// See cli.enableFeatureInteractive for usage.
 type QueryFeatureResponse struct {
 	// Complete is true when the feature is already enabled.
 	Complete bool `json:",omitempty"`
@@ -2287,14 +2289,18 @@ type QueryFeatureResponse struct {
 	// When empty, there is no action for this user to take.
 	URL string `json:",omitempty"`
 
-	// WaitOn specifies the self node capability required to use
+	// ShouldWait specifies whether the CLI should block and
-	// the feature. The CLI can watch for changes to the presence,
+	// wait for the user to enable the feature.
-	// of this capability, and once included, can proceed with
-	// using the feature.
 	//
-	// If WaitOn is empty, the user does not have an action that
+	// If this is true, the enablement from the control server
-	// the CLI should block on.
+	// is expected to be a quick and uninterrupted process for
-	WaitOn string `json:",omitempty"`
+	// the user, and blocking allows them to immediately start
+	// using the feature once enabled without rerunning the
+	// command (e.g. no need to re-run "funnel on").
+	//
+	// The CLI can watch the IPN notification bus for changes in
+	// required node capabilities to know when to continue.
+	ShouldWait bool `json:",omitempty"`
 }
 
 // OverTLSPublicKeyResponse is the JSON response to /key?v=<n>

tsnet/tsnet.go

@@ -928,6 +928,10 @@ func (s *Server) ListenFunnel(network, addr string, opts ...FunnelOption) (net.L
 	if err != nil {
 		return nil, err
 	}
+	// TODO(sonia,tailscale/corp#10577): We may want to use the interactive enable
+	// flow here instead of CheckFunnelAccess to allow the user to turn on Funnel
+	// if not already on. Specifically when running from a terminal.
+	// See cli.serveEnv.verifyFunnelEnabled.
 	if err := ipn.CheckFunnelAccess(uint16(port), st.Self.Capabilities); err != nil {
 		return nil, err
 	}

tstest/integration/testcontrol/testcontrol.go

@@ -556,6 +556,7 @@ func (s *Server) serveRegister(w http.ResponseWriter, r *http.Request, mkey key.
 		Hostinfo:          req.Hostinfo.View(),
 		Name:              req.Hostinfo.Hostname,
 		Capabilities: []string{
+			tailcfg.CapabilityHTTPS,
 			tailcfg.NodeAttrFunnel,
 			tailcfg.CapabilityFunnelPorts + "?ports=8080,443",
 		},