Add version handshake before communication is stablished

Details on how it works is in wire.Handshake.

Updates #4805

PiperOrigin-RevId: 448552448

examples/seccheck/server.cc

@@ -28,6 +28,7 @@
 
 #include "absl/cleanup/cleanup.h"
 #include "absl/strings/string_view.h"
+#include "pkg/sentry/seccheck/points/common.pb.h"
 #include "pkg/sentry/seccheck/points/container.pb.h"
 #include "pkg/sentry/seccheck/points/sentry.pb.h"
 #include "pkg/sentry/seccheck/points/syscall.pb.h"
@@ -163,6 +164,41 @@ void startPollThread(int poll_fd) {
   pthread_detach(thread);
 }
 
+// handshake performs version exchange with client. See common.proto for details
+// about the protocol.
+bool handshake(int client_fd) {
+  std::vector<char> buf(10240);
+  int bytes = read(client_fd, buf.data(), buf.size());
+  if (bytes < 0) {
+    printf("Error receiving handshake message: %d\n", errno);
+    return false;
+  } else if (bytes == buf.size()) {
+    // Protect against the handshake becoming larger than the buffer allocated
+    // for it.
+    printf("handshake message too big\n");
+    return false;
+  }
+  ::gvisor::common::Handshake in = {};
+  if (!in.ParseFromArray(buf.data(), bytes)) {
+    printf("Error parsing handshake message\n");
+    return false;
+  }
+
+  constexpr uint32_t minSupportedVersion = 1;
+  if (in.version() < minSupportedVersion) {
+    printf("Client has unsupported version %u\n", in.version());
+    return false;
+  }
+
+  ::gvisor::common::Handshake out;
+  out.set_version(1);
+  if (!out.SerializeToFileDescriptor(client_fd)) {
+    printf("Error sending handshake message: %d\n", errno);
+    return false;
+  }
+  return true;
+}
+
 extern "C" int main(int argc, char** argv) {
   for (int c = 0; (c = getopt(argc, argv, "q")) != -1;) {
     switch (c) {
@@ -221,6 +257,11 @@ extern "C" int main(int argc, char** argv) {
     }
     printf("Connection accepted\n");
 
+    if (!handshake(client)) {
+      close(client);
+      continue;
+    }
+
     struct epoll_event evt;
     evt.data.fd = client;
     evt.events = EPOLLIN;

pkg/sentry/seccheck/checkers/remote/BUILD

@@ -12,7 +12,7 @@ go_library(
         "//pkg/fd",
         "//pkg/log",
         "//pkg/sentry/seccheck",
-        "//pkg/sentry/seccheck/checkers/remote/header",
+        "//pkg/sentry/seccheck/checkers/remote/wire",
         "//pkg/sentry/seccheck/points:points_go_proto",
         "@org_golang_google_protobuf//proto:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
@@ -31,6 +31,7 @@ go_test(
         "//pkg/fd",
         "//pkg/sentry/seccheck",
         "//pkg/sentry/seccheck/checkers/remote/test",
+        "//pkg/sentry/seccheck/checkers/remote/wire",
         "//pkg/sentry/seccheck/points:points_go_proto",
         "//pkg/test/testutil",
         "@com_github_cenkalti_backoff//:go_default_library",

pkg/sentry/seccheck/checkers/remote/remote.go

@@ -17,7 +17,9 @@
 package remote
 
 import (
+	"errors"
 	"fmt"
+	"io"
 	"os"
 
 	"golang.org/x/sys/unix"
@@ -27,7 +29,7 @@ import (
 	"gvisor.dev/gvisor/pkg/fd"
 	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/sentry/seccheck"
-	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/header"
+	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire"
 	pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto"
 )
 
@@ -81,6 +83,38 @@ func setup(path string) (*os.File, error) {
 	if err := unix.Connect(int(f.Fd()), &addr); err != nil {
 		return nil, fmt.Errorf("connect(%q): %w", path, err)
 	}
+
+	// Perform handshake. See common.proto for details about the protocol.
+	hsOut := pb.Handshake{Version: wire.CurrentVersion}
+	out, err := proto.Marshal(&hsOut)
+	if err != nil {
+		return nil, fmt.Errorf("marshalling handshake message: %w", err)
+	}
+	if _, err := f.Write(out); err != nil {
+		return nil, fmt.Errorf("sending handshake message: %w", err)
+	}
+
+	in := make([]byte, 10240)
+	read, err := f.Read(in)
+	if err != nil && !errors.Is(err, io.EOF) {
+		return nil, fmt.Errorf("reading handshake message: %w", err)
+	}
+	// Protect against the handshake becoming larger than the buffer allocated
+	// for it.
+	if read == len(in) {
+		return nil, fmt.Errorf("handshake message too big")
+	}
+	hsIn := pb.Handshake{}
+	if err := proto.Unmarshal(in[:read], &hsIn); err != nil {
+		return nil, fmt.Errorf("unmarshalling handshake message: %w", err)
+	}
+
+	// Check that remote version can be supported.
+	const minSupportedVersion = 1
+	if hsIn.Version < minSupportedVersion {
+		return nil, fmt.Errorf("remote version (%d) is smaller than minimum supported (%d)", hsIn.Version, minSupportedVersion)
+	}
+
 	cu.Release()
 	return f, nil
 }
@@ -90,13 +124,6 @@ func New(_ map[string]interface{}, endpoint *fd.FD) (seccheck.Checker, error) {
 	if endpoint == nil {
 		return nil, fmt.Errorf("remote sink requires an endpoint")
 	}
-	// TODO(gvisor.dev/issue/4805): perform version handshake with remote:
-	//   1. sentry and remote exchange versions
-	//	 2. sentry continues if remote >= min(sentry)
-	//   3. remote continues if sentry >= min(remote).
-	// min() being the minimal supported version. Let's say current sentry
-	// supports batching but remote doesn't, sentry can chose to not batch or
-	// refuse the connection.
 	return &Remote{endpoint: endpoint}, nil
 }
 
@@ -115,11 +142,11 @@ func (r *Remote) write(msg proto.Message, msgType pb.MessageType) {
 		log.Debugf("Marshal(%+v): %v", msg, err)
 		return
 	}
-	hdr := header.Header{
-		HeaderSize:  uint16(header.HeaderStructSize),
+	hdr := wire.Header{
+		HeaderSize:  uint16(wire.HeaderStructSize),
 		MessageType: uint16(msgType),
 	}
-	var hdrOut [header.HeaderStructSize]byte
+	var hdrOut [wire.HeaderStructSize]byte
 	hdr.MarshalUnsafe(hdrOut[:])
 
 	// TODO(gvisor.dev/issue/4805): Change to non-blocking write. Count as dropped

pkg/sentry/seccheck/checkers/remote/remote_test.go

@@ -31,6 +31,7 @@ import (
 	"gvisor.dev/gvisor/pkg/fd"
 	"gvisor.dev/gvisor/pkg/sentry/seccheck"
 	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/test"
+	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire"
 	pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto"
 	"gvisor.dev/gvisor/pkg/test/testutil"
 )
@@ -153,6 +154,37 @@ func TestBasic(t *testing.T) {
 	}
 }
 
+func TestVersionUnsupported(t *testing.T) {
+	server, err := test.NewServer()
+	if err != nil {
+		t.Fatalf("newServer(): %v", err)
+	}
+	defer server.Close()
+
+	server.SetVersion(0)
+
+	_, err = setup(server.Path)
+	if err == nil || !strings.Contains(err.Error(), "remote version") {
+		t.Fatalf("Wrong error: %v", err)
+	}
+}
+
+func TestVersionNewer(t *testing.T) {
+	server, err := test.NewServer()
+	if err != nil {
+		t.Fatalf("newServer(): %v", err)
+	}
+	defer server.Close()
+
+	server.SetVersion(wire.CurrentVersion + 10)
+
+	endpoint, err := setup(server.Path)
+	if err != nil {
+		t.Fatalf("setup(): %v", err)
+	}
+	_ = endpoint.Close()
+}
+
 // Test that the example C++ server works. It's easier to test from here and
 // also changes that can break it will likely originate here.
 func TestExample(t *testing.T) {

pkg/sentry/seccheck/checkers/remote/test/BUILD

@@ -10,11 +10,12 @@ go_library(
     deps = [
         "//pkg/cleanup",
         "//pkg/log",
-        "//pkg/sentry/seccheck/checkers/remote/header",
+        "//pkg/sentry/seccheck/checkers/remote/wire",
         "//pkg/sentry/seccheck/points:points_go_proto",
         "//pkg/sync",
         "//pkg/test/testutil",
         "//pkg/unet",
+        "@org_golang_google_protobuf//proto:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
     ],
 )

pkg/sentry/seccheck/checkers/remote/test/server.go

@@ -24,9 +24,10 @@ import (
 	"time"
 
 	"golang.org/x/sys/unix"
+	"google.golang.org/protobuf/proto"
 	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/log"
-	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/header"
+	"gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire"
 	pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/pkg/test/testutil"
@@ -46,6 +47,8 @@ type Server struct {
 
 	// +checklocks:mu
 	points []Message
+
+	version uint32
 }
 
 // Message corresponds to a single message sent from checkers.Remote.
@@ -97,8 +100,9 @@ func newServerPath(path string) (*Server, error) {
 	}
 
 	server := &Server{
-		Path:   path,
-		socket: ss,
+		Path:    path,
+		socket:  ss,
+		version: wire.CurrentVersion,
 	}
 	go server.run()
 	cu.Release()
@@ -115,6 +119,11 @@ func (s *Server) run() {
 			}
 			return
 		}
+		if err := s.handshake(client); err != nil {
+			log.Warningf(err.Error())
+			_ = client.Close()
+			continue
+		}
 		s.mu.Lock()
 		s.clients = append(s.clients, client)
 		s.mu.Unlock()
@@ -122,6 +131,33 @@ func (s *Server) run() {
 	}
 }
 
+// handshake performs version exchange with client. See common.proto for details
+// about the protocol.
+func (s *Server) handshake(client *unet.Socket) error {
+	var in [1024]byte
+	read, err := client.Read(in[:])
+	if err != nil {
+		return fmt.Errorf("reading handshake message: %w", err)
+	}
+	hsIn := pb.Handshake{}
+	if err := proto.Unmarshal(in[:read], &hsIn); err != nil {
+		return fmt.Errorf("unmarshalling handshake message: %w", err)
+	}
+	if hsIn.Version != wire.CurrentVersion {
+		return fmt.Errorf("wrong version number, want: %d, got, %d", wire.CurrentVersion, hsIn.Version)
+	}
+
+	hsOut := pb.Handshake{Version: s.version}
+	out, err := proto.Marshal(&hsOut)
+	if err != nil {
+		return fmt.Errorf("marshalling handshake message: %w", err)
+	}
+	if _, err := client.Write(out); err != nil {
+		return fmt.Errorf("sending handshake message: %w", err)
+	}
+	return nil
+}
+
 func (s *Server) handleClient(client *unet.Socket) {
 	defer func() {
 		s.mu.Lock()
@@ -144,11 +180,11 @@ func (s *Server) handleClient(client *unet.Socket) {
 		if read == 0 {
 			return
 		}
-		if read < header.HeaderStructSize {
+		if read < wire.HeaderStructSize {
 			panic("invalid message")
 		}
-		hdr := header.Header{}
-		hdr.UnmarshalUnsafe(buf[0:header.HeaderStructSize])
+		hdr := wire.Header{}
+		hdr.UnmarshalUnsafe(buf[0:wire.HeaderStructSize])
 		if read < int(hdr.HeaderSize) {
 			panic(fmt.Sprintf("message truncated, header size: %d, readL %d", hdr.HeaderSize, read))
 		}
@@ -209,3 +245,8 @@ func (s *Server) WaitForCount(count int) error {
 		return nil
 	}, 5*time.Second)
 }
+
+// SetVersion sets the version to be used in handshake.
+func (s *Server) SetVersion(newVersion uint32) {
+	s.version = newVersion
+}

pkg/sentry/seccheck/checkers/remote/wire/BUILD

@@ -3,15 +3,15 @@ load("//tools:defs.bzl", "go_library", "go_test")
 package(licenses = ["notice"])
 
 go_library(
-    name = "header",
-    srcs = ["header.go"],
+    name = "wire",
+    srcs = ["wire.go"],
     marshal = True,
     visibility = ["//:sandbox"],
 )
 
 go_test(
-    name = "header_test",
+    name = "wire_test",
     size = "small",
-    srcs = ["header_test.go"],
-    library = ":header",
+    srcs = ["wire_test.go"],
+    library = ":wire",
 )

pkg/sentry/seccheck/checkers/remote/wire/wire.go

@@ -12,8 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package header contains the message header used in the remote checker.
-package header
+// Package wire defines structs used in the wire format for the remote checker.
+package wire
+
+// CurrentVersion is the current wire and protocol version.
+const CurrentVersion = 1
 
 // HeaderStructSize size of header struct in bytes.
 const HeaderStructSize = 8

pkg/sentry/seccheck/checkers/remote/wire/wire_test.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package header
+package wire
 
 import "testing"
 

pkg/sentry/seccheck/points/common.proto

@@ -16,6 +16,54 @@ syntax = "proto3";
 
 package gvisor.common;
 
+// Handshake message is used when establishing a connection. Version information
+// is exchanged to determine if the communication can proceed. Each side reports
+// a single version of the protocol that it supports. If they can't support the
+// version reported by the peer, they must close the connection. If the peer
+// version is higher (newer), it should continue to communicate, making the peer
+// responsible to send messages that are compatible with your version. If the
+// peer can't support it, the peer should close the connection.
+//
+// In short:
+//   1. sentry and remote exchange versions
+//   2. sentry continues if remote >= min(sentry)
+//   3. remote continues if sentry >= min(remote)
+//
+// Suppose that peer A is at version 1 and peer B is at 2. Peer A sees that B is
+// at a newer version and continues with communication. Peer B will see that A
+// is at version 1 (older) and will check if it can send messages that are
+// compatible with version 1. If yes, then the communication can continue. If
+// not, A should close the connection.
+//
+// Here are 2 practical examples:
+//   1. New field added to the header: this requires a change in protocol
+//      version (e.g. 1 => 2). However, if not essential to communication, the
+//      new field can be ignored by a peer that is still using version 1.
+//      Sentry version 1, remote version 2: remote doesn't get the new field,
+//      but can still receive messages.
+//      Sentry version 2, remote version 1: remote gets the new field, but
+//      ignores it since it's not aware the field exists yet. Note that remote
+//      must rely on header length to determine where the payload is.
+//
+//   2. Change in message format for batching: this requires a change in
+//      protocol version (2 => 3). Batching can only be used if both sides can
+//      handle it.
+//      Sentry version 2, remote version 3: remote gets a message at a time. If
+//      it still can do that, remote can accept that sentry is in version 2.
+//      Sentry version 3, remote version 2: remote is not able to process
+//      batched messages. If the sentry can still produce one message at a time
+//      the communication can continue, otherwise the sentry should close the
+//      connection.
+//
+// Note that addition of new message types do not require version changes.
+// Server implementations should gracefully handle messages that it doesn't
+// understand. Similarly, payload for message can change following protobuf
+// rules for compatibilty. For example, adding new fields to a protobuf type
+// doesn't require version bump.
+message Handshake {
+  uint32 version = 1;
+}
+
 message Credentials {
   uint32 real_uid = 1;
   uint32 effective_uid = 2;