feat(stelae): authenticate a registry with basic credentials - #1184
Conversation
The transport spoke bearer-token-or-anonymous, which is the shape GHCR wanted. The registry stele distribution is heading for authenticates every request with an HTTP Basic pair instead: access stays free and identity-less, and is still credentialed. `Options` now carries the credentials, and the caller decides them. `Auth::from_env` still owns the environment grammar — the crate owns the variable names — but nothing calls it behind a caller's back: which identity a program authenticates as is that program's policy. Two sources, one rule, resolved once in `dolos_snapshot::registry::auth`. A consumer's published read-only pair comes from `[stelae.registry]` in dolos.toml, which `dolos init` seeds from a single constants site (empty until the registry that issues it exists, and a commented template in the generated file until then). A publisher's full-access pair comes from STELAE_REGISTRY_USER/_PASSWORD and overrides it, so a node carrying the read-only pair can publish without having it removed first. A token and a pair set together is a refusal naming both, not a precedence puzzle, and so is half a pair. The e2e harnesses are rebound rather than duplicated: every registry they spawn is behind htpasswd and every transport they open carries the pair. Both server families are configured unconditionally — distribution reads REGISTRY_AUTH_* from the environment and zot reads /etc/zot/config.json, and neither notices the other's — so there is no per-image branch. New tests assert that the registry really does refuse an unauthenticated request, which is also what keeps the fixture honest: a server it could not put behind htpasswd would run anonymous with everything else passing. A 401 read as absence would silently restart a publisher's history chain, so the refusal is checked against a server that really answers 401. Verified: the three registry suites against registry:2, registry:3 and zot v2.1.20; `cargo test --workspace --all-targets`; `cargo test --workspace --all-features` less the three service crates; clippy clean; `cargo tree -p stelae -e normal --all-features` still matches no dolos package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRegistry authentication now uses caller-supplied ChangesRegistry authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dolos
participant stele_registry_auth
participant registry_open as registry::open
participant OCIRegistry as Registry::open
Dolos->>stele_registry_auth: Resolve configured credentials
stele_registry_auth->>registry_open: Supply Auth
registry_open->>OCIRegistry: Pass Auth in registry options
OCIRegistry->>OCIRegistry: Create authenticated OCI client
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`rustfmt.toml` turns on `wrap_comments`, which only nightly applies — the CI job runs `cargo +nightly fmt --all -- --check` for exactly that reason, and stable had nothing to say about these. Two lines are reworded rather than rewrapped: a doc comment that nightly would have split an identifier across, and one it would have split `dolos init` across. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI note: the first two attempts at |
Two corrections to the shape `dolos init` writes.
The generated `dolos.toml` no longer carries a commented-out template.
Serde cannot emit comments, so it was appended after serialization —
a mechanism this repo uses nowhere else, for a section the schema page
already documents.
And the password is no longer seeded into the file. `dolos init` writes
`[stelae.registry]` with the official registry's `user` and nothing else;
`StelaeRegistryConfig::password()` falls back to a compiled-in constant
when the file names none. A password copied into every generated config
is a password that has to be found again in every one of them, whereas a
compiled-in default rotates with a release. A node pointing at a private
registry sets `password` and keeps its own; the environment still
overrides both.
`OFFICIAL_REGISTRY_USER` and `OFFICIAL_REGISTRY_PASSWORD` are the two
constants the registry deployment fills, still one site, still empty
here — so today init writes no section at all rather than one naming
nobody. With them set the file reads:
[stelae.registry]
user = "dolos"
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed No commented-out template. The generated The password is compiled in, not seeded. Both constants still ship empty, so today init writes no section rather than one naming nobody. With them filled the generated file reads: [stelae.registry]
user = "dolos" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/stelae/src/oci.rs (1)
1688-1722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
with_envleaks registry environment variables when a test panics. Both crates carry a copy of the same helper, and both restore the previous values only on the normal return path. A failed assertion insidebody()unwinds past the restore loop, soSTELAE_REGISTRY_TOKEN,STELAE_REGISTRY_USER, andSTELAE_REGISTRY_PASSWORDstay set for every later test in the same binary. The result is that one genuine failure can turn into several unrelated ones, which is exactly what the helper's own doc comment says it exists to prevent.
crates/stelae/src/oci.rs#L1688-L1722: capturepreviousinto a guard struct whoseDropwrites the values back, then callbody()as the tail expression.crates/snapshot/src/registry.rs#L967-L1003: apply the same guard here, or move the helper into one shared test-support location so the two copies cannot diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stelae/src/oci.rs` around lines 1688 - 1722, Update with_env in crates/stelae/src/oci.rs (lines 1688-1722) and crates/snapshot/src/registry.rs (lines 967-1003) to store the captured environment values in a guard whose Drop implementation restores them, then invoke body() as the tail expression so restoration also occurs during unwinding; apply the same change to both copies or consolidate them into shared test support.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/config.rs`:
- Around line 778-790: Update StelaeRegistryConfig::password to return
Option<&str> and treat both a missing configured password and an empty
OFFICIAL_REGISTRY_PASSWORD as absent, returning None in those cases. Preserve
non-empty configured passwords, and let dolos_snapshot::registry::auth handle
None by selecting Auth::Anonymous rather than sending an empty password.
In `@docs/content/configuration/schema.mdx`:
- Around line 428-434: Update the introductory sentence above the credentials
table to clarify that the two table rows describe three variables, while
preserving the existing explanation that these variables override the section
and contain publisher credentials.
---
Nitpick comments:
In `@crates/stelae/src/oci.rs`:
- Around line 1688-1722: Update with_env in crates/stelae/src/oci.rs (lines
1688-1722) and crates/snapshot/src/registry.rs (lines 967-1003) to store the
captured environment values in a guard whose Drop implementation restores them,
then invoke body() as the tail expression so restoration also occurs during
unwinding; apply the same change to both copies or consolidate them into shared
test support.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ae9f641-a83f-4e93-8c5e-8e99b523d269
📒 Files selected for processing (12)
adrs/004_stelae_snapshots.mdcrates/core/src/config.rscrates/snapshot/src/registry.rscrates/snapshot/tests/registry_fixture/mod.rscrates/snapshot/tests/restore_registry.rscrates/stelae/src/lib.rscrates/stelae/src/oci.rscrates/stelae/tests/oci.rsdocs/content/configuration/schema.mdxsrc/bin/dolos/bootstrap/stelae.rssrc/bin/dolos/init.rssrc/bin/dolos/snapshot/publish.rs
`stelae` had no business naming an environment variable. A protocol
library that reads one is choosing its host's credential policy, and
publishing the name freezes that choice into its API: every program
embedding the transport inherits three `STELAE_*` variables it never
asked for and cannot rename. The justification in the code was circular
— the crate owned the names because they started with STELAE.
So `TOKEN_ENV`, `USER_ENV`, `PASSWORD_ENV`, `Auth::from_env` and the two
refusal variants leave `stelae` entirely; the crate no longer references
`std::env` at all. What stays is the interface: `Options::auth` takes an
`Auth` the caller constructs.
`dolos_snapshot::registry::open` takes an `Auth` too rather than a
config type, so the profile crate does not source credentials either.
The names, the precedence and both refusals land in
`dolos::common::stele_registry_auth` — the binary whose deployment they
actually describe. It reads `&StelaeConfig` and the environment, and the
refusals become plain miette errors, which is also why the CLI now says:
Error: × STELAE_REGISTRY_TOKEN and STELAE_REGISTRY_USER/…
are both set; …
rather than the same sentence three times through two error types.
ADR-004 follows: the protocol section states that credentials arrive
from the caller and names no variable and no configuration key. The
variable table moves to "CLI and configuration", where it is Dolos's
answer rather than the format's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stelae/src/oci.rs (1)
1621-1644: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: assert redaction for the
BearerandBasicvariant labels only, and addAnonymous.The test covers
BearerandBasic. Add a case forAuth::Anonymousso a futureDebugimplementation change cannot silently make the anonymous case print something unexpected. This is low value; skip it if you prefer the current scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stelae/src/oci.rs` around lines 1621 - 1644, The credentials_are_redacted_in_debug_output test should also cover Auth::Anonymous. Format the anonymous variant with Debug and assert its output preserves the expected Anonymous variant label without introducing credential-like content, while leaving the existing Basic and Bearer assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bin/dolos/common.rs`:
- Around line 430-470: Update with_env to restore the captured registry
environment variables during unwinding as well as normal completion. Introduce a
local Drop guard around the existing previous-value restoration, ensure the
guard is created after apply([token, user, password]), and remove the direct
restoration loop after body() so the guard handles cleanup before returning or
propagating a panic.
---
Nitpick comments:
In `@crates/stelae/src/oci.rs`:
- Around line 1621-1644: The credentials_are_redacted_in_debug_output test
should also cover Auth::Anonymous. Format the anonymous variant with Debug and
assert its output preserves the expected Anonymous variant label without
introducing credential-like content, while leaving the existing Basic and Bearer
assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8bea7f9-c449-4124-b87b-827469f239e5
📒 Files selected for processing (8)
adrs/004_stelae_snapshots.mdcrates/snapshot/src/registry.rscrates/snapshot/tests/registry_fixture/mod.rscrates/snapshot/tests/restore_registry.rscrates/stelae/src/oci.rssrc/bin/dolos/bootstrap/stelae.rssrc/bin/dolos/common.rssrc/bin/dolos/snapshot/publish.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bin/dolos/snapshot/publish.rs
- crates/snapshot/tests/restore_registry.rs
- src/bin/dolos/bootstrap/stelae.rs
- adrs/004_stelae_snapshots.md
| fn with_env<T>( | ||
| token: Option<&str>, | ||
| user: Option<&str>, | ||
| password: Option<&str>, | ||
| body: impl FnOnce() -> T, | ||
| ) -> T { | ||
| let _guard = ENV.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
|
|
||
| let names = [ | ||
| STELE_REGISTRY_TOKEN_ENV, | ||
| STELE_REGISTRY_USER_ENV, | ||
| STELE_REGISTRY_PASSWORD_ENV, | ||
| ]; | ||
|
|
||
| let previous: Vec<(&str, Option<String>)> = names | ||
| .into_iter() | ||
| .map(|name| (name, std::env::var(name).ok())) | ||
| .collect(); | ||
|
|
||
| let apply = |values: [Option<&str>; 3]| { | ||
| for (name, value) in names.into_iter().zip(values) { | ||
| match value { | ||
| Some(value) => std::env::set_var(name, value), | ||
| None => std::env::remove_var(name), | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| apply([token, user, password]); | ||
|
|
||
| let outcome = body(); | ||
|
|
||
| for (name, value) in previous { | ||
| match value { | ||
| Some(value) => std::env::set_var(name, value), | ||
| None => std::env::remove_var(name), | ||
| } | ||
| } | ||
|
|
||
| outcome | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the environment even when body panics.
Lines 462-467 run only if body() returns normally. An assertion failure inside a with_env closure panics, so the restore loop is skipped and the registry variables stay set for every later test in this binary. That is the exact leak the comment at lines 427-429 describes. Move the restore into a Drop guard so unwinding also restores it.
🛡️ Proposed fix using a restore guard
+ struct Restore(Vec<(&'static str, Option<String>)>);
+
+ impl Drop for Restore {
+ fn drop(&mut self) {
+ for (name, value) in self.0.drain(..) {
+ match value {
+ Some(value) => std::env::set_var(name, value),
+ None => std::env::remove_var(name),
+ }
+ }
+ }
+ }
+
let previous: Vec<(&str, Option<String>)> = names
.into_iter()
.map(|name| (name, std::env::var(name).ok()))
.collect();
+ let _restore = Restore(previous);
@@
apply([token, user, password]);
- let outcome = body();
-
- for (name, value) in previous {
- match value {
- Some(value) => std::env::set_var(name, value),
- None => std::env::remove_var(name),
- }
- }
-
- outcome
+ body()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn with_env<T>( | |
| token: Option<&str>, | |
| user: Option<&str>, | |
| password: Option<&str>, | |
| body: impl FnOnce() -> T, | |
| ) -> T { | |
| let _guard = ENV.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); | |
| let names = [ | |
| STELE_REGISTRY_TOKEN_ENV, | |
| STELE_REGISTRY_USER_ENV, | |
| STELE_REGISTRY_PASSWORD_ENV, | |
| ]; | |
| let previous: Vec<(&str, Option<String>)> = names | |
| .into_iter() | |
| .map(|name| (name, std::env::var(name).ok())) | |
| .collect(); | |
| let apply = |values: [Option<&str>; 3]| { | |
| for (name, value) in names.into_iter().zip(values) { | |
| match value { | |
| Some(value) => std::env::set_var(name, value), | |
| None => std::env::remove_var(name), | |
| } | |
| } | |
| }; | |
| apply([token, user, password]); | |
| let outcome = body(); | |
| for (name, value) in previous { | |
| match value { | |
| Some(value) => std::env::set_var(name, value), | |
| None => std::env::remove_var(name), | |
| } | |
| } | |
| outcome | |
| } | |
| fn with_env<T>( | |
| token: Option<&str>, | |
| user: Option<&str>, | |
| password: Option<&str>, | |
| body: impl FnOnce() -> T, | |
| ) -> T { | |
| let _guard = ENV.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); | |
| let names = [ | |
| STELE_REGISTRY_TOKEN_ENV, | |
| STELE_REGISTRY_USER_ENV, | |
| STELE_REGISTRY_PASSWORD_ENV, | |
| ]; | |
| struct Restore(Vec<(&'static str, Option<String>)>); | |
| impl Drop for Restore { | |
| fn drop(&mut self) { | |
| for (name, value) in self.0.drain(..) { | |
| match value { | |
| Some(value) => std::env::set_var(name, value), | |
| None => std::env::remove_var(name), | |
| } | |
| } | |
| } | |
| } | |
| let previous: Vec<(&str, Option<String>)> = names | |
| .into_iter() | |
| .map(|name| (name, std::env::var(name).ok())) | |
| .collect(); | |
| let _restore = Restore(previous); | |
| let apply = |values: [Option<&str>; 3]| { | |
| for (name, value) in names.into_iter().zip(values) { | |
| match value { | |
| Some(value) => std::env::set_var(name, value), | |
| None => std::env::remove_var(name), | |
| } | |
| } | |
| }; | |
| apply([token, user, password]); | |
| body() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bin/dolos/common.rs` around lines 430 - 470, Update with_env to restore
the captured registry environment variables during unwinding as well as normal
completion. Introduce a local Drop guard around the existing previous-value
restoration, ensure the guard is created after apply([token, user, password]),
and remove the direct restoration loop after body() so the guard handles cleanup
before returning or propagating a panic.
|
Pushed
Side effect worth having: the refusals are now plain miette errors raised in the binary, so the CLI prints the sentence once instead of three times through two error types. The resolution tests moved with the code, to |
…ing is Dolos already has one answer to "how does an operator override a setting from the environment": `load_config` layers `config::Environment` with the `DOLOS` prefix over the whole of `RootConfig`, and the docs teach it (`DOLOS_UPSTREAM_PEER_ADDRESS` and friends). Nothing in the workspace uses clap's declarative `env`, and before the OCI transport landed nothing read `std::env::var` outside a test. So the three `STELAE_REGISTRY_*` variables were a second mechanism for something the first already did. `DOLOS_STELAE_REGISTRY_USER` and `DOLOS_STELAE_REGISTRY_PASSWORD` work today with no code at all — they did before this commit, which is what makes the hand-rolled reader redundant rather than merely inconsistent. Breaking, deliberately: `STELAE_REGISTRY_TOKEN`, `STELAE_REGISTRY_USER` and `STELAE_REGISTRY_PASSWORD` are gone with no alias. `[stelae.registry]` gains `token` so the bearer case is reachable on the same route, and `user`/`password` become optional so the section can name one identity or the other. `stele_registry_auth` is now a pure function of that section; the two refusals survive as validation over the resolved configuration — `token` with `user`, and `password` with no `user`. A test pins the mapping itself, building the same `config::Environment` source `load_config` does. A rename of either field, or a change to the prefix or separator, would otherwise fail silently at run time: the override would stop applying and a publisher would authenticate as the read-only user. Production code is back to zero hand-rolled environment reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed Checking how the rest of the binary handles environment variables settled this. clap's declarative s = s.add_source(::config::Environment::with_prefix("DOLOS").separator("_"));Every So What changed:
ADR-004 now says Dolos introduces no environment variable of its own for this, and the schema page points at the existing env-override docs instead of listing bespoke names. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bin/dolos/common.rs`:
- Around line 97-106: Update the authentication validation before the token
branch in the relevant registry-auth function to reject any configuration where
token is set alongside either user or password, while preserving the existing
token-only and Basic-credential validation behavior. Add a test covering token
plus password without user and assert that configuration is rejected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11375c49-5c8e-4dbf-917f-46e97930d044
📒 Files selected for processing (5)
adrs/004_stelae_snapshots.mdcrates/core/src/config.rsdocs/content/configuration/schema.mdxsrc/bin/dolos/common.rssrc/bin/dolos/init.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bin/dolos/init.rs
- adrs/004_stelae_snapshots.md
- docs/content/configuration/schema.mdx
| if registry.token.is_some() && registry.user.is_some() { | ||
| miette::bail!( | ||
| "[stelae.registry] sets both `token` and `user`; a registry client authenticates as \ | ||
| one identity and which one was meant is not something to guess at — drop the one \ | ||
| you did not mean, or unset DOLOS_STELAE_REGISTRY_TOKEN / DOLOS_STELAE_REGISTRY_USER" | ||
| ); | ||
| } | ||
|
|
||
| if let Some(token) = ®istry.token { | ||
| return Ok(Auth::Bearer(token.clone())); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject a token with a password.
Line 105 returns Auth::Bearer before the password-only validation runs. Therefore, a configuration with token and password, but no user, silently ignores the password. This violates the credential policy for conflicting token/Basic credentials and incomplete Basic credentials.
Reject token when either user or password is set. Add a test for this configuration.
Proposed fix
- if registry.token.is_some() && registry.user.is_some() {
+ if registry.token.is_some() && (registry.user.is_some() || registry.password.is_some()) {
miette::bail!(
- "[stelae.registry] sets both `token` and `user`; a registry client authenticates as \
+ "[stelae.registry] sets `token` with Basic credentials; a registry client authenticates as \
one identity and which one was meant is not something to guess at — drop the one \
- you did not mean, or unset DOLOS_STELAE_REGISTRY_TOKEN / DOLOS_STELAE_REGISTRY_USER"
+ you did not mean, or unset DOLOS_STELAE_REGISTRY_TOKEN / DOLOS_STELAE_REGISTRY_USER \
+ / DOLOS_STELAE_REGISTRY_PASSWORD"
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if registry.token.is_some() && registry.user.is_some() { | |
| miette::bail!( | |
| "[stelae.registry] sets both `token` and `user`; a registry client authenticates as \ | |
| one identity and which one was meant is not something to guess at — drop the one \ | |
| you did not mean, or unset DOLOS_STELAE_REGISTRY_TOKEN / DOLOS_STELAE_REGISTRY_USER" | |
| ); | |
| } | |
| if let Some(token) = ®istry.token { | |
| return Ok(Auth::Bearer(token.clone())); | |
| if registry.token.is_some() && (registry.user.is_some() || registry.password.is_some()) { | |
| miette::bail!( | |
| "[stelae.registry] sets `token` with Basic credentials; a registry client authenticates as \ | |
| one identity and which one was meant is not something to guess at — drop the one \ | |
| you did not mean, or unset DOLOS_STELAE_REGISTRY_TOKEN / DOLOS_STELAE_REGISTRY_USER \ | |
| / DOLOS_STELAE_REGISTRY_PASSWORD" | |
| ); | |
| } | |
| if let Some(token) = ®istry.token { | |
| return Ok(Auth::Bearer(token.clone())); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bin/dolos/common.rs` around lines 97 - 106, Update the authentication
validation before the token branch in the relevant registry-auth function to
reject any configuration where token is set alongside either user or password,
while preserving the existing token-only and Basic-credential validation
behavior. Add a test covering token plus password without user and assert that
configuration is rejected.
…efaults The read-only user and password the official stele registry is reached with sat in `dolos-core`, next to the config types. Every other default a generated `dolos.toml` points at — the Demeter and CF relay addresses, the Mithril aggregators and their genesis keys — lives on `KnownNetwork` in the binary, and this belongs there with them. `StelaeRegistryConfig::password()` is what made the old placement wrong rather than merely inconsistent: it put the answer to "which registry is ours" on the config type itself, so a library shipped an opinion about a specific deployment. The type now carries the shape and none of the values. The seed is `init::official_stelae()`, and the runtime fallback for a section naming a user and no password resolves in `stele_registry_auth`, which is where the rest of the credential policy already is — reaching into `init` the way `doctor` already reaches into it for `KnownNetwork`. The tests split on the same line: `init` asserts the file round-trip, `common` asserts what a passwordless section authenticates with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bin/dolos/common.rs (1)
109-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a private user with no password.
The fallback at Lines 115-118 applies to every
user. A private registry configuration withuser = "publisher"and nopasswordsends the official fallback password instead of failing validation.Apply the fallback only when
userequals the non-empty official registry user. Reject every other user with no configured password. Updatea_seeded_user_takes_the_compiled_in_passwordto cover only the official identity. Add a test that rejects a private user with no password.Proposed fix
-const OFFICIAL_REGISTRY_USER: &str = ""; +pub(crate) const OFFICIAL_REGISTRY_USER: &str = "";- match ®istry.user { - Some(user) => Ok(Auth::Basic { + match (®istry.user, ®istry.password) { + (Some(user), Some(password)) => Ok(Auth::Basic { user: user.clone(), - password: registry - .password - .clone() - .unwrap_or_else(|| crate::init::OFFICIAL_REGISTRY_PASSWORD.to_owned()), + password: password.clone(), }), + (Some(user), None) + if !crate::init::OFFICIAL_REGISTRY_USER.is_empty() + && user.as_str() == crate::init::OFFICIAL_REGISTRY_USER => + { + Ok(Auth::Basic { + user: user.clone(), + password: crate::init::OFFICIAL_REGISTRY_PASSWORD.to_owned(), + }) + } + (Some(_), None) => miette::bail!( + "[stelae.registry] sets `user` with no `password`; basic registry credentials are a pair" + ), // A password with nobody to be. Anonymous would be the quiet answer and // the wrong one: the operator supplied a secret and it would go unused. - None if registry.password.is_some() => miette::bail!( + (None, Some(_)) => miette::bail!( "[stelae.registry] sets `password` with no `user`; basic registry credentials are a \ pair" ), - None => Ok(Auth::Anonymous), + (None, None) => Ok(Auth::Anonymous), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bin/dolos/common.rs` around lines 109 - 126, Update the registry authentication match in the user-handling branch so the compiled-in password fallback is used only when the configured user matches the non-empty official registry identity; reject any other user lacking a password. Narrow a_seeded_user_takes_the_compiled_in_password to the official identity and add coverage proving a private user without a password is rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/bin/dolos/common.rs`:
- Around line 109-126: Update the registry authentication match in the
user-handling branch so the compiled-in password fallback is used only when the
configured user matches the non-empty official registry identity; reject any
other user lacking a password. Narrow
a_seeded_user_takes_the_compiled_in_password to the official identity and add
coverage proving a private user without a password is rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa321ffe-e2c1-4b78-bcff-41934a735b63
📒 Files selected for processing (3)
crates/core/src/config.rssrc/bin/dolos/common.rssrc/bin/dolos/init.rs
1 participant
Plan
plans/dolos-stelae-registry-basic-auth.md— Stelae: Basic credentials for the registry client (Brain/txpipe,org/coder). The Cloudflare registry deployment plan awaits it: its validation gates run through this client.What changed
Client surface.
stelae::oci::Optionsgains anauthvalue — anonymous / bearer / basic — supplied by the caller. The crate keeps the environment grammar (it owns the variable names) inAuth::from_env, but nothing calls it behind a caller's back: which identity a program authenticates as is that program's policy.Auth'sDebugredacts, so a password cannot reach a backtrace throughOptions.Consumer surface.
[stelae.registry]indolos.tomlnames theuser; the password is compiled in.dolos initseeds the section fromdolos_core::config::OFFICIAL_REGISTRY_USERand writes no password, andStelaeRegistryConfig::password()falls back toOFFICIAL_REGISTRY_PASSWORDwhen the file names none. A password copied into every generated config has to be found again in every one of them; a compiled-in default rotates with a release. A node pointing at a private registry setspasswordand keeps its own.Both constants ship empty — one site, two lines, filled once the deployment plan provisions the registry. Until then
initwrites no section rather than one naming nobody.Publisher surface.
STELAE_REGISTRY_USER/STELAE_REGISTRY_PASSWORD, read whereSTELAE_REGISTRY_TOKENis.dolos_snapshot::registry::authresolves both directions in one rule: environment wins, configuration is the fallback, neither is anonymous. A token and a pair together is a refusal naming both; so is half a pair.Testing. The canonical e2e harnesses are rebound, not duplicated: every registry they spawn is behind htpasswd and every transport carries the pair. Both server families are configured unconditionally —
distributionreadsREGISTRY_AUTH_*from the environment,zotreads/etc/zot/config.json, and neither notices the other's — so there is no per-image branch.credentials_are_required(stelae) anda_node_authenticates_with_its_configured_pair(dolos-snapshot) assert the server really refuses an unauthenticated request; that is also the fixture's honesty check, since a server it could not authenticate would run anonymous with every other test passing. A 401 read as absence would silently restart a publisher's history chain, so that refusal is now checked against a server that really answers 401.Done criterion
1. Config credentials on restore, env pair on publish, precedence, and the refusal. Against a real htpasswd
registry:2, with[stelae.registry]deliberately holding the wrong password:The environment got past auth; the only remaining complaint is that the repository is empty. And the refusal:
2. A fresh
dolos initseeds the section, without a password. With the constants temporarily filled (OFFICIAL_REGISTRY_USER = "dolos"), the generateddolos.tomlreads:With them empty as they ship, the section is absent and nothing else changes.
init::tests::a_fresh_config_seeds_the_official_registry_and_no_passwordholds both true — the generated config parses, round-trips to exactlyStelaeConfig::official(), and never contains the wordpassword— anda_configured_password_overrides_the_compiled_in_onecovers the private-registry case and the fallback.3. Canonical harness against a basic-authed
registry:2.The whole matrix, authenticated:
registry:2,registry:3andzot v2.1.20— 9/9 forstelae, 5/5 fordolos-snapshot'spublish, 5/5 forrestore_registry, on each, locally and in CI.4. Docs. ADR-004's "What the transport requires of its host" now states the three shapes, the variable table and both refusals; its config sketch names
[stelae.registry]and says where the password lives.docs/content/configuration/schema.mdxgains astelaesection documenting the user, the optional password and the environment variables.Verification
cargo clippy --workspace --all-targets --all-features— cleancargo test --workspace --all-targets— passcargo test --workspace --all-features --exclude dolos-minibf --exclude dolos-minikupo --exclude dolos-trp— pass#[ignore]d registry suites against all three matrix images — passcargo tree -p stelae -e normal --all-featuresstill matches nodolos-*package; no new dependencyNotes for the reviewer
dolos.tomlis the opposite — it is the seeded shape, and means "use the compiled-in one".)dolos snapshot publishalso passes the configured credentials. They are read-only, so authenticating with them fails the push at the registry rather than a step earlier — the honest place for "these credentials cannot publish" to be said.dolos bootstrap stelaepage underdocs/content/— a pre-existing gap, not opened by this change. The config section and the environment variables are documented in the schema page and ADR-004.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation