plex: a stored server becomes current only if its origin may carry the credential - #183
Conversation
…e credential Discovery already refused to activate a plaintext candidate, but restoring a stored session did not: installation gated on SourceRef::dialable, which is well-formedness — the port fits an i32, the address parses — and says nothing about whether a token may ride that scheme. A legacy entry holding only address and port has its origin synthesised as http, and servers::activate published it as the current credentialed client without a second look. The transport then did its job and refused every token-bearing request, so the session was not merely artwork-less: hubs, metadata and library browsing all failed together, and the only trace was one `refused plaintext PMS credentials; HTTPS required` line. That is the sequence in the log attached to #107. CredentialPolicy is unchanged and remains the single cfg! for this rule; what changes is where it is asked. A CREDENTIAL_ELIGIBLE bitmask now sits beside ACTIVE and is written in activate(), the one point every registration path reaches — stored primary, roster, controlled bootstrap, discovery activation and endpoint repair alike — and set_current refuses a slot that is not in it. An ineligible origin is NOT dropped. A user whose only stored entry is plaintext would otherwise lose their server with no way back, so the entry stays active as recovery metadata with a blank token, visible to Sources and to the rediscovery loop; a later eligible HTTPS registration re-points the same slot, clears the verdict and restores ordinary activation. Developer builds under AllowPlaintext behave exactly as before. client()'s doc no longer claims an unchanged contract: its precondition moved from "something installed" to "something credential-eligible installed". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review Two things worth attacking directly:
|
![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: c09fc23ea4
ℹ️ 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".
| pub(crate) fn register_captured_origin_with_connection(machine_id: &str, origin: &Origin, | ||
| token: &str, pin: Option<&ResolvePin>, client_id: &str, connection: ConnectionFacts) -> ServerId { | ||
| let id = register_lazy(machine_id, origin, token, pin, connection, &|| client_id.to_owned()); | ||
| let policy = CredentialPolicy::build(); |
There was a problem hiding this comment.
Update rust-modules/src/plex/origin.rs:112-114 alongside this new call: that documentation still says only the two auth live edges invoke CredentialPolicy::build() themselves, but this commit adds several registry entry points that do so. This leaves the credential-policy architecture documentation factually incorrect and can mislead future security-policy audits.
AGENTS.md reference: AGENTS.md:L104-L105
Useful? React with 👍 / 👎.
origin.rs said only the two discovery live edges call CredentialPolicy::build() themselves. #183 added three more callers inside servers.rs's registry (register_origin, register_captured_origin_with_connection, register_pinned_with_client_id), so a stored origin is graded by the same policy a discovered one is — the doc now says so, and names the transport boundary (http::credential_transport_allowed) as the third, per-request ask. session.rs's SourceRef::dialable() doc said a plaintext credential is refused "solely... at the point of the actual probe or request — never here [at registration]". #183 is exactly the fix that makes that false: registration now asks CredentialPolicy too, before a stored origin can become the current client. Updated to describe both decision points. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eab7e57bab
ℹ️ 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".
| if credential_eligible && !current().is_set() { | ||
| CURRENT.store(id.0 as u32, Ordering::Release); |
There was a problem hiding this comment.
When a stored primary is rejected as plaintext but another stored source is credential-eligible, that fallback source becomes CURRENT; a later endpoint repair that re-points the intended primary to HTTPS reaches this branch with current().is_set() == true, so it never restores the primary. The RegistryPlan::Endpoint handler in auth.rs only registers, describes, and publishes the probe result, without calling set_current, leaving current-based artwork, server-info, and route fallbacks pointed at the other authority until an explicit switch or restart.
Useful? React with 👍 / 👎.
Closes part of #107.
The defect
Discovery already refused to activate a plaintext candidate. Restoring a stored session did not.
Installation gated on
SourceRef::dialable, which is well-formedness — the port fits ani32, the address parses — and says nothing about whether a token may ride that scheme. A legacy entry holding onlyaddress+porthas its origin synthesised ashttp, andservers::activatepublished it as the current credentialed client without a second look.The transport then did its job and refused every token-bearing request. So the session was not merely artwork-less: hubs, metadata and library browsing all failed together, and the only trace was one
refused plaintext PMS credentials; HTTPS requiredline. That is exactly the sequence in the log attached to #107.This is reachable in a shipping build.
CredentialPolicy::build()isHttpsOnlythere — but that only made the transport refuse; it never stopped such a client from becoming active.The change
CredentialPolicyis unchanged and remains the singlecfg!for this rule. What changes is where it is asked.A
CREDENTIAL_ELIGIBLEbitmask now sits besideACTIVEand is written inactivate()— the one point every registration path reaches: stored primary, roster, controlled bootstrap, discovery activation and endpoint repair alike.set_currentrefuses a slot that is not in it.An ineligible origin is not dropped. A user whose only stored entry is plaintext would otherwise lose their server with no way back, so the entry stays active as recovery metadata with a blank token, visible to Sources and to the rediscovery loop; a later eligible HTTPS registration re-points the same slot, clears the verdict and restores ordinary activation. Developer builds under
AllowPlaintextbehave exactly as before.client()'s doc no longer claims an unchanged contract: its precondition genuinely moved from "something installed" to "something credential-eligible installed". No reachable panic was found — production paths useclient_for/client_opt, and directclient()calls are test support — but the stale claim was worth correcting before someone relies on it.Verification
Gates on the committed tree, working tree empty before and after:
--no-default-features(the shipping set, and the feature set this defect lives in): clean.ci/check-deps.sh: all gates green.New tests cover the plaintext stored source under
HttpsOnly, the legacyaddress+portsynthesised-HTTP path, the unchangedAllowPlaintextbehaviour, a cold boot carrying one HTTP-only stored source through install plus boot activation, and recovery of retained insecure metadata to HTTPS.Relationship to #182
Independent. #107 has two separate causes and this is the other one; neither PR depends on the other, and the files do not overlap.
🤖 Generated with Claude Code