amadan
clamd: dial tcp or unix, decided by the address
A leading slash is the whole rule: a unix socket is always an absolute path and a host never is. Everything else was already transport-independent — the INSTREAM framing, the deadlines, the reply parsing — so this is a dialer change, and the existing protocol tests pass unaltered over a real TCP socket. Ordered before the wiring change deliberately. If the console learned to accept CARLOS_CLAMD_ADDR=host:port first, that address would have been dialled as a unix path for one commit. The TCP case has no authentication, because clamd has none at all: anything that reaches the port can have it scan arbitrary bytes and answer back. So a TCP address belongs on a private interface with the port firewalled to the console, never a public one. That is recorded on New as a hard constraint rather than advice — an earlier draft of the design refused TCP outright for exactly this reason, and the reversal is only safe because the network answers the objection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+102
−12
internal/clamd/clamd.go+31 −12internal/clamd/clamd_test.go+71 −0
diff --git a/internal/clamd/clamd.go b/internal/clamd/clamd.go| index 8b57fe288..a6e51630c 100644 |
| --- a/internal/clamd/clamd.go |
| +++ b/internal/clamd/clamd.go |
| @@ -92,11 +92,12 @@ func (t Timeouts) withDefaults() Timeouts { |
| return t |
| } |
| -// Client talks to one clamd over a unix socket. It holds no connection between |
| -// calls: clamd's INSTREAM is one command per connection, so each call dials, |
| -// scans and hangs up. That also means a Client is safe to share. |
| +// Client talks to one clamd, over a unix socket or TCP. It holds no connection |
| +// between calls: clamd's INSTREAM is one command per connection, so each call |
| +// dials, scans and hangs up. That also means a Client is safe to share. |
| type Client struct { |
| - socket string |
| + network string // "unix" or "tcp" |
| + addr string |
| timeouts Timeouts |
| // conn, when set, is used for exactly one call and then dropped. Tests set |
| @@ -105,10 +106,28 @@ type Client struct { |
| conn net.Conn |
| } |
| -// New returns a Client dialling socket, which is a unix socket path such as |
| -// /run/clamav/clamd.ctl. |
| -func New(socket string, timeouts Timeouts) *Client { |
| - return &Client{socket: socket, timeouts: timeouts.withDefaults()} |
| +// New returns a Client dialling addr — a unix socket path such as |
| +// /run/clamav/clamd.ctl, or a host:port such as scanner.internal:3310. |
| +// |
| +// The two are told apart by a leading slash, and that is the whole rule: a |
| +// unix socket is always an absolute path and a host is never one. |
| +// |
| +// THE TCP CASE HAS NO AUTHENTICATION. clamd has none at all — anything that |
| +// can reach the port can have it scan arbitrary bytes and answer back — so a |
| +// TCP address MUST be on a private interface with the port firewalled to the |
| +// console, never a public one. The design spec records this as a hard |
| +// constraint on the infrastructure, not a recommendation, and it is the reason |
| +// an earlier draft refused TCP outright. |
| +func New(addr string, timeouts Timeouts) *Client { |
| + return &Client{network: networkFor(addr), addr: addr, timeouts: timeouts.withDefaults()} |
| +} |
| + |
| +// networkFor picks the dial network from the shape of the address. |
| +func networkFor(addr string) string { |
| + if strings.HasPrefix(addr, "/") { |
| + return "unix" |
| + } |
| + return "tcp" |
| } |
| // newWithConn returns a Client that will run its next — and only its next — |
| @@ -123,13 +142,13 @@ func (c *Client) dial(ctx context.Context) (net.Conn, error) { |
| c.conn = nil |
| return conn, nil |
| } |
| - if c.socket == "" { |
| - return nil, errors.New("clamd: no socket configured") |
| + if c.addr == "" { |
| + return nil, errors.New("clamd: no address configured") |
| } |
| d := net.Dialer{Timeout: c.timeouts.Dial} |
| - conn, err := d.DialContext(ctx, "unix", c.socket) |
| + conn, err := d.DialContext(ctx, c.network, c.addr) |
| if err != nil { |
| - return nil, fmt.Errorf("clamd: dial %s: %w", c.socket, err) |
| + return nil, fmt.Errorf("clamd: dial %s %s: %w", c.network, c.addr, err) |
| } |
| return conn, nil |
| } |
diff --git a/internal/clamd/clamd_test.go b/internal/clamd/clamd_test.go| index 55fa03fc9..aa88665e8 100644 |
| --- a/internal/clamd/clamd_test.go |
| +++ b/internal/clamd/clamd_test.go |
| @@ -235,3 +235,74 @@ func TestZeroTimeoutsTakeDefaults(t *testing.T) { |
| t.Fatalf("zero timeouts left an infinite wait: %+v", c.timeouts) |
| } |
| } |
| + |
| +// The address decides the network, and a leading slash is the whole rule: a |
| +// unix socket is always an absolute path and a host never is. |
| +func TestNetworkForAddress(t *testing.T) { |
| + for addr, want := range map[string]string{ |
| + "/run/clamav/clamd.ctl": "unix", |
| + "/tmp/clamd.sock": "unix", |
| + "scanner.internal:3310": "tcp", |
| + "127.0.0.1:3310": "tcp", |
| + "[::1]:3310": "tcp", |
| + } { |
| + if got := networkFor(addr); got != want { |
| + t.Errorf("networkFor(%q) = %q, want %q", addr, got, want) |
| + } |
| + } |
| +} |
| + |
| +// A TCP client dials TCP and speaks the same protocol. The framing, deadlines |
| +// and reply parsing are transport-independent, so this asserts the dial and |
| +// then the whole INSTREAM exchange over a real socket. |
| +func TestScanStreamOverTCP(t *testing.T) { |
| + ln, err := net.Listen("tcp", "127.0.0.1:0") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer ln.Close() |
| + |
| + got := make(chan []byte, 1) |
| + go func() { |
| + conn, err := ln.Accept() |
| + if err != nil { |
| + got <- nil |
| + return |
| + } |
| + defer conn.Close() |
| + _ = conn.SetDeadline(time.Now().Add(fakeDeadline)) |
| + br := bufio.NewReader(conn) |
| + if _, err := br.ReadString(0); err != nil { |
| + got <- nil |
| + return |
| + } |
| + payload, _ := io.ReadAll(&frameReader{r: br, cmdRead: true}) |
| + got <- payload |
| + _, _ = conn.Write([]byte("stream: OK\x00")) |
| + }() |
| + |
| + c := New(ln.Addr().String(), Timeouts{}) |
| + if c.network != "tcp" { |
| + t.Fatalf("network = %q, want tcp", c.network) |
| + } |
| + res, err := c.ScanStream(context.Background(), bytes.NewReader([]byte("hello"))) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if res.Status != StatusClean { |
| + t.Fatalf("status = %v, want clean", res.Status) |
| + } |
| + if b := <-got; !bytes.Equal(b, []byte("hello")) { |
| + t.Fatalf("payload over tcp = %q", b) |
| + } |
| +} |
| + |
| +// An empty address is refused rather than dialled, and the message says |
| +// address rather than socket now that either shape is legitimate. |
| +func TestDialRefusesEmptyAddress(t *testing.T) { |
| + c := New("", Timeouts{}) |
| + if _, err := c.Version(context.Background()); err == nil || |
| + !strings.Contains(err.Error(), "no address configured") { |
| + t.Fatalf("err = %v", err) |
| + } |
| +} |