Repository navigation
feat(proxy): expose rate limiting, IP allow lists and client-IP identification - #88
Merged
Merged
Conversation
…ification
The whole R3 access-control batch was unreachable from deploy.yml.
proxy:
client_ip:
header: CF-Connecting-IP
trusted_proxies: [ 173.245.48.0/20 ]
rate_limit:
requests: 100
burst: 20
exempt: [ 10.0.0.0/8 ]
allow_ips: [ 10.0.0.0/8, 192.168.0.0/16 ]
All three ship together because the first two are only as correct as the client
address they key on, and the third is what establishes it.
The issue pointed at eb4273f for the forwarded-chain semantics; that commit is a
two-line staticcheck fix whose own message says "Same behavior". The semantics
are in internal/server/ip_allow_list.go: with no trusted proxies the peer is
always the client; with trusted proxies AND a peer that is one of them, the chain
is walked from the nearest hop backwards past every declared proxy, and the first
address none of them wrote is the client. An unresolvable chain denies rather
than falling back to the peer. The docs now say that rather than guessing.
Access control is stripped from the per-app deploy when load balancing and
re-added at the load balancer, like host/tls/basic-auth. This is sharper than the
TLS case: the per-host proxy's peer is the load balancer, so allow_ips there
would 403 every request and one rate limiter would count the entire fleet as a
single client. Leaving them on is an outage, not a degraded feature.
Eight things kamal-proxy rejects only once the deploy reaches a host now fail at
config time, plus two Ruby-specific ones: IPAddr silently drops an IPv6 zone and
accepts IPv4-mapped forms, both of which match nothing at runtime while looking
configured.
And one warning rather than an error, per the issue: rate_limit with
forward_headers and no trusted_proxies is legal but almost certainly wrong, so it
says so instead of refusing to deploy.
rate_limit/requests is checked by hand via validate_key_override! -- the proxy's
flag is a Float64, so 0.5 is a legal rate, and the docs example can only
demonstrate one numeric type.
Closes #77
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The R3 access-control batch, reachable from
deploy.yml. All three blocks ship together because rate limiting and IP allow lists are only as correct as the client address they key on, andclient_ipis what establishes it.lib/kamal/configuration/proxy.rb— anaccess_control_optionsgroup merged intodeploy_optionsand stripped when load balancing.lib/kamal/configuration/loadbalancer.rb— re-adds it at the edge.lib/kamal/configuration/validator/proxy.rb— ten config-time rejections, plus avalidate_key_override!forrate_limit.lib/kamal/configuration.rb—ensure_rate_limit_can_identify_clients, the AC1 warning.test/proxy_flag_coverage_test.rb— issue-77 waiver deleted; all six flags proven emitted.Closes #77
The forwarded-chain semantics, from the source rather than guessed
The issue said to read
eb4273fbefore designingtrusted_proxies. That commit is a two-line staticcheck fix (S1011) whose own message says "Same behavior" — it decides nothing. The semantics live ininternal/server/ip_allow_list.go, inforwardedResolver#clientAddr/#forwardedAddr:trusted_proxies, the client is always the connecting address. Nothing a client sends can influence it — that is what makes the allow list meaningful.trusted_proxiesset and the connecting address being one of them, the chain is walked from the nearest hop backwards past every declared proxy; the first address none of them wrote is the client.Hence the docs' emphasis on listing every hop, not just the one that connects to kamal-proxy.
Config-time rejections
Eight kamal-proxy enforces only once the deploy reaches a host:
trusted_proxiesalonetrusted_proxies has no effect without allow_ips or rate_limitclient_ip.headerwithouttrusted_proxiesheader requires trusted_proxies, or the header would be ignored while appearing to be honoredtrusted_proxies: [ 0.0.0.0/0 ]is a default route - trusting every address means trusting every client to speak for someone elserate_limit.burst/.exemptwithoutrequests… has no effect without requestsrequests/burst… cannot be negative… is not a valid address or CIDR rangehealthcheck.path: /with either feature onpath cannot be '/' … served without an address check or a rate limitPlus two that are Ruby-specific —
IPAddris looser than the proxy'snetip:fe80::1%eth0IPAddrsilently drops the zone; the proxy rejects it because a zoned address matches nothing::ffff:10.0.0.0/104IPAddraccepts it; it would never match a plain IPv4 rangeTest plan
bundle exec rubocop --parallel— clean, 212 filesbuilder_test/build_test, identical on pristinedashHEAD (host-arch dependent, pass in CI)allow_ipsand confirm an outside address gets 403 while the health check stays reachableclient_ipset and confirm the limiter buckets per visitor, not per CDN edgerate_limit.requests: 100returns 429 past the limit andexemptranges are not throttleddeployand not the per-app oneAutomated coverage
allow_ips reach the proxy as repeated flags--allow-ip=, checked againstinternal/cmd/deploy.go'sStringSliceVara malformed CIDR fails locally+ the zone/IPv4-mapped testsrate limiting behind a proxy without trusted_proxies warns(+ two negative cases)no access control keys leave the deploy command unchangedaccess control moves to the load balancer when load balancinga fractional rate is passed through as written0.5survives; the Float64 flag is not roundedevery kamal-proxy deploy flag is exposed or waivedDeviations & judgment calls
Deviations
eb4273fpointer was a dead end (see above); the docs were written fromip_allow_list.goinstead.Judgment calls
host,tls,basic-authand the Expose on-demand TLS, mTLS client CA and ACME cache path in deploy.yml #74 TLS options. Not in the issue, and sharper than the TLS case: the per-host proxy's peer is always the load balancer, soallow_ipsthere would 403 every request unless the operator happened to list the load balancer's own address, and one rate limiter would count the entire fleet as a single client. Leaving these on the per-app service is an outage, not a degraded feature.forward_headers: true. kamal-proxy also enables header forwarding by default whenssl: false, which would widen the warning to most non-TLS configs — and a warning that fires on nearly every deploy is one operators learn to skip.forward_headers: trueis a deliberate "something is in front of me" statement, which is exactly the population that needstrusted_proxies.rate_limit.requestsis validated by hand throughvalidate_key_override!rather than from the docs example. The proxy's flag is aFloat64, so0.5(one request every two seconds) is legal, but a YAML example can only demonstrate one numeric type and100there would reject it. The override still checks unknown keys and the other two types, so nothing is lost.healthcheck.pathis only rejected at the literal/. The gem omits--health-check-pathwhen unset and the proxy defaults to/up, so an unset path is safe and does not need to be forced.