Repository navigation
feat(security): optional SOCKS source-IP access control via SocksPolicy (ISSUE-18c) - #112
Conversation
ISSUE-18c (#108): Tor's SocksPort is unauthenticated by design (torrc manual: 'unencrypted and (as we use it) unauthenticated'); client username/password is only stream isolation, never a credential check. So username/password env vars rendered into torrc would be a misleading no-op. Implementable control is Tor's SocksPolicy (source IP): - Add optional TORPROXY_SOCKS_ALLOWED_CIDRS (comma-separated CIDRs, validated with ipaddress.ip_network). When set, render_torrc emits 'SocksPolicy accept loopback, accept <cidrs>, reject *' — Tor accepts anything unmatched, so the final reject is required. Loopback is always accepted for the internal probes and Privoxy. - Default unset = accept all (legacy behavior, additive). - Document the Tor auth limitation in .env.example, docs/configuration.md and docs/compatibility.md. Closes #108
MustermenschAlex
left a comment
There was a problem hiding this comment.
Thanks — the scope correction is the most valuable part of this PR.
Verifying against the torrc manual that SocksPort is "unencrypted and unauthenticated" and that client username/password is only stream isolation — then shipping the control Tor actually offers (SocksPolicy) instead of a misleading no-op env var — is exactly right. The issue comment documents the reasoning.
Implementation-wise: CIDR validation via ip_network, loopback always accepted (probes/Privoxy keep working), final reject * present so the policy actually restricts, default unset = legacy behavior. Docs and .env.example placeholders included, no real secrets. All checks green. Approving.
kuenstlichedummheit-sys
left a comment
There was a problem hiding this comment.
This is how you handle a feature request that turns out to be impossible as written: check the manual, say so plainly, ship the thing that actually works. "Tor SOCKS auth is isolation, not authentication" is the kind of sentence that saves the next person a week.
The SocksPolicy rendering looks right — final reject *, loopback pinned open for the probes. The one thing I'd keep an eye on is operators misunderstanding this as password auth, but the docs say it three times, so that's on them now.
Approving.
majorungustl
left a comment
There was a problem hiding this comment.
Good call not faking username/password. Source-IP policy is honest, additive, and actually enforces something. Merge it. Approving.
Summary
Implements ISSUE-18c (#108) with a researched scope correction (see the comment on #108):
Finding: Tor's
SocksPortis unauthenticated by design (torrc manual: "unencrypted and (as we use it) unauthenticated"). Client-supplied username/password is only stream isolation (IsolateSOCKSAuth), never a credential check — soTORPROXY_SOCKS_USER/PASSWORDenv vars intorrcwould be a misleading no-op. True username/password auth would need an additional front-proxy daemon (separate ADR if ever desired).Implemented instead — the control Tor itself offers: optional, additive
TORPROXY_SOCKS_ALLOWED_CIDRS:ipaddress.ip_network()inSettings(invalid →ValueErrorin the existing style).render_torrcemitsSocksPolicy accept 127.0.0.0/8, accept ::1/128, accept <cidrs>, reject *when set (Tor accepts anything unmatched, so the finalreject *is required). Loopback is always accepted for the internal SOCKS readiness probe and Privoxy..env.example(no real secrets), docs indocs/configuration.md/docs/compatibility.mdincluding the Tor auth limitation.Hermetic tests (
tests/unit/test_socks_policy.py, 6 tests) cover env parsing, CIDR validation (v4/v6/invalid), default no-policy rendering and policy rendering.Compatibility: additive only — no env var set means exactly the current behavior.
Verification
uv run pytest tests/unit/ -q --cov=torproxy --cov-fail-under=85→ 323 passed, coverage 85.74%uv run ruff check .,uv run ruff format --check .,uv run mypy,shellcheck docker/*.sh tests/integration/*.sh→ cleanNot run: live container test with
SocksPolicyenforced (requires Docker daemon + live Tor; CI covers the image build).Closes #108