diff --git a/cmd/derper/derper.go b/cmd/derper/derper.go index 3c6fda68c..840de3fba 100644 --- a/cmd/derper/derper.go +++ b/cmd/derper/derper.go @@ -96,9 +96,6 @@ var ( var ( tlsRequestVersion = &metrics.LabelMap{Label: "version"} tlsActiveVersion = &metrics.LabelMap{Label: "version"} - - // Exactly 64 hexadecimal lowercase digits. - validMeshKey = regexp.MustCompile(`^[0-9a-f]{64}$`) ) const setecMeshKeyName = "meshkey" @@ -159,14 +156,6 @@ func writeNewConfig() config { return cfg } -func checkMeshKey(key string) (string, error) { - key = strings.TrimSpace(key) - if !validMeshKey.MatchString(key) { - return "", errors.New("key must contain exactly 64 hex digits") - } - return key, nil -} - func main() { flag.Parse() if *versionFlag { @@ -246,10 +235,9 @@ func main() { log.Printf("No mesh key configured for --dev mode") } else if meshKey == "" { log.Printf("No mesh key configured") - } else if key, err := checkMeshKey(meshKey); err != nil { + } else if err := s.SetMeshKey(meshKey); err != nil { log.Fatalf("invalid mesh key: %v", err) } else { - s.SetMeshKey(key) log.Println("DERP mesh key configured") } diff --git a/cmd/derper/derper_test.go b/cmd/derper/derper_test.go index 12686ce4e..6dce1fcdf 100644 --- a/cmd/derper/derper_test.go +++ b/cmd/derper/derper_test.go @@ -138,46 +138,3 @@ func TestTemplate(t *testing.T) { t.Error("Output is missing debug info") } } - -func TestCheckMeshKey(t *testing.T) { - testCases := []struct { - name string - input string - want string - wantErr bool - }{ - { - name: "KeyOkay", - input: "f1ffafffffffffffffffffffffffffffffffffffffffffffffffff2ffffcfff6", - want: "f1ffafffffffffffffffffffffffffffffffffffffffffffffffff2ffffcfff6", - wantErr: false, - }, - { - name: "TrimKeyOkay", - input: " f1ffafffffffffffffffffffffffffffffffffffffffffffffffff2ffffcfff6 ", - want: "f1ffafffffffffffffffffffffffffffffffffffffffffffffffff2ffffcfff6", - wantErr: false, - }, - { - name: "NotAKey", - input: "zzthisisnotakey", - want: "", - wantErr: true, - }, - } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - k, err := checkMeshKey(tt.input) - if err != nil && !tt.wantErr { - t.Errorf("unexpected error: %v", err) - } - if k != tt.want && err == nil { - t.Errorf("want: %s doesn't match expected: %s", tt.want, k) - } - - }) - } - -} diff --git a/derp/derp_client.go b/derp/derp_client.go index 7a646fa51..a9b92299c 100644 --- a/derp/derp_client.go +++ b/derp/derp_client.go @@ -30,7 +30,7 @@ type Client struct { logf logger.Logf nc Conn br *bufio.Reader - meshKey string + meshKey key.DERPMesh canAckPings bool isProber bool @@ -56,7 +56,7 @@ func (f clientOptFunc) update(o *clientOpt) { f(o) } // clientOpt are the options passed to newClient. type clientOpt struct { - MeshKey string + MeshKey key.DERPMesh ServerPub key.NodePublic CanAckPings bool IsProber bool @@ -66,7 +66,7 @@ type clientOpt struct { // access to join the mesh. // // An empty key means to not use a mesh key. -func MeshKey(key string) ClientOpt { return clientOptFunc(func(o *clientOpt) { o.MeshKey = key }) } +func MeshKey(k key.DERPMesh) ClientOpt { return clientOptFunc(func(o *clientOpt) { o.MeshKey = k }) } // IsProber returns a ClientOpt to pass to the DERP server during connect to // declare that this client is a a prober. @@ -182,7 +182,7 @@ type clientInfo struct { func (c *Client) sendClientKey() error { msg, err := json.Marshal(clientInfo{ Version: ProtocolVersion, - MeshKey: c.meshKey, + MeshKey: c.meshKey.String(), CanAckPings: c.canAckPings, IsProber: c.isProber, }) diff --git a/derp/derp_server.go b/derp/derp_server.go index abda9da73..6f86c3ea4 100644 --- a/derp/derp_server.go +++ b/derp/derp_server.go @@ -134,7 +134,7 @@ type Server struct { publicKey key.NodePublic logf logger.Logf memSys0 uint64 // runtime.MemStats.Sys at start (or early-ish) - meshKey string + meshKey key.DERPMesh limitedLogf logger.Logf metaCert []byte // the encoded x509 cert to send after LetsEncrypt cert+intermediate dupPolicy dupPolicy @@ -464,8 +464,13 @@ func genDroppedCounters() { // amongst themselves. // // It must be called before serving begins. -func (s *Server) SetMeshKey(v string) { - s.meshKey = v +func (s *Server) SetMeshKey(v string) error { + k, err := key.ParseDERPMesh(v) + if err != nil { + return err + } + s.meshKey = k + return nil } // SetVerifyClients sets whether this DERP server verifies clients through tailscaled. @@ -506,10 +511,10 @@ func (s *Server) SetTCPWriteTimeout(d time.Duration) { } // HasMeshKey reports whether the server is configured with a mesh key. -func (s *Server) HasMeshKey() bool { return s.meshKey != "" } +func (s *Server) HasMeshKey() bool { return !s.meshKey.IsZero() } // MeshKey returns the configured mesh key, if any. -func (s *Server) MeshKey() string { return s.meshKey } +func (s *Server) MeshKey() key.DERPMesh { return s.meshKey } // PrivateKey returns the server's private key. func (s *Server) PrivateKey() key.NodePrivate { return s.privateKey } @@ -1355,7 +1360,18 @@ func (c *sclient) requestMeshUpdate() { // isMeshPeer reports whether the client is a trusted mesh peer // node in the DERP region. func (s *Server) isMeshPeer(info *clientInfo) bool { - return info != nil && info.MeshKey != "" && info.MeshKey == s.meshKey + // Compare mesh keys in constant time to prevent timing attacks. + // Since mesh keys are a fixed length, we don’t need to be concerned + // about timing attacks on client mesh keys that are the wrong length. + // See https://github.com/tailscale/corp/issues/28720 + if info == nil || info.MeshKey == "" { + return false + } + k, err := key.ParseDERPMesh(info.MeshKey) + if err != nil { + return false + } + return s.meshKey.Equal(k) } // verifyClient checks whether the client is allowed to connect to the derper, diff --git a/derp/derp_test.go b/derp/derp_test.go index c5a92bafa..0093ee2b1 100644 --- a/derp/derp_test.go +++ b/derp/derp_test.go @@ -511,11 +511,13 @@ func (ts *testServer) close(t *testing.T) error { return nil } +const testMeshKey = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + func newTestServer(t *testing.T, ctx context.Context) *testServer { t.Helper() logf := logger.WithPrefix(t.Logf, "derp-server: ") s := NewServer(key.NewNode(), logf) - s.SetMeshKey("mesh-key") + s.SetMeshKey(testMeshKey) ln, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) @@ -591,8 +593,12 @@ func newRegularClient(t *testing.T, ts *testServer, name string) *testClient { func newTestWatcher(t *testing.T, ts *testServer, name string) *testClient { return newTestClient(t, ts, name, func(nc net.Conn, priv key.NodePrivate, logf logger.Logf) (*Client, error) { + mk, err := key.ParseDERPMesh(testMeshKey) + if err != nil { + return nil, err + } brw := bufio.NewReadWriter(bufio.NewReader(nc), bufio.NewWriter(nc)) - c, err := NewClient(priv, nc, brw, logf, MeshKey("mesh-key")) + c, err := NewClient(priv, nc, brw, logf, MeshKey(mk)) if err != nil { return nil, err } @@ -1627,3 +1633,96 @@ func TestGetPerClientSendQueueDepth(t *testing.T) { }) } } + +func TestSetMeshKey(t *testing.T) { + for name, tt := range map[string]struct { + key string + want key.DERPMesh + wantErr bool + }{ + "clobber": { + key: testMeshKey, + wantErr: false, + }, + "invalid": { + key: "badf00d", + wantErr: true, + }, + } { + t.Run(name, func(t *testing.T) { + s := &Server{} + + err := s.SetMeshKey(tt.key) + if tt.wantErr { + if err == nil { + t.Fatalf("expected err") + } + return + } + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + + want, err := key.ParseDERPMesh(tt.key) + if err != nil { + t.Fatal(err) + } + if !s.meshKey.Equal(want) { + t.Fatalf("got %v, want %v", s.meshKey, want) + } + }) + } +} + +func TestIsMeshPeer(t *testing.T) { + s := &Server{} + err := s.SetMeshKey(testMeshKey) + if err != nil { + t.Fatal(err) + } + for name, tt := range map[string]struct { + info *clientInfo + want bool + wantAllocs float64 + }{ + "nil": { + info: nil, + want: false, + wantAllocs: 0, + }, + "empty": { + info: &clientInfo{MeshKey: ""}, + want: false, + wantAllocs: 0, + }, + "invalid": { + info: &clientInfo{MeshKey: "invalid"}, + want: false, + wantAllocs: 2, // error message + }, + "mismatch": { + info: &clientInfo{MeshKey: "0badf00d00000000000000000000000000000000000000000000000000000000"}, + want: false, + wantAllocs: 1, + }, + "match": { + info: &clientInfo{MeshKey: testMeshKey}, + want: true, + wantAllocs: 1, + }, + } { + t.Run(name, func(t *testing.T) { + var got bool + allocs := testing.AllocsPerRun(1, func() { + got = s.isMeshPeer(tt.info) + }) + if got != tt.want { + t.Fatalf("got %t, want %t: info = %#v", got, tt.want, tt.info) + } + + if allocs != tt.wantAllocs && tt.want { + t.Errorf("%f allocations, want %f", allocs, tt.wantAllocs) + } + }) + } +} diff --git a/derp/derphttp/derphttp_client.go b/derp/derphttp/derphttp_client.go index faa218ca2..8c42e9070 100644 --- a/derp/derphttp/derphttp_client.go +++ b/derp/derphttp/derphttp_client.go @@ -57,7 +57,7 @@ type Client struct { TLSConfig *tls.Config // optional; nil means default HealthTracker *health.Tracker // optional; used if non-nil only DNSCache *dnscache.Resolver // optional; nil means no caching - MeshKey string // optional; for trusted clients + MeshKey key.DERPMesh // optional; for trusted clients IsProber bool // optional; for probers to optional declare themselves as such // WatchConnectionChanges is whether the client wishes to subscribe to diff --git a/derp/derphttp/derphttp_test.go b/derp/derphttp/derphttp_test.go index cfb3676cd..8d02db922 100644 --- a/derp/derphttp/derphttp_test.go +++ b/derp/derphttp/derphttp_test.go @@ -212,6 +212,8 @@ func TestPing(t *testing.T) { } } +const testMeshKey = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + func newTestServer(t *testing.T, k key.NodePrivate) (serverURL string, s *derp.Server) { s = derp.NewServer(k, t.Logf) httpsrv := &http.Server{ @@ -224,7 +226,7 @@ func newTestServer(t *testing.T, k key.NodePrivate) (serverURL string, s *derp.S t.Fatal(err) } serverURL = "http://" + ln.Addr().String() - s.SetMeshKey("1234") + s.SetMeshKey(testMeshKey) go func() { if err := httpsrv.Serve(ln); err != nil { @@ -243,7 +245,11 @@ func newWatcherClient(t *testing.T, watcherPrivateKey key.NodePrivate, serverToW if err != nil { t.Fatal(err) } - c.MeshKey = "1234" + k, err := key.ParseDERPMesh(testMeshKey) + if err != nil { + t.Fatal(err) + } + c.MeshKey = k return } diff --git a/types/key/derp.go b/types/key/derp.go new file mode 100644 index 000000000..1fe690189 --- /dev/null +++ b/types/key/derp.go @@ -0,0 +1,68 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package key + +import ( + "crypto/subtle" + "encoding/hex" + "errors" + "fmt" + "strings" + + "go4.org/mem" + "tailscale.com/types/structs" +) + +var ErrInvalidMeshKey = errors.New("invalid mesh key") + +// DERPMesh is a mesh key, used for inter-DERP-node communication and for +// privileged DERP clients. +type DERPMesh struct { + _ structs.Incomparable // == isn't constant-time + k [32]byte // 64-digit hexadecimal numbers fit in 32 bytes +} + +// DERPMeshFromRaw32 parses a 32-byte raw value as a DERP mesh key. +func DERPMeshFromRaw32(raw mem.RO) DERPMesh { + if raw.Len() != 32 { + panic("input has wrong size") + } + var ret DERPMesh + raw.Copy(ret.k[:]) + return ret +} + +// ParseDERPMesh parses a DERP mesh key from a string. +// This function trims whitespace around the string. +// If the key is not a 64-digit hexadecimal number, ErrInvalidMeshKey is returned. +func ParseDERPMesh(key string) (DERPMesh, error) { + key = strings.TrimSpace(key) + if len(key) != 64 { + return DERPMesh{}, fmt.Errorf("%w: must be 64-digit hexadecimal number", ErrInvalidMeshKey) + } + decoded, err := hex.DecodeString(key) + if err != nil { + return DERPMesh{}, fmt.Errorf("%w: %v", ErrInvalidMeshKey, err) + } + return DERPMeshFromRaw32(mem.B(decoded)), nil +} + +// IsZero reports whether k is the zero value. +func (k DERPMesh) IsZero() bool { + return k.Equal(DERPMesh{}) +} + +// Equal reports whether k and other are the same key. +func (k DERPMesh) Equal(other DERPMesh) bool { + // Compare mesh keys in constant time to prevent timing attacks. + // Since mesh keys are a fixed length, we don’t need to be concerned + // about timing attacks on client mesh keys that are the wrong length. + // See https://github.com/tailscale/corp/issues/28720 + return subtle.ConstantTimeCompare(k.k[:], other.k[:]) == 1 +} + +// String returns k as a hex-encoded 64-digit number. +func (k DERPMesh) String() string { + return hex.EncodeToString(k.k[:]) +} diff --git a/types/key/derp_test.go b/types/key/derp_test.go new file mode 100644 index 000000000..b91cbbf8c --- /dev/null +++ b/types/key/derp_test.go @@ -0,0 +1,133 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package key + +import ( + "errors" + "testing" + + "go4.org/mem" +) + +func TestDERPMeshIsValid(t *testing.T) { + for name, tt := range map[string]struct { + input string + want string + wantErr error + }{ + "good": { + input: "0123456789012345678901234567890123456789012345678901234567890123", + want: "0123456789012345678901234567890123456789012345678901234567890123", + wantErr: nil, + }, + "hex": { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + want: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + wantErr: nil, + }, + "uppercase": { + input: "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF", + want: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + wantErr: nil, + }, + "whitespace": { + input: " 0123456789012345678901234567890123456789012345678901234567890123 ", + want: "0123456789012345678901234567890123456789012345678901234567890123", + wantErr: nil, + }, + "short": { + input: "0123456789abcdef", + wantErr: ErrInvalidMeshKey, + }, + "long": { + input: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0", + wantErr: ErrInvalidMeshKey, + }, + } { + t.Run(name, func(t *testing.T) { + k, err := ParseDERPMesh(tt.input) + if !errors.Is(err, tt.wantErr) { + t.Errorf("err %v, want %v", err, tt.wantErr) + } + + got := k.String() + if got != tt.want && tt.wantErr == nil { + t.Errorf("got %q, want %q", got, tt.want) + } + + }) + } + +} + +func TestDERPMesh(t *testing.T) { + t.Parallel() + + for name, tt := range map[string]struct { + str string + hex []byte + equal bool // are str and hex equal? + }{ + "zero": { + str: "0000000000000000000000000000000000000000000000000000000000000000", + hex: []byte{ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + }, + equal: true, + }, + "equal": { + str: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + hex: []byte{ + 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef, + 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef, + 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef, + 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef, + }, + equal: true, + }, + "unequal": { + str: "0badc0de00000000000000000000000000000000000000000000000000000000", + hex: []byte{ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + }, + equal: false, + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + k, err := ParseDERPMesh(tt.str) + if err != nil { + t.Fatal(err) + } + + // string representation should round-trip + s := k.String() + if s != tt.str { + t.Fatalf("string %s, want %s", s, tt.str) + } + + // if tt.equal, then tt.hex is intended to be equal + if k.k != [32]byte(tt.hex) && tt.equal { + t.Fatalf("decoded %x, want %x", k.k, tt.hex) + } + + h := DERPMeshFromRaw32(mem.B(tt.hex)) + if k.Equal(h) != tt.equal { + if tt.equal { + t.Fatalf("%v != %v", k, h) + } else { + t.Fatalf("%v == %v", k, h) + } + } + + }) + } +}