diff --git a/docs/design/056-aimdb-join-cli.md b/docs/design/056-aimdb-join-cli.md index 5c3b63a7..e64e28d8 100644 --- a/docs/design/056-aimdb-join-cli.md +++ b/docs/design/056-aimdb-join-cli.md @@ -4,26 +4,21 @@ **Scope:** the `join` subcommand of `tools/aimdb-cli`, speaking the provisioning format of [043](./043-join-endpoint-v1.md) (rev 2). Client only; -provisioning services are out of scope. +provisioning services are out of scope. Implemented from scratch on `main`. --- -## 1. Starting point +## 1. Requirements -An implementation exists on the `planning` branch (`e0dcf9b`, -`tools/aimdb-cli/src/commands/join.rs`, ~480 lines with 9 tests), written -against 043 rev 1. Ported onto `main` it compiles unchanged and its tests -pass. A review found five defects to fix before release: - -| # | Defect | Effect | -|---|---|---| -| D1 | The GitHub OAuth client ID is read with `option_env!` at build time | Binaries built without the variable (including `cargo install`) cannot run the device flow | -| D2 | `prompt()` treats end of input as an empty answer and asks again | `aimdb join … [--token ] [--app =]... | Option | Behaviour | |---|---| -| `` | Deployment base URL. Must be `https://`, except loopback hosts (`localhost`, `127.0.0.0/8`, `::1`) for local testing (D5). | +| `` | Deployment base URL. Must be `https://`, except loopback hosts (`localhost`, `127.0.0.0/8`, `::1`), which may use `http://` for local testing (R5). | | `--token` | Use the `claim-token` kind; skips the device flow. | -| `--app name=value` | Repeatable. Values for the deployment's `app_fields` (043 §2). Never prompted (D2, D3). | +| `--app name=value` | Repeatable. Values for the deployment's `app_fields` (043 §2). Split at the first `=`, so values may contain `=`. A repeated name is a usage error. | | `--out` | Profile path. Default `aimdb-profile.toml`. | -| `--force` | Replace an existing profile. Without it, an existing file is an error (D4). | +| `--force` | Replace an existing profile. Without it, an existing file is an error (R4). | + +The global `--connect` option does not apply to `join`. ## 3. Flow -1. **Discover.** `GET /v1/join`. Read the accepted auth kinds and - `app_fields`. A 404 means `claim-token` only. -2. **Check inputs.** `--app` names not listed in `app_fields` produce a - warning; required fields that are missing are an error before any - network call to GitHub. +1. **Discover.** `GET /v1/join`. + - `200` with a valid document: read `auth` and `app_fields`. + - `404`: the deployment accepts `claim-token` only and announces no + `app_fields`. + - Any other status, a malformed body, or a network/TLS error: exit 3. +2. **Check inputs** before any further request. + - `--app` names not listed in `app_fields`: warning, sent anyway (the + service decides, 043 §3.2). After a `404` there is no list, so no + warnings are issued. + - Required `app_fields` missing from `--app`: exit 2. 3. **Authenticate.** - - `--token` given: `claim-token`. - - Otherwise, `github` from discovery: run the OAuth device flow with the - **discovered** `client_id` (D1), no scopes. Print the user code and - verification URL, poll respecting `interval` and `slow_down`, stop at - `expires_in`. Print the GitHub login for confirmation. - - Neither available: error naming the kinds the deployment accepts. -4. **Join.** `POST /v1/join` with `auth` and `app`. + - `--token` given: `claim-token`, whether or not discovery lists it (the + service rejects kinds it does not accept with `403`). + - Otherwise, `github` listed in discovery: run the OAuth device flow with + the discovered `client_id`, requesting no scopes. Print the user code + and verification URL, poll respecting `interval` and `slow_down` (use + the returned `interval`, else add 5 s), stop at `expires_in`. Then + fetch `GET /user` and print the GitHub login for confirmation; failure + of that call is cosmetic and ignored. + - Neither: exit 2, naming the kinds the deployment accepts. +4. **Join.** `POST /v1/join` with `auth` and `app` (`app` omitted when + empty). 5. **Write.** On `200`, write the profile (§4) and print `node_id` and the - path. On an error status, print the body's `message` as-is and exit with - code 1 (§5). - -The token is held in memory only: never printed, logged or written. + path. On a `043 §4.3` error status, print the body's `message` as-is + (or the status if the body has none) and exit 1. Any other status: exit 3 + with the status and body. + +### 3.1 Transport rules + +- One HTTP client for all requests: rustls with the platform trust store, + user agent `aimdb-cli/`, 30 s timeout per request. +- **No redirects are followed** on requests to the deployment. A `3xx` is an + unexpected response (exit 3) that names the `Location`, so the user can + pass the right base URL. Otherwise an `https://` base could redirect a + `307`/`308` POST, token included, to `http://`. +- The GitHub endpoints (`github.com/login/device/code`, + `github.com/login/oauth/access_token`, `api.github.com/user`) are + constants. Tests reach a mock through an internal parameter of the flow + function, never through an environment variable or flag: an override + would let anyone redirect the token. +- Types that hold the token implement `Debug` with the token redacted, or + not at all. Error messages never include request bodies. ## 4. Writing the profile -- Serialize the response as TOML (043 §5). -- Write to a temporary file in the target directory, created with mode - `0600` on Unix (`OpenOptions::mode` + `create_new`), then `fsync` and - rename over the target. The mode is correct from the first byte whether or - not a file existed (D4), and a crash never leaves a half-written profile. -- Refuse an existing target without `--force`. -- Warn when `profile_version` is newer than the CLI knows; write anyway - (043 §6). +- **Content.** The success body is parsed as a JSON object. `profile_version` + (integer), `node_id` (string), and `broker.url`, `broker.username`, + `broker.password` (strings) must be present; otherwise exit 3. The + **whole object** is then converted to TOML (R6). `null` values are + dropped, since TOML has no null; every other JSON value has a TOML + equivalent. Keys keep the order of the response. +- **Version.** Warn when `profile_version` is newer than the CLI knows; + write anyway (043 §6). +- **Creation.** Write to a temporary file in the target directory (the + `tempfile` crate: `0600` on Unix from creation), `fsync` it, then + - without `--force`: persist with no-clobber (`persist_noclobber`), so a + file that appeared since the check is still not replaced (exit 2); + - with `--force`: rename over the target. + + Then `fsync` the directory on Unix. The mode is correct whether or not a + file existed, and a crash never leaves a half-written profile. +- An existing target without `--force` is detected before any network + request (exit 2), and checked again atomically at the end. +- On Windows the file gets the default ACL of its directory; the user is + responsible for where they put it. ## 5. Exit codes | Code | Meaning | |---|---| | 0 | Profile written | -| 1 | Rejected by the deployment (403, 409, 422, 429, 503) — its `message` is printed | -| 2 | Usage error: bad URL, `http://` to a non-loopback host, existing profile without `--force`, missing required `--app` | -| 3 | Network, TLS or unexpected response | -| 4 | Device flow failed: denied, expired | +| 1 | Rejected by the deployment (403, 409, 422, 429, 503); its `message` is printed | +| 2 | Usage error: bad URL, `http://` to a non-loopback host, bad or repeated `--app`, missing required `--app`, no usable auth kind, existing profile without `--force` | +| 3 | Network, TLS, redirect, or unexpected response (discovery or join), malformed profile | +| 4 | Device flow failed: denied, expired, or an unknown error from GitHub | -Distinct codes let scripts retry network failures and not rejections. +The CLI's `main` currently exits with 1 for every error. `CliError` gains an +`exit_code()` (1 for all existing variants) and `main` uses it. ## 6. Build and dependencies - Behind a `join` cargo feature, on by default. -- `reqwest` with `rustls` and the platform trust store - (`rustls-tls-native-roots`), `json` feature; no OpenSSL. The bundled - `webpki-roots` are avoided because their license is not on the - `deny.toml` allowlist (as on `planning`, `5e728a8`). -- `toml` for the profile. -- No OAuth client ID or other deployment value is compiled in. +- `reqwest` `0.12` (already in the lockfile) with `default-features = false` + and `json`, `rustls-tls-native-roots`; no OpenSSL. Not the bundled + `webpki-roots`, whose license is not on the `deny.toml` allowlist. + Pinned to `0.12`: `0.13` renames the TLS features. +- `toml` for the profile, `tempfile` moved from dev to normal dependency. +- `cargo deny` must pass with the feature on. ## 7. Tests -Kept from `planning` (adapted to rev 2): response parsing, unknown fields -ignored, error `message` surfaced as-is, malformed success body, mock-server -round trip, profile mode `0600`. - -Added: - -1. Discovery: `github` + `client_id` used for the device flow; 404 → - `claim-token` only; neither available → exit code 2 with the accepted - kinds. -2. `--app` parsing (`=` inside values, repeated names rejected), unknown - names warned, missing required names rejected before any request. -3. `http://example.com` rejected; `http://127.0.0.1:…` and - `http://localhost:…` accepted. -4. Existing profile: refused without `--force`; with `--force`, a - pre-existing `0644` file ends up `0600`. -5. Closed stdin (`