Repository navigation
T54 — Prompt the attached human on ask, refuse when nobody is attached, and audit every decision - #1891
Conversation
NET-046: every dynamic ingress decision gets one record in an append-only log under the daemon's state directory, one JSON line each naming the box, the port, the decision, who made it, the outcome and any refusal reason. - audit.rs: the append-only log itself (audit/decisions.log), one best-effort record per decision — a decision already made is never refused because its record could not be written; the failure is warned about instead. - diag.rs: the state dump ships the log's tail beside the session records, skipping cleanly when the daemon has recorded no decisions yet. The session-side wiring that writes these records lands next.
NET-045: a session decided 'ask' needs a human to answer, and the human that can answer is whoever is attached — so the session host learns to hand an ask to its binding, which renders the shell-exit prompt's own dialog over the same channel halves. - BindingMsg::AskExpose: the host hands the binding a port and the reply the answer goes back on. - The binding stashes the ask in its mainloop and renders the dialog at the next iteration's head, because the select's own arms borrow the channel halves the dialog needs. Deny stands highlighted, so a reflexive Enter — a cancel, an EOF, a client that leaves mid-prompt — fails the request closed. - The dialog is raced against the shed: a stalled client being discarded is not a client to wait on. - Message::AskExpose + HostHandle::ask_expose: the session's route to whoever is attached, answering None when nobody is, which is the fail-closed nobody-attached case rather than an error.
NET-045: a runtime port-publish request a box decided 'ask' now reaches whoever is bound to the session's host, and their answer is the answer the in-box caller gets; nobody attached fails the request closed with the typed nobody-is-attached error. Every decision — the box's own, the human's, and the daemon's fail-closed one — is logged and audited (NET-046). - session.rs: the actor routes the ask to the host and parks the caller's reply in a pending-ask book, because parking the actor on a human would hang policy reads, manager probes and shutdown behind a dialog nobody may be looking at. The ask runs on a spawned task and comes back as AskAnswered, which answers the parked reply: an allow runs the publish half, a deny is the box's own deny answer in the human's hand, and nobody attached is the typed refusal. stop_running drains the book fail-closed so a session that goes away strands no asker. - The policy gate moves out of publish_exposed_port into the decision, so the resumed ask publishes without re-deciding. - session_host.rs: the host bridges the binding's answer on a spawned task, never by parking its own loop — a parked host stops pumping the pty and answering probes, and the probes that decide is_alive would report a live host dead. - answer_expose writes the one info line per request (now naming who decided it) and the decision's audit record beside it; DecidedBy gains a Display so the line and the record spell the decider the same way. - env.rs: the expose handler's docs say the reply can now wait for the human, and is the applied answer either way.
NET-045 end to end against a harness server, in the session-host tests beside the binding they exercise: - expose_ask_prompts_attached_human: a request a box decided ask renders the exit prompt's own dialog on the attached client's terminal, naming the box and the port; an allow publishes — the same request shape an allow box's takes — and the prompt, the answer, and the decision's audit record all say so. - expose_ask_without_client_refused: nobody attached is refused with the typed nobody-is-attached error, the switch is asked nothing, the refusal says its log line with the typed reason, and the audit record names the daemon as the decider. - expose_unenrolled_decision_audited: on the un-enrolled host, the allow that published and the deny-all default's refusal are both one parseable record each, naming box, port, decision, decider, outcome and the typed reason. - ask_expose_without_a_binding_answers_no_one: the host-level arm — an ask reaching a host nobody is bound to answers None rather than parking. session/tests.rs: the dynamic-ingress fixtures the new tests reuse (the request builder, the finalize driver, the stand-in forwarder) become pub(crate) — the sibling test module reaches them through crate::session::tests, which session.rs now declares pub(crate) under test.
The strict lint set denies a bare let-underscore on a must_use, so each ignored oneshot send the ask path adds says why its result is not the fact under test: an asker that is gone has nothing to answer, a terminal that cannot take the lead-in cannot take the dialog, and the host this test minted is not the test's own subject.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
/revise Round 1. All lanes are green at 70af41b. The daemon layers and their three tests are in. Four defects remain in the ask path.
Optional: give a human's deny its own refusal reason, distinct from Out of scope this round: the CLI layer ( |
|
Plan amended (T54 #1669): the CLI layer ( |
The dialog held the binding off its own mailbox, so a session that printed more than the mailbox holds while the human thought wedged the pty feed behind it: the host stopped reading the pty, and after OUTPUT_STALL_TIMEOUT it shed the very client the dialog was waiting on, answering the ask as the human's refusal. The dialog now runs beside a drain of the mailbox. The output parks in a buffer bounded at 4 MiB and flushes to the terminal once the dialog is down, so past the bound the mailbox fills as it does for any client that stopped keeping up and the stall bound - not an unbounded park - decides what happens to the binding. Two structural notes the drain forced: - The four teardown arms share one rendering, because a teardown that interrupts a dialog renders through the same path as one that arrives between dialogs. - The loop head revisits the dialog queue itself: an ask the dialog's drain queued while another stood has no message left to wake the select that would render it.
A parked ask was keyed by its port, so an answer popped whichever ask parked first on that port: two exposes of the same port - a retry after a failed hand-off is enough - sent the answer to the wrong asker, and the one it was meant for never heard. The session now mints an id per request and the continuation carries it back, so every answer reaches the ask it answers. A late answer that reaches no asker says one line and fabricates no decision, the same way the audit treats a box that is gone.
Four tests, one per hole the review found in the ask dialog: - the deny half of the dialog: the human denies, the publish is refused as policy would, and the audit records the decision as the human's, not the box's - a session that prints over the stall bound while the dialog stands is not shed: the output parks behind the ask, the answer after the bound publishes, and the terminal receives every byte - a live host nobody is attached to refuses the ask with the typed error, audits it as the daemon's refusal, and the box survives it - two asks on one port: the failed hand-off's continuation reaches its own ask, the standing dialog still publishes, and the late answer says its line and fabricates no decision The stall test needs the session printing while the dialog holds the channel's reader, so the host takes a cfg(test) feed for its own pty alongside a setter for the stall bound, and the actor takes a cfg(test) peek at the parked asks so a test can name the ask it means to answer.
|
Approved: no findings at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none: |
|
/revise Round 2. The round-1 asks hold at ae4108f: the mailbox drains during the dialog, the deny test passes, and asks are keyed by
Add the three test names as |
…-attached-human-on-ask-refuse-when-nob
A dialog that ends without the human's choice — a shed, a teardown, the host going away, a terminal that could not carry it — is the daemon's fail-closed refusal, not the human's deny. `Binding::run` broke every such path out of the dialog with the default `Refused` answer and sent it up as the human's own, so `resume_ask` audited `decided_by = attached-human` for a denial no human made. Now only a dialog that completed with a choice in hand answers; every other end drops the reply sender, which the host reads as nobody-attached, and `ask_prompt` maps a render/read I/O failure to no answer rather than a deny, while an explicit cancel stays the human's. The host's nobody-attached branch also says the plan's observability line now — one info line per refusal for want of a client, naming the box and the port — which is what tells a live host's refusal apart from the session's no-host shortcut in the detach test, and that test now asserts both. Tests: a shed mid-dialog and a supersede mid-dialog each end their ask with the typed nobody-is-attached refusal, audited as the daemon's.
russh's channel writer keeps the state of a write cancelled mid-send: ChannelTx parks its send in `send_fut` and answers the next `write_all` with the interrupted write's byte count, a count that can run past a shorter buffer's end - where tokio's `write_all` panics (`split_at`: mid > len). The shed notice and every farewell are the shorter buffers, so a binding shed while its output flooded (or mid-dialog) died in its own epilogue and left the client hanging on a channel that never closes. The state is per-writer, so the mainloop now replaces the writer at every race that drops a write - the dialog select, the held flush, and the plain relay race - and the shed notice rides a clean writer. The cost is the interrupted chunk alone, unsent with its window space, at most one max_packet_size, and only ever on a client that stopped reading. The shed-mid-dialog test caught this as a flake (a coin flip on the select's branch order) and now also asserts the notice still reaches the client after the flood.
…-attached-human-on-ask-refuse-when-nob
|
Round 2 of 6 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
An expose decided ask that reaches a human prompts them and applies their answer, and every decision lands in the local audit log; one whose dialog ends without a human answer — a terminal shed for stalling, a superseding attach, the host going away, a terminal that could not carry the dialog — is refused with the typed nobody-is-attached error and audited as the daemon's decision (decided_by=daemon, outcome=refused), never as the human's deny. The host's nobody-attached branch now says its one info line naming the box and the port, and the detach test proves both that the refusing host was live and that this line was said — so the live-host refusal is told apart from the session's no-host shortcut. A shed mid-dialog still closes the client's channel — EOF, exit status 255, and the stall notice delivered even when the shed dropped a write mid-send. Notes from the agent: Three asks done: (1) Binding::run carries Option, None until a dialog completes under the human; every other exit from the dialog select (shed, the four teardown arms, host gone) drops the reply sender, which resume_ask records as decided_by=daemon with AskNeedsAnswer; ask_prompt maps an I/O-dead dialog to None while an explicit cancel stays the human's Refused. (2) The same rule for a supersede mid-dialog. (3) The remote=None branch of Message::AskExpose says one info line per refusal for want of a client (session=, port), and the detach test asserts a live host (get_attrs) before the ask and then this line. The session.rs change is comment-only: round 1's code already routed None to the typed refusal. Out of scope kept out: an explicit human deny still refuses with DeniedByPolicy; no CLI change; no live_ingress teardown; no PR-body rewrite. Flake found and fixed while confirming the shed test: the first version failed on re-run because russh 0.63.3's ChannelTx keeps the state of a write cancelled mid-send — it parks the send in send_fut and answers the next write_all wi…[truncated] |
|
/revise Round 3. Round 2's asks are met: no answer means
Out of scope: log rotation and caps (the LOW at |
The write side of the log's symlink guard, which the read side (diag.rs's audit collector) already held: the state volume is guest-writable on a VM host, so a planted decisions.log — or a planted audit/ directory, which create_dir_all follows — would otherwise steer the daemon's per-decision appends into a target of someone else's choosing, with this daemon's privileges. The file now opens with O_NOFOLLOW exactly as diagnostics' read of the same file does, and the audit/ directory is refused unless symlink_metadata says it is a real directory; both refusals log the loss through the warn line that already names it.
async-dialog reports an input EOF as the same Selection::Cancelled a keyed Ctrl-C produces, so ask_prompt read the two alike and the audit recorded a denial no human made. Under a raw ssh -tt tty no keystroke produces channel EOF: EOF means the connection or client went away with the dialog standing, a terminal that can no longer carry it — nobody answered, and that is the daemon's refusal, not the human's deny. The dialog now reads through a wrapper that records the input EOF without touching the bytes, and ask_prompt maps it to no answer — the same road a shed or teardown takes — so resume_ask records decided_by=daemon with the typed nobody-is-attached refusal. A keyed cancel stays the human's own deny, and the session.rs comment no longer calls EOF the human's one.
The local held the render's outcome — an allow, a keyed cancel, an input EOF, an I/O failure — so a name that says only the keyed half reads as if the EOF arms below were unreachable.
…-attached-human-on-ask-refuse-when-nob
|
Round 3 of 6 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
An expose decided ask prompts the attached human and applies the answer; every way the dialog can end without a human's choice — nobody attached, a shed, a teardown, and now a client whose channel reaches EOF mid-dialog — is refused with the typed nobody-is-attached error and audited decided_by=daemon, while a keyed cancel stays the human's deny; every decision appears in the local audit log (on a VM-backed host, in-guest only, with the host-side record of an answered yes still T84's), and the log's append now refuses a planted symlink in the file's or the audit/ directory's place. Notes from the agent: Both asks of round 3 are done, each in its own commit; async-dialog was not touched. All four files are inside the task's layers. clippy-strict still reports 9 let_underscore_must_use hits on render_farewell's |
`append` checked `audit/` with `symlink_metadata` and then opened `decisions.log` by full path, `O_NOFOLLOW` on the final component only: on the guest-writable state volume a guest that swaps `audit/` for a link between the check and the open steers the daemon's append — with the daemon's privileges — through it into a target of its own choosing, and the check cannot see it. Nothing is checked now: the directory is opened `O_NOFOLLOW | O_DIRECTORY`, and the log is `openat`ed relative to that descriptor, also `O_NOFOLLOW`, so both components of the log's path are pinned at the moment they are opened and there is no window at all. A link standing in either place is refused at open, the same refusal `crate::diag`'s audit collector makes on the read side, and the warn that reports the loss names the half of the path that refused.
…-attached-human-on-ask-refuse-when-nob
|
Round 4 of 6 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
An expose decided ask prompts the attached human and applies the answer, is refused with a typed error when nobody is attached, and every decision appears in the local audit log — whose appends are now anchored to the audit directory the daemon opened itself ( Notes from the agent: Round 4 addressed the race the review named on |
|
Un-drafting. Round 4 (fa87d50) closes the review MEDIUM: |
There was a problem hiding this comment.
Own review and ARCH PASS at 84d1314; CI green; review concluded; no open threads.
T54 — Prompt the attached human on ask, refuse when nobody is attached, and audit every decision
slice: S13
kind: task
covers: NET-045, NET-046
size: M
est: 300 code, 180 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: the daemon's state dump carries the tail of the local audit log, with each decision, its box, its port and who decided
observability: one audit record per decision; one info log line per prompt shown and per refusal for want of a client
layers:
- daemon:
- crates/minimald/src/session_host.rs: a binding message prompts the attached client with the exit prompt's dialog, and answers no-one-attached when there is no binding
- crates/minimald/src/session.rs: routes an ask decision to the host
- crates/minimald/src/env.rs: relays the answer or the typed error to the in-box caller
- crates/minimald/src/audit.rs (new): the append-only local decision log under the state directory
- crates/minimald/src/lib.rs: declares the audit module
- crates/minimald/src/diag.rs: writes the audit log tail into the daemon's state dump
- crates/minimald/src/session_host/tests.rs: hosts the prompt and no-client tests
test: cargo nextest run -p minimald expose_ask_prompts_attached_human
test: cargo nextest run -p minimald expose_ask_without_client_refused
test: cargo nextest run -p minimald expose_unenrolled_decision_audited
- cli:
- crates/minimal/src/cmd/session.rs: on a VM-backed host the client records a yes answer for the port in the VM host daemon's table over its control socket before relaying the answer, so the publish that follows is admitted (NET-138)
test: cargo nextest run -p minimal ask_answer_recorded_in_host_table
produces:
- crates/minimald/src/audit.rs (entry point)
consumes: none
proof: An expose decided ask prompts the attached human and applies the answer, is refused with a typed error when nobody is attached, and every decision appears in the local audit log.
depends on:
- blocked by #1666
- blocked by #1711
Proof
A
min net exposefrom inside a box whosedynamic_ingressisasknow renders the shell-exit prompt's own dialog (deny highlighted, so a reflexive Enter fails closed) on whoever is attached to that box's session, and the human's answer is what the in-box caller gets: an allow publishes — same request shape an allow box's request takes — and a deny or cancel is the box's own deny answer; with nobody attached the request is refused with the typed AskNeedsAnswer error, decided by the daemon and never parked on a dialog nobody can see (the session actor and the host loop both route the ask off their own loops so probes, policy reads and shutdown never wait behind a human). Every dynamic ingress decision — the box's, the attached human's, and the daemon's fail-closed one — is one JSON record in the append-onlyaudit/decisions.logunder the daemon's state directory naming box, port, decision, decider, outcome and the typed reason, and the daemon's state dump ships that log's tail.Notes from the agent: CLI layer not implemented — the note cannot be built inside its layers, and the task's fourth test line (exit 4, no tests matched) reflects that. Two independent reasons: (1) the VM host daemon's control socket wire has no verb for it —
minimald_rpc::BoxControlRequestcarries only Register and Withdraw, Register always allocates a fresh address pair, and the publish gate the note targets reads a live row'sadmitted_ports(crates/minvmd/src/box_registry.rs), so a runtime port needs a new verb incrates/minimald-rpc/src/lib.rspluscrates/minvmd/src/control.rsandbox_registry.rs— all outside this task'slayers:; (2) the interactive client cannot learn the answer to record:session_via_ssh(crates/minimal/src/cmd/session.rs) attaches by shelling out tossh -tt, so the dialog bytes and keystrokes flow terminal↔ssh↔daemon and theminprocess never sees them. The daemon does know the answer — it audited it — so the natural shape for th…[truncated]Plan amendment and interim
ask_answer_recorded_in_host_table) moved to T84 #1897: recording the answer in the VM host daemon's table needs a control verb thatBoxControlRequestlacks. The exit-4 row in the table below is that removed line, so it is not a failure of this PR.live_ingressteardown belongs to #1884, not this PR.After round 2
AskNeedsAnswerand auditeddecided_by=daemon. An explicit keyed deny or cancel stays the human'sDeniedByPolicy.expose_ask_shed_mid_dialog_is_the_daemons_refusalandexpose_ask_superseded_mid_dialog_is_the_daemons_refusalpin this. After an interrupted write, the shed path replaces the channel writer, so the shed notice and close still reach the client.expose_ask_after_detaching_is_refused_by_the_live_hostasserts the host is live and that this line was logged.crates/minimald/src/session/tests.rschange only makes three test helperspub(crate)sosession_host/tests.rscan reuse them.Tests
cargo nextest run -p minimald expose_ask_prompts_attached_humancargo nextest run -p minimald expose_ask_without_client_refusedcargo nextest run -p minimald expose_unenrolled_decision_auditedcargo nextest run -p minimal ask_answer_recorded_in_host_tableVerify
min task run agent-verifypassed ([tasks.verify]).Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/minimald/src/session/tests.rsCloses #1669