From a4f2739693ce4cfd521a43c26b52df27a522bde4 Mon Sep 17 00:00:00 2001 From: Austin Vazquez Date: Tue, 20 Oct 2026 15:11:53 -0500 Subject: [PATCH 1/2] fix(socketforward): release relay shutdown watcher on disconnect Signed-off-by: Austin Vazquez --- internal/shim/task/socketforward.go | 14 ++++++++--- internal/shim/task/socketforward_test.go | 32 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/internal/shim/task/socketforward.go b/internal/shim/task/socketforward.go index 4cf77057..9d793cf6 100644 --- a/internal/shim/task/socketforward.go +++ b/internal/shim/task/socketforward.go @@ -291,22 +291,30 @@ func (fwd *socketForwarder) handleConnection(ctx context.Context, sb sandbox.San // closes or errors. Both connections are closed when done. func relay(a, b io.ReadWriteCloser, closeCh <-chan struct{}) { done := make(chan struct{}, 2) + relayDone := make(chan struct{}) cp := func(dst io.Writer, src io.Reader) { io.Copy(dst, src) done <- struct{}{} } go cp(a, b) go cp(b, a) + watcherDone := make(chan struct{}) go func() { - <-closeCh - a.Close() - b.Close() + defer close(watcherDone) + select { + case <-closeCh: + a.Close() + b.Close() + case <-relayDone: + } }() <-done // one side finished a.Close() b.Close() <-done // wait for the other + close(relayDone) + <-watcherDone } func (fwd *socketForwarder) shutdown() error { diff --git a/internal/shim/task/socketforward_test.go b/internal/shim/task/socketforward_test.go index 7e660903..d01c1042 100644 --- a/internal/shim/task/socketforward_test.go +++ b/internal/shim/task/socketforward_test.go @@ -18,7 +18,10 @@ package task import ( "context" + "net" + "runtime" "testing" + "time" "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" @@ -134,3 +137,32 @@ func TestSocketForwardsProviderFromBundle(t *testing.T) { }) } } + +func TestRelayFinishesWithoutForwarderShutdown(t *testing.T) { + baseline := runtime.NumGoroutine() + closeCh := make(chan struct{}) // stays open until after the leak check + defer close(closeCh) + for i := 0; i < 64; i++ { + a, peerA := net.Pipe() + b, peerB := net.Pipe() + finished := make(chan struct{}) + go func() { + relay(a, b, closeCh) + close(finished) + }() + require.NoError(t, peerA.Close()) + select { + case <-finished: + case <-time.After(5 * time.Second): + t.Fatal("relay did not finish after normal connection close") + } + require.NoError(t, peerB.Close()) + } + // The old relay returned while leaving one goroutine per completed + // connection waiting for closeCh. Allow some unrelated test goroutines. + deadline := time.Now().Add(2 * time.Second) + for runtime.NumGoroutine() > baseline+16 && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + require.LessOrEqual(t, runtime.NumGoroutine(), baseline+16, "completed relays retained shutdown watchers") +} From fcc4de1058e1a094939b18f0330cb0665f1709eb Mon Sep 17 00:00:00 2001 From: Austin Vazquez Date: Tue, 20 Oct 2026 15:11:54 -0500 Subject: [PATCH 2/2] feat(socketforward): support private guest-owned UDS listeners Signed-off-by: Austin Vazquez --- api/next.txtpb | 245 +++++++++++--- .../socketforward/v1/socketforward.proto | 8 +- .../socketforward/v1/socketforward.pb.go | 95 ++++-- internal/shim/task/socketforward.go | 42 ++- internal/shim/task/socketforward_bind_test.go | 106 ++++++ internal/shim/task/socketforward_test.go | 75 +++++ .../vminit/socketforward/socketforward.go | 166 ++++++++- .../socketforward/socketforward_test.go | 315 ++++++++++++++++++ plugins/services/system/service_test.go | 30 ++ 9 files changed, 979 insertions(+), 103 deletions(-) create mode 100644 internal/shim/task/socketforward_bind_test.go create mode 100644 internal/vminit/socketforward/socketforward_test.go create mode 100644 plugins/services/system/service_test.go diff --git a/api/next.txtpb b/api/next.txtpb index f8b786ef..755206a8 100644 --- a/api/next.txtpb +++ b/api/next.txtpb @@ -961,6 +961,27 @@ file: { type: TYPE_STRING json_name: "socketPath" } + field: { + name: "mode" + number: 3 + label: LABEL_OPTIONAL + type: TYPE_UINT32 + json_name: "mode" + } + field: { + name: "uid" + number: 4 + label: LABEL_OPTIONAL + type: TYPE_UINT32 + json_name: "uid" + } + field: { + name: "gid" + number: 5 + label: LABEL_OPTIONAL + type: TYPE_UINT32 + json_name: "gid" + } } message_type: { name: "ConnectRequest" @@ -1018,7 +1039,7 @@ file: { location: { span: 16 span: 0 - span: 94 + span: 100 span: 1 } location: { @@ -1059,7 +1080,7 @@ file: { path: 0 span: 38 span: 0 - span: 52 + span: 53 span: 1 leading_comments: " SocketForward provides UNIX domain socket forwarding across the VM boundary\n over vsock streams. Container processes connect to listener sockets created\n by vminitd; connections are relayed to the corresponding host-side socket.\n\n The ttrpc server runs inside the VM (vminitd) and the ttrpc client runs on\n the host (shim). All RPCs are initiated by the host:\n\n - Bind: the host tells the VM which sockets to set up. The VM creates\n UNIX listener sockets in the root mount ns so that crun can bind-mount\n them into the container. This must be called before the container is\n created.\n - Accept: the host opens a bidirectional streaming channel. The VM sends\n ConnectRequest messages when a process inside the container connects to\n a forwarded socket, and the host replies with ConnectResult messages.\n" } @@ -1076,10 +1097,10 @@ file: { path: 0 path: 2 path: 0 - span: 43 + span: 44 span: 4 span: 58 - leading_comments: " Bind sets up the socket forward entries on the VM side. The VM creates\n UNIX listener sockets at the paths given in socket_path. This RPC returns\n only after all socket files have been created, so the caller can safely\n proceed with container creation (crun bind mounts).\n" + leading_comments: " Bind sets up the socket forward entries on the VM side. The VM creates\n UNIX listener sockets at the paths given in socket_path. This RPC returns\n only after all socket files have been created, so the caller can safely\n proceed with container creation (crun bind mounts). The shim and guest\n versions are coupled, including the private socket mode contract.\n" } location: { path: 6 @@ -1087,7 +1108,7 @@ file: { path: 2 path: 0 path: 1 - span: 43 + span: 44 span: 8 span: 12 } @@ -1097,7 +1118,7 @@ file: { path: 2 path: 0 path: 2 - span: 43 + span: 44 span: 13 span: 24 } @@ -1107,7 +1128,7 @@ file: { path: 2 path: 0 path: 3 - span: 43 + span: 44 span: 35 span: 56 } @@ -1116,7 +1137,7 @@ file: { path: 0 path: 2 path: 1 - span: 51 + span: 52 span: 4 span: 69 leading_comments: " Accept is a bidirectional streaming RPC used to coordinate forwarded\n connections. The VM sends a ConnectRequest when a container process\n connects to a forwarded socket; the host resolves the forward_id,\n dials the target host socket, opens a vsock stream, and sends back a\n ConnectResult reporting success or failure. On failure the VM closes\n the pending container connection immediately.\n" @@ -1127,7 +1148,7 @@ file: { path: 2 path: 1 path: 1 - span: 51 + span: 52 span: 8 span: 14 } @@ -1137,7 +1158,7 @@ file: { path: 2 path: 1 path: 5 - span: 51 + span: 52 span: 15 span: 21 } @@ -1147,7 +1168,7 @@ file: { path: 2 path: 1 path: 2 - span: 51 + span: 52 span: 22 span: 35 } @@ -1157,7 +1178,7 @@ file: { path: 2 path: 1 path: 6 - span: 51 + span: 52 span: 46 span: 52 } @@ -1167,16 +1188,16 @@ file: { path: 2 path: 1 path: 3 - span: 51 + span: 52 span: 53 span: 67 } location: { path: 4 path: 0 - span: 56 + span: 57 span: 0 - span: 58 + span: 59 span: 1 leading_comments: " BindRequest is sent by the host before container creation to set up socket\n forwards on the VM side.\n" } @@ -1184,7 +1205,7 @@ file: { path: 4 path: 0 path: 1 - span: 56 + span: 57 span: 8 span: 19 } @@ -1194,7 +1215,7 @@ file: { path: 2 path: 0 path: 4 - span: 57 + span: 58 span: 4 span: 12 } @@ -1203,7 +1224,7 @@ file: { path: 0 path: 2 path: 0 - span: 57 + span: 58 span: 4 span: 32 } @@ -1213,7 +1234,7 @@ file: { path: 2 path: 0 path: 6 - span: 57 + span: 58 span: 13 span: 19 } @@ -1223,7 +1244,7 @@ file: { path: 2 path: 0 path: 1 - span: 57 + span: 58 span: 20 span: 27 } @@ -1233,16 +1254,16 @@ file: { path: 2 path: 0 path: 3 - span: 57 + span: 58 span: 30 span: 31 } location: { path: 4 path: 1 - span: 61 + span: 62 span: 0 - span: 70 + span: 76 span: 1 leading_comments: " Socket describes a single forwarded UNIX socket.\n" } @@ -1250,7 +1271,7 @@ file: { path: 4 path: 1 path: 1 - span: 61 + span: 62 span: 8 span: 14 } @@ -1260,7 +1281,7 @@ file: { path: 2 path: 0 path: 5 - span: 64 + span: 65 span: 4 span: 10 } @@ -1269,7 +1290,7 @@ file: { path: 1 path: 2 path: 0 - span: 64 + span: 65 span: 4 span: 26 leading_comments: " Opaque identifier for this forward. Each side uses it to resolve the local\n socket path from its own configuration.\n" @@ -1280,7 +1301,7 @@ file: { path: 2 path: 0 path: 1 - span: 64 + span: 65 span: 11 span: 21 } @@ -1290,7 +1311,7 @@ file: { path: 2 path: 0 path: 3 - span: 64 + span: 65 span: 24 span: 25 } @@ -1300,7 +1321,7 @@ file: { path: 2 path: 1 path: 5 - span: 69 + span: 70 span: 4 span: 10 } @@ -1309,7 +1330,7 @@ file: { path: 1 path: 2 path: 1 - span: 69 + span: 70 span: 4 span: 27 leading_comments: " Path of the UNIX listener socket in the VM's root filesystem (e.g.\n /run/socketfwd/{forward_id}.sock). vminitd creates the socket here;\n the shim has already rewritten the uds mount to a bind mount from\n this path to the user-specified destination inside the container.\n" @@ -1320,7 +1341,7 @@ file: { path: 2 path: 1 path: 1 - span: 69 + span: 70 span: 11 span: 22 } @@ -1330,16 +1351,134 @@ file: { path: 2 path: 1 path: 3 - span: 69 + span: 70 span: 25 span: 26 } location: { path: 4 + path: 1 + path: 2 + path: 2 + path: 5 + span: 73 + span: 4 + span: 10 + } + location: { + path: 4 + path: 1 + path: 2 + path: 2 + span: 73 + span: 4 + span: 20 + leading_comments: " Zero preserves the legacy socket mode and owner. Mode 0600 requests a\n private listener owned by UID and GID in the VM.\n" + } + location: { + path: 4 + path: 1 + path: 2 + path: 2 + path: 1 + span: 73 + span: 11 + span: 15 + } + location: { + path: 4 + path: 1 + path: 2 + path: 2 + path: 3 + span: 73 + span: 18 + span: 19 + } + location: { + path: 4 + path: 1 path: 2 + path: 3 + path: 5 + span: 74 + span: 4 + span: 10 + } + location: { + path: 4 + path: 1 + path: 2 + path: 3 span: 74 + span: 4 + span: 19 + } + location: { + path: 4 + path: 1 + path: 2 + path: 3 + path: 1 + span: 74 + span: 11 + span: 14 + } + location: { + path: 4 + path: 1 + path: 2 + path: 3 + path: 3 + span: 74 + span: 17 + span: 18 + } + location: { + path: 4 + path: 1 + path: 2 + path: 4 + path: 5 + span: 75 + span: 4 + span: 10 + } + location: { + path: 4 + path: 1 + path: 2 + path: 4 + span: 75 + span: 4 + span: 19 + } + location: { + path: 4 + path: 1 + path: 2 + path: 4 + path: 1 + span: 75 + span: 11 + span: 14 + } + location: { + path: 4 + path: 1 + path: 2 + path: 4 + path: 3 + span: 75 + span: 17 + span: 18 + } + location: { + path: 4 + path: 2 + span: 80 span: 0 - span: 82 + span: 88 span: 1 leading_comments: " ConnectRequest is sent by the VM on the Accept stream to notify the host of\n a new container-initiated connection.\n" } @@ -1347,7 +1486,7 @@ file: { path: 4 path: 2 path: 1 - span: 74 + span: 80 span: 8 span: 22 } @@ -1357,7 +1496,7 @@ file: { path: 2 path: 0 path: 5 - span: 77 + span: 83 span: 4 span: 10 } @@ -1366,7 +1505,7 @@ file: { path: 2 path: 2 path: 0 - span: 77 + span: 83 span: 4 span: 25 leading_comments: " ID of the vsock stream. The host must open a stream with this ID after\n a successful ConnectResult so that the relay can start.\n" @@ -1377,7 +1516,7 @@ file: { path: 2 path: 0 path: 1 - span: 77 + span: 83 span: 11 span: 20 } @@ -1387,7 +1526,7 @@ file: { path: 2 path: 0 path: 3 - span: 77 + span: 83 span: 23 span: 24 } @@ -1397,7 +1536,7 @@ file: { path: 2 path: 1 path: 5 - span: 81 + span: 87 span: 4 span: 10 } @@ -1406,7 +1545,7 @@ file: { path: 2 path: 2 path: 1 - span: 81 + span: 87 span: 4 span: 26 leading_comments: " Identifier of the socket forward entry. The host uses this to resolve the\n target host socket path from its own configuration rather than trusting a\n path supplied by the VM.\n" @@ -1417,7 +1556,7 @@ file: { path: 2 path: 1 path: 1 - span: 81 + span: 87 span: 11 span: 21 } @@ -1427,16 +1566,16 @@ file: { path: 2 path: 1 path: 3 - span: 81 + span: 87 span: 24 span: 25 } location: { path: 4 path: 3 - span: 88 - span: 0 span: 94 + span: 0 + span: 100 span: 1 leading_comments: " ConnectResult is sent by the host on the Accept stream in response to each\n ConnectRequest. It reports whether the host successfully dialed the target\n socket. On failure the VM closes the pending container connection\n immediately.\n" } @@ -1444,7 +1583,7 @@ file: { path: 4 path: 3 path: 1 - span: 88 + span: 94 span: 8 span: 21 } @@ -1454,7 +1593,7 @@ file: { path: 2 path: 0 path: 5 - span: 90 + span: 96 span: 4 span: 10 } @@ -1463,7 +1602,7 @@ file: { path: 3 path: 2 path: 0 - span: 90 + span: 96 span: 4 span: 25 leading_comments: " ID of the vsock stream from the corresponding ConnectRequest.\n" @@ -1474,7 +1613,7 @@ file: { path: 2 path: 0 path: 1 - span: 90 + span: 96 span: 11 span: 20 } @@ -1484,7 +1623,7 @@ file: { path: 2 path: 0 path: 3 - span: 90 + span: 96 span: 23 span: 24 } @@ -1494,7 +1633,7 @@ file: { path: 2 path: 1 path: 5 - span: 93 + span: 99 span: 4 span: 10 } @@ -1503,7 +1642,7 @@ file: { path: 3 path: 2 path: 1 - span: 93 + span: 99 span: 4 span: 21 leading_comments: " Non-empty if the host failed to dial the target socket. The VM closes\n the pending connection when error is set.\n" @@ -1514,7 +1653,7 @@ file: { path: 2 path: 1 path: 1 - span: 93 + span: 99 span: 11 span: 16 } @@ -1524,7 +1663,7 @@ file: { path: 2 path: 1 path: 3 - span: 93 + span: 99 span: 19 span: 20 } diff --git a/api/proto/nerdbox/services/socketforward/v1/socketforward.proto b/api/proto/nerdbox/services/socketforward/v1/socketforward.proto index b8f373e9..93ff9e01 100644 --- a/api/proto/nerdbox/services/socketforward/v1/socketforward.proto +++ b/api/proto/nerdbox/services/socketforward/v1/socketforward.proto @@ -40,7 +40,8 @@ service SocketForward { // Bind sets up the socket forward entries on the VM side. The VM creates // UNIX listener sockets at the paths given in socket_path. This RPC returns // only after all socket files have been created, so the caller can safely - // proceed with container creation (crun bind mounts). + // proceed with container creation (crun bind mounts). The shim and guest + // versions are coupled, including the private socket mode contract. rpc Bind(BindRequest) returns (google.protobuf.Empty); // Accept is a bidirectional streaming RPC used to coordinate forwarded @@ -68,6 +69,11 @@ message Socket { // the shim has already rewritten the uds mount to a bind mount from // this path to the user-specified destination inside the container. string socket_path = 2; + // Zero preserves the legacy socket mode and owner. Mode 0600 requests a + // private listener owned by UID and GID in the VM. + uint32 mode = 3; + uint32 uid = 4; + uint32 gid = 5; } // ConnectRequest is sent by the VM on the Accept stream to notify the host of diff --git a/api/services/socketforward/v1/socketforward.pb.go b/api/services/socketforward/v1/socketforward.pb.go index 5a9fc66a..f1efc1c7 100644 --- a/api/services/socketforward/v1/socketforward.pb.go +++ b/api/services/socketforward/v1/socketforward.pb.go @@ -99,6 +99,11 @@ type Socket struct { // the shim has already rewritten the uds mount to a bind mount from // this path to the user-specified destination inside the container. SocketPath string `protobuf:"bytes,2,opt,name=socket_path,json=socketPath,proto3" json:"socket_path,omitempty"` + // Zero preserves the legacy socket mode and owner. Mode 0600 requests a + // private listener owned by UID and GID in the VM. + Mode uint32 `protobuf:"varint,3,opt,name=mode,proto3" json:"mode,omitempty"` + Uid uint32 `protobuf:"varint,4,opt,name=uid,proto3" json:"uid,omitempty"` + Gid uint32 `protobuf:"varint,5,opt,name=gid,proto3" json:"gid,omitempty"` } func (x *Socket) Reset() { @@ -147,6 +152,27 @@ func (x *Socket) GetSocketPath() string { return "" } +func (x *Socket) GetMode() uint32 { + if x != nil { + return x.Mode + } + return 0 +} + +func (x *Socket) GetUid() uint32 { + if x != nil { + return x.Uid + } + return 0 +} + +func (x *Socket) GetGid() uint32 { + if x != nil { + return x.Gid + } + return 0 +} + // ConnectRequest is sent by the VM on the Accept stream to notify the host of // a new container-initiated connection. type ConnectRequest struct { @@ -287,39 +313,42 @@ var file_proto_nerdbox_services_socketforward_v1_socketforward_proto_rawDesc = [ 0x6e, 0x65, 0x72, 0x64, 0x62, 0x6f, 0x78, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, 0x2e, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x2e, 0x76, 0x31, 0x2e, 0x53, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x52, 0x07, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, - 0x73, 0x22, 0x48, 0x0a, 0x06, 0x53, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x12, 0x1d, 0x0a, 0x0a, 0x66, - 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x5f, 0x69, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, - 0x09, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x49, 0x64, 0x12, 0x1f, 0x0a, 0x0b, 0x73, 0x6f, - 0x63, 0x6b, 0x65, 0x74, 0x5f, 0x70, 0x61, 0x74, 0x68, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, - 0x0a, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x50, 0x61, 0x74, 0x68, 0x22, 0x4c, 0x0a, 0x0e, 0x43, - 0x6f, 0x6e, 0x6e, 0x65, 0x63, 0x74, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x1b, 0x0a, - 0x09, 0x73, 0x74, 0x72, 0x65, 0x61, 0x6d, 0x5f, 0x69, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, - 0x52, 0x08, 0x73, 0x74, 0x72, 0x65, 0x61, 0x6d, 0x49, 0x64, 0x12, 0x1d, 0x0a, 0x0a, 0x66, 0x6f, - 0x72, 0x77, 0x61, 0x72, 0x64, 0x5f, 0x69, 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x09, - 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x49, 0x64, 0x22, 0x42, 0x0a, 0x0d, 0x43, 0x6f, 0x6e, - 0x6e, 0x65, 0x63, 0x74, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x12, 0x1b, 0x0a, 0x09, 0x73, 0x74, - 0x72, 0x65, 0x61, 0x6d, 0x5f, 0x69, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, 0x73, - 0x74, 0x72, 0x65, 0x61, 0x6d, 0x49, 0x64, 0x12, 0x14, 0x0a, 0x05, 0x65, 0x72, 0x72, 0x6f, 0x72, - 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x05, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x32, 0xd2, 0x01, - 0x0a, 0x0d, 0x53, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x46, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x12, - 0x4e, 0x0a, 0x04, 0x42, 0x69, 0x6e, 0x64, 0x12, 0x2e, 0x2e, 0x6e, 0x65, 0x72, 0x64, 0x62, 0x6f, - 0x78, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, 0x2e, 0x73, 0x6f, 0x63, 0x6b, 0x65, - 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x2e, 0x76, 0x31, 0x2e, 0x42, 0x69, 0x6e, 0x64, - 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, - 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x45, 0x6d, 0x70, 0x74, 0x79, 0x12, - 0x71, 0x0a, 0x06, 0x41, 0x63, 0x63, 0x65, 0x70, 0x74, 0x12, 0x30, 0x2e, 0x6e, 0x65, 0x72, 0x64, - 0x62, 0x6f, 0x78, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, 0x2e, 0x73, 0x6f, 0x63, - 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x2e, 0x76, 0x31, 0x2e, 0x43, 0x6f, - 0x6e, 0x6e, 0x65, 0x63, 0x74, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x1a, 0x31, 0x2e, 0x6e, 0x65, - 0x72, 0x64, 0x62, 0x6f, 0x78, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, 0x2e, 0x73, - 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x2e, 0x76, 0x31, 0x2e, - 0x43, 0x6f, 0x6e, 0x6e, 0x65, 0x63, 0x74, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x28, 0x01, - 0x30, 0x01, 0x42, 0x4b, 0x5a, 0x49, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, - 0x2f, 0x63, 0x6f, 0x6e, 0x74, 0x61, 0x69, 0x6e, 0x65, 0x72, 0x64, 0x2f, 0x6e, 0x65, 0x72, 0x64, - 0x62, 0x6f, 0x78, 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, - 0x2f, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x2f, 0x76, - 0x31, 0x3b, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x62, - 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x73, 0x22, 0x80, 0x01, 0x0a, 0x06, 0x53, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x12, 0x1d, 0x0a, 0x0a, + 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x5f, 0x69, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, + 0x52, 0x09, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x49, 0x64, 0x12, 0x1f, 0x0a, 0x0b, 0x73, + 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x5f, 0x70, 0x61, 0x74, 0x68, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, + 0x52, 0x0a, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x50, 0x61, 0x74, 0x68, 0x12, 0x12, 0x0a, 0x04, + 0x6d, 0x6f, 0x64, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x0d, 0x52, 0x04, 0x6d, 0x6f, 0x64, 0x65, + 0x12, 0x10, 0x0a, 0x03, 0x75, 0x69, 0x64, 0x18, 0x04, 0x20, 0x01, 0x28, 0x0d, 0x52, 0x03, 0x75, + 0x69, 0x64, 0x12, 0x10, 0x0a, 0x03, 0x67, 0x69, 0x64, 0x18, 0x05, 0x20, 0x01, 0x28, 0x0d, 0x52, + 0x03, 0x67, 0x69, 0x64, 0x22, 0x4c, 0x0a, 0x0e, 0x43, 0x6f, 0x6e, 0x6e, 0x65, 0x63, 0x74, 0x52, + 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x1b, 0x0a, 0x09, 0x73, 0x74, 0x72, 0x65, 0x61, 0x6d, + 0x5f, 0x69, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, 0x73, 0x74, 0x72, 0x65, 0x61, + 0x6d, 0x49, 0x64, 0x12, 0x1d, 0x0a, 0x0a, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x5f, 0x69, + 0x64, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x09, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, + 0x49, 0x64, 0x22, 0x42, 0x0a, 0x0d, 0x43, 0x6f, 0x6e, 0x6e, 0x65, 0x63, 0x74, 0x52, 0x65, 0x73, + 0x75, 0x6c, 0x74, 0x12, 0x1b, 0x0a, 0x09, 0x73, 0x74, 0x72, 0x65, 0x61, 0x6d, 0x5f, 0x69, 0x64, + 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, 0x73, 0x74, 0x72, 0x65, 0x61, 0x6d, 0x49, 0x64, + 0x12, 0x14, 0x0a, 0x05, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, + 0x05, 0x65, 0x72, 0x72, 0x6f, 0x72, 0x32, 0xd2, 0x01, 0x0a, 0x0d, 0x53, 0x6f, 0x63, 0x6b, 0x65, + 0x74, 0x46, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x12, 0x4e, 0x0a, 0x04, 0x42, 0x69, 0x6e, 0x64, + 0x12, 0x2e, 0x2e, 0x6e, 0x65, 0x72, 0x64, 0x62, 0x6f, 0x78, 0x2e, 0x73, 0x65, 0x72, 0x76, 0x69, + 0x63, 0x65, 0x73, 0x2e, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, + 0x64, 0x2e, 0x76, 0x31, 0x2e, 0x42, 0x69, 0x6e, 0x64, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, + 0x1a, 0x16, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, + 0x75, 0x66, 0x2e, 0x45, 0x6d, 0x70, 0x74, 0x79, 0x12, 0x71, 0x0a, 0x06, 0x41, 0x63, 0x63, 0x65, + 0x70, 0x74, 0x12, 0x30, 0x2e, 0x6e, 0x65, 0x72, 0x64, 0x62, 0x6f, 0x78, 0x2e, 0x73, 0x65, 0x72, + 0x76, 0x69, 0x63, 0x65, 0x73, 0x2e, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, 0x72, 0x77, + 0x61, 0x72, 0x64, 0x2e, 0x76, 0x31, 0x2e, 0x43, 0x6f, 0x6e, 0x6e, 0x65, 0x63, 0x74, 0x52, 0x65, + 0x73, 0x75, 0x6c, 0x74, 0x1a, 0x31, 0x2e, 0x6e, 0x65, 0x72, 0x64, 0x62, 0x6f, 0x78, 0x2e, 0x73, + 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, 0x2e, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, 0x66, 0x6f, + 0x72, 0x77, 0x61, 0x72, 0x64, 0x2e, 0x76, 0x31, 0x2e, 0x43, 0x6f, 0x6e, 0x6e, 0x65, 0x63, 0x74, + 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x28, 0x01, 0x30, 0x01, 0x42, 0x4b, 0x5a, 0x49, 0x67, + 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x63, 0x6f, 0x6e, 0x74, 0x61, 0x69, + 0x6e, 0x65, 0x72, 0x64, 0x2f, 0x6e, 0x65, 0x72, 0x64, 0x62, 0x6f, 0x78, 0x2f, 0x61, 0x70, 0x69, + 0x2f, 0x73, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x73, 0x2f, 0x73, 0x6f, 0x63, 0x6b, 0x65, 0x74, + 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x2f, 0x76, 0x31, 0x3b, 0x73, 0x6f, 0x63, 0x6b, 0x65, + 0x74, 0x66, 0x6f, 0x72, 0x77, 0x61, 0x72, 0x64, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/internal/shim/task/socketforward.go b/internal/shim/task/socketforward.go index 9d793cf6..4039c785 100644 --- a/internal/shim/task/socketforward.go +++ b/internal/shim/task/socketforward.go @@ -23,6 +23,7 @@ import ( "fmt" "io" "net" + "strconv" "strings" "github.com/containerd/log" @@ -55,6 +56,9 @@ type socketForwardEntry struct { // containerPath is the user-specified destination: where the socket // appears inside the container rootfs. containerPath string + mode uint32 + uid uint32 + gid uint32 } // socketForwardsProvider parses OCI mounts with type "uds" and generates the @@ -72,7 +76,8 @@ type socketForwardsProvider struct { // UDS mount fields: // - Source: host-side UNIX socket path (the target service) // - Destination: container-side UNIX socket path -// - Options: none +// - Options: none, or uid=,gid= to create a +// private (0600) guest listener owned by that user and group // // Each UDS mount is replaced in-place with a bind mount whose source is // vmPath (/run/socketfwd/{id}.sock) and whose destination is the original @@ -115,8 +120,27 @@ func parseUDSMount(containerID string, m specs.Mount) (socketForwardEntry, error if m.Destination == "" { return socketForwardEntry{}, fmt.Errorf("destination (container path) is required") } - if len(m.Options) > 0 { - return socketForwardEntry{}, fmt.Errorf("unknown option %q", strings.Join(m.Options, ", ")) + var mode uint32 + var uid, gid uint32 + if len(m.Options) != 0 { + if len(m.Options) != 2 || !strings.HasPrefix(m.Options[0], "uid=") || !strings.HasPrefix(m.Options[1], "gid=") { + return socketForwardEntry{}, fmt.Errorf("unknown option %q", strings.Join(m.Options, ", ")) + } + parsedUID, err := strconv.ParseUint(strings.TrimPrefix(m.Options[0], "uid="), 10, 32) + if err != nil { + return socketForwardEntry{}, fmt.Errorf("invalid guest socket UID: %w", err) + } + if parsedUID == uint64(^uint32(0)) { + return socketForwardEntry{}, fmt.Errorf("invalid guest socket UID: reserved chown no-change value") + } + parsedGID, err := strconv.ParseUint(strings.TrimPrefix(m.Options[1], "gid="), 10, 32) + if err != nil { + return socketForwardEntry{}, fmt.Errorf("invalid guest socket GID: %w", err) + } + if parsedGID == uint64(^uint32(0)) { + return socketForwardEntry{}, fmt.Errorf("invalid guest socket GID: reserved chown no-change value") + } + mode, uid, gid = 0600, uint32(parsedUID), uint32(parsedGID) } hash := sha256.Sum256([]byte(containerID + ":" + m.Destination)) @@ -125,6 +149,9 @@ func parseUDSMount(containerID string, m specs.Mount) (socketForwardEntry, error hostPath: m.Source, vmPath: fmt.Sprintf("/run/socketfwd/%x.sock", hash), containerPath: m.Destination, + mode: mode, + uid: uid, + gid: gid, }, nil } @@ -146,13 +173,12 @@ func bindSockets(ctx context.Context, sb sandbox.Sandbox, entries []socketForwar sockets = append(sockets, &socketforward.Socket{ ForwardID: e.id, SocketPath: e.vmPath, + Mode: e.mode, + Uid: e.uid, + Gid: e.gid, }) } - - sfClient := socketforward.NewTTRPCSocketForwardClient(vmc) - if _, err := sfClient.Bind(ctx, &socketforward.BindRequest{ - Sockets: sockets, - }); err != nil { + if _, err := socketforward.NewTTRPCSocketForwardClient(vmc).Bind(ctx, &socketforward.BindRequest{Sockets: sockets}); err != nil { return fmt.Errorf("bind RPC: %w", err) } return nil diff --git a/internal/shim/task/socketforward_bind_test.go b/internal/shim/task/socketforward_bind_test.go new file mode 100644 index 00000000..269462a9 --- /dev/null +++ b/internal/shim/task/socketforward_bind_test.go @@ -0,0 +1,106 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package task + +import ( + "context" + "errors" + "net" + "testing" + + "github.com/containerd/ttrpc" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/emptypb" + + socketforward "github.com/containerd/nerdbox/api/services/socketforward/v1" + "github.com/containerd/nerdbox/internal/shim/sandbox" +) + +type socketForwardTestSandbox struct { + sandbox.Sandbox + client *ttrpc.Client +} + +func (s socketForwardTestSandbox) Client() (*ttrpc.Client, error) { return s.client, nil } + +func TestBindSocketsPassesRequestedMode(t *testing.T) { + for _, tc := range []struct { + name string + mode uint32 + }{ + {name: "legacy"}, + {name: "private", mode: 0600}, + } { + t.Run(tc.name, func(t *testing.T) { + server, err := ttrpc.NewServer() + require.NoError(t, err) + bindRequests := make(chan *socketforward.BindRequest, 1) + server.Register("nerdbox.services.socketforward.v1.SocketForward", map[string]ttrpc.Method{ + "Bind": func(_ context.Context, unmarshal func(interface{}) error) (interface{}, error) { + var req socketforward.BindRequest + if err := unmarshal(&req); err != nil { + return nil, err + } + bindRequests <- &req + return &emptypb.Empty{}, nil + }, + }) + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + go func() { _ = server.Serve(t.Context(), listener) }() + conn, err := net.Dial("tcp", listener.Addr().String()) + require.NoError(t, err) + client := ttrpc.NewClient(conn) + t.Cleanup(func() { + require.NoError(t, client.Close()) + require.NoError(t, server.Close()) + }) + + err = bindSockets(t.Context(), socketForwardTestSandbox{client: client}, []socketForwardEntry{{ + id: "test", vmPath: "/run/socketfwd/test.sock", mode: tc.mode, + uid: 1000, gid: 1000, + }}) + require.NoError(t, err) + req := <-bindRequests + require.Len(t, req.Sockets, 1) + require.Equal(t, tc.mode, req.Sockets[0].Mode) + require.Equal(t, uint32(1000), req.Sockets[0].Uid) + require.Equal(t, uint32(1000), req.Sockets[0].Gid) + }) + } +} + +func TestBindSocketsRejectsGuestFailure(t *testing.T) { + server, err := ttrpc.NewServer() + require.NoError(t, err) + server.Register("nerdbox.services.socketforward.v1.SocketForward", map[string]ttrpc.Method{ + "Bind": func(context.Context, func(interface{}) error) (interface{}, error) { + return nil, errors.New("guest rejected mode") + }, + }) + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + go func() { _ = server.Serve(t.Context(), listener) }() + conn, err := net.Dial("tcp", listener.Addr().String()) + require.NoError(t, err) + client := ttrpc.NewClient(conn) + t.Cleanup(func() { + require.NoError(t, client.Close()) + require.NoError(t, server.Close()) + }) + require.ErrorContains(t, bindSockets(t.Context(), socketForwardTestSandbox{client: client}, []socketForwardEntry{{mode: 0600}}), "guest rejected mode") +} diff --git a/internal/shim/task/socketforward_test.go b/internal/shim/task/socketforward_test.go index d01c1042..25d60311 100644 --- a/internal/shim/task/socketforward_test.go +++ b/internal/shim/task/socketforward_test.go @@ -70,6 +70,81 @@ func TestSocketForwardsProviderFromBundle(t *testing.T) { }, wantErr: `unknown option "first, second"`, }, + { + name: "invalid guest UID", + cid: "c1", + mounts: []specs.Mount{{ + Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=-1", "gid=1000"}, + }}, + wantErr: "invalid guest socket UID", + }, + { + name: "missing guest GID", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=1000"}}}, + wantErr: "unknown option", + }, + { + name: "reordered guest options", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"gid=1000", "uid=1000"}}}, + wantErr: "unknown option", + }, + { + name: "overflow guest UID", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=4294967296", "gid=1000"}}}, + wantErr: "invalid guest socket UID", + }, + { + name: "reserved guest UID", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=4294967295", "gid=1000"}}}, + wantErr: "reserved chown no-change value", + }, + { + name: "reserved guest GID", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=1000", "gid=4294967295"}}}, + wantErr: "reserved chown no-change value", + }, + { + name: "invalid guest GID", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=1000", "gid=-1"}}}, + wantErr: "invalid guest socket GID", + }, + { + name: "extra guest option", + cid: "c1", + mounts: []specs.Mount{{Type: "uds", Source: "/host/sock", Destination: "/guest/sock", + Options: []string{"uid=1000", "gid=1000", "mode=0666"}}}, + wantErr: "unknown option", + }, + { + name: "private guest owner", + cid: "container-abc", + mounts: []specs.Mount{{ + Type: "uds", Source: "/host/sock", Destination: "/var/run/bar.sock", + Options: []string{"uid=1000", "gid=1000"}, + }}, + wantMounts: []specs.Mount{{ + Type: "bind", Source: "/run/socketfwd/954a6df32e91bb55e6fcd9df9f90728e56b4f87aa92b22fe3b63df33f18a3188.sock", + Destination: "/var/run/bar.sock", Options: []string{"bind"}, + }}, + wantEntries: []socketForwardEntry{{ + id: "954a6df32e91bb55e6fcd9df9f90728e56b4f87aa92b22fe3b63df33f18a3188", + hostPath: "/host/sock", vmPath: "/run/socketfwd/954a6df32e91bb55e6fcd9df9f90728e56b4f87aa92b22fe3b63df33f18a3188.sock", + containerPath: "/var/run/bar.sock", mode: 0600, uid: 1000, gid: 1000, + }}, + }, { name: "uds mount rewritten to bind", cid: "container-abc", diff --git a/internal/vminit/socketforward/socketforward.go b/internal/vminit/socketforward/socketforward.go index c6b64775..08ea8559 100644 --- a/internal/vminit/socketforward/socketforward.go +++ b/internal/vminit/socketforward/socketforward.go @@ -89,11 +89,60 @@ func (s *Service) RegisterTTRPC(server *ttrpc.Server) error { // creates a UNIX listener socket at the given socket_path. This method // returns only after all socket files have been created. func (s *Service) Bind(ctx context.Context, req *socketforward.BindRequest) (*emptypb.Empty, error) { + return s.bindAll(ctx, req) +} + +func (s *Service) bindAll(ctx context.Context, req *socketforward.BindRequest) (*emptypb.Empty, error) { s.mu.Lock() defer s.mu.Unlock() + // Never unlink a live listener, even if the duplicate appears later in + // this request. Preflight before creating anything so a duplicate cannot + // tear down another container's forward during rollback. + paths := make(map[string]struct{}, len(s.listeners)+len(req.Sockets)) + for _, l := range s.listeners { + path, err := canonicalSocketPath(l.Addr().String()) + if err != nil { + return nil, fmt.Errorf("resolving bound socket path: %w", err) + } + paths[path] = struct{}{} + } + for _, sock := range req.Sockets { + if sock == nil { + return nil, fmt.Errorf("nil socket forward entry") + } + // Cleaning a path containing ".." before following a symlink can + // change which socket it names. Reject lexical aliases rather than + // trying to infer their target from the cleaned path. + if filepath.Clean(sock.SocketPath) != sock.SocketPath { + return nil, fmt.Errorf("non-canonical socket forward path %q", sock.SocketPath) + } + // Linux chown interprets UINT32_MAX as "leave unchanged". Reject it + // before binding any socket so the permission-aware RPC cannot report + // success without setting the requested owner. + if sock.Mode != 0 && sock.Mode != 0600 { + return nil, fmt.Errorf("unsupported guest socket mode %#o for %s", sock.Mode, sock.SocketPath) + } + if sock.Mode != 0 && (sock.Uid == ^uint32(0) || sock.Gid == ^uint32(0)) { + return nil, fmt.Errorf("reserved guest socket UID or GID for %s", sock.SocketPath) + } + path, err := canonicalSocketPath(sock.SocketPath) + if err != nil { + return nil, fmt.Errorf("resolving socket path %s: %w", sock.SocketPath, err) + } + if _, ok := paths[path]; ok { + return nil, fmt.Errorf("socket forward listener already bound at %s", sock.SocketPath) + } + paths[path] = struct{}{} + } + + initial := len(s.listeners) for _, sock := range req.Sockets { - if err := s.bind(ctx, sock.ForwardID, sock.SocketPath); err != nil { + if err := s.bind(ctx, sock); err != nil { + for _, l := range s.listeners[initial:] { + _ = l.Close() + } + s.listeners = s.listeners[:initial] return nil, fmt.Errorf("binding socket forward listener at %s: %w", sock.SocketPath, err) } } @@ -101,6 +150,36 @@ func (s *Service) Bind(ctx context.Context, req *socketforward.BindRequest) (*em return &emptypb.Empty{}, nil } +// canonicalSocketPath resolves aliases in the parent directory without +// following the socket itself. Some parents may not exist until bind creates +// them, so resolve the nearest existing ancestor and append the missing tail. +func canonicalSocketPath(path string) (string, error) { + abs, err := filepath.Abs(path) + if err != nil { + return "", err + } + dir := filepath.Dir(abs) + var missing []string + for { + resolved, err := filepath.EvalSymlinks(dir) + if err == nil { + for i := len(missing) - 1; i >= 0; i-- { + resolved = filepath.Join(resolved, missing[i]) + } + return filepath.Join(resolved, filepath.Base(abs)), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + parent := filepath.Dir(dir) + if parent == dir { + return "", err + } + missing = append(missing, filepath.Base(dir)) + dir = parent + } +} + // Accept is a bidirectional streaming RPC used to coordinate forwarded // connections. The VM sends a ConnectRequest when a container process // connects to a forwarded socket; the host resolves the forward_id, @@ -152,19 +231,25 @@ func (s *Service) Accept(ctx context.Context, srv socketforward.TTRPCSocketForwa // it generates a stream ID, sends a ConnectRequest on the notify channel // (for the Accept stream), then waits for the host to report the dial // result via a ConnectResult before proceeding with the vsock relay. -func (s *Service) bind(ctx context.Context, forwardID, socketPath string) error { +func (s *Service) bind(ctx context.Context, sock *socketforward.Socket) error { + forwardID, socketPath := sock.ForwardID, sock.SocketPath if err := os.MkdirAll(filepath.Dir(socketPath), 0755); err != nil { return fmt.Errorf("creating parent directory for %s: %w", socketPath, err) } - // Remove any stale socket file. - if err := os.Remove(socketPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("removing stale socket file %s: %w", socketPath, err) + var l net.Listener + var err error + if sock.Mode == 0600 { + l, err = listenPrivate(socketPath, sock.Uid, sock.Gid) + } else { + // Keep legacy socket behavior unchanged when no ownership is requested. + if err = os.Remove(socketPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("removing stale socket file %s: %w", socketPath, err) + } + l, err = net.Listen("unix", socketPath) } - - l, err := net.Listen("unix", socketPath) if err != nil { - return fmt.Errorf("listening on %s: %w", socketPath, err) + return err } s.listeners = append(s.listeners, l) @@ -178,6 +263,71 @@ func (s *Service) bind(ctx context.Context, forwardID, socketPath string) error return nil } +// listenPrivate prepares the socket behind a root-owned, 0700 staging +// directory. No unprivileged guest process can connect before its owner and +// 0600 mode are set, even if the VM process has a permissive umask. +func listenPrivate(socketPath string, uid, gid uint32) (net.Listener, error) { + stagingDir, err := os.MkdirTemp(filepath.Dir(socketPath), ".socketfwd-") + if err != nil { + return nil, fmt.Errorf("creating private socket staging directory: %w", err) + } + defer os.Remove(stagingDir) // empty after publication or closing on failure + stagedPath := filepath.Join(stagingDir, "socket") + l, err := net.Listen("unix", stagedPath) + if err != nil { + return nil, fmt.Errorf("listening on private socket: %w", err) + } + published := false + defer func() { + if !published { + _ = l.Close() // unlinks the unpublished socket + } + }() + if err := os.Chown(stagedPath, int(uid), int(gid)); err != nil { + return nil, fmt.Errorf("setting socket owner: %w", err) + } + if err := os.Chmod(stagedPath, 0600); err != nil { + return nil, fmt.Errorf("setting socket mode: %w", err) + } + identity, err := os.Stat(stagedPath) + if err != nil { + return nil, fmt.Errorf("stat private socket: %w", err) + } + if err := os.Rename(stagedPath, socketPath); err != nil { + return nil, fmt.Errorf("publishing private socket: %w", err) + } + l.(*net.UnixListener).SetUnlinkOnClose(false) + published = true + return &privateListener{Listener: l, path: socketPath, identity: identity}, nil +} + +// privateListener owns the published path: net.UnixListener only knows the +// staging path and must not leave the renamed socket behind on Close. +type privateListener struct { + net.Listener + path string + identity os.FileInfo +} + +func (l *privateListener) Addr() net.Addr { + return &net.UnixAddr{Name: l.path, Net: "unix"} +} + +func (l *privateListener) Close() error { + closeErr := l.Listener.Close() + info, err := os.Stat(l.path) + if os.IsNotExist(err) { + return closeErr + } + if err != nil { + return errors.Join(closeErr, err) + } + if os.SameFile(info, l.identity) { + return errors.Join(closeErr, os.Remove(l.path)) + } + return closeErr +} + func (s *Service) acceptLoop(ctx context.Context, l net.Listener, forwardID string) { for { conn, err := l.Accept() diff --git a/internal/vminit/socketforward/socketforward_test.go b/internal/vminit/socketforward/socketforward_test.go new file mode 100644 index 00000000..abcea67c --- /dev/null +++ b/internal/vminit/socketforward/socketforward_test.go @@ -0,0 +1,315 @@ +//go:build linux + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package socketforward + +import ( + "fmt" + "net" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "github.com/stretchr/testify/require" + + socketforward "github.com/containerd/nerdbox/api/services/socketforward/v1" +) + +func TestBind(t *testing.T) { + dir, err := os.MkdirTemp("", "sf-") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, os.RemoveAll(dir)) }) + path := filepath.Join(dir, "agent.sock") + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "test", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), + }}}) + require.NoError(t, err) + info, err := os.Stat(path) + require.NoError(t, err) + require.Equal(t, os.FileMode(0600), info.Mode().Perm()) + require.Equal(t, uint32(os.Getuid()), info.Sys().(*syscall.Stat_t).Uid) + require.Equal(t, uint32(os.Getgid()), info.Sys().(*syscall.Stat_t).Gid) + require.NoError(t, assertNoStagingDirectories(dir)) +} + +func TestBindRejectsReservedOwner(t *testing.T) { + for _, tc := range []struct { + name string + uid uint32 + gid uint32 + }{ + {name: "UID", uid: ^uint32(0), gid: 1000}, + {name: "GID", uid: 1000, gid: ^uint32(0)}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + firstPath := filepath.Join(dir, "first.sock") + reservedPath := filepath.Join(dir, "reserved.sock") + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err := s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{ + {ForwardID: "first", SocketPath: firstPath}, + {ForwardID: "reserved", SocketPath: reservedPath, Mode: 0600, Uid: tc.uid, Gid: tc.gid}, + }}) + require.ErrorContains(t, err, "reserved guest socket UID or GID") + require.Empty(t, s.listeners) + for _, path := range []string{firstPath, reservedPath} { + _, err := os.Stat(path) + require.True(t, os.IsNotExist(err), "failed bind left a socket at %s: %v", path, err) + } + require.NoError(t, assertNoStagingDirectories(dir)) + }) + } +} + +func TestBindRejectsUnsupportedModeBeforeCreatingSockets(t *testing.T) { + dir := t.TempDir() + firstPath := filepath.Join(dir, "first.sock") + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err := s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{ + {ForwardID: "first", SocketPath: firstPath}, + {ForwardID: "unsupported", SocketPath: filepath.Join(dir, "other.sock"), Mode: 0666}, + }}) + require.ErrorContains(t, err, "unsupported guest socket mode") + require.Empty(t, s.listeners) + _, err = os.Lstat(firstPath) + require.True(t, os.IsNotExist(err)) +} + +func assertNoStagingDirectories(dir string) error { + entries, err := os.ReadDir(dir) + if err != nil { + return err + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".socketfwd-") { + return fmt.Errorf("private socket staging directory left behind: %s", entry.Name()) + } + } + return nil +} + +func TestBindRollsBackPartialBind(t *testing.T) { + dir, err := os.MkdirTemp("", "sf-") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, os.RemoveAll(dir)) }) + path := filepath.Join(dir, "agent.sock") + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{ + {ForwardID: "first", SocketPath: path, Mode: 0600, Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid())}, + {ForwardID: "second", SocketPath: "/dev/null/impossible.sock"}, + }}) + require.Error(t, err) + require.Empty(t, s.listeners) + _, err = os.Stat(path) + require.True(t, os.IsNotExist(err), "partially bound socket remained accessible: %v", err) + require.NoError(t, assertNoStagingDirectories(dir)) +} + +func TestShutdownRemovesForwardedSocket(t *testing.T) { + dir, err := os.MkdirTemp("", "sf-") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, os.RemoveAll(dir)) }) + path := filepath.Join(dir, "agent.sock") + s := NewService(nil) + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "test", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), + }}}) + require.NoError(t, err) + _, err = os.Stat(path) + require.NoError(t, err) + require.NoError(t, s.Shutdown(t.Context())) + _, err = os.Stat(path) + require.True(t, os.IsNotExist(err), "closed listener remains visible: %v", err) + require.NoError(t, assertNoStagingDirectories(dir)) +} + +func TestBindWithoutPermissionsKeepsDefaultMode(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "agent.sock") + // The legacy behavior is net.Listen's mode (subject to the process umask), + // not the private 0600 mode requested by an owned socket. + baseline, err := net.Listen("unix", filepath.Join(dir, "baseline.sock")) + require.NoError(t, err) + baselineInfo, err := os.Stat(filepath.Join(dir, "baseline.sock")) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, baseline.Close()) }) + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "test", SocketPath: path, + }}}) + require.NoError(t, err) + info, err := os.Stat(path) + require.NoError(t, err) + require.Equal(t, baselineInfo.Mode().Perm(), info.Mode().Perm()) +} + +func TestBindRejectsDuplicateWithoutUnlinkingLiveListener(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "agent.sock") + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + sock := &socketforward.Socket{ForwardID: "test", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid())} + _, err := s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{sock}}) + require.NoError(t, err) + + // A later bind must not remove the first listener's filesystem entry. + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{sock}}) + require.ErrorContains(t, err, "already bound") + require.Len(t, s.listeners, 1) + conn, err := net.Dial("unix", path) + require.NoError(t, err) + require.NoError(t, conn.Close()) + aliasDir := filepath.Join(dir, "alias") + require.NoError(t, os.Symlink(dir, aliasDir)) + for _, alias := range []struct { + path string + wantErr string + }{ + {path: dir + "/./agent.sock", wantErr: "non-canonical"}, + {path: aliasDir + "/../agent.sock", wantErr: "non-canonical"}, + {path: filepath.Join(aliasDir, "agent.sock"), wantErr: "already bound"}, + } { + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{ + {ForwardID: "new", SocketPath: filepath.Join(dir, "new.sock")}, + {ForwardID: "alias", SocketPath: alias.path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid())}, + }}) + require.ErrorContains(t, err, alias.wantErr) + require.Len(t, s.listeners, 1) + _, err = os.Stat(filepath.Join(dir, "new.sock")) + require.True(t, os.IsNotExist(err)) + conn, err = net.Dial("unix", path) + require.NoError(t, err, "aliased bind disrupted original listener") + require.NoError(t, conn.Close()) + } + + // The same applies when the duplicate is later in a single batch: no + // earlier entry in that batch should be bound or left on disk. + newPath := filepath.Join(dir, "new.sock") + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{ + {ForwardID: "new", SocketPath: newPath}, + {ForwardID: "duplicate", SocketPath: newPath}, + }}) + require.ErrorContains(t, err, "already bound") + require.Len(t, s.listeners, 1) + _, err = os.Stat(newPath) + require.True(t, os.IsNotExist(err)) + conn, err = net.Dial("unix", path) + require.NoError(t, err) + require.NoError(t, conn.Close()) +} + +func TestBindRollbackPreservesPreviouslyBoundListener(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "existing.sock") + newPath := filepath.Join(dir, "new.sock") + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err := s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "existing", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), + }}}) + require.NoError(t, err) + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{ + {ForwardID: "new", SocketPath: newPath}, + {ForwardID: "invalid", SocketPath: "/dev/null/impossible.sock"}, + }}) + require.Error(t, err) + require.Len(t, s.listeners, 1) + _, err = os.Stat(newPath) + require.True(t, os.IsNotExist(err)) + conn, err := net.Dial("unix", path) + require.NoError(t, err) + require.NoError(t, conn.Close()) + require.NoError(t, assertNoStagingDirectories(dir)) +} + +func TestPrivateBindFailureRemovesStagingDirectory(t *testing.T) { + dir := t.TempDir() + // A non-empty directory cannot be replaced by a socket. Publication + // must fail without leaking the protected staging directory or listener. + path := filepath.Join(dir, "occupied") + require.NoError(t, os.Mkdir(path, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(path, "keep"), []byte("data"), 0600)) + s := NewService(nil) + _, err := s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "test", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), + }}}) + require.Error(t, err) + require.Empty(t, s.listeners) + require.NoError(t, assertNoStagingDirectories(dir)) + content, err := os.ReadFile(filepath.Join(path, "keep")) + require.NoError(t, err) + require.Equal(t, "data", string(content)) +} + +func TestPrivateBindReplacesStaleSocket(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "agent.sock") + stale, err := net.Listen("unix", path) + require.NoError(t, err) + stale.(*net.UnixListener).SetUnlinkOnClose(false) + require.NoError(t, stale.Close()) + staleInfo, err := os.Stat(path) + require.NoError(t, err) + + s := NewService(nil) + t.Cleanup(func() { require.NoError(t, s.Shutdown(t.Context())) }) + _, err = s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "replacement", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), + }}}) + require.NoError(t, err) + info, err := os.Stat(path) + require.NoError(t, err) + require.False(t, os.SameFile(staleInfo, info)) + require.Equal(t, os.FileMode(0600), info.Mode().Perm()) + conn, err := net.Dial("unix", path) + require.NoError(t, err) + require.NoError(t, conn.Close()) + require.NoError(t, assertNoStagingDirectories(dir)) +} + +func TestShutdownDoesNotRemoveReplacementAtPrivatePath(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "agent.sock") + s := NewService(nil) + _, err := s.Bind(t.Context(), &socketforward.BindRequest{Sockets: []*socketforward.Socket{{ + ForwardID: "test", SocketPath: path, Mode: 0600, + Uid: uint32(os.Getuid()), Gid: uint32(os.Getgid()), + }}}) + require.NoError(t, err) + require.NoError(t, os.Remove(path)) + require.NoError(t, os.WriteFile(path, []byte("replacement"), 0600)) + require.NoError(t, s.Shutdown(t.Context())) + content, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, "replacement", string(content)) +} diff --git a/plugins/services/system/service_test.go b/plugins/services/system/service_test.go new file mode 100644 index 00000000..fdb665f2 --- /dev/null +++ b/plugins/services/system/service_test.go @@ -0,0 +1,30 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package version + +import ( + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/emptypb" +) + +func TestInfoReturnsVersion(t *testing.T) { + info, err := (&service{}).Info(t.Context(), &emptypb.Empty{}) + require.NoError(t, err) + require.Equal(t, "dev", info.GetVersion()) +}