[HYPERSHELL-133] hsctl login - #206
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
7123984 to
afbc392
Compare
Amber reviewStatus: Complete VerdictCOMMENT — no blockers, but merge coordination is required. The Hi, Amber here. I reviewed the OIDC login work against the HyperShell conventions (CLAUDE.md), the security standard, and the control-plane conventions. Overall this is a clean, thoughtful implementation: PKCE uses S256 with a random Key observationsTesting (Major). Duplication (Minor). The eager-refresh block is copy-pasted between Config-vs-code (Minor). Hardening (Minor). The PKCE callback Route default behavior change (informational). |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — no blockers, but merge coordination is required. The hsctl OIDC login feature (PKCE browser flow, device flow, eager refresh, whoami, logout revocation) is well-structured and the Keycloak hypershell-cli client is correctly modeled, but the security-sensitive auth package ships without unit tests and there are a few convention/hardening gaps. Most importantly, this PR shares an audience contract with #182 and duplicates gateway-connection guidance already being added by #208/#210 — both need maintainer coordination before merge.
Hi, Amber here. I reviewed the OIDC login work against the HyperShell conventions (CLAUDE.md), the security standard, and the control-plane conventions. Overall this is a clean, thoughtful implementation: PKCE uses S256 with a random state that is validated in the callback, the loopback listener binds to 127.0.0.1:0, tokens are persisted with 0600 perms, --insecure is opt-in, and error paths generally use %w. My findings are quality/hardening and coordination items, not correctness blockers.
Key observations
Testing (Major). components/cli/pkg/auth/{pkce,device,auth}.go, whoami/cmd.go, and the new printConnectionInstructions/shellArg helpers in get/gateway/cmd.go have no unit tests. These are security-sensitive, pure-ish functions that are easy to cover: generatePKCE (verifier/challenge S256 relationship), buildAuthURL (params), shellArg (quoting/injection), resolveEndpoint, and the callback state mismatch path. Please add tests before merge.
Duplication (Minor). The eager-refresh block is copy-pasted between pkg/connection/connection.go and cmd/hypershell/whoami/cmd.go. Extract a single helper (e.g. config.EnsureFreshToken(cfg) or an auth helper) so the refresh/persist/expiry policy lives in one place.
Config-vs-code (Minor). buildConnectionScript hardcodes a specific provider (google-vertex-ai/my-gcp), model (claude-haiku-4-5), and sandbox name. That is opinionated static content baked into the CLI binary and will drift from the web-console guidance (see Cross-PR below).
Hardening (Minor). The PKCE callback http.Server has no ReadHeaderTimeout; the hypershell-cli Keycloak client does not enforce PKCE server-side (pkce.code.challenge.method: S256); and the device-flow poller does not honor slow_down by widening the interval (RFC 8628).
Route default behavior change (informational). create gateway now always sends route={"enabled":true} when --route is omitted. The field is a *string on GatewayCreateRequest, so serialization is correct, and the PR body calls this out. Just flagging that this is a behavioral default change consistent with commit #213.
Cross-PR coordination
I compared this PR against the 23 other open PRs (listed below). Two material coordination items and one duplicate-solution item stand out; the rest are unrelated.
1. Shared management-API audience contract with #182 (fix(auth): enforce management API JWT audience) — coordination + ordering required.
- #182 hardens the API server to require
audcontainshypershell-frontendand to reject tokens not minted for the management API. - This PR is what actually makes
hsctltokens satisfy that requirement: the newhypershell-cliKeycloak client carries anoidc-audience-mapperwithincluded.client.audience: hypershell-frontend. - They are complementary but interdependent: if #182 merges first,
hsctlOIDC login is unusable until this PR adds the CLI client (there is nohypershell-cliclient at all today). If this PR merges first, the audience mapper exists but isn't enforced. - Both PRs edit the same rationale table in
specs/platform/oidc-integration.spec.md; #182 adds a "Shared management API resource audience" row that names hsctl explicitly. Maintainers should confirm the audience value stayshypershell-frontendon both sides and decide the merge order (ideally land the CLI client with, or before, enforcement).
2. Duplicate gateway-connection guidance with #210 and #208 — single source of truth needed.
- This PR's
get gateway --show-connectionemits anopenshell gateway add …registration command plus provider-create and sandbox-create steps. - #210 (
feat(web-console): add gateway-matched CLI installation) adds gateway registration + provider setup commands to the console Connection tab. - #208 (
web-console: instructions for sandbox connecting) adds anopenshell sandbox connect …section to the same tab. - The same "how to connect openshell to a gateway" flow is now authored independently in Go (this PR) and TypeScript (#208/#210), with different opinionated defaults. This is a duplicate-solution / drift risk, not a merge conflict. Maintainers should decide on a canonical command sequence (and where provider/model defaults live) so the CLI and console stay consistent.
3. #216 (fix(console): support OpenShift Route ingress) — related, not conflicting. This PR consumes the gateway route_address/console_address fields (verified present on the model); #216 changes how console_address is published. No design conflict — just a producer/consumer relationship worth being aware of.
No other open PR modifies deploy/base/keycloak/keycloak.yaml or components/cli/, so there is no competing ownership of the CLI or the Keycloak client definition.
Other open PRs reviewed for conflicts
#216 console Route ingress, #214 UI adjustments, #212 e2e perf harness, #211 kind metrics/connectivity, #210 gateway-matched CLI install, #209 Dashboard UI, #208 sandbox connect UI, #207 reconcile trace correlation, #201 Red Hat openshell images, #200 control-plane reconciliation contract, #194 OpenShell Helm chart, #189/#188/#135 dep bumps, #185 world sync spec, #182 JWT audience, #179 keycloak client reconcile, #151 gateway re-provision gate, #150 local images worktree, #148 openshell branch build, #109 security tools, #75/#73 dep bumps.
Findings Summary (ordered by severity, highest first)
- [Major] New auth package +
whoami+ connection-script helpers have no unit tests despite being security-sensitive — Missing Tests (pkg/auth/pkce.go,pkg/auth/device.go,cmd/hypershell/whoami/cmd.go,cmd/hypershell/get/gateway/cmd.go) - [Minor] Eager-refresh logic duplicated between
connection.goandwhoami/cmd.go— Maintainability (pkg/connection/connection.go:55,whoami/cmd.go:47) - [Minor] Connection script hardcodes provider/model/sandbox defaults (config-vs-code; drifts from console PRs) — Convention (
get/gateway/cmd.go:132) - [Minor] PKCE callback
http.ServerlacksReadHeaderTimeout— Hardening (pkg/auth/pkce.go:69) - [Minor]
hypershell-cliKeycloak client does not enforce PKCE server-side (pkce.code.challenge.method: S256) — Hardening (deploy/base/keycloak/keycloak.yaml:197) - [Minor] Device-flow poller ignores
slow_down(does not widen interval per RFC 8628) — Correctness (pkg/auth/device.go:75) - [Minor]
%vused instead of%wwhen wrapping the gateway-parse error — Error Wrapping (get/gateway/cmd.go:89)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf("...: %w", err) |
Fail (get/gateway:89) |
| No secrets in logs or error messages | Pass (token only printed via explicit --show-token) |
| Secrets stored as references / config perms | Pass (config written 0600) |
| Input validated / injection prevented | Pass (shellArg quoting; OAuth state validated) |
| Config separate from code | Fail (hardcoded provider/model in connection script) |
| Conventional commit message | Pass |
| Tests accompany new logic | Fail (auth package untested) |
| OpenAPI client not manually edited | Pass (N/A — generated client untouched) |
| return parseTokenResponse(resp) | ||
| } | ||
|
|
||
| func generatePKCE() (verifier, challenge string, err error) { |
There was a problem hiding this comment.
[Major] Missing tests. This new auth package (PKCE + device flow), whoami, and the printConnectionInstructions/shellArg helpers are security-sensitive but ship without unit tests. These are cheap to cover as pure functions:
generatePKCE()— assertchallenge == base64url(sha256(verifier)).buildAuthURL()— assertresponse_type,code_challenge_method=S256, encodedredirect_uri.- The
/callbackhandlerstate-mismatch path returns an error and does not leak a code. shellArg()— quoting/injection cases.
Please add tests before merge.
| codeCh <- code | ||
| }) | ||
|
|
||
| srv := &http.Server{Handler: mux} |
There was a problem hiding this comment.
[Minor] Hardening. &http.Server{Handler: mux} has no ReadHeaderTimeout (gosec G112). Even though this is a short-lived loopback callback server, set a small ReadHeaderTimeout (e.g. 5 * time.Second) so a stuck client connection can't hold the goroutine open until the 5-minute context deadline.
| return tr, nil | ||
| } | ||
|
|
||
| if strings.Contains(err.Error(), "authorization_pending") || strings.Contains(err.Error(), "slow_down") { |
There was a problem hiding this comment.
[Minor] Correctness. Per RFC 8628 §3.5, on a slow_down response the client MUST increase the polling interval by 5s. Here slow_down is treated identically to authorization_pending and keeps polling at the same rate, which Keycloak may reject. Consider bumping interval when slow_down is seen.
| return | ||
| } | ||
|
|
||
| // Refresh the access token eagerly if it is expired and a refresh token is available. |
There was a problem hiding this comment.
[Minor] Duplication. This eager-refresh block (check expiry → auth.Refresh → persist → warn/return) is duplicated almost verbatim in cmd/hypershell/whoami/cmd.go. Extract a single helper (e.g. config.EnsureFreshToken(cfg) or an auth helper) so the refresh/persist/expiry policy lives in one place and can't drift.
| func printConnectionInstructions(w io.Writer, body []byte) error { | ||
| var gw gatewayResponse | ||
| if err := json.Unmarshal(body, &gw); err != nil { | ||
| return fmt.Errorf("can't parse gateway response: %v", err) |
There was a problem hiding this comment.
[Minor] Error wrapping. Use %w instead of %v to preserve the wrapped error (CLAUDE.md: fmt.Errorf("context: %w", err)). login.go in this same PR was converted %v→%w; keep the new code consistent.
|
|
||
| func buildConnectionScript(name, endpoint string, oidc oidcConfig) string { | ||
| const ( | ||
| providerName = "my-gcp" |
There was a problem hiding this comment.
[Minor] Config-vs-code + cross-PR drift. The provider (my-gcp/google-vertex-ai), model (claude-haiku-4-5), and sandbox name are hardcoded into the binary. This is opinionated guidance baked into code, and it overlaps with the console connection instructions being added in #210 (gateway registration + provider setup) and #208 (openshell sandbox connect). Please align on a single canonical command sequence / defaults source so the CLI and web console don't drift.
| { | ||
| "clientId": "hypershell-cli", | ||
| "enabled": true, | ||
| "publicClient": true, |
There was a problem hiding this comment.
[Minor] Hardening. This is a public client using the authorization-code flow. The CLI always sends PKCE, but the client doesn't require it server-side. Consider adding "attributes": { "pkce.code.challenge.method": "S256" } so Keycloak rejects any non-PKCE code exchange (prevents a downgrade).
| "protocolMapper": "oidc-audience-mapper", | ||
| "consentRequired": false, | ||
| "config": { | ||
| "included.client.audience": "hypershell-frontend", |
There was a problem hiding this comment.
Cross-PR coordination (#182). This audience mapper (included.client.audience: hypershell-frontend) is exactly the contract #182 (fix(auth): enforce management API JWT audience) enforces — it requires aud to contain hypershell-frontend and rejects tokens not minted for the management API. These PRs are interdependent: without this client hsctl OIDC login can't produce an accepted token, and #182 makes the audience mandatory. Please coordinate merge order (land this client with/before enforcement) and keep the audience value identical on both sides. Both PRs also edit the same rationale table in specs/platform/oidc-integration.spec.md.
2 participants
Summary
hsctlCLI with two flows:--no-browser): Device Authorization Grant -- prints a verification URL + user code, polls until the user authenticatesrefresh_token,issuer_url, andclient_idin the config file; eagerly refresh expired access tokens on each connectionhsctl whoamicommand showing username, email, issuer, API URL, and token expiry--show-token/-t: prints only the raw token (pipe-friendly)--show-token-decoded: prints only the decoded JWT claims as pretty-printed JSONhsctl logoutto revoke the refresh token at Keycloak before clearing confighypershell-cliKeycloak client (public, device flow enabled,http://127.0.0.1:*redirect URIs)hsctlin all--helpusage output (was incorrectly showinghypershell)hsctl list gatewaysdefault columns: showname,phase, andconsole_addressinstead of internal IDs and rarely-populatedexternal_dnsget gateway {id} --show-connectionwill display help commands on how to connect with openshell CLIroute enabledby default if not provided, so to not create a route from the CLI you should specify `--route '{"enabled":"false"}'Test plan
hsctl login --url "$API_URL" --issuer-url "$OIDC_ISSUER"-- browser opens, tokens storedhsctl login --no-browser --url "$API_URL" --issuer-url "$OIDC_ISSUER"-- verification URL + code printed, tokens stored after device authhsctl list fleets-- succeeds with stored tokenhsctl list gateways-- shows name, phase, console_address columnshsctl whoami-- shows correct username, email, expiryhsctl whoami --show-token-- prints only the raw token, no other outputhsctl whoami --show-token-decoded-- prints decoded JWT claims as JSONhsctl logout-- clears config; subsequent commands require re-loginhsctl login --token-file "$FILE"-- static token path still works🤖 Generated with Claude Code