Stop writing callers' addresses into the log (GRYT-791) - #127
Merged
Conversation
GRYT-791 said the rate limiter was the only part of a Gryt server that writes a client's address down, and named two lines in it. There were four. The other two are in the socket layer and are far worse: one logs an address on every connection any client ever makes, the other on every disconnect. The two ban lines only fire when somebody is banned. All four log a label now. `addressLabel` is an HMAC of the address under a salt generated fresh at startup and never written anywhere, truncated to eight hex characters. The same address keeps the same label while the process lives — so "one caller hammering us" and "eighty doing it once" still read differently, which is the only reason the address was ever in there — and the label is not the address. The salt is random per process on purpose. IPv4 is small enough to enumerate, so an unsalted hash is a reversible encoding of an address rather than a replacement for it, and a salt that survived a restart would eventually sit in a backup beside the logs it was protecting. The cost is that labels mean nothing across a restart or between two servers, which is the correlation this exists to prevent. The rate limiter's key is untouched. It is also the map key, and rewriting it would forget every ban in flight — so the ban lines log the parts rather than the composed key. `reachableAddresses` still prints the server's own interfaces at startup. That is the operator's machine and the reason the line exists. `logAddresses.test.ts` reads the source rather than the behaviour, because the failure is a correct value written in the wrong place and the fifth one somebody adds will look like the four that were here. It fails if a `consola` call mentions `getClientIp` or `requestIp` without `addressLabel`, which I checked by putting one back. The Privacy Policy (site#70) describes only the rate-limiter case and now overstates what is kept — worth a second look, filed as GRYT-886. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
GRYT-791 said the rate limiter is "the only thing on a Gryt server that writes an address down" and named two lines in it.
There were four. The other two are in the socket layer and are the worse pair: one logs an address on every connection any client ever makes, the other on every disconnect. The two ban lines only fire when somebody is actually banned.
The label
All four log
addressLabel(...)now — an HMAC of the address under a salt generated fresh at startup and never written anywhere, truncated to eight hex characters.The same address keeps the same label for the life of the process, so "one caller hammering us" and "eighty doing it once" still read differently in the log. That distinction is the only reason an address was ever in there — everything public arrives through a tunnel, so the handshake address is the tunnel for everyone and says nothing.
The salt is random per process on purpose. IPv4 is small enough to enumerate, so an unsalted hash is a reversible encoding of the address rather than a replacement for it; and a salt that survived a restart would eventually sit in a backup next to the logs it was protecting. The cost is that labels mean nothing across a restart or between two servers — which is exactly the correlation this exists to prevent.
Two things deliberately untouched
The rate limiter's key. It is also the map key, and rewriting it would forget every ban in flight. The ban lines log the parts instead of the composed key.
reachableAddresses. It prints the server's own interfaces at startup. That is the operator's machine, and it is the reason the line exists.The test reads the source
logAddresses.test.tsfails if aconsolacall mentionsgetClientIporrequestIpwithoutaddressLabel. A unit test cannot catch this — the failure is a correct value written in the wrong place, and the fifth site somebody adds will look exactly like the four that were already here. I checked it by putting one back: 634 pass / 1 fail, with the offending file and line named.It is coarse on purpose, and it will occasionally be wrong at somebody who has a good reason. That argument is worth having in a review rather than silently.
Follow-up filed
The Privacy Policy (site#70) describes only the rate-limiter case, so it understated what was collected and now overstates what is kept. GRYT-886 — not urgent, overstating is the safe direction.
635 tests pass, 6 new.
🤖 Generated with Claude Code