Repository navigation
tailcat: add Server.AllowClient hook for admitting clients - #120
Closed
jormenjanssen wants to merge 1 commit into
Closed
jormenjanssen wants to merge 1 commit into
jormenjanssen wants to merge 1 commit into
Conversation
Server.AllowedClients only admits a fixed list of node keys, so a program that decides at runtime which clients may connect, from a database or a directory service say, has to know every key before Start or keep the list current itself with AddAllowedClient, and can never turn a key down once it is on the list. Add Server.AllowClient, a func(key.NodePublic) bool in the shape of AllowProxy, consulted when a client that is not already connected and not in AllowedClients announces itself. A listed key is admitted without the hook; any other key is admitted if the hook approves it; with neither configured the server stays open to all clients, as before. A rejected client is ignored like an unlisted one and asked about again when it retries its meow. The hook runs under the backend's mutex, which peerByIP and peerForIP on the data path also take, so it must return quickly and not do I/O. That keeps this change small. A blocking-capable variant would need the check moved out of the lock, a context canceled at Close, and deduplication of the client's once-a-second meow retries while a decision is pending; a program gating clients on another service can instead decide ahead of time and have the hook look the decision up. onMeow now recognizes an already connected client before the admission check rather than after it, so a connected client's later pings never reach the hook. For the list this changes nothing, since a key can't be removed from it. Updates tailscale#119 Signed-off-by: Jormen Janssen <j.janssen@riwo.eu>
jormenjanssen
force-pushed
the
jormen/allow-client-callback
branch
from
September 16, 2026 12:29
45a0a2b to
a95bb65
Compare
bradfitz
reviewed
Sep 16, 2026
Comment on lines
449
to
+470
| // AllowedClients, if non-empty, restricts which client node keys | ||
| // may connect; all others are silently ignored. If empty, all | ||
| // clients are allowed. See [Server.AddAllowedClient] to add more | ||
| // at runtime. | ||
| // at runtime, and AllowClient to decide per client instead of | ||
| // from a fixed list. | ||
| AllowedClients []key.NodePublic | ||
|
|
||
| // AllowClient, if non-nil, reports whether the client with node | ||
| // key k may connect. It is consulted when a client that is not | ||
| // already connected and not in AllowedClients announces itself; | ||
| // a client it rejects is silently ignored, like an unlisted one, | ||
| // and is asked about again if it retries. If AllowClient is nil | ||
| // and AllowedClients is empty, all clients are allowed. | ||
| // | ||
| // It is called with the server's internal lock held, so it must | ||
| // return quickly and must not block on I/O: a slow call stalls | ||
| // every connected client's traffic. A program gating clients on | ||
| // another service should decide ahead of time and have AllowClient | ||
| // look the decision up. | ||
| // | ||
| // It must be set before calling Start. | ||
| AllowClient func(k key.NodePublic) bool |
Member
There was a problem hiding this comment.
I think we need to tighten the docs on the precedence here about when we fail open.
Actually, maybe we should just ditch the slice and only use the func? We can provide a tiny little adapter for people migrating from the slice style (probably very few people)
bradfitz
added a commit
that referenced
this pull request
Sep 26, 2026
…and DisconnectClient Server.AllowedClients and Server.AddAllowedClient are replaced by a single Server.AllowClient func(key.NodePublic) bool, asked about each client as it connects. A nil hook allows all clients, which is the one fail-open rule. The old slice's "empty means open" semantics forced the CLI's --allow=none to insert a zero key as a sentinel; an empty KeySet now simply allows nobody. The hook runs with the backend mutex released, so unlike the hook proposed in #120 it may block on an external lookup and may call other Server methods. onMeow already runs in its own goroutine per meow ping, so a slow answer delays only that client. A pendingAllow map keeps a client's once-a-second meow retries from starting a second call for the same key while one is in flight. The new KeySet type is a mutex-guarded set of node keys whose Contains method is the ready-made hook for a fixed or hand-maintained list, so the runtime add that AddAllowedClient offered is still available, and removal comes with it. The new Server.DisconnectClient drops a connected client from the network map so its traffic is blackholed in both directions; it does not reset its connections. Revoking a client is then two orthogonal steps the caller composes: make the hook reject the key, then disconnect it. Client IDs now come from a counter that is never reused, since after a removal len(clients)+2 could collide with a live peer. Supersedes #120 and #125, which each added a second admission form alongside the slice. Fixes #119 Fixes #124 Co-authored-by: Jormen Janssen <j.janssen@riwo.eu> Co-authored-by: Yann <y.baelli05@gmail.com>
Member
|
Folded into #134 |
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
2 participants
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.
Adds
Server.AllowClient, afunc(key.NodePublic) boolhook in the shape ofAllowProxy, for programs embedding tailcat that decide at runtime which clients may connect and can't list every node key inAllowedClientsup front.Semantics: a key in
AllowedClientsis admitted without consulting the hook, any other key is admitted if the hook approves it, and with neither configured the server stays open to all clients as before. Rejected clients are ignored the same way unlisted ones are, and are asked about again when they retry their meow.onMeownow recognizes an already connected client before the admission check, so a connected client's later pings never reach the hook.Design choice for review. The hook runs under the backend's mutex, which
peerByIPandpeerForIPalso take, so it is documented as non-blocking: return quickly, no I/O, cache decisions from an external service for the hook to look up. That kept the change small. The alternative is a blocking-capablefunc(ctx context.Context, k key.NodePublic) bool, which needs the check moved out of the lock, a ctx canceled atServer.Close, and deduplication of the client's once-a-second meow retries while a decision is pending (the client's 10 second ping timeout bounds it either way). Happy to rework to that variant if you'd rather have it; say which and I'll update this PR.Tests:
TestAllowClientcovers a listed key admitted without the hook, an approved key admitted with exactly one hook call across two pings, and a rejected key that gets no meowed and does reach the hook.TestAllowClientOnlychecks that a hook without anyAllowedClientsstill closes the server to rejected clients. Both run against the local DERP and STUN fromtstest/integrationlike the other server tests.go test ./...,go vet ./...andmake tidyare clean on Windows; the CI matrix covers the rest.No CLI change;
--allowis untouched.Updates #119