Skip to content

Replace a server's accepted short IDs at runtime and revoke sessions - #18

Merged
myleshorton merged 2 commits into
mainfrom
fisk/set-short-ids
Oct 21, 2026
Merged

myleshorton merged 2 commits into
mainfrom
fisk/set-short-ids

Conversation

@myleshorton

@myleshorton myleshorton commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Lets a running samizdat server change which client short IDs it accepts, without rebinding its listener. Sessions already authenticated with a removed ID are ended.

This is the transport half of Lantern's helper invites direct mode. A volunteer serves specific paired users. Each pairing gets its own short ID, which lantern-cloud rotates every 15 minutes, and the volunteer's app pushes the current set to its running inbound about every 150s. Until now the short-ID set was fixed at NewServer, so the only way to change credentials was to rebuild the server, which drops the port and resets every client.

Changes

  • (*Server).SetShortIDs([][8]byte) error:
    • atomically replaces the accepted set (an atomic.Pointer holding a private copy);
    • new handshakes use the new set immediately;
    • an empty set is rejected and the previous set stays in force, as NewServer already does.
  • (*Server).ShortIDs() [][8]byte returns a copy of the current set.
  • Revocation:
    • The server now records each authenticated session's short ID.
    • SetShortIDs closes the connections of sessions whose ID was removed. It closes them after releasing its lock, and stream handlers see an ordinary disconnect.
    • A session that verified against the old set and registers after the swap is refused at track time, so no session survives revocation by racing it.
  • Compatibility: no behaviour change for callers that never call SetShortIDs. The configured set is copied at NewServer, so mutating the caller's ServerConfig.ShortIDs slice afterwards no longer silently changes what the server accepts.

Tests

  • shortids_test.go, end to end through a real client and server:
    • a client authenticates with an ID added by SetShortIDs, and is rejected after the ID is removed;
    • a held session is closed on revocation;
    • an empty set is rejected;
    • ShortIDs() returns a copy.
  • TestTrackSessionRefusesIDRevokedAfterVerify covers the refuse-at-track-time branch deterministically.
  • go test -race -count=1 ./... passes. The new tests also passed -count=20 under -race.

Pre-PR review (local Codex gate)

  • Rounds: 1 — final: round=1 CODEX GATE: verdict=approve critical=0 important=0 minor=0 reviewed=12
  • Exit: approve
  • Fixed: none
  • Dismissed: none

golangci-lint's default errcheck reports unchecked Close and SetReadDeadline calls, 17 in the new test file and one in server.go. The repo has no lint config, its CI doesn't run golangci-lint, and existing code follows the same style, so I left them.

A separate per-task review (spec compliance plus code quality) also ran. It confirmed the swap/scan versus track-time re-check closes the verify-then-revoke race. Its minor findings (the track-time test, the doc wording, a goroutine leak in the test relay) are fixed in 46dbfb0.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KqE58aYaai3u8CFZe5nXdE

Summary by CodeRabbit

  • New Features
    • Added runtime management of server short IDs, including adding, replacing, and revoking IDs.
    • Revoking an ID now closes active sessions authenticated with it.
    • Added safeguards to reject empty ID updates and protect configured ID values from unintended changes.
    • Authentication now prevents connections using IDs revoked during verification.

Adam Fisk and others added 2 commits October 21, 2026 11:54
Copilot AI balanced review requested due to automatic review settings October 21, 2026 17:58
@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 39430b3a-43ce-4531-8fac-0d16a7f8a115
📥 Commits

Reviewing files that changed from the base of the PR and between 18bd41c and 46dbfb0.

📒 Files selected for processing: 2
  • server.go
  • shortids_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The server now supports replacing its active short IDs. It tracks authenticated sessions, closes sessions associated with revoked IDs, and checks that an ID remains authorized before registering a session.

Changes: Short-ID Management

Layer / File(s) Summary
Short-ID state and update API
server.go, shortids_test.go
Server initializes and atomically replaces its short-ID set. SetShortIDs rejects empty updates, and ShortIDs returns a copy. Tests cover empty updates and input/output slice copying.
Authentication and session revocation
server.go, shortids_test.go
Authentication checks the current IDs. The server registers authenticated connections, refuses registration when an ID was revoked, and closes sessions authenticated with revoked IDs. TCP tests cover adding and removing IDs, session closure, and revocation during verification.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Server
  participant Caller
  Client->>Server: Authenticate with short ID
  Server->>Server: Verify ID and register session
  Caller->>Server: SetShortIDs with replacement IDs
  Server->>Server: Remove sessions with revoked IDs
  Server-->>Client: Close revoked session
Loading
Merge Risk: ⚪ Minimal · up to 46dbf

Short-ID rotation appears mergeable after normal checks; no unresolved behavior issue was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main changes: runtime replacement of accepted short IDs and revocation of affected sessions.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No blocking issues were identified in credential replacement, synchronized revocation, or the targeted tests.

No open issues 🎉

What changed in this PR

Enables runtime updates to accepted client short IDs without restarting the server, supporting helper-invite credential rotation.

Changes:

  • Adds atomic credential replacement and a copy-returning accessor.
  • Revokes affected sessions and prevents registration from bypassing revocation.
  • Adds tests for updates, revocation, defensive copies, and registration timing.
File Description
shortids_test.go Tests credential updates, session revocation, and copy isolation.
server.go Adds runtime credential management and synchronized session tracking.

Review effort: Balanced

@myleshorton
myleshorton merged commit 34a7d25 into main Oct 21, 2026
4 checks passed
@myleshorton
myleshorton deleted the fisk/set-short-ids branch October 21, 2026 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants