Skip to content

remote: trust only an installed tailscale, and prove every tailnet link - #75

Merged
krishhgg merged 14 commits into
mainfrom
fix/remote-trust
Oct 6, 2026
Merged

krishhgg merged 14 commits into
mainfrom
fix/remote-trust

Conversation

@krishhgg

@krishhgg krishhgg commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Four Greptile findings on Tailscale remote access.

An agent could choose the program that names the sender (#61, comment 4189642088)

The inbox gives the full session to a tailnet request that tailscale whois names as the owner. It ran whatever TOKENSTASH_TAILSCALE named, or the first tailscale on PATH, with the caller's environment. The inbox often inherits an agent's environment, so an agent could run a script that gave a local address to status and the owner's login to whois, then approve its own cards from a second local address.

  • Release builds run only a tailscale from where Tailscale installs it. On Linux that is /usr/bin, /usr/sbin, /usr/local/bin or the NixOS system profile. On macOS it is the app bundle, /usr/local/bin or /opt/homebrew/bin. PATH is never searched, and the error names the places it looked.
  • TOKENSTASH_TAILSCALE sits behind #[cfg(debug_assertions)], so the release binary does not contain the name. The integration test runs the debug binary and still uses it.
  • tailscale runs with a fixed PATH and none of the caller's environment except HOME, because the CLI reads variables of its own and finds lsof on PATH on macOS. I checked that the real Linux CLI answers status and whois with an empty environment.
  • SECURITY.md says what this covers and what it does not. It does not stop a process running as you from replacing the program where you can write to it (Homebrew's directories usually belong to you), or from reading the session file.

Test: remote::tests::the_tailscale_program_and_its_environment_are_not_the_callers.

Links could reuse an old ownership proof (#63, comment 4189607232; #71, comment 4189613086)

link_base kept a successful proof of the tailnet listener for 30 seconds. If the inbox stopped, or remote access went off and on, and another process took the address and port, a link with a card credential or the session could name it. The 30-second cache is gone. Each batch of links a command prints now rests on one fresh proof. A util::Links, made where the command already looks at the inbox on loopback, proves the address the first time a link needs it. Nothing outlives the command, and a command that waits makes a new one afterwards. A proof per link would have made a need with eight cards wait on nine proofs in a row (Greptile on this PR, comment 4189832693, fixed in 518d933).

While working on this I found that the inbox's own "Send the link to my desktop" button also built its link through link_base. That made the inbox probe its own tailnet listener from the one thread that answers the probe, so it waited out the 1.5 s timeout and then fell back to loopback. That link is now on loopback from the start. The notification shows on this computer, where 127.0.0.1 opens.

Tests: remote::tests::every_link_proves_the_tailnet_listener_again, cmd::inbox::tests::the_desktop_link_stays_on_loopback, and one_need_proves_the_tailnet_address_once in crates/cli/tests/remote_tailscale.rs. That last one puts a relay that counts connections on the Tailscale address. A need for three keys makes one proof there, where a proof per link made four.

A failed whois was kept for a minute (#63, comment 4189607267)

whois kept a failed lookup as "no login" for 60 seconds, so one Tailscale hiccup turned the owner's device away for a minute. Now only Tailscale's answers are kept, and a failure is asked again on the next request. The cache lock is no longer held while tailscale runs.

Test: remote::tests::a_failed_whois_is_asked_again_and_an_answer_is_kept.

whois held up the local inbox (#60, comment 4189643162)

The inbox ran whois, for up to 3 s, on the thread that answers every request. The reader thread that read a tailnet request now looks the sender up before it hands the request over, and handle reads the result. A device Tailscale cannot name still gets a 404, and this machine is still not looked up.

Test: a_slow_tailscale_lookup_does_not_hold_up_loopback in crates/cli/tests/remote_tailscale.rs. A device whose whois takes 2 s connects, and a loopback /verify must answer within 1 s. It fails with the lookup moved back to the main thread. The fake tailscale now takes its "slow status" flag from a file, since it no longer sees tokenstash's environment.

Checks

cargo clippy --workspace --all-targets --locked -- -D warnings, cargo test --workspace --locked, and scripts/leak-test.sh against both the debug and the release binary all pass.

🤖 Generated with Claude Code

RetriggerConfidence Score: 5/5 Tier: plus

The PR appears safe to merge; no new blocking issue was established.

What we checked:

  • Slow lookup blocks the local inbox: No. A tailnet reader runs the lookup before handing the request to the thread that answers local requests.
Summary

The PR limits which tailscale program the CLI runs and moves tailnet lookups off the inbox’s main thread.

  • It retries failed whois calls and replaces the 30-second link-proof cache with one proof per batch of links.
  • It sends the inbox’s desktop link to loopback.
  • krishhgg accepted the reader-availability risk because it requires a device on the user’s tailnet and slow lookups. krishhgg accepted the brief reused-link risk because a second proof would double the checks for that gap.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Tailnet request] --> B[Reader checks whois]
  B --> C[Inbox checks current setting and Host]
  C --> D{Login matches owner?}
  D -->|Yes| E[Serve as person]
  D -->|No| F[Return 404]
  G[Command builds links] --> H[Prove tailnet listener once]
  H --> I[Print link batch]
Loading

Reviews (3) · Last reviewed commit: "remote: prove the tailnet address once f..."

Comment thread crates/cli/src/remote.rs
krishhgg and others added 5 commits October 5, 2026 23:17
A read that found a key only in its session keyring (a copy keyring-rs's
keyutils store left) linked it into the user and persistent keyrings
without the lock a write takes. A store from another session between the
search and the links lost to the old copy, in both keyrings. A read that
finds the key linked into both keyrings now changes nothing. Any other
read takes the lock, looks again, and links what it finds then, so a key
stored meanwhile is the one read and kept.

An older tokenstash still running in another login session can still put
an old value back. Its read links its own session copy into the persistent
keyring, and its write does the same after updating that copy. No other
session may write that copy, and the kernel records no write time, so the
two cases leave the same state and no read rule is right for both. Reads
keep the user keyring's value, as before. doctor now names each key whose
two keyrings hold different values, or that has a copy in another live
login session (found in /proc/keys), and says to stop the older process.
troubleshooting.md says what cannot be fixed.

Co-Authored-By: Claude <noreply@anthropic.com>
store_generated kept another process's value only when the key's index
row was created at or after the stash read that found the key missing.
tokenstash import keeps the created time from its bundle, so an import
that landed between that read and the store looked unfinished, and need
replaced the imported signing or encryption key with a new one. The read
now takes a mark, the newest audit row id, and the store keeps the value
when a store, import or adoption of the key was recorded after the mark.
Each of those writes its index row and audit row in one transaction; the
adoption in need now does too. Audit ids only grow, so the check no
longer rests on one-second timestamps either.

Co-Authored-By: Claude <noreply@anthropic.com>
A compact service-account JSON holds its private key on one line, with
the key's line breaks written as the two characters \n. Redactor::add
made line patterns only for values with real line breaks, so a child
that decoded the JSON and printed the key put each line of it on the
agent's stream unmasked. add now also splits on an escaped \n or \r, with
the same length rules as real lines.

Co-Authored-By: Claude <noreply@anthropic.com>
The older copies line failed doctor whenever another login session held
its own copy of a key. Someone upgrading from 0.2.0 or 0.3.0 has idle
sessions holding such copies for a while, and they do nothing until an
older tokenstash runs there, so doctor exited 1 for no fault. The line
now fails only when a key's user keyring and persistent keyring hold
different values, which is when a read can return the old key. A copy
another session holds is printed as a passing line that says when it
matters and what to do. stray_copies returns each case separately so
doctor can tell them apart.

Co-Authored-By: Claude <noreply@anthropic.com>
The older copies check looked only at the keys this home's index lists.
A key stored under this home's service name with no index row (a lost
index, another home with the same service name) is still read and
adopted by need, and was never checked. The check now also takes every
key /proc/keys lists under this home's service name.

When /proc/keys could not be read, the check counted no copies in other
sessions and doctor said nothing about it. It now still compares the user
and persistent keyrings for the indexed keys, and the line says what it
could not check and why, without failing.

The line was printed only when a copy turned up, and could be printed
twice. doctor now prints one older copies line whenever the backend is
the kernel keyring: it fails when two keyrings hold different values for
a key, and otherwise passes with what it found, or with how many keys it
checked. reference.md describes the line.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread crates/cli/src/cmd/inbox.rs
Comment thread crates/cli/src/cmd/need.rs
krishhgg and others added 9 commits October 6, 2026 00:05
Greptile on #63, #65 and #67:
- An agent's need --force passed force for every key, even when its
  first check found no denial. A "no" given after that check was
  skipped, so a broad grant could deliver the key with no second-ask
  card. NeedOpts.ask_again now lists the NAME@identity entries that
  hold a reserved extra ask, and need sets aside only their denials.
  force is now only the person's own --force.
- In a multi-key call, every new card said "Asked again after you
  declined". need now adds that line only to the card of a key in
  ask_again.
- Running the same need NAME --force while its second-ask card waited
  failed with "already asked for again once". It now returns that card
  as a pending result with its link and next (Db::force_card). Once
  the person answers the card, the same command is an ordinary request
  and delivers the key.
- The extra ask was reserved by project and key name, while the
  denial check is per identity, so asking again for one declined
  identity used up the other's. The reservation now records the
  identity, and only a card for that identity spends it. A row written
  before this change has no identity and still counts for every
  identity. denied_here returns the identity it found the "no" under,
  resolved the way need resolves it, a generated secret included.

Co-Authored-By: Claude <noreply@anthropic.com>
…ntity's card

Greptile on #73:
- An agent asking again for a declined key that is stored gets a
  pairing or sensitive approval card, and only paste cards said it was
  a second ask. create_approval_task now takes the entries asked again
  and puts a sentence in front of the card's why for each one ("Asked
  again after you declined NAME, because you asked AGENT to."), on a
  new card and when the entry joins an open card. A key asked for the
  first time on the same card gets no such sentence. A new card that
  holds only keys asked again leaves out "First time this directory
  asks for stored keys".
- force_card returned the card that a reservation without an identity
  named, whatever identity the card asked for. Such rows were written
  before reservations recorded one, so a retry for personal could come
  back with the pending card for work. force_card now returns a card
  only if it asks for the identity being retried. The old row still
  spends the ask for every identity, so a retry for another identity
  gets the "already asked" error.

Co-Authored-By: Claude <noreply@anthropic.com>
Deny on an approval card called tasks::deny, which closes the card as it
is now. If an agent added a key to the card after the page loaded, Deny
closed the grown card and recorded the added key as denied without the
person having seen it. Allow already compared the card with the page's
`seen` list under the index write lock in answer_approval.

Deny on an approval card now takes the same path: answer_approval with
Decision::Deny and the page's `seen` list. A card that grew is refused
with "this card changed since you read it" and stays open. The agent's
card link still cannot close an approval card. Other cards' Deny is
unchanged. The new inbox test grows a pairing card after the page loads
and checks that a Deny on the old list is refused and one on the
current list lands.

Co-Authored-By: Claude <noreply@anthropic.com>
…s about

A confirmed action card is claimed while its action runs. The claim
lapsed after CLAIM_HOLDS_SECS (120) whatever the action was doing, so an
action that ran longer (`claude mcp add` on a slow machine) lost its
card. Another confirm could take it over and a decline could land while
the first command still finished. `confirm` now renews the claim from a
thread with its own index connection every CLAIM_RENEW_SECS (20) until
the action returns. A process that stops stops renewing, and the claim
runs out as before. The claim's token is now what identifies it, so a
renewed claim still finishes or releases its card.

expire_overdue marked a card expired while its confirmed action ran, so
finish_action, which needs a pending card, could not close it, and the
person saw "expired" for a change that happened. A card with a live
claim no longer expires. Once the claim runs out or is given back, it
expires on the next call.

If a confirm stopped after forget deleted the key but before the card
was closed, the card stayed open, and confirming it again deleted
whatever was stored under that name by then, including a key stored
since. Every store now gives the value it writes a random value id
(new column secrets.value_id). Before deleting anything, the first
confirm of a forget card records the stored value's id and time on the
card (new column tasks.acts_on) and commits that on its own. A later
confirm of the same card deletes only that value. If the name holds a
different value by then, it is kept and the card says so.

forget deleted the stash value and then the index row as separate
statements. A store landing between the two kept its new value and lost
its index row. Both deletions now run under the index write lock that
the store path holds around its own stash write and index row. The
forget logic moves to tokenstash_core::actions::forget.

New tests cover a claim kept past its hold time and freed once nothing
renews it, a card that does not expire while its action runs, forget
retried after a stop at either point with and without a key stored
since, and a store that starts between forget's two deletions.

Co-Authored-By: Claude <noreply@anthropic.com>
recent_audit_for showed an agent the rows for its path with ts at or
after the workspace's creation time. Both times are whole seconds, so a
directory replaced and paired again within the second of an old event
showed the new agent the old directory's key names and events.

A workspace now records the id of the last audit row written before it
(new column workspaces.audit_from, set when the workspace is inserted),
and the agent's view holds only rows after that id. Row ids only grow,
so no earlier row shows whatever its time. Workspaces recorded before
this column existed, or by an older version, get the last row up to and
including their creation second when the index opens; until then their
view starts after that second. The new test pairs a directory again in
the same second as the old one's event, and covers its own events in
that second and the fill-in for older records.

Co-Authored-By: Claude <noreply@anthropic.com>
init ran `claude mcp add` and `claude mcp remove` with no time limit.
An action card that runs one keeps its claim renewed while the command
runs, so a `claude` that hung kept the card claimed for as long as the
inbox ran, and the person could not decline it.

claude_mcp now polls the child against a 60-second deadline, the way
tailscale_json does. Its output goes to /dev/null, so it needs no reader
threads. Past the deadline it kills the child, waits for it, and returns
an error, so the confirm gives the card back. A command that finishes in
time reports how it exited, as before, and one that cannot start is
still a failure. When `claude mcp add` times out, init keeps its record
of the registration, because the stopped command may have made it and
`--undo` can remove a recorded one. A new test runs a fake `claude` that
sleeps past a 300 ms limit.

Co-Authored-By: Claude <noreply@anthropic.com>
…d whose keeper fails

When the first confirm of a forget card found no index row, it recorded
"-" on the card. A later confirm then deleted whatever stash value was
under the name, which could be a replacement written by a store that
stopped before its index row. Such a value has no id to tell it apart,
and the database must not hold a hash of it. A later confirm of a card
recorded as "-" now deletes no value that lacks an index row. It keeps
the value, closes the card, and says that a value is still stored under
the name and that a new forget card removes it. pin_action_target now
also says whether this call made the record, so the first confirm still
deletes as before.

confirm claims the card before keep_claim opens a second connection to
the index. If that open failed, nothing ran, but the card had to wait
for the claim to run out before it could be confirmed again or
declined. keep_claim now gives the claim back before it returns the
error, so every caller gets this. confirm calls it before perform and
returns its error directly.

New tests cover a retry that finds a replacement with no index row, and
a keeper that cannot open the index because the schema version is newer
than this build.

Co-Authored-By: Claude <noreply@anthropic.com>
The inbox asks tailscale whois who sent a tailnet request, and gives the
full session to a device signed in as the owner. It ran whatever
TOKENSTASH_TAILSCALE named, or the first tailscale on PATH, with the
caller's environment. The inbox often inherits an agent's environment, so
an agent could run a script that gave a local address to status and the
owner's login to whois, then approve its own cards from a second local
address. Release builds now run only a tailscale from where Tailscale
installs it, with a fixed PATH and no other variable but HOME.
TOKENSTASH_TAILSCALE is read by debug builds only, for the tests.

link_base kept a successful ownership proof of the tailnet listener for
30 seconds. If the inbox stopped, or remote access went off and on, and
another process took the address and port, links with a card credential
or the session could name it. Every link now proves the listener again.
The inbox's own "Send the link to my desktop" link is on loopback, since
the inbox cannot answer a proof of itself while it builds that link.

whois kept a failed lookup for 60 seconds, so one Tailscale hiccup turned
the owner's device away for a minute. Only answers are kept now.

The inbox ran whois, for up to 3 seconds, on the thread that answers every
request, so a slow lookup for a new device held up local page loads. The
reader thread that read the request now asks before it hands the request
over. A device Tailscale cannot name still gets a 404.

Co-Authored-By: Claude <noreply@anthropic.com>
Every link proved the Tailscale listener on its own, one after another.
A need that filed eight cards waited on nine proofs (the notification
and each card) before it printed anything.

Links are now built on a util::Links, made where a command already
looked at the inbox on loopback. It proves the Tailscale address the
first time a link needs it and reuses that answer for the rest of the
batch. It is a value the command holds, so nothing outlives the command.
need, the MCP secrets_request and human_request, run and rotate reuse
the notification's Links when they print their own links before any
wait, and make a new one after a wait. task_check builds all its
Replace links on one.

A new test turns remote access on at an address held by a relay that
counts connections and passes them to the inbox. One need for three
keys makes one proof there; with a proof per link it made four.

Co-Authored-By: Claude <noreply@anthropic.com>
@krishhgg
krishhgg changed the base branch from main to fix/action-cards October 6, 2026 00:15
@krishhgg
krishhgg changed the base branch from fix/action-cards to main October 6, 2026 00:26
@krishhgg
krishhgg merged commit b8cd808 into main Oct 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant