Repository navigation
add connection timeout & fix banning for x-forwarded-for - #41
LilithSilver wants to merge 4 commits into
Conversation
Co-authored-by: Dan Nicholson <dbn@endlessaccess.org>
| .on_upgrade(async move |socket: WebSocket| { | ||
| tracing::info!("Upgrade successful for {}", addr.ip()); | ||
| state.server.accept_socket(SocketInfo(addr, socket)).await; | ||
| // TODO: this may be insecure -- we're blindly trusting an X-Forwarded-For header, which might be spoofed. |
There was a problem hiding this comment.
Typically the way servers that are potentially behind proxies handle this is a "trust proxy" setting. Here are a couple I've seen:
Caddy - https://caddyserver.com/docs/caddyfile/options#trusted-proxies
Express - https://expressjs.com/en/5x/guide/behind-proxies/
So, I think a very reasonable thing would be a --trust-proxy boolean option that's false by default. You could take an IP/CIDR or something like Caddy does, but IMO a boolean is plenty good enough for anyone in this position.
There was a problem hiding this comment.
@dbnicholson Happy to do this! What would be your ideal behavior for the --trust-proxy flag? Trust all provided proxies, like the current code? Trust exactly 1 proxy?
There was a problem hiding this comment.
Thanks! I think simple boolean implying trust all is fine, IMO. There are definitely scenarios where someone could spoof it, but I don't think they're worth bothering with here. The most likely way this is going to setup is a trusted proxy on the same host with very little chance of being bypassed. A project like Caddy that's going to be used in all sorts of environments would want to provide more control. I don't think backstitch-sync-server needs that level of control.
There was a problem hiding this comment.
This comment is no longer accurate. I think you could remove it completely.
…titch-sync-server into config-connection-timeout
dbnicholson
left a comment
dbnicholson
left a comment
There was a problem hiding this comment.
Looks about right. I think the example compose files that use Caddy (docker-compose.caddy.yml and docker-compose.endless.yml) should have TRUST_PROXY: true since that demonstrates the exact use case --trust-proxy is being added for.
| --oidc-client-id "${OIDC_CLIENT_ID:-backstitch}" | ||
| --oidc-client-id "${OIDC_CLIENT_ID:-backstitch}" \ | ||
| --connection-timeout "${CONNECTION_TIMEOUT:-10000}" \ | ||
| --connection-timeout "${TRUST_PROXY:-false}" |
There was a problem hiding this comment.
I think you meant --trust-proxy here. However, I don't think clap boolean args work with a value. I think this needs to be:
if [ "${TRUST_PROXY:-false}" = "true" ]; then
set -- "$@" --trust-proxy
fi
| .on_upgrade(async move |socket: WebSocket| { | ||
| tracing::info!("Upgrade successful for {}", addr.ip()); | ||
| state.server.accept_socket(SocketInfo(addr, socket)).await; | ||
| // TODO: this may be insecure -- we're blindly trusting an X-Forwarded-For header, which might be spoofed. |
There was a problem hiding this comment.
This comment is no longer accurate. I think you could remove it completely.
No description provided.