feat(auth): add scoped temporary credentials - #5
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50f54c6b87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
acking-you
commented
Aug 17, 2026
acking-you
left a comment
acking-you
left a comment
Owner
Author
There was a problem hiding this comment.
Review
The slot table, derived secrets, namespace scoping, and WAL-before-expose path are in good shape. Temporary keys cannot reach admin operations, and same-name services in different namespaces do not collide.
Two issues should be fixed before merge: root rotation does not bind the decrypted first frame to the current administrator key, and first-frame replay protection is too weak for non-idempotent admin mutations.
Note on the existing inline comment in src/pb_server/client.rs: subscriber revocation is already raced at the subscribe dispatch site (tokio::select! around handle_client_conn). That path looks correct. Please still add a regression where credential A only connects to a service registered by B, then A is revoked — the current data-stream test uses one credential for both sides.
Suggested test additions
- In-flight V2 admin handshake that has already sent the prefix / derived the old key, then
root-key rotate, then completes the frame: must fail, must not bind the new admin lease. - Replay of a captured
key issuefirst frame after the bloom window (or after a relay restart): must not mint a second credential. - Cross-credential subscribe revoke, as above.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17217795df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54e28d756c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c17d0b3a30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
acking-you
commented
Aug 18, 2026
acking-you
left a comment
acking-you
left a comment
Owner
Author
There was a problem hiding this comment.
Summary
Second-pass review of the current head (c17d0b3), including the three new Codex comments posted after that commit.
c17d0b3 closed the previous installer / isolated-first-frame / desktop-auth-dir / 300s Bloom-window threads. Two additional leftovers remain in the same areas, and the three new Codex findings are valid.
I will fix all five in the next commit:
- Isolated-relay legacy continuation frames still checksum with the process credential.
- First-flight Bloom retention still does not cover a max-future timestamp at the end of a rotation window.
- Rotated root keys that contain NUL persist and then panic in
set_process_msg_header_key. - Status reads are not raced against credential cancellation.
--force-init-admin-keyoverwritesadmin.keywithout resetting snapshot/WAL, so the next start enters safe mode.
Issue counts by severity
- bugs: 2 (this review)
- already-open Codex P2s to fix in the same pass: 3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86037cbf67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61399b3969
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e770a7d256
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0010df8e97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Auth errors after an already-accepted salt no longer reuse the first response nonce. Namespace stream counts stay until the client stream deregisters, even if the registration control socket drops.
Authentication and replay admission share one mutex off the Tokio worker. Unavailable admissions omit a session, restored Bloom generations age from loaded records, and a recovery MSG_HEADER_KEY is not written unless it decrypts existing state.
Replay waits stay off Tokio workers. Limited and stale-root salts are reserved before a nonce-0 error. Recovery keys must decrypt WAL-only state, and UI config rolls back if persistence fails.
Limited first flights no longer persist unique salts after the Bloom budget is full, and they omit a nonce-0 error session. The container entrypoint leaves admin.key unset when encrypted state remains so the runtime can verify a legacy recovery key.
Revoked or expired first flights reserve their salt before a nonce-0 error. High-slot expiries are scheduled instead of scanned every tick. Admin retries stay pre-send, connect fails without a credential, and truncated replay records fail closed.
abort_actor now times out instead of spinning if AuthStateInner does not drop. pb-mapper register and status return a nonzero exit status when the process credential is missing or rejected.
Dropping a registration task now aborts every control-pool worker. Compose upgrades keep the machine-derived key and persist the legacy /var/lib/pb-mapper-server path.
Accept no longer walks every live connection handle. Timing-wheel fast-forwards collect due tombstones and merge them in one sort.
Drop scheduled high-slot maps, merge-sorted tombstones, and split credential/endpoint maps. Keep nonce-0 uniqueness, WAL-first recovery, and pinned tunnels.
Keep types in the auth root. Move config, key loading, snapshot/WAL/fs, and actor lifecycle/epoch into focused files. Share reset/rotate wipe, snapshot construction, and recovery-key persistence.
Resolve the register-pool credential before spawn, put every control worker in one JoinSet, and reuse ClientHeaderSession::exchange plus a shared v2 frame reader. FFI now keeps handle and pin together per role.
linux_default_auth_state_dir is only compiled off Windows/macOS, but its re-export and its test were gated on cfg(test) alone, so cargo test failed to build on macOS. Co-authored-by: Cursor <cursoragent@cursor.com>
The timing wheel scanned every bucket on a clock jump and walked every second in between, so elapsed time cost work even with nothing to do. Its entries also held `Weak` leases, which forced a separate owner map plus a `wheel_version` counter to tell a renewed entry from a stale one, and left the actual cleanup — cancelling the lease, marking the row, queuing the tombstone, freeing the row, dropping the metadata — spread across the actor tick, the GC sweep, revoke, and the rotation wipe. Entries now own a callback that performs one phase of a teardown and returns when it next wants to run. A key's whole life is one entry: phase one cancels the lease and marks the row dead, phase two frees the row and forgets the key. Dropping an entry runs whatever phases it has left, so expiry, revoke, GC, root rotation, and shutdown all reach the same code, and no call site performs cleanup. That removes the tombstone queue, the owner map, `wheel_version`, and `clear_retained_high_slot_entries`. Advancing is now one bucket per level that turns over, matching the one-second tick, with a single-pass drain reserved for a jump past the longest schedulable delay so a corrected hardware clock cannot spin for hours. Also give the identity types names: `KeyId`, `Generation`, and `SlotIndex` replace interchangeable integers, so `KeyId::new(generation, slot)` cannot take its arguments in either order and a key id cannot be used as an array index. All three are `#[serde(transparent)]`, so snapshots and the admin protocol keep their plain-integer encoding. Level and slot indices narrow to `u8`, `issued_epoch` was written but never read and is gone, and the slot table's layout, generation rule, and tombstone window are documented where the types are defined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wheel had a `HashMap<KeyId, Position>` beside its buckets so it could find a key's entry, which put credential identity inside a scheduler and made every insert maintain two structures. `advance` was worse: a clock jump past the longest schedulable delay folded every bucket in the wheel into one pass, exactly the whole-structure walk a timing wheel exists to avoid. The wheel now schedules opaque `Arc<Timer>`s and ticks one second at a time, draining one bucket per level that turns over. It holds the only strong references, so a timer runs when the last entry referring to it is dropped, and it never looks anything up: no key map, no positions, no identity comparison. 314 lines to 168. `Leases` keeps the `KeyId -> Weak<Timer>` map instead, which is what the business side needed all along. Renewing schedules the same timer again at the later deadline — the earlier placement still drains, but it is no longer the last reference, so it fires nothing. Revoking fires the retire timer early and leaves its scheduled placement inert, so the retention window that follows still runs on its own schedule. A clock correction past any schedulable delay drops the schedule wholesale rather than making the wheel walk itself. Each key now owns two independent timers, retire and reap, both scheduled at issue. The reap timer holds the lease `Arc`, because firing a timer consumes its callback: an `Arc` held by the retire stage would be released the moment that stage ran, and the slot table holds only a `Weak`, so a request during the retention window would find a vanished lease instead of learning the key was revoked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wheel kept a `HashMap<KeyId, Position>` beside its buckets and, on a large clock jump, folded every bucket into a single pass — a whole-structure walk, which is the one thing a timing wheel exists to avoid. Entries were also indexed by absolute deadline, so draining a coarse bucket had to recompute where each entry belonged and refile it. Buckets now hold routes instead. Scheduling decomposes the delay into base-`radix` digits and builds one nested `Link` per digit, coarsest outermost; each `Link::Relay` waits in one bucket and, when that bucket comes off the front, hands the leg nested inside it to the next, finer bucket. The chain *is* the route, so a tick is `pop_front`, `push_back`, and handing legs on — no arithmetic per entry, no key map, no positions. Level count is derived from the longest delay the wheel must support rather than hardcoded, and radix is a parameter. Placement is verified exhaustively rather than sampled: every delay from every starting offset must fire on exactly the tick it asked for. A radix decomposition is easy to get wrong by one bucket, and the first draft was. Locks: the wheel's per-level mutexes are gone, because they were not a requirement. They existed only so `Link::drop` could file its own successor, which needed shared access to the queues. Dropping now yields data and the wheel files it with `&mut self`, so the sharing — and the locks — disappear. `Timer::callback` keeps a lock: two routes share one timer, and `Arc<T>: Send` (which `tokio::spawn` demands) implies `T: Sync`, so `Cell` will not do. The common path skips it anyway, since `Drop` has `&mut self`. Replace the remaining std locks with parking_lot. Nothing used poisoning — every call site was `unwrap_or_else(|poisoned| poisoned.into_inner())`, plus a `recover_lock` helper to hide the `LockResult`. All of that goes, along with an unreachable "state is poisoned" error path in the FFI crate. Document the reasoning in docs/rust-shared-mutability-and-locks.zh-CN.md: `Arc` governs lifetime, `Send`/`Sync` govern cross-thread safety, interior mutability governs writing through `&T` — three orthogonal mechanisms — and the judgement order that follows (can it be exclusive? must it cross threads? integer or critical section?), worked through the locks above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `case` that decides whether to run the Rust job matched `src/*` and a root-level `Cargo.toml`, and nothing else. Once sources move under `crates/`, no path would match: every subsequent commit would report green without having been compiled or tested. Add `crates/*`, plus a `*/Cargo.toml` arm for per-crate manifests — the existing unanchored entry only ever catches the root one. `ui/native/*` still precedes it, so the FFI crate keeps landing on the Rust side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
None of this has a consumer, and carrying it into the new crates would just relocate the confusion: - Five `common::error::Error` variants (`Stm*`, `Lsn*`) that nothing has constructed since streams and listeners moved out to `uni-stream`. Verified by searching for both the variant and its generated snafu context selector. - `once_cell`, declared twice and imported nowhere; the code already uses `std::sync::LazyLock`. - Three `#[macro_export]` macros in `message/forward.rs` (`create_component` and the two `start_*_with_codec_key`) with no callers. This also retires the two `$crate::common::message::` paths inside them, which the split would otherwise have had to rewrite. - `Aes256GcmCodec::try_new_with_default_key`, uncalled, and the only reason `utils::codec` reached back into `common::checksum` — so `utils` no longer depends on `common` at all. - Profiles `wasm-dev`, `server-dev`, `android-dev` and the empty `[profile]` table. Nothing references them, the latter two are no-op `inherits = "dev"`, and there is no wasm target here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`rust-toolchain.toml` plus the eight `toolchain:` pins across the four workflows. Deliberately no edition change yet: the edition migration is its own commit, so a failure there cannot be confused with a toolchain regression. Two clippy lints new in this release had to be settled first. `cargo check` would not have caught either, only `clippy -D warnings` does: - `manual_is_multiple_of` in the timing wheel — taken as written, since `is_multiple_of` says what the modulo was checking. - `result_large_err` on five signatures in `impl ServerSecurity`. Allowed rather than boxed, with the reason recorded at the impl: I measured both halves, and at 264 bytes `ServerInitialMessage` is already wider than the 256-byte `ServerInitialError`, so the `Result` is sized by its `Ok` variant. Boxing the error would add an allocation and change a public type while leaving `Result` at 264 bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`trust-dns-resolver` 0.23.2 has been frozen since 2023; hickory is its continuation. Kept separate from the other dependency bumps because it is the one change here with runtime behavior attached. hickory removed the blocking `Resolver` outright, so the sync path can no longer reach the custom DNS servers and now goes straight to `std`. That is the behavior it already had: `get_custom_resolver` returned `None` inside a Tokio runtime, and outside one the caller fell back to `std` whenever the resolver was missing. The async path is unaffected and still prefers the custom servers. `build()` is fallible, so the resolver is a `LazyLock<Option<...>>` built once — retrying per lookup would just repeat the same failure — and `get_ip_addrs_async` reports a missing resolver through the existing fallback. The unused `get_ip_addrs` and `DNS_QUERY_PORT` go away with it; `NameServerConfig::udp_and_tcp` already implies port 53 and trusting negative responses, which is what the old call passed explicitly. Verified both paths at runtime against `localhost` and a public domain. `trust-dns-resolver` remains in the tree only via `uni-stream`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The four bumps that cross a compatibility boundary: snafu 0.8.7 → 0.9.2, hashbrown 0.16 → 0.17.1, base64 0.22.1 → 0.23.1, and dirs 5.0 → 6.0.0 in the FFI manifest, which declares it directly rather than through the workspace. None needed a source change. snafu 0.9 was the one worth checking, since every error type here goes through it, and all of `display`, `visibility(pub(super))`, `context`, `ensure!` and `.fail()` compile as written. hashbrown 0.15 and 0.16 stay in the lockfile via `uni-stream`; our own crates resolve to 0.17.1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FFI crate hardcodes its edition rather than inheriting the workspace one, so it needs the same change and turned out to carry most of the work. Mechanical, in four groups: - Four `extern "C"` blocks become `unsafe extern "C"`. - 28 `#[no_mangle]` become `#[unsafe(no_mangle)]`, all in the FFI crate. These name the symbols Dart dlopens, so I diffed `nm` output before and after: the same 28 `pb_mapper_*` symbols, unchanged. - 17 `env::set_var`/`remove_var` calls now need `unsafe`. Each got a safety note saying why it is sound rather than a blanket wrap, because the reasons differ: `set_process_msg_header_key` keeps the environment only as a mirror while the authoritative credential lives behind an `RwLock`; the CLI overrides run on the main thread before any task spawns; the tests either hold `PROCESS_CREDENTIAL_TEST_LOCK` or own the variable outright and restore it. - 17 nested `if let` blocks become let-chains, which edition 2024 stabilised — `clippy::collapsible_if` now flags them. Not in the plan's count: the `#[no_mangle]` attributes and the let-chains, both of which only appear once the earlier group compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bottom layer: checksum, config, conn_id, error, addr, codec, timeout. All seven land as git renames, so blame follows them. The old paths keep working through re-exports in `common/mod.rs` and a new `utils.rs`, so the rest of the tree is untouched and every later crate can move on its own. Three things had to move rather than be copied, because each was a cycle or would not survive the boundary: - `DataLenType` now lives in `core`, since `checksum` and `error` both name it while `message` depends on them. `message` re-exports it rather than redeclaring, or the two would be distinct names for the same width. - `PROCESS_CREDENTIAL_TEST_LOCK` moves to `core::test_support`, next to the credential it guards. It was `#[cfg(test)] pub(crate)` in auth, and a test-only item is invisible to another crate's tests — four future crates read it, so it is now unconditionally `pub`. - `replace_file` and `sync_parent_directory` become `core::durable_file`, reporting `io::Result`. They are general file primitives that happened to be written where the first caller was. `MngWaitForTask` did not come along: only `manager` constructs it, so it is now `manager`'s own error and `kanal` stays out of the bottom layer. The `Error` selectors widen from `pub(super)` to `pub` — 13 of them are built from other crates. Also dropped `socket2` and `futures`, declared but referenced nowhere, and gated `lib.rs`'s `mod tests` behind `#[cfg(test)]` — it was compiling into release builds. 136 tests still pass, now 87 in the old crate plus 12 that moved to core. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All 17 auth files, as git renames. It is the largest subsystem and the most self-contained — its only outward reference was `checksum`, now reached through `pb-mapper-core` — so it gains the most from standing alone. The 11 platform `cfg` blocks are all in here too. `pub(in crate::common::auth)` becomes `pub(crate)` throughout: the same scope, now that the module is the crate. Four items the protocol layer calls had to widen to `pub` — `admin_key`, `derive_key`, `derive_previous_key`, and `admin_cancellation_token` — which the compiler found by reporting them as dead code from inside the crate before reporting them as private from outside it. `replace_file` is gone from auth; `sync_parent_directory` stays as a four-line wrapper mapping `core`'s `io::Result` onto `AuthFailure`, so its callers here are unchanged. The replay log in the protocol layer calls `core` directly now, which drops two `io::Error::other(error.to_string())` round trips that existed only to get back the error it started with. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Message framing, the v2 secure session, forwarding, and `buffer`, which moves with them because the framing is its only consumer. It sits above auth rather than beside it: `message::secure` reads `AuthRuntime` and `KeyId`, and auth never looks back the other way. `DataLenType` is re-exported from `core` instead of redeclared, so the protocol and the checksum that validates it name the same type. The `#![allow(async_fn_in_trait)]` that used to sit on `mod common` in the old lib.rs is now at this crate's root, where the traits that need it live, with a note on why sending them across tasks is not a case that arises. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The last three crates, and the end of `src/`. `pb-mapper-server` and `pb-mapper-client` are peers, not layers — they have never referenced each other, in either direction. `manager` moves into the server crate, whose routing runtime is its only caller. `pb-mapper-cli` owns the binary, plus the integration tests and examples, because it is the crate that depends on everything they exercise. The binary is still called `pb-mapper`: the crate is named differently but the file is still `src/bin/pb-mapper.rs`, and `cargo build --bin pb-mapper` resolves it from the workspace root, so the release workflows, both Dockerfiles, and the install scripts need no change. Verified by building it and checking the path. The 26 downstream files now name the crate they actually want instead of going through a facade. In the three error modules that alias the old module and then write `common::error::Error` on nearly every variant, the alias is retargeted rather than the variants rewritten. The FFI crate replaces its one `path = "../../../"` dependency with the five crates it uses. `libpb_mapper_ffi.so` and all 28 exported symbols are byte-identical, which is what the Dart loader and the release-ui hash checks depend on. `test_serde_mapper_header` moved to `protocol::command`, next to the type it pins; it was in the old crate root and the test count caught it going missing. 136 tests pass, matching the pre-split baseline exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The FFI crate has denied these since it was written, because a panic
crossing the C ABI is undefined behaviour rather than a stack trace. Every
crate is now reachable from that boundary, so the deny moves to
`[workspace.lints]` and the FFI crate inherits it instead of keeping its own
copy.
A `clippy.toml` exempts test code — a panicking assertion is a failing test,
which is the point. That covers `#[cfg(test)]` modules but not `tests/` or
`examples/` targets, whose entire body is test code, so those carry a
file-level allow with the same reasoning.
That left nine real sites in production paths. Six are now errors rather
than panics, and each was already inside a function returning `Result`:
credential and v2-prefix width conversions that parse bytes from an
unauthenticated peer, the administrator-key UTF-8 conversion, and the state
blob's nonce, which reads a file that may have been truncated. Two accessors
replace `expect("v2 session material")`, where `Some` and `HeaderProtocol::V2`
are the same condition without the type saying so. The timing wheel's
`schedule` returns `()`, so its unreachable case now takes the same path as
an out-of-range delay: drop the timer, which fires it — the safe direction
for a credential deadline.
`replay.rs` keeps a module-level allow, with the reason recorded there: its
conversions slice a `[u8; 40]` at constant offsets or take SHA-256's 32-byte
output, widths the array types already fix, in functions returning `()`.
Two more `mod tests` were missing `#[cfg(test)]` and compiled into release
builds; the lint is what surfaced them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`claude-code-review.yml` feeds `AGENTS.md` and `CLAUDE.md` into every PR
review, so a stale path there actively misleads. Both now describe the crate
layout, and the paths in them exist.
Most of what was wrong predates this refactor. `CLAUDE.md` documented
`src/common/stream.rs`, `src/common/listener.rs`, and `src/utils/udp.rs`,
all deleted when the stream abstractions moved out to `uni-stream`; five
Flutter files that do not exist, including a "Server Management" section for
a view that was never there; `LocalService` for a type named `LocalServer`;
and four role commands where there are five — `admin` was missing from both
files. It listed three environment variables against roughly twenty read by
the code, so it now names the common ones and points at the `PB_MAPPER_*`
constants for the rest. Three places claimed web/wasm support; there is no
wasm target and the UI loads a native library over `dart:ffi`, which the web
cannot do. The dead build-profiles section went with the profiles.
`AGENTS.md` was the only file naming both the edition and the toolchain
version, and now points at `[workspace.package]` and `rust-toolchain.toml`
instead, so the next upgrade does not have to touch prose.
The auth docs used brace expansion — `auth/{actor,persistence,...}.rs` —
which expanded to `actor.rs` and `persistence.rs`; both are directories.
The README badges and the intro doc said Rust 2021. Both architecture
diagrams were regenerated, since their six boxes were labelled `src/...`.
`ui-cli-mode-spec.md` and `rust-async-send-sync-pin-deep-dive.md` are
narrative articles whose line references had already drifted before this
work — one cites `pb_server/mod.rs:932` in a 384-line file. They carry a
historical-archive note rather than line-by-line fixes, which would drift
again on the next change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
acking-you
force-pushed
the
feat/temporary-credential-auth
branch
from
August 21, 2026 13:52
58c6595 to
7b7cebe
Compare
Comment on lines
+113
to
+118
| *inner.admin.write() = AdminState { | ||
| key: new_key, | ||
| lease: Arc::downgrade(&new_admin_lease), | ||
| }; | ||
| if inner.sync_process_credential { | ||
| set_process_msg_header_key(Some(&new_key_string)).map_err(AuthFailure::internal)?; |
There was a problem hiding this comment.
Permanent admin lockout if set_process_msg_header_key fails mid-rotation.
In actor_rotate_root, inner.admin.write() is updated to store only a Weak reference to new_admin_lease (line 113-116), and new_admin_lease is a local Arc — the only strong owner at that point (the caller's admin_lease isn't swapped in until line 122, and old_admin_lease isn't cancelled until line 121).
If set_process_msg_header_key (line 118) returns Err — which can happen for ordinary reasons like a permissions/env-var failure — the ? propagates immediately and the function returns. new_admin_lease then goes out of scope and is dropped, since nothing else holds a strong reference to it. inner.admin.lease is left as a permanently dangling Weak.
At this point the new admin key has already been durably written to disk (write_admin_key succeeded in the .and_then chain above) and is live in inner.admin.key, but no valid lease exists for it and none can ever be upgraded again. Every subsequent call to validate_admin_authority will fail with administrator_key_rotated ("active administrator credential lease is unavailable"), because inner.admin.read().lease.upgrade() returns None. This locks out all administrator operations until the process is restarted, even though the rotation itself technically succeeded.
The fix should ensure new_admin_lease (or an equivalent strong owner, e.g. by promoting it into admin_lease/cancelling old_admin_lease before the fallible set_process_msg_header_key call, or treating that failure as non-fatal/best-effort) survives this error path.
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.
Summary
Add expiring, renewable temporary credentials without changing the existing persistent-control and on-demand data-connection topology. The administrator key remains the sole root credential, while temporary credentials can register, connect to, and inspect only their own namespace.
Related Issues
No linked issue.
Change Type
What Changed
Verification
cargo fmt --allcargo clippy --all-targets -- -D warningsCommands run:
cargo fmt --all cargo test --workspace --all-targets cargo clippy --workspace --all-targets -- -D warnings bash -n scripts/install-server-github.sh scripts/install-server-gitee.sh scripts/release/entrypoint/pb-mapper.shAdditional checks:
quick_validate.py.toastificationdependency, which requires Flutter 3.38 or newer; CI uses Flutter 3.44.9.0700auth state directory and0600administrator-key and instance-ID files.Compatibility / Risk
/var/lib/pb-mapper-server/msg_header_keyis migrated automatically when no new administrator key or environment override exists.Screenshots / Logs
Not applicable; the user-facing change is limited to credential validation text and the setup flow.