From 12d468532873ca7a8a88e43c45e5ebe538ee170a Mon Sep 17 00:00:00 2001 From: Marwan Sulaiman Date: Mon, 11 Sep 2023 19:33:20 -0400 Subject: [PATCH] ipn/localapi, ipn/ipnlocal: add etag support for SetServeConfig This PR adds optimistic concurrency control in the local client and api in order to ensure multiple writes of the ServeConfig do not conflict with each other. Updates #9273 Signed-off-by: Marwan Sulaiman --- ipn/ipnlocal/serve.go | 40 ++++++++-- ipn/ipnlocal/serve_test.go | 149 +++++++++++++++++++++++++++++-------- ipn/localapi/localapi.go | 21 +++++- 3 files changed, 168 insertions(+), 42 deletions(-) diff --git a/ipn/ipnlocal/serve.go b/ipn/ipnlocal/serve.go index 38c47867a..b15e90601 100644 --- a/ipn/ipnlocal/serve.go +++ b/ipn/ipnlocal/serve.go @@ -5,7 +5,9 @@ package ipnlocal import ( "context" + "crypto/sha256" "crypto/tls" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -33,6 +35,11 @@ import ( "tailscale.com/version" ) +// ErrETagMismatch signals that the given +// If-Match header does not match with the +// current etag of a resource. +var ErrETagMismatch = errors.New("etag mismatch") + // serveHTTPContextKey is the context.Value key for a *serveHTTPContext. type serveHTTPContextKey struct{} @@ -214,13 +221,15 @@ func (b *LocalBackend) updateServeTCPPortNetMapAddrListenersLocked(ports []uint1 } // SetServeConfig establishes or replaces the current serve config. -func (b *LocalBackend) SetServeConfig(config *ipn.ServeConfig) error { +// ETag is an optional parameter to enforce Optimistic Concurrency Control. +// If it is an empty string, then the config will be overwritten. +func (b *LocalBackend) SetServeConfig(config *ipn.ServeConfig, etag string) error { b.mu.Lock() defer b.mu.Unlock() - return b.setServeConfigLocked(config) + return b.setServeConfigLocked(config, etag) } -func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig) error { +func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig, etag string) error { prefs := b.pm.CurrentPrefs() if config.IsFunnelOn() && prefs.ShieldsUp() { return errors.New("Unable to turn on Funnel while shields-up is enabled") @@ -233,8 +242,24 @@ func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig) error { if !nm.SelfNode.Valid() { return errors.New("netMap SelfNode is nil") } - profileID := b.pm.CurrentProfile().ID - confKey := ipn.ServeConfigKey(profileID) + + // If etag is present, check that it has + // not changed from the last config. + if etag != "" { + // Note that we marshal b.serveConfig + // and not use b.lastServeConfJSON as that might + // be a Go nil value, which produces a different + // checksum from a JSON "null" value. + previousCfg, err := json.Marshal(b.serveConfig) + if err != nil { + return fmt.Errorf("error encoding previous config: %w", err) + } + sum := sha256.Sum256(previousCfg) + previousEtag := hex.EncodeToString(sum[:]) + if etag != previousEtag { + return ErrETagMismatch + } + } var bs []byte if config != nil { @@ -244,6 +269,9 @@ func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig) error { } bs = j } + + profileID := b.pm.CurrentProfile().ID + confKey := ipn.ServeConfigKey(profileID) if err := b.store.WriteState(confKey, bs); err != nil { return fmt.Errorf("writing ServeConfig to StateStore: %w", err) } @@ -271,7 +299,7 @@ func (b *LocalBackend) DeleteForegroundSession(sessionID string) error { } sc := b.serveConfig.AsStruct() delete(sc.Foreground, sessionID) - return b.setServeConfigLocked(sc) + return b.setServeConfigLocked(sc, "") } func (b *LocalBackend) HandleIngressTCPConn(ingressPeer tailcfg.NodeView, target ipn.HostPort, srcAddr netip.AddrPort, getConnOrReset func() (net.Conn, bool), sendRST func()) { diff --git a/ipn/ipnlocal/serve_test.go b/ipn/ipnlocal/serve_test.go index f36ab955a..4fad5bb42 100644 --- a/ipn/ipnlocal/serve_test.go +++ b/ipn/ipnlocal/serve_test.go @@ -6,7 +6,11 @@ package ipnlocal import ( "bytes" "context" + "crypto/sha256" "crypto/tls" + "encoding/hex" + "encoding/json" + "errors" "fmt" "net/http" "net/http/httptest" @@ -24,6 +28,7 @@ import ( "tailscale.com/types/logid" "tailscale.com/types/netmap" "tailscale.com/util/cmpx" + "tailscale.com/util/mak" "tailscale.com/util/must" "tailscale.com/wgengine" ) @@ -169,50 +174,82 @@ func TestGetServeHandler(t *testing.T) { } } -func TestServeHTTPProxy(t *testing.T) { - sys := &tsd.System{} - e, err := wgengine.NewUserspaceEngine(t.Logf, wgengine.Config{SetSubsystem: sys.Set}) +func getEtag(t *testing.T, b any) string { + t.Helper() + bts, err := json.Marshal(b) if err != nil { t.Fatal(err) } - sys.Set(e) - sys.Set(new(mem.Store)) - b, err := NewLocalBackend(t.Logf, logid.PublicID{}, sys, 0) + sum := sha256.Sum256(bts) + return hex.EncodeToString(sum[:]) +} + +func TestServeConfigETag(t *testing.T) { + b := newTestBackend(t) + + // a nil config with initial etag should succeed + err := b.SetServeConfig(nil, getEtag(t, nil)) if err != nil { t.Fatal(err) } - defer b.Shutdown() - dir := t.TempDir() - b.SetVarRoot(dir) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf)) - pm.currentProfile = &ipn.LoginProfile{ID: "id0"} - b.pm = pm + // a nil config with an invalid etag should fail + err = b.SetServeConfig(nil, "abc") + if !errors.Is(err, ErrETagMismatch) { + t.Fatal("expected an error but got nil") + } - b.netMap = &netmap.NetworkMap{ - SelfNode: (&tailcfg.Node{ - Name: "example.ts.net", - }).View(), - UserProfiles: map[tailcfg.UserID]tailcfg.UserProfile{ - tailcfg.UserID(1): { - LoginName: "someone@example.com", - DisplayName: "Some One", - ProfilePicURL: "https://example.com/photo.jpg", - }, + // a new config with no etag should succeed + conf := &ipn.ServeConfig{ + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + "example.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ + "/": {Proxy: "http://127.0.0.1:3000"}, + }}, }, } - b.nodeByAddr = map[netip.Addr]tailcfg.NodeView{ - netip.MustParseAddr("100.150.151.152"): (&tailcfg.Node{ - ComputedName: "some-peer", - User: tailcfg.UserID(1), - }).View(), - netip.MustParseAddr("100.150.151.153"): (&tailcfg.Node{ - ComputedName: "some-tagged-peer", - Tags: []string{"tag:server", "tag:test"}, - User: tailcfg.UserID(1), - }).View(), + err = b.SetServeConfig(conf, getEtag(t, nil)) + if err != nil { + t.Fatal(err) + } + + confView := b.ServeConfig() + etag := getEtag(t, confView) + if etag == "" { + t.Fatal("expected to get an etag but got an empty string") + } + conf = confView.AsStruct() + mak.Set(&conf.AllowFunnel, "example.ts.net:443", true) + + // replacing an existing config with an invalid etag should fail + err = b.SetServeConfig(conf, "invalid etag") + if !errors.Is(err, ErrETagMismatch) { + t.Fatalf("expected an etag mismatch error but got %v", err) + } + + // replacing an existing config with a valid etag should succeed + err = b.SetServeConfig(conf, etag) + if err != nil { + t.Fatal(err) + } + + // replacing an existing config with a previous etag should fail + err = b.SetServeConfig(nil, etag) + if !errors.Is(err, ErrETagMismatch) { + t.Fatalf("expected an etag mismatch error but got %v", err) } + // replacing an existing config with the new etag should succeed + newCfg := b.ServeConfig() + etag = getEtag(t, newCfg) + err = b.SetServeConfig(nil, etag) + if err != nil { + t.Fatal(err) + } +} + +func TestServeHTTPProxy(t *testing.T) { + b := newTestBackend(t) + // Start test serve endpoint. testServ := httptest.NewServer(http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { @@ -232,7 +269,7 @@ func TestServeHTTPProxy(t *testing.T) { }}, }, } - if err := b.SetServeConfig(conf); err != nil { + if err := b.SetServeConfig(conf, ""); err != nil { t.Fatal(err) } @@ -309,6 +346,52 @@ func TestServeHTTPProxy(t *testing.T) { } } +func newTestBackend(t *testing.T) *LocalBackend { + sys := &tsd.System{} + e, err := wgengine.NewUserspaceEngine(t.Logf, wgengine.Config{SetSubsystem: sys.Set}) + if err != nil { + t.Fatal(err) + } + sys.Set(e) + sys.Set(new(mem.Store)) + b, err := NewLocalBackend(t.Logf, logid.PublicID{}, sys, 0) + if err != nil { + t.Fatal(err) + } + t.Cleanup(b.Shutdown) + dir := t.TempDir() + b.SetVarRoot(dir) + + pm := must.Get(newProfileManager(new(mem.Store), t.Logf)) + pm.currentProfile = &ipn.LoginProfile{ID: "id0"} + b.pm = pm + + b.netMap = &netmap.NetworkMap{ + SelfNode: (&tailcfg.Node{ + Name: "example.ts.net", + }).View(), + UserProfiles: map[tailcfg.UserID]tailcfg.UserProfile{ + tailcfg.UserID(1): { + LoginName: "someone@example.com", + DisplayName: "Some One", + ProfilePicURL: "https://example.com/photo.jpg", + }, + }, + } + b.nodeByAddr = map[netip.Addr]tailcfg.NodeView{ + netip.MustParseAddr("100.150.151.152"): (&tailcfg.Node{ + ComputedName: "some-peer", + User: tailcfg.UserID(1), + }).View(), + netip.MustParseAddr("100.150.151.153"): (&tailcfg.Node{ + ComputedName: "some-tagged-peer", + Tags: []string{"tag:server", "tag:test"}, + User: tailcfg.UserID(1), + }).View(), + } + return b +} + func TestServeFileOrDirectory(t *testing.T) { td := t.TempDir() writeFile := func(suffix, contents string) { diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 82bb28797..d0e4bc761 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -7,6 +7,8 @@ package localapi import ( "bytes" "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -830,9 +832,17 @@ func (h *Handler) serveServeConfig(w http.ResponseWriter, r *http.Request) { http.Error(w, "serve config denied", http.StatusForbidden) return } - w.Header().Set("Content-Type", "application/json") config := h.b.ServeConfig() - json.NewEncoder(w).Encode(config) + bts, err := json.Marshal(config) + if err != nil { + http.Error(w, "error encoding config: "+err.Error(), http.StatusInternalServerError) + return + } + sum := sha256.Sum256(bts) + etag := hex.EncodeToString(sum[:]) + w.Header().Set("Etag", etag) + w.Header().Set("Content-Type", "application/json") + w.Write(bts) case "POST": if !h.PermitWrite { http.Error(w, "serve config denied", http.StatusForbidden) @@ -843,7 +853,12 @@ func (h *Handler) serveServeConfig(w http.ResponseWriter, r *http.Request) { writeErrorJSON(w, fmt.Errorf("decoding config: %w", err)) return } - if err := h.b.SetServeConfig(configIn); err != nil { + etag := r.Header.Get("If-Match") + if err := h.b.SetServeConfig(configIn, etag); err != nil { + if errors.Is(err, ipnlocal.ErrETagMismatch) { + http.Error(w, err.Error(), http.StatusPreconditionFailed) + return + } writeErrorJSON(w, fmt.Errorf("updating config: %w", err)) return }