Default config allows a remote stranger to lock the operator out of the admin UI, and nothing warns about it #149
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found by the final integration review of #111. Blocks the 1.0.0 tag.
internal/middleware/ratelimit.go:151-172,219-226.LoginRateLimitallows 5 POST/min per bucket. Since #88 the bucket key is the peer address unless the peer is listed inTRUSTED_PROXIES, which defaults to empty.README.md:79-80requires a TLS-terminating reverse proxy in production, andREPO_POLICIES.md:328mandates it. So in the deployment shape the project itself prescribes, every login POST from every client keys on the PROXY's address and shares a single 5/min bucket.Consequence: a stranger sending roughly 0.08 requests per second — 5 per minute, from anywhere — keeps that bucket permanently full. The operator's own login returns 429 indefinitely, and there is no second administrative path.
PasswordChangeRateLimithas the same shape.Restarting the service clears the in-memory buckets, so it is recoverable, but a sustained trickle re-locks it immediately.
The #88 default is correct and should not change — trusting forwarded headers from arbitrary peers is worse. The defect is that this consequence is invisible:
README.md:1120-1122asserts the opposite: "5 POST attempts per minute per IP". That is only true whenTRUSTED_PROXIESis configured.README.md:891-895calls the shared bucket "the safe direction to be wrong in". True for the receiver, where sharing costs throughput. Inverted for login, where sharing costs availability of the only admin path.README.md:101-146, the section an operator actually configures from, states no downside at all.Definition of done
TRUSTED_PROXIESis empty: name the variable, say that all clients will share one rate-limit bucket, and say that this makes the admin login remotely deniable. An operator must not have to read the whole README to discover this.README.md:1120-1122corrected — the per-IP claim is conditional onTRUSTED_PROXIES.README.md:891-895distinguishes the receiver case (sharing costs throughput, safe) from the login case (sharing costs availability of the only admin path, not safe).TRUSTED_PROXIESas the remedy.TRUSTED_PROXIESis empty in production mode, and does not fire when it is set.Explicitly out of scope
Do NOT change the rate-limit keying, the limits, or the
TRUSTED_PROXIESdefault. Whether login should additionally be limited per-username, or whetherTRUSTED_PROXIESshould be mandatory in production, is a design question for the owner and is filed separately as #150.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).make checkplus the Docker lint path with the cache defeated.PR: #153
Visibility only — keying, limits and the
TRUSTED_PROXIESdefault are unchanged.internal/config/config.go:warnSharedRateLimitBucket, called fromNew, logs atWARNwhenIsProd()andTRUSTED_PROXIESis empty: "TRUSTED_PROXIES is empty: rate limits key on the connecting peer, so behind the reverse proxy a production deployment runs behind, every client shares one bucket per limit. Any remote client can then keep the login limit full and deny the admin login, the only administrative path, until restart. Set TRUSTED_PROXIES to your reverse proxy's address."TRUSTED_PROXIES.Verified:
make checkexit 0;docker build --no-cache-filter=lint,builderexit 0 with in-container lint at0 issues.and uncachedinternal/configtests. Mutation-checked both directions — removing the warning fails the prod-empty case, removing the condition fails both quiet cases.