From 7815fbe17a68eb033267bc18aad348b638a8ffde Mon Sep 17 00:00:00 2001 From: Sonia Appasamy Date: Wed, 9 Aug 2023 10:06:58 -0400 Subject: [PATCH] tailscale/cli: add interactive flow for enabling Funnel Updates tailscale/corp#10577 Signed-off-by: Sonia Appasamy --- client/tailscale/localclient.go | 20 +-- cmd/tailscale/cli/funnel.go | 49 +++++++- cmd/tailscale/cli/serve.go | 72 ++++++++++- cmd/tailscale/cli/serve_test.go | 116 +++++++++++++++++- ipn/serve.go | 18 ++- ipn/serve_test.go | 15 ++- tailcfg/tailcfg.go | 22 ++-- tsnet/tsnet.go | 4 + tstest/integration/testcontrol/testcontrol.go | 1 + 9 files changed, 275 insertions(+), 42 deletions(-) diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index 79650440c..fba916804 100644 --- a/client/tailscale/localclient.go +++ b/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 -// feature, such as Funnel, for the node's tailnet. +// QueryFeature makes a request for instructions on how to enable +// 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 -// of the node, but rather returns information that can be presented -// to the acting user about where/how to enable the feature. +// If you are looking to use QueryFeature, you'll likely want to +// use cli.enableFeatureInteractive instead, which handles the logic +// 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 -// 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". +// 2023-08-09: 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) diff --git a/cmd/tailscale/cli/funnel.go b/cmd/tailscale/cli/funnel.go index f8c15e19d..b27390c1c 100644 --- a/cmd/tailscale/cli/funnel.go +++ b/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) { diff --git a/cmd/tailscale/cli/serve.go b/cmd/tailscale/cli/serve.go index 7242d2e7b..4688012dd 100644 --- a/cmd/tailscale/cli/serve.go +++ b/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 + } + } + } +} diff --git a/cmd/tailscale/cli/serve_test.go b/cmd/tailscale/cli/serve_test.go index 398ff7fae..88e7aeb87 100644 --- a/cmd/tailscale/cli/serve_test.go +++ b/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 - setCount int // counts calls to SetServeConfig + config *ipn.ServeConfig + 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 } diff --git a/ipn/serve.go b/ipn/serve.go index 48e3343a1..efff23cea 100644 --- a/ipn/serve.go +++ b/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 -// 2. HTTPS is enabled on the Tailnet -// 3. the node has the "funnel" nodeAttr -// 4. the port is allowed for Funnel +// 1. HTTPS is enabled on the Tailnet +// 2. the node has the "funnel" nodeAttr +// 3. 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) { - return errors.New("Funnel not enabled; See https://tailscale.com/s/no-funnel.") - } - if slices.Contains(nodeAttrs, tailcfg.CapabilityWarnFunnelNoHTTPS) { + if !slices.Contains(nodeAttrs, tailcfg.CapabilityHTTPS) { 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) diff --git a/ipn/serve_test.go b/ipn/serve_test.go index d08aba047..cf3b14768 100644 --- a/ipn/serve_test.go +++ b/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.CapabilityWarnFunnelNoHTTPS}, true}, - {443, []string{portAttr, tailcfg.NodeAttrFunnel}, false}, - {8443, []string{portAttr, tailcfg.NodeAttrFunnel}, false}, - {8321, []string{portAttr, tailcfg.NodeAttrFunnel}, true}, - {8083, []string{portAttr, tailcfg.NodeAttrFunnel}, false}, - {8091, []string{portAttr, tailcfg.NodeAttrFunnel}, true}, - {3000, []string{portAttr, tailcfg.NodeAttrFunnel}, true}, + {443, []string{portAttr, tailcfg.NodeAttrFunnel}, true}, + {443, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, false}, + {8443, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, false}, + {8321, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, true}, + {8083, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, false}, + {8091, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, true}, + {3000, []string{portAttr, tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel}, true}, } for _, tt := range tests { err := CheckFunnelAccess(tt.port, tt.caps) diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 334aa3101..2f53be39b 100644 --- a/tailcfg/tailcfg.go +++ b/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 - // the feature. The CLI can watch for changes to the presence, - // of this capability, and once included, can proceed with - // using the feature. + // ShouldWait specifies whether the CLI should block and + // wait for the user to enable the feature. // - // If WaitOn is empty, the user does not have an action that - // the CLI should block on. - WaitOn string `json:",omitempty"` + // If this is true, the enablement from the control server + // is expected to be a quick and uninterrupted process for + // 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= diff --git a/tsnet/tsnet.go b/tsnet/tsnet.go index 6fed40e34..444990f27 100644 --- a/tsnet/tsnet.go +++ b/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 } diff --git a/tstest/integration/testcontrol/testcontrol.go b/tstest/integration/testcontrol/testcontrol.go index 654eeb8d5..89a66ef7d 100644 --- a/tstest/integration/testcontrol/testcontrol.go +++ b/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", },