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 <marwan@tailscale.com>

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()) {

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) {

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
 		}