From bf704a52187376e1d6af769e75f2ca22629fd14c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 4 Mar 2020 09:35:32 -0800 Subject: [PATCH] derp: protocol negotiation, add v2: send src pub keys to clients in packets Signed-off-by: Brad Fitzpatrick --- derp/derp.go | 18 +++++++++++- derp/derp_client.go | 50 ++++++++++++++++++++++++--------- derp/derp_server.go | 44 +++++++++++++++++++---------- derp/derp_test.go | 5 +++- derp/derphttp/derphttp_test.go | 2 +- wgengine/magicsock/magicsock.go | 41 +++++++++++++++++++-------- 6 files changed, 117 insertions(+), 43 deletions(-) diff --git a/derp/derp.go b/derp/derp.go index 1bcd55486..5c4eee226 100644 --- a/derp/derp.go +++ b/derp/derp.go @@ -38,6 +38,15 @@ const ( keepAlive = 60 * time.Second ) +// protocolVersion is bumped whenever there's a wire-incompatible change. +// * version 1 (zero on wire): consistent box headers, in use by employee dev nodes a bit +// * version 2: received packets have src addrs in frameRecvPacket at beginning +const protocolVersion = 2 + +const ( + protocolSrcAddrs = 2 // protocol version at which client expects src addresses +) + // frameType is the one byte frame type at the beginning of the frame // header. The second field is a big-endian uint32 describing the // length of the remaining frame (not including the initial 5 bytes). @@ -62,7 +71,7 @@ const ( frameClientInfo = frameType(0x02) // 32B pub key + 24B nonce + naclbox(json) frameServerInfo = frameType(0x03) // 24B nonce + naclbox(json) frameSendPacket = frameType(0x04) // 32B dest pub key + packet bytes - frameRecvPacket = frameType(0x05) // packet bytes + frameRecvPacket = frameType(0x05) // v0/1: packet bytes, v2: 32B src pub key + packet bytes frameKeepAlive = frameType(0x06) // no payload, no-op (to be replaced with ping/pong) ) @@ -155,3 +164,10 @@ func writeFrame(bw *bufio.Writer, t frameType, b []byte) error { } return bw.Flush() } + +func minInt(a, b int) int { + if a < b { + return a + } + return b +} diff --git a/derp/derp_client.go b/derp/derp_client.go index 4ade477d0..5779eb8c8 100644 --- a/derp/derp_client.go +++ b/derp/derp_client.go @@ -20,14 +20,15 @@ import ( ) type Client struct { - serverKey key.Public // of the DERP server; not a machine or node key - privateKey key.Private - publicKey key.Public // of privateKey - logf logger.Logf - nc net.Conn - br *bufio.Reader - bw *bufio.Writer - readErr error // sticky read error + serverKey key.Public // of the DERP server; not a machine or node key + privateKey key.Private + publicKey key.Public // of privateKey + protoVersion int // min of server+client + logf logger.Logf + nc net.Conn + br *bufio.Reader + bw *bufio.Writer + readErr error // sticky read error } func NewClient(privateKey key.Private, nc net.Conn, brw *bufio.ReadWriter, logf logger.Logf) (*Client, error) { @@ -46,11 +47,11 @@ func NewClient(privateKey key.Private, nc net.Conn, brw *bufio.ReadWriter, logf if err := c.sendClientKey(); err != nil { return nil, fmt.Errorf("derp.Client: failed to send client key: %v", err) } - _, err := c.recvServerInfo() + info, err := c.recvServerInfo() if err != nil { return nil, fmt.Errorf("derp.Client: failed to receive server info: %v", err) } - + c.protoVersion = minInt(protocolVersion, info.Version) return c, nil } @@ -104,12 +105,19 @@ func (c *Client) recvServerInfo() (*serverInfo, error) { return info, nil } +type clientInfo struct { + Version int // `json:"version,omitempty"` +} + func (c *Client) sendClientKey() error { var nonce [nonceLen]byte if _, err := crand.Read(nonce[:]); err != nil { return err } - msg := []byte("{}") // no clientInfo for now + msg, err := json.Marshal(clientInfo{Version: protocolVersion}) + if err != nil { + return err + } msgbox := box.Seal(nil, msg, &nonce, c.serverKey.B32(), c.privateKey.B32()) buf := make([]byte, 0, nonceLen+keyLen+len(msgbox)) @@ -156,7 +164,12 @@ type ReceivedMessage interface { } // ReceivedPacket is a ReceivedMessage representing an incoming packet. -type ReceivedPacket []byte +type ReceivedPacket struct { + Source key.Public + // Data is the received packet bytes. It aliases the memory + // passed to Client.Recv. + Data []byte +} func (ReceivedPacket) msg() {} @@ -189,7 +202,18 @@ func (c *Client) Recv(b []byte) (m ReceivedMessage, err error) { // require ack pongs. continue case frameRecvPacket: - return ReceivedPacket(b[:n]), nil + var rp ReceivedPacket + if c.protoVersion < protocolSrcAddrs { + rp.Data = b[:n] + } else { + if n < keyLen { + c.logf("[unexpected] dropping short packet from DERP server") + continue + } + copy(rp.Source[:], b[:keyLen]) + rp.Data = b[keyLen:n] + } + return rp, nil } } } diff --git a/derp/derp_server.go b/derp/derp_server.go index e6f4b8fdb..17d431ad4 100644 --- a/derp/derp_server.go +++ b/derp/derp_server.go @@ -237,7 +237,7 @@ func (s *Server) accept(nc net.Conn, brw *bufio.ReadWriter) error { } dst.mu.Lock() - err = s.sendPacket(dst.bw, c.key, contents) + err = s.sendPacket(dst.bw, &dst.info, c.key, contents) dst.mu.Unlock() if err != nil { @@ -260,7 +260,7 @@ func (s *Server) sendClientKeepAlives(ctx context.Context, c *sclient) { } } -func (s *Server) verifyClient(clientKey key.Public, info *sclientInfo) error { +func (s *Server) verifyClient(clientKey key.Public, info *clientInfo) error { // TODO(crawshaw): implement policy constraints on who can use the DERP server // TODO(bradfitz): ... and at what rate. return nil @@ -273,12 +273,20 @@ func (s *Server) sendServerKey(bw *bufio.Writer) error { return writeFrame(bw, frameServerKey, buf) } +type serverInfo struct { + Version int // `json:"version,omitempty"` +} + func (s *Server) sendServerInfo(bw *bufio.Writer, clientKey key.Public) error { var nonce [24]byte if _, err := crand.Read(nonce[:]); err != nil { return err } - msg := []byte("{}") // no serverInfo for now + msg, err := json.Marshal(serverInfo{Version: protocolVersion}) + if err != nil { + return err + } + msgbox := box.Seal(nil, msg, &nonce, clientKey.B32(), s.privateKey.B32()) if err := writeFrameHeader(bw, frameServerInfo, nonceLen+uint32(len(msgbox))); err != nil { return err @@ -295,7 +303,7 @@ func (s *Server) sendServerInfo(bw *bufio.Writer, clientKey key.Public) error { // recvClientKey reads the frameClientInfo frame from the client (its // proof of identity) upon its initial connection. It should be // considered especially untrusted at this point. -func (s *Server) recvClientKey(br *bufio.Reader) (clientKey key.Public, info *sclientInfo, err error) { +func (s *Server) recvClientKey(br *bufio.Reader) (clientKey key.Public, info *clientInfo, err error) { fl, err := readFrameTypeHeader(br, frameClientInfo) if err != nil { return key.Public{}, nil, err @@ -325,19 +333,32 @@ func (s *Server) recvClientKey(br *bufio.Reader) (clientKey key.Public, info *sc if !ok { return key.Public{}, nil, fmt.Errorf("msgbox: cannot open len=%d with client key %x", msgLen, clientKey[:]) } - info = new(sclientInfo) + info = new(clientInfo) if err := json.Unmarshal(msg, info); err != nil { return key.Public{}, nil, fmt.Errorf("msg: %v", err) } return clientKey, info, nil } -func (s *Server) sendPacket(bw *bufio.Writer, srcKey key.Public, contents []byte) error { +func (s *Server) sendPacket(bw *bufio.Writer, dstInfo *clientInfo, srcKey key.Public, contents []byte) error { s.packetsSent.Add(1) s.bytesSent.Add(int64(len(contents))) - if err := writeFrameHeader(bw, frameRecvPacket, uint32(len(contents))); err != nil { + + sendSrc := dstInfo.Version >= protocolSrcAddrs + + pktLen := len(contents) + if sendSrc { + pktLen += len(srcKey) + } + + if err := writeFrameHeader(bw, frameRecvPacket, uint32(pktLen)); err != nil { return err } + if sendSrc { + if _, err := bw.Write(srcKey[:]); err != nil { + return err + } + } if _, err := bw.Write(contents); err != nil { return err } @@ -373,7 +394,7 @@ func (s *Server) recvPacket(ctx context.Context, br *bufio.Reader, frameLen uint type sclient struct { nc net.Conn key key.Public - info sclientInfo + info clientInfo keepAliveTimer *time.Timer keepAliveReset chan struct{} @@ -417,13 +438,6 @@ func (c *sclient) keepAliveLoop(ctx context.Context) error { } } -// sclientInfo is the client info sent by the client to the server. -type sclientInfo struct { -} - -type serverInfo struct { -} - func (s *Server) expVarFunc(f func() interface{}) expvar.Func { return expvar.Func(func() interface{} { s.mu.Lock() diff --git a/derp/derp_test.go b/derp/derp_test.go index 28e0d6b30..f1e7a19e9 100644 --- a/derp/derp_test.go +++ b/derp/derp_test.go @@ -86,7 +86,10 @@ func TestSendRecv(t *testing.T) { t.Errorf("unexpected message type %T", m) continue case ReceivedPacket: - recvChs[i] <- []byte(m) + if m.Source.IsZero() { + t.Errorf("zero Source address in ReceivedPacket") + } + recvChs[i] <- m.Data } } }(i) diff --git a/derp/derphttp/derphttp_test.go b/derp/derphttp/derphttp_test.go index 272e3d226..3253f79ed 100644 --- a/derp/derphttp/derphttp_test.go +++ b/derp/derphttp/derphttp_test.go @@ -104,7 +104,7 @@ func TestSendRecv(t *testing.T) { t.Errorf("unexpected message type %T", m) continue case derp.ReceivedPacket: - recvChs[i] <- []byte(m) + recvChs[i] <- m.Data } } }(i) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 2bea8ea19..c68abaad8 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -631,13 +631,18 @@ func (c *Conn) derpWriteChanOfAddr(addr *net.UDPAddr) chan<- derpWriteRequest { // derpReadResult is the type sent by runDerpClient to ReceiveIPv4 // when a DERP packet is available. +// +// Notably, it doesn't include the derp.ReceivedPacket because we +// don't want to give the receiver access to the aliased []byte. To +// get at the packet contents they need to call copyBuf to copy it +// out, which also releases the buffer. type derpReadResult struct { derpAddr *net.UDPAddr - n int // length of data received - + n int // length of data received + src key.Public // may be zero until server deployment if v2+ // copyBuf is called to copy the data to dst. It returns how // much data was copied, which will be n if dst is large - // enough. + // enough. copyBuf can only be called once. copyBuf func(dst []byte) int } @@ -648,9 +653,11 @@ var logDerpVerbose, _ = strconv.ParseBool(os.Getenv("DEBUG_DERP_VERBOSE")) func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr *net.UDPAddr, dc *derphttp.Client) { didCopy := make(chan struct{}, 1) var buf [derp.MaxPacketSize]byte - var bufValid int // bytes in buf that are valid - copyFn := func(dst []byte) int { - n := copy(dst, buf[:bufValid]) + + res := derpReadResult{derpAddr: derpFakeAddr} + var pkt derp.ReceivedPacket + res.copyBuf = func(dst []byte) int { + n := copy(dst, pkt.Data) didCopy <- struct{}{} return n } @@ -674,19 +681,21 @@ func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr *net.UDPAddr, dc } switch m := msg.(type) { case derp.ReceivedPacket: - bufValid = len(m) + pkt = m + res.n = len(m.Data) + res.src = m.Source + if logDerpVerbose { + log.Printf("got derp %v packet: %q", derpFakeAddr, m.Data) + } default: // Ignore. // TODO: handle endpoint notification messages. continue } - if logDerpVerbose { - log.Printf("got derp %v packet: %q", derpFakeAddr, buf[:bufValid]) - } select { case <-c.donec(): return - case c.derpRecvCh <- derpReadResult{derpFakeAddr, bufValid, copyFn}: + case c.derpRecvCh <- res: <-didCopy } } @@ -771,6 +780,8 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, addr *net.UDPAddr } }() + var addrSet *AddrSet + select { case dm := <-c.derpRecvCh: // Cancel the pconn read goroutine @@ -797,6 +808,10 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, addr *net.UDPAddr return 0, nil, nil, err } + // TODO: look up addrSet from dm.Source public key, if + // found (Source might be zero for a short period of + // time until DERP servers re-deployed) + case um := <-c.udpRecvCh: if um.err != nil { return 0, nil, nil, err @@ -804,7 +819,9 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, addr *net.UDPAddr n, addr = um.n, um.addr } - addrSet := c.findAddrSet(addr) + if addrSet == nil { + addrSet = c.findAddrSet(addr) + } if addrSet == nil { // The peer that sent this packet has roamed beyond the // knowledge provided by the control server.