Skip to content

fix(daemon): refuse a PROXY header from an untrusted direct peer - #7648

Merged
oferchen merged 1 commit into
masterfrom
fix/proxy-protocol-trusted-peer
Sep 3, 2026
Merged

fix(daemon): refuse a PROXY header from an untrusted direct peer#7648
oferchen merged 1 commit into
masterfrom
fix/proxy-protocol-trusted-peer

Conversation

@oferchen

@oferchen oferchen commented Sep 3, 2026

Copy link
Copy Markdown
Owner

With proxy protocol = true, oc read the PROXY protocol header from
whatever connected and adopted the address it claimed. That address then
is the peer for everything downstream - hosts allow / hosts deny,
%h, RSYNC_HOST_NAME, and every log line - so any client that could
reach the listener could choose its own source address and walk straight
through an address-based ACL.

Upstream gates the read on a trusted-proxy list and fails closed
(clientserver.c:1443-1446):

if (lp_proxy_protocol()) {
        if (!proxy_peer_allowed(f_in) || !read_proxy_protocol_header(f_in))
                return -1;
}

proxy_peer_allowed() consults proxy protocol hosts through
allow_proxy_protocol_peer() (access.c:300-306), whose whole body is the
policy:

if (!list || !*list)
        return 0;
allow_forward_dns = 0;
return access_match(list, addr, host_ptr, 0);

Two properties of that body are load-bearing and both are ported here:

  • An EMPTY list rejects everyone. Unset does not mean "trust anybody";
    it means "trust nobody", which is why upstream warns at startup when
    proxy protocol = true has no list (clientserver.c:1747-1756) - the
    fail-closed default is correct but otherwise silent.
  • NO DNS is consulted. allow_forward_dns = 0 kills the forward-resolve
    half, and clientserver.c:1391-1393 passes the UNDETERMINED sentinel
    rather than a resolved name, so match_hostname can only compare a
    token against that sentinel. The trusted-proxy list is effectively
    address-only. oc passes the same sentinel, so the gate costs no lookup
    • and a test pins that a name-based token matches no real peer, since
      "resolve it properly" is the tempting wrong fix.

oc had no gate at all, so this adds the proxy protocol hosts global
directive alongside it. The two directives are carried as one
ProxyProtocolPolicy value rather than a bool plus a list: they are only
ever meaningful together (a list with the feature off decides nothing; the
feature on without a list rejects everyone), and they are threaded through
five layers to the session handler, which is exactly the distance over
which a bool and its list drift apart.

Both accept paths carry the gate: the sync accept loop and the
async-daemon one (daemon.rs run_async_daemon). The async site is
behind a cfg feature, so a default-feature cargo check compiles right
past it - it has to be built with --all-features to be seen at all.

Measured end to end against the fixed binary, proxy protocol = true with
no trusted-proxy list, an unlisted direct peer sending
PROXY TCP4 10.9.8.7 127.0.0.1 12345 873:

  • before: the daemon believed the header and served the connection
  • after: the connection is dropped with no @RSYNCD greeting, and the
    startup warning is emitted

Mutation-proven by restoring the fail-open arm (Enabled(_) => Trusted):

test mutated
an_unlisted_direct_peer_may_not_supply_a_proxy_header FAILED
proxy_protocol_with_no_trusted_hosts_rejects_every_peer FAILED
a_cidr_trusted_proxy_token_admits_its_range_and_nothing_. FAILED
a_hostname_trusted_proxy_token_matches_no_real_peer FAILED
a_listed_trusted_proxy_may_supply_a_proxy_header ok
the_gate_is_inert_when_proxy_protocol_is_off ok

The two survivors are the non-vacuity proof: the refusal pins cannot pass
by every decision collapsing onto Untrusted, and the availability half -
with the feature off the gate refuses nobody - is pinned separately.

The 3.5.0 proxy-protocol-trusted-peer cell is NOT closed by this and its
manifest rows are deliberately left at fail. Measured: the cell's first
assertion ("daemon trusted a PROXY header from an unlisted direct peer")
now passes, but the remaining two grep a daemon log for the refusal, and
oc's log does not cover the window the refusal happens in.

That second half is a SEPARATE defect in a different subsystem, diagnosed
here rather than fixed. A/B on one fixture - same plain config, same probe,
log file set in the global section - against real 3.5.0:

  • upstream's log opens with rsyncd version 3.5.0 starting, listening on port N and carries a connect from localhost (127.0.0.1) line per
    connection, before any module is chosen
  • oc's log is created and written, but its first line is
    rsync allowed access on module mod - every pre-module event is absent

So oc opens the log sink once a module has been SELECTED; upstream opens it
at daemon STARTUP. Upstream's log file is P_LOCAL (daemon-parm.h:289),
exactly as oc classifies it, but log_init(0) at clientserver.c:1768 runs
with module_id == -1 and log.c:217 takes
logfile_name = lp_log_file(module_id) - so the global section's value
opens a daemon-wide sink that covers the listener itself. A proxy-header
refusal is decided at accept time, before any module exists, which is
precisely the window oc's sink does not reach; that is why the startup
warning above lands on stderr. Moving oc's sink to daemon startup changes
where every daemon log line goes and belongs in its own PR.

daemon 1906/1906 (--all-features); fmt clean.

@github-actions github-actions Bot added the bug Something isn't working label Sep 3, 2026
@oferchen
oferchen force-pushed the fix/proxy-protocol-trusted-peer branch 2 times, most recently from 6d4b8dc to e85dfcd Compare September 3, 2026 16:12
With `proxy protocol = true`, oc read the PROXY protocol header from
whatever connected and adopted the address it claimed. That address then
*is* the peer for everything downstream - `hosts allow` / `hosts deny`,
`%h`, `RSYNC_HOST_NAME`, and every log line - so any client that could
reach the listener could choose its own source address and walk straight
through an address-based ACL.

Upstream gates the read on a trusted-proxy list and fails closed
(clientserver.c:1443-1446):

    if (lp_proxy_protocol()) {
            if (!proxy_peer_allowed(f_in) || !read_proxy_protocol_header(f_in))
                    return -1;
    }

`proxy_peer_allowed()` consults `proxy protocol hosts` through
`allow_proxy_protocol_peer()` (access.c:300-306), whose whole body is the
policy:

    if (!list || !*list)
            return 0;
    allow_forward_dns = 0;
    return access_match(list, addr, host_ptr, 0);

Two properties of that body are load-bearing and both are ported here:

  * An EMPTY list rejects everyone. Unset does not mean "trust anybody";
    it means "trust nobody", which is why upstream warns at startup when
    `proxy protocol = true` has no list (clientserver.c:1747-1756) - the
    fail-closed default is correct but otherwise silent.
  * NO DNS is consulted. `allow_forward_dns = 0` kills the forward-resolve
    half, and clientserver.c:1391-1393 passes the `UNDETERMINED` sentinel
    rather than a resolved name, so `match_hostname` can only compare a
    token against that sentinel. The trusted-proxy list is effectively
    address-only. oc passes the same sentinel, so the gate costs no lookup
    - and a test pins that a name-based token matches no real peer, since
    "resolve it properly" is the tempting wrong fix.

oc had no gate at all, so this adds the `proxy protocol hosts` global
directive alongside it. The two directives are carried as one
`ProxyProtocolPolicy` value rather than a bool plus a list: they are only
ever meaningful together (a list with the feature off decides nothing; the
feature on without a list rejects everyone), and they are threaded through
five layers to the session handler, which is exactly the distance over
which a bool and its list drift apart.

Both accept paths carry the gate: the sync accept loop and the
`async-daemon` one (daemon.rs `run_async_daemon`). The async site is
behind a cfg feature, so a default-feature `cargo check` compiles right
past it - it has to be built with `--all-features` to be seen at all.

Measured end to end against the fixed binary, `proxy protocol = true` with
no trusted-proxy list, an unlisted direct peer sending
`PROXY TCP4 10.9.8.7 127.0.0.1 12345 873`:

  * before: the daemon believed the header and served the connection
  * after:  the connection is dropped with no `@RSYNCD` greeting, and the
            startup warning is emitted

Mutation-proven by restoring the fail-open arm (`Enabled(_) => Trusted`):

  | test                                                     | mutated |
  |----------------------------------------------------------|---------|
  | an_unlisted_direct_peer_may_not_supply_a_proxy_header     | FAILED  |
  | proxy_protocol_with_no_trusted_hosts_rejects_every_peer   | FAILED  |
  | a_cidr_trusted_proxy_token_admits_its_range_and_nothing_. | FAILED  |
  | a_hostname_trusted_proxy_token_matches_no_real_peer       | FAILED  |
  | a_listed_trusted_proxy_may_supply_a_proxy_header          | ok      |
  | the_gate_is_inert_when_proxy_protocol_is_off              | ok      |

The two survivors are the non-vacuity proof: the refusal pins cannot pass
by every decision collapsing onto `Untrusted`, and the availability half -
with the feature off the gate refuses nobody - is pinned separately.

The 3.5.0 `proxy-protocol-trusted-peer` cell is NOT closed by this and its
manifest rows are deliberately left at `fail`. Measured: the cell's first
assertion ("daemon trusted a PROXY header from an unlisted direct peer")
now passes, but the remaining two grep a daemon log for the refusal, and
oc's log does not cover the window the refusal happens in.

That second half is a SEPARATE defect in a different subsystem, diagnosed
here rather than fixed. A/B on one fixture - same plain config, same probe,
`log file` set in the global section - against real 3.5.0:

  * upstream's log opens with `rsyncd version 3.5.0 starting, listening on
    port N` and carries a `connect from localhost (127.0.0.1)` line per
    connection, before any module is chosen
  * oc's log is created and written, but its first line is
    `rsync allowed access on module mod` - every pre-module event is absent

So oc opens the log sink once a module has been SELECTED; upstream opens it
at daemon STARTUP. Upstream's `log file` is P_LOCAL (daemon-parm.h:289),
exactly as oc classifies it, but `log_init(0)` at clientserver.c:1768 runs
with `module_id == -1` and log.c:217 takes
`logfile_name = lp_log_file(module_id)` - so the global section's value
opens a daemon-wide sink that covers the listener itself. A proxy-header
refusal is decided at accept time, before any module exists, which is
precisely the window oc's sink does not reach; that is why the startup
warning above lands on stderr. Moving oc's sink to daemon startup changes
where every daemon log line goes and belongs in its own PR.

daemon 1906/1906 (--all-features); fmt clean.
@oferchen
oferchen force-pushed the fix/proxy-protocol-trusted-peer branch from e85dfcd to 2f24e2c Compare September 3, 2026 16:21
@oferchen
oferchen merged commit 06b3fd8 into master Sep 3, 2026
75 checks passed
@oferchen
oferchen deleted the fix/proxy-protocol-trusted-peer branch September 3, 2026 17:18
oferchen added a commit that referenced this pull request Sep 4, 2026
… them (#7661)

`README.md` ships the exact `awk` command that reads a leg's expected-outcome
manifest and prints its pass/fail/skip counts. Running it disagrees with every
row of the table directly beneath it.

| row | documented | re-derived |
|---|---|---|
| non-root, pipe | 257 / 3 / 85 | **259 / 1 / 85** |
| root, pipe | 286 / 3 / 56 | **288 / 1 / 56** |
| non-root, tcp | 101 / 21 / 33 | **108 / 14 / 33** |
| root, tcp | 113 / 27 / 15 | **120 / 20 / 15** |

`SECURITY.md` carries the same four rows and the same two summary figures, so
both files drifted together. Corrected throughout, along with:

- **"3 of 345 tests currently diverge"** across the full-corpus legs. It is
  **one** - `filter-merge-content-echo` - and the README names it now rather
  than leaving the reader to count.
- **"29 across all four"** distinct failures. It is **23**, and across
  **five** manifests, not four: the README's own glob
  `tools/ci/upstream-3.5.0-expect.*.txt` has always matched the macOS file its
  prose excluded.
- **The macOS leg was missing from both tables.** It runs on every PR as
  `upstream-testsuite-macos` (`ci.yml`), on the full 345-cell corpus, with its
  own committed manifest. It is the only leg that can observe a
  platform-conditional divergence - one of its three remaining failures *skips*
  on Linux, so it had never executed in this repository's CI before the leg
  existed. Added as a fifth row, with its non-required status stated.
- **"the required checks being the two stdio-pipe legs."** All four Linux legs
  have been required contexts since #7408 wired the TCP pair into PR CI; the
  ruleset returns ten contexts, not eight.
- **`proxy protocol hosts` described as "not yet implemented"** and "still
  under audit" in two places. It shipped in #7648: parsed into a
  `ProxyProtocolPolicy` that mirrors upstream's `allow_proxy_protocol_peer()`,
  rejecting every peer when the trusted list is empty or unset, and warning at
  startup on the combination upstream warns about.
- **The `MAX_PROXY_LINE_BYTES` citation** pointed at `connect/proxy.rs:344`.
  The constant moved and, more usefully, stopped being a typed literal: it is
  now `PROXY_BUF_SIZE - 1`, so the doc records the derivation rather than a
  line number that will drift again.

The two macOS-leg rationale comments in the workflows carried the same pre-fix
counts and a failure list six entries out of date. Both are recounted from the
manifests.

`CHANGELOG.md` stopped at #7632, leaving the 26 PRs merged since then
unrecorded. Added under Security / Fixed / Testing and CI / Documentation.

## Why the numbers were wrong in a way reading could not catch

Every figure here is the outcome column of a committed manifest. The previous
values were transcribed once and then maintained by hand, so they decayed as
fixes landed - and a reader checking the table against the prose beside it
would find them perfectly consistent with each other.

The check that finds this class is re-running the derivation, not re-reading
the text, so that is what was done: a script recomputes all five legs and both
summary figures from `tools/ci/upstream-3.5.0-expect*.txt` and asserts the two
documents contain the results, with the superseded strings blacklisted so a
partial edit cannot pass. It also caught a live error in this changeset -
#7659 landed mid-review and flipped `operator-path-partial-dir-daemon`, taking
macOS from 236/4 to 237/3 and the distinct-failure count from 24 to 23. The
figures here are derived from master with that merge in place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant