amadan

carlos / platform Public

unixdial: fold copy-paste acceptance tests, cover abstract and empty-name peers, document the lexical/relative-path edges (fix round 1, task 3)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YZ1ZvWj7naf73GjTX8MBv5
Paul Campbell pushed by paul@keymail.dev ce07753c81b0da20228044acef41abc362a4b8c0 parent 67e40b9
2 files changed, +64 −26
  • internal/unixdial/unixdial.go +10 −0
  • internal/unixdial/unixdial_test.go +54 −26
diff --git a/internal/unixdial/unixdial.go b/internal/unixdial/unixdial.go
index 8b2ae969b..ee89bdc8f 100644
--- a/internal/unixdial/unixdial.go
+++ b/internal/unixdial/unixdial.go
@@ -11,6 +11,16 @@
// or at the control socket therefore yields a mismatch, and the
// connection is closed unused.
//
+// The first check is purely lexical: it filepath.Cleans the dialled
+// string and compares it to the control directory, before any connect.
+// A relative path, or a symlinked parent directory that only resolves to
+// the control directory at connect time, is not caught here — it is
+// still caught by the second check, once getpeername() names the
+// control listener's real bound path after connect. A listener bound by
+// a relative path is refused by the second check for the same reason:
+// the kernel reports the bind string verbatim, and a relative name can
+// never filepath.Clean-equal the absolute path Checked was dialled with.
+//
// Credentials are deliberately not consulted: a listener systemd created
// and passed to a DynamicUser service reports root in SO_PEERCRED, and
// that shape (carlos-app@.socket) must keep working (codex R4-1).
diff --git a/internal/unixdial/unixdial_test.go b/internal/unixdial/unixdial_test.go
index b4ab7b8e8..5100d6bf4 100644
--- a/internal/unixdial/unixdial_test.go
+++ b/internal/unixdial/unixdial_test.go
@@ -3,6 +3,7 @@ package unixdial
import (
"context"
"errors"
+ "fmt"
"net"
"os"
"path/filepath"
@@ -40,14 +41,27 @@ func listen(t *testing.T, path string) net.Listener {
return ln
}
-func TestCheckedAcceptsTheListenerItDialled(t *testing.T) {
- path := short(t, "app.sock")
- listen(t, path)
- c, err := Checked(context.Background(), &net.Dialer{}, path)
- if err != nil {
- t.Fatalf("Checked: %v", err)
+// TestCheckedAcceptsAListenerBoundAtTheDialledPath pins acceptance across a
+// few ordinary path shapes. It is not testing anything Checked branches on
+// by file name: the only path-based refusal is the control directory
+// (codex R5-1, covered by TestCheckedRefusesTheControlEndpointBeforeDialling
+// and the package doc), and the peer check consults no credentials by
+// construction — Checked never looks at SO_PEERCRED, so there is nothing
+// about "who created the listener" for a test to exercise (codex R4-1,
+// also covered by the package doc). These rows exist only to show the
+// ordinary-acceptance path isn't an accident of one particular name.
+func TestCheckedAcceptsAListenerBoundAtTheDialledPath(t *testing.T) {
+ for _, name := range []string{"app.sock", "apps.sock", "example.sock"} {
+ t.Run(name, func(t *testing.T) {
+ path := short(t, name)
+ listen(t, path)
+ c, err := Checked(context.Background(), &net.Dialer{}, path)
+ if err != nil {
+ t.Fatalf("Checked: %v", err)
+ }
+ c.Close()
+ })
}
- c.Close()
}
func TestCheckedRefusesASymlinkToAnotherListener(t *testing.T) {
@@ -75,29 +89,43 @@ func TestCheckedRefusesTheControlEndpointBeforeDialling(t *testing.T) {
}
}
-func TestCheckedAcceptsAListenerCreatedByAnotherProcessShape(t *testing.T) {
- // The carlos-app@.socket shape: the listener's creator is not the
- // process that serves it. Credentials are never consulted, only the
- // bound path, so this passes as long as the path matches.
- path := short(t, "apps.sock")
- listen(t, path)
- c, err := Checked(context.Background(), &net.Dialer{}, path)
- if err != nil {
- t.Fatalf("Checked: %v", err)
+// TestCheckedRefusesAnAbstractPeer dials an abstract-namespace listener
+// (name[0] == '@', no filesystem path, so no symlink attack even applies)
+// directly by its abstract name. An abstract name never equals a cleaned
+// filesystem path, and verifyPeer refuses any peer whose bound name is
+// abstract outright rather than comparing it — abstract sockets carry no
+// filesystem permissions, so an abstract peer is never an acceptable
+// upstream.
+func TestCheckedRefusesAnAbstractPeer(t *testing.T) {
+ name := fmt.Sprintf("@carlos-unixdial-test-%d", os.Getpid())
+ listen(t, name)
+ c, err := Checked(context.Background(), &net.Dialer{}, name)
+ if c != nil {
+ c.Close()
+ }
+ if !errors.Is(err, ErrPeerMismatch) {
+ t.Fatalf("err = %v, want ErrPeerMismatch", err)
}
- c.Close()
}
-func TestCheckedAcceptsACustomPathOutsideRunCarlos(t *testing.T) {
- // codex R5-1: an operator's --socket /run/titogo/example.sock keeps
- // working; only the control endpoint is refused by path.
- path := short(t, "example.sock")
- listen(t, path)
- c, err := Checked(context.Background(), &net.Dialer{}, path)
- if err != nil {
- t.Fatalf("Checked: %v", err)
+// nameOnlyConn implements just enough of net.Conn for verifyPeer, which
+// only ever calls RemoteAddr on the connection it is checking.
+type nameOnlyConn struct {
+ net.Conn
+ remote net.Addr
+}
+
+func (c nameOnlyConn) RemoteAddr() net.Addr { return c.remote }
+
+// TestVerifyPeerRefusesAnEmptyNamePeer covers the unnamed-peer branch
+// directly: an unnamed unix socket (as from socketpair, or a client's own
+// unconnected end) reports an empty Name from getpeername, which must
+// never be treated as matching any dialled path.
+func TestVerifyPeerRefusesAnEmptyNamePeer(t *testing.T) {
+ conn := nameOnlyConn{remote: &net.UnixAddr{Net: "unix", Name: ""}}
+ if err := verifyPeer(conn, "/some/dialled/path.sock"); !errors.Is(err, ErrPeerMismatch) {
+ t.Fatalf("verifyPeer = %v, want ErrPeerMismatch", err)
}
- c.Close()
}
func TestCheckedPassesConnectErrorsThrough(t *testing.T) {