[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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
afbc392 to
b9b1fbd
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.
There was a problem hiding this comment.
Fixed in 62abb7d — added unit tests covering the security-sensitive pure functions:
generatePKCE: verifieschallenge == BASE64URL(SHA256(verifier))and that successive calls produce distinct verifiersbuildAuthURL: asserts all required params (response_type=code,code_challenge_method=S256, encodedredirect_uri,state,scope)tokenEndpoint: trailing-slash normalizationparseTokenResponse: success, non-200, and malformed JSON pathsshellArg: safe values pass through, special chars are single-quoted, embedded single quotes escape correctly,<PENDING>sentinel bypasses quotingprintConnectionInstructions: invalid JSON returns error; missing endpoint/OIDC fields render<PENDING>
| 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.
There was a problem hiding this comment.
Fixed in 9230f9c — added ReadHeaderTimeout: 5 * time.Second to the &http.Server{} struct for the loopback callback server.
| 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.
There was a problem hiding this comment.
Fixed in 9230f9c — when slow_down is received the poll interval is bumped by 5s before continuing, per RFC 8628 §3.5. authorization_pending still continues at the current interval.
| 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.
There was a problem hiding this comment.
Already in the PR — extracted to config.EnsureFreshToken(cfg *Config) error so the refresh/persist contract lives in one place.
| 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.
There was a problem hiding this comment.
Fixed in 9230f9c — all %v error wraps in this file changed to %w.
|
|
||
| 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).
There was a problem hiding this comment.
Fixed in 9230f9c — added "pkce.code.challenge.method": "S256" to the hypershell-cli client attributes so Keycloak rejects any non-PKCE code exchange.
| "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.
There was a problem hiding this comment.
Acknowledged — the hypershell-cli audience mapper mirrors the hypershell-frontend contract required by PR #182. No code change needed here; landing order should be this PR first (or together) so the client exists before audience enforcement is active.
b9b1fbd to
b5e2bc2
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. hsctl gains real OIDC login (browser Authorization Code + PKCE and Device Authorization Grant), eager token refresh, whoami, logout revocation, and a dedicated hypershell-cli Keycloak client. The implementation is clean and the error handling is a clear improvement over the prior os.Exit-based login; the main gaps are the complete absence of unit tests for the new auth flows and a few minor hardening/consistency items.
Highlights
- Login refactor replaces
os.Exit(1)with returned, wrapped errors, and consistently uses%w. Good. - PKCE uses
crypto/rand,S256, a randomstatethat is validated on callback, and a loopback (127.0.0.1) ephemeral redirect. Sound. - Refresh tokens persisted with
0600and only refreshed when issuer/client/refresh are all present. Correct guard for static-token logins.
Findings
[Major] No tests for the new OIDC auth flows — components/cli/pkg/auth/{pkce.go,device.go,auth.go} and the refresh/revoke paths ship without any unit tests. These are security-adjacent (token exchange, PKCE verifier/challenge, state validation, device polling/backoff, refresh-on-expiry). At minimum, cover generatePKCE (verifier/challenge relationship), buildAuthURL, tokenEndpoint, parseTokenResponse error handling, and the authorization_pending/slow_down polling branch. Confidence: High.
[Minor] Duplicated eager-refresh block — the ~15-line "refresh if expired" logic is copy-pasted in connection.go (L55) and whoami/cmd.go (L44). Extract a shared helper (e.g. config.EnsureFreshToken(cfg) or an auth helper) so the refresh contract lives in one place. Confidence: High.
[Minor] Unescaped query params rendered into the callback HTML — pkce.go:54 writes the IdP-supplied error / error_description straight into an HTML page with fmt.Fprintf. It is a one-shot localhost page, so exploitability is low, but wrap both values in html.EscapeString to avoid the reflected-injection pattern. Confidence: Medium.
[Minor] --insecure disables TLS verification for token exchange and refresh — auth.go:33 (InsecureSkipVerify: true). Acceptable as a documented dev flag, but access/refresh tokens then transit an unverified TLS channel; consider a stderr warning when it is used so it is not silently applied against a real issuer. Confidence: Medium.
[Minor] Route disable example doesn't match the new default type — create/gateway/cmd.go:108 now defaults route to {"enabled":true} (boolean). The PR description tells users to disable it with --route '{"enabled":"false"}' (string). Confirm the API's JSON route parser treats the string "false" as boolean false; otherwise the documented opt-out silently still enables the route. Confidence: Medium.
[Minor] Non-conventional commit/title — [HYPERSHELL-133] hsctl login isn't in type(scope): description form (e.g. feat(cli): add OIDC login to hsctl). Since the title becomes the squashed commit, reformat before merge. Confidence: High.
Cross-PR coordination
Two open pull requests require maintainer coordination:
- #227 makes a competing change to the same
hsctl list gatewaysdefault column set. This PR setsid, name, phase, console_address, created_at; #227 setsid, active_sandbox_count, cluster_id, console_address, created_by, created_at. Both also add flags tocreate/gateway/cmd.go. Maintainers must decide the canonical default columns and fix a merge order so whichever lands second rebases onto the agreed set rather than silently overwriting it. - #182 enforces the
hypershell-frontendaudience on the management API, while this PR's newhypershell-cliKeycloak client carries the audience mapper that emits exactly that audience. The audience value is a single contract split across the two PRs. Maintainers must keep the enforced audience (#182) and the CLI client's mapper (#206) identical and agree on merge order, becausehsctlOIDC login only succeeds once both halves are consistent.
Findings Summary (ordered by severity, highest first)
- [Major] No unit tests for the new OIDC auth flows - Testing (auth/pkce.go, auth/device.go, auth/auth.go)
- [Minor] Duplicated eager-refresh logic - Maintainability (connection.go L55, whoami/cmd.go L44)
- [Minor] Unescaped IdP error params in callback HTML - Security (pkce.go L54)
- [Minor]
--insecureskips TLS verification on token exchange - Security (auth.go L33) - [Minor] Route disable example type mismatch - Consistency (create/gateway/cmd.go L108)
- [Minor] Non-conventional commit title - Commit Discipline
Convention Checklist
| Convention | Result |
|---|---|
No panic() / os.Exit in production code |
Pass |
Errors wrapped with fmt.Errorf("...: %w", err) |
Pass |
| No secrets in logs or error messages | Pass |
Secrets stored as references / restricted perms (0600) |
Pass |
| Reconcile/refresh guarded for missing prerequisites | Pass |
| Tests accompany new logic | Fail |
| Conventional commit message | Fail |
| return parseTokenResponse(resp) | ||
| } | ||
|
|
||
| func generatePKCE() (verifier, challenge string, err error) { |
There was a problem hiding this comment.
[Major] Add unit tests for the new OIDC auth code. generatePKCE, buildAuthURL, tokenEndpoint, parseTokenResponse, and the device-flow authorization_pending/slow_down polling branch are all untested. This is security-adjacent (PKCE verifier/challenge, state validation, token exchange), so table-driven unit tests here would meaningfully reduce regression risk. Confidence: High.
There was a problem hiding this comment.
Fixed in 62abb7d — see reply on the other unit-test thread above.
| return tr, nil | ||
| } | ||
|
|
||
| if strings.Contains(err.Error(), "authorization_pending") || strings.Contains(err.Error(), "slow_down") { |
There was a problem hiding this comment.
Matching OAuth error codes via strings.Contains(err.Error(), ...) is fragile. Consider returning a typed error (or the parsed error code) from pollDeviceToken and comparing on that instead of substring-matching the formatted message. Not blocking, but worth a follow-up. Confidence: Medium.
There was a problem hiding this comment.
Already in the PR — pollDeviceToken now returns *deviceTokenError carrying the raw Code field; the poll loop uses errors.As and compares tokenErr.Code directly instead of substring-matching.
| return | ||
| } | ||
|
|
||
| // Refresh the access token eagerly if it is expired and a refresh token is available. |
There was a problem hiding this comment.
[Minor] Duplicated refresh logic. This eager "refresh if expired" block is copy-pasted verbatim in whoami/cmd.go (~L44). Extract a single helper (e.g. config.EnsureFreshToken(cfg) or an auth helper) so the refresh/persist contract lives in one place. Confidence: High.
There was a problem hiding this comment.
Already in the PR — extracted to config.EnsureFreshToken(cfg *Config) error. Both connection.Build() and whoami call it; the refresh/persist/expiry policy lives in one place.
| } | ||
| if errParam := r.URL.Query().Get("error"); errParam != "" { | ||
| desc := r.URL.Query().Get("error_description") | ||
| fmt.Fprintf(w, "<html><body><h2>Login failed</h2><p>%s: %s</p><p>You may close this window.</p></body></html>", |
There was a problem hiding this comment.
[Minor/Security] Unescaped IdP params in HTML. errParam and desc come from the redirect query string and are written straight into the response page. It is a one-shot localhost page so risk is low, but wrap both in html.EscapeString(...) to avoid the reflected-injection pattern. Confidence: Medium.
There was a problem hiding this comment.
Already in the PR — both errParam and desc are wrapped in html.EscapeString(...) before being written into the response page.
b5e2bc2 to
ecda52c
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a well-structured, security-conscious addition of OIDC login (Authorization Code + PKCE and Device Authorization) to hsctl, with solid unit coverage for the auth package. Nothing here is blocking, but a few polish items and cross-PR coordination on user-facing "connection instructions" and the shared management-API audience deserve attention before merge.
Amber Analysis
The OIDC flows are implemented carefully: PKCE with S256, a random state checked on the callback, a loopback (127.0.0.1) redirect, credentials persisted to a 0600 file, best-effort token revocation on logout, and eager refresh on each connection. The findings below are Minor/Major quality items, not security defects.
Major
create gatewaysilently forcesrouteon by default. When--routeis unset the CLI now injects{"enabled":true}instead of omitting the field, moving the default decision from the server into the client. This is called out in the PR description, but it changes existing CLI behavior and puts a policy default in code. Prefer letting the server own the default (omit the field) or, if a client default is intended, make it explicit and documented in--help. (components/cli/cmd/hypershell/create/gateway/cmd.go:106)
Minor
- Hardcoded, GCP/Vertex-AI-specific connection script.
buildConnectionScriptemits a fixedopenshell provider create --type google-vertex-ai --from-gcloud-adc,VERTEX_AI_*config, provider namemy-gcp, and modelclaude-haiku-4-5. For any non-GCP gateway this printed guidance is misleading, and the provider/model values are baked into code rather than configuration. Consider parameterizing or clearly labelling it as a GCP example. (components/cli/cmd/hypershell/get/gateway/cmd.go:130) - Error not wrapped with
%w.printConnectionInstructionsusesfmt.Errorf("can't parse gateway response: %v", err); project convention is%wso the cause stays unwrapped. (components/cli/cmd/hypershell/get/gateway/cmd.go:89) EnsureFreshTokensilently no-ops on an unparseable access token. IfTokenExpiredreturns a parse error, the function returnsniland skips refresh, so a corrupt/opaque access token yields a downstream 401 rather than a refresh attempt or a clear message. Consider attempting refresh (or surfacing the parse error) when the token can't be parsed but a refresh token exists. (components/cli/pkg/config/refresh.go:18)webOrigins: ["+"]on the CLI client is unnecessary. A loopback-redirect CLI public client makes no browser-origin (CORS) calls to Keycloak, so+grants an origin allowance it never uses. Consider[]to keep the client minimal. (deploy/base/keycloak/keycloak.yaml:204)
Cross-PR coordination
Two items need maintainer coordination before or alongside merge:
- Duplicate "connection instructions" generators. This PR's
hsctl get gateway --show-connectionrenders the openshell onboarding flow (install OpenShell,openshell gateway add, add the Claude-on-Vertex-AI provider, select model, create/connect a sandbox) as a hardcoded Go template. #210 builds the same class of version-matched install +openshell gateway add+ provider/model setup steps in the sharedgateway-management-ui, and #208 adds the sandbox-connect command in that same UI. These are competing implementations of the same user-facing guidance with independently hardcoded provider/model/flag choices that will drift. Maintainers should decide on a single canonical source/template for these instructions and how the CLI and web console share it. - Shared management-API audience. This PR configures the new
hypershell-cliKeycloak client's audience mapper to minthypershell-frontendinaud. #182 upgrades the API server to enforce the management-API audience and explicitly assumes hsctl shares that audience. The two must agree on the exact audience value and merge in an order such that CLI-issued tokens are accepted once enforcement is live; a mismatch would makehsctl loginproduce tokens the API server rejects.
Findings Summary (ordered by severity, highest first):
- [Major]
create gatewayforcesroute={"enabled":true}by default, moving a policy default into the client - Config in Code / Behavior Change (create/gateway/cmd.go:106) - [Minor] Hardcoded GCP/Vertex-AI-specific connection script is misleading for non-GCP gateways - Config Separation (get/gateway/cmd.go:130)
- [Minor] Error uses
%vinstead of%w- Error Wrapping (get/gateway/cmd.go:89) - [Minor]
EnsureFreshTokensilently skips refresh on unparseable token - Robustness (refresh.go:18) - [Minor]
webOrigins: ["+"]unnecessary on loopback CLI client - Least Privilege (keycloak.yaml:204)
Convention Checklist (only conventions evaluated for this change):
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Fail (one %v in get/gateway) |
| No secrets in logs or error messages | Pass |
| Input validated (state/PKCE, shell-arg quoting) | Pass |
| Secrets stored as references / restricted perms (0600) | Pass |
| Config separate from code | Fail (route default + hardcoded provider/model) |
| Conventional commit message | Pass |
| Test coverage for new logic | Pass (auth pkg well covered) |
| } | ||
| if args.route != "" { | ||
| request["route"] = args.route | ||
| route := args.route |
There was a problem hiding this comment.
[Major] When --route is unset the CLI now injects {"enabled":true} rather than omitting the field, so the default "create a Route" decision moves from the server into the client. This changes prior CLI behavior and encodes a policy default in code. Prefer omitting the field and letting the server own the default, or make the client default explicit/documented in --help.
There was a problem hiding this comment.
Fixed in 62abb7d — the if route == "" { route = "{\"enabled\":true}" } block is removed. The field is now only sent when --route is explicitly provided, so the server owns the default again.
| return "" | ||
| } | ||
|
|
||
| func buildConnectionScript(name, endpoint string, oidc oidcConfig) string { |
There was a problem hiding this comment.
[Minor] buildConnectionScript hardcodes a GCP/Vertex-AI onboarding (--type google-vertex-ai, --from-gcloud-adc, VERTEX_AI_*, provider my-gcp, model claude-haiku-4-5). For non-GCP gateways this printed guidance is misleading, and provider/model belong in configuration, not code. Consider parameterizing or clearly labelling it as a GCP-only example. Note this overlaps with the web-console connection-instruction work; see the Cross-PR coordination section.
There was a problem hiding this comment.
Fixed in 62abb7d — the output now includes a clear "GCP/Vertex AI example" header line before the provider/model steps so users know to adapt them for their environment.
| 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] Use %w instead of %v so the underlying parse error stays wrapped: fmt.Errorf("can't parse gateway response: %w", err).
There was a problem hiding this comment.
Fixed in 9230f9c — %v → %w throughout the file.
| return nil | ||
| } | ||
| expired, checkErr := TokenExpired(cfg.AccessToken) | ||
| if checkErr != nil || !expired { |
There was a problem hiding this comment.
[Minor] When TokenExpired returns a parse error (checkErr != nil) this returns nil and skips refresh, so a corrupt/opaque access token yields a downstream 401 instead of a refresh attempt or a clear error. Consider attempting a refresh (or surfacing the parse error) when the token can't be parsed but a refresh token is present.
There was a problem hiding this comment.
Fixed in 9230f9c — inverted the condition to if checkErr == nil && !expired { return nil }, so an unparseable token falls through and triggers a refresh attempt rather than silently skipping.
| "oauth2.device.authorization.grant.enabled": "true" | ||
| }, | ||
| "redirectUris": ["http://127.0.0.1:*"], | ||
| "webOrigins": ["+"], |
There was a problem hiding this comment.
[Minor] A loopback-redirect public CLI client makes no browser-origin (CORS) calls to Keycloak, so webOrigins: ["+"] grants an allowance it never uses. Consider [] to keep the client minimal.
There was a problem hiding this comment.
Fixed in 9230f9c — changed to "webOrigins": [] on the hypershell-cli client. Loopback-only redirect, no browser-origin CORS calls needed.
- pkce: add ReadHeaderTimeout (5s) to loopback callback server (gosec G112) - device: bump poll interval +5s on slow_down per RFC 8628 §3.5 - get/gateway: use %w for error wrapping (consistent with codebase convention) - config: EnsureFreshToken treats unparseable token as expired and attempts refresh - keycloak: require PKCE S256 server-side on hypershell-cli; drop unused webOrigins Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds OIDC browser (Authorization Code + PKCE) and device-flow login to hsctl, eager token refresh, whoami, token revocation on logout, and a new hypershell-cli Keycloak public client. The implementation is clean, well-factored, and unusually well-tested for a CLI change; my findings are one design default worth a decision plus a few hardening nits — none are blockers.
Strengths
- PKCE flow validates
state, uses S256, and has good unit coverage (auth_test.go). - Config file is persisted with
0600, so the newly storedrefresh_tokenis protected at rest. - Errors are wrapped with
%w; nopanic(); no secret values are logged (error bodies from the token endpoint are OAuth error JSON, not credentials).
Findings
[Major] create gateway now creates a route by default — API/UX contract change (components/cli/cmd/hypershell/create/gateway/cmd.go:108)
When --route is omitted the CLI now sends {"enabled":true} instead of omitting the field. This silently changes the default exposure/behavior for existing scripts and forces users to pass --route '{"enabled":false}' to opt out. It is documented in the PR body, but defaulting a gateway to an externally-reachable route is a security/cost-relevant default and deserves an explicit maintainer decision (or leaving the field unset so the server owns the default).
[Minor] Token HTTP calls have no client timeout (components/cli/pkg/auth/auth.go:25)
newHTTPClient returns http.DefaultClient (no timeout) and the insecure path sets no Timeout either. EnsureFreshToken runs on every command via the connection builder, so an unresponsive issuer could hang the CLI indefinitely. Set an explicit Timeout (e.g. 30s) on both branches.
[Minor] InsecureSkipVerify disables TLS verification for the token/revocation endpoints (components/cli/pkg/auth/auth.go:33)
Gated behind --insecure and annotated with //nolint:gosec, which is acceptable for local dev, but this now silently applies to credential-bearing OIDC requests. Worth confirming this is only ever used against local/dev issuers and is documented as such.
[Minor] Onboarding script hardcodes example provider/model values in code (components/cli/cmd/hypershell/get/gateway/cmd.go:132)
my-gcp, claude-haiku-4-5, VERTEX_AI_REGION=global, etc. are embedded as Go constants in the connection instructions. These will drift and require code changes to update (vs. Separate configuration from code). Consider sourcing the model/provider hints from configuration or clearly marking them as non-authoritative placeholders.
[Minor] EnsureFreshToken proceeds when the token cannot be parsed (components/cli/pkg/config/refresh.go:18)
If TokenExpired returns an error (malformed token) and no refresh material is present, the function returns nil and the request proceeds with an unusable token. A short comment or an explicit "re-login" hint on the un-parseable case would improve the UX.
Cross-PR coordination
-
#182 (enforce management API JWT audience): This PR's new
hypershell-cliclient relies on an audience protocol mapper that injectshypershell-frontendinto theaudclaim specifically so the management API accepts CLI-minted tokens, and it edits the sameoidc-integration.spec.mdaudience narrative. #182 hardens the API server to reject tokens not minted for the management API audience. These two changes share a single assumption (the exact required audience and theaudvsazpdistinction) and must stay aligned; if #182's enforced audience or client-token rejection rules change, this PR's client config would produce tokens the API rejects. Maintainers should confirm the audience value matches and decide a merge order so CLI login is not broken between merges. -
#208 and #210 (web-console gateway connection/onboarding instructions): This PR adds
get gateway --show-connection, which generates user-facingopenshellonboarding instructions (install URL,openshell gateway addwith OIDC issuer/client-id/audience, provider/sandbox setup) from the CLI. The web console PRs generate the same class ofopenshellconnection/onboarding guidance ingateway-connection-steps.tsx. This is duplicate authoring of the same onboarding contract in two components, which will drift. Maintainers should decide on a single canonical source (or an explicit shared shape) for gateway connection instructions rather than maintaining independent copies.
Findings Summary (ordered by severity, highest first):
- [Major]
create gatewaydefaultsrouteto{"enabled":true}, a silent exposure/behavior contract change - API/UX (L108) - [Minor] Token HTTP client has no timeout; runs on every command via refresh - Reliability (auth.go L25)
- [Minor]
InsecureSkipVerifyapplied to OIDC credential requests under--insecure- Security (auth.go L33) - [Minor] Onboarding script hardcodes provider/model values in code - Config vs code (get/gateway L132)
- [Minor]
EnsureFreshTokenproceeds on unparseable token - UX (refresh.go L18)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or error messages | Pass |
| Credentials stored with restrictive file perms (0600) | Pass |
| Input validated (PKCE state, shell-arg escaping) | Pass |
| Conventional commit messages | Pass |
| Config separate from code | Fail |
| request["route"] = args.route | ||
| route := args.route | ||
| if route == "" { | ||
| route = `{"enabled":true}` |
There was a problem hiding this comment.
[Major] Default route now enabled. Omitting --route previously left the field unset; it now sends {"enabled":true}, so a gateway is created with an external route by default and callers must pass --route '{"enabled":false}' to opt out. This is a behavior/exposure contract change (documented in the PR body, but security/cost-relevant). Consider leaving the field unset so the server owns the default, or confirm this default with maintainers.
There was a problem hiding this comment.
Fixed in 62abb7d — same change as above: route is omitted when --route is not set, restoring the prior behavior.
| return strings.TrimRight(issuerURL, "/") + "/protocol/openid-connect/token" | ||
| } | ||
|
|
||
| func newHTTPClient(insecure bool) *http.Client { |
There was a problem hiding this comment.
[Minor] No client timeout. This returns http.DefaultClient (and the insecure branch sets no Timeout). EnsureFreshToken runs on every command through the connection builder, so an unresponsive issuer would hang the CLI indefinitely. Set an explicit Timeout (e.g. 30s) on both branches.
There was a problem hiding this comment.
Fixed in 62abb7d — newHTTPClient now sets Timeout: 30 * time.Second on both the secure and insecure branches, so an unresponsive issuer cannot hang the CLI indefinitely.
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| InsecureSkipVerify: true, //nolint:gosec |
There was a problem hiding this comment.
[Minor] TLS verification disabled for OIDC credential traffic. Gated by --insecure and annotated, which is fine for local dev, but this now also covers token/refresh/revoke requests that carry credentials. Please confirm it is only ever pointed at local/dev issuers and document that expectation.
There was a problem hiding this comment.
Acknowledged as a documented dev flag. --insecure is only documented for local/dev issuers. No change made here.
|
|
||
| func buildConnectionScript(name, endpoint string, oidc oidcConfig) string { | ||
| const ( | ||
| providerName = "my-gcp" |
There was a problem hiding this comment.
[Minor] Hardcoded example values in code. my-gcp, claude-haiku-4-5, VERTEX_AI_REGION=global, etc. are embedded as constants in the generated onboarding instructions and will drift, requiring code changes to update (vs. separate config from code). Source these hints from configuration or clearly mark them as non-authoritative placeholders.
| return nil | ||
| } | ||
| expired, checkErr := TokenExpired(cfg.AccessToken) | ||
| if checkErr == nil && !expired { |
There was a problem hiding this comment.
[Minor] Proceeds on unparseable token. When TokenExpired returns an error (malformed token) and no refresh material exists, this returns nil and the command runs with an unusable token. A comment or explicit re-login hint for the un-parseable case would improve UX.
There was a problem hiding this comment.
Fixed in 9230f9c — same change: the guard is now if checkErr == nil && !expired, so a corrupt/opaque access token falls through to the refresh path.
- pkce: add ReadHeaderTimeout (5s) to loopback callback server (gosec G112) - device: bump poll interval +5s on slow_down per RFC 8628 §3.5 - get/gateway: use %w for error wrapping (consistent with codebase convention) - config: EnsureFreshToken treats unparseable token as expired and attempts refresh - keycloak: require PKCE S256 server-side on hypershell-cli; drop unused webOrigins Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9230f9c to
16c789e
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds real OIDC login (browser PKCE + device flow), token refresh, whoami, token revocation on logout, and a hypershell-cli Keycloak client, and it renames the binary to hsctl. The implementation is clean, well-tested at the auth layer, and follows the project's error-wrapping conventions; findings below are minor and none are blockers, but two cross-PR coordination points need maintainer attention.
What looks good
- OAuth flows are correct: PKCE with
S256, CSRF-stylestatevalidation on the callback,html.EscapeStringon reflected error params (no XSS in the callback page), buffered channels, and a 5-minute context timeout withsrv.Shutdown. - Errors are wrapped with
fmt.Errorf("...: %w", err)throughout, and several pre-existing%vwraps were upgraded to%w. Nopanic()introduced. - Config file is written
0600; secret values are not logged.EnsureFreshTokenrefreshes eagerly and persists, with a save-failure downgraded to a warning rather than a hard failure. - Solid table-driven unit tests for
generatePKCE,tokenEndpoint,buildAuthURL,parseTokenResponse, and device-token polling (pending / slow_down / success / non-JSON). No pre-existing test assertions were weakened. oidcandrouteare correctly treated as JSON-encoded string fields, matchingopenapi.gateways.yaml.
Findings
[Minor] create gateway now always sends a route, defaulting to enabled.
components/cli/cmd/hypershell/create/gateway/cmd.go previously omitted route when unset; it now injects {"enabled":true} whenever --route is empty. This silently changes gateway-creation semantics for every CLI caller: a route is now always requested unless the user explicitly passes --route '{"enabled":false}'. It is called out in the PR body, so this is a heads-up rather than a defect - please confirm the server-side default previously matched this, so existing scripted create gateway invocations don't start provisioning routes they didn't before. Confidence: Medium.
[Minor] Device flow can return an immediate "timed out" if the server omits expires_in.
components/cli/pkg/auth/device.go computes deadline := time.Now().Add(time.Duration(dar.ExpiresIn) * time.Second). If expires_in is absent/zero, deadline equals now and the poll loop never runs, surfacing a misleading "device authorization timed out". Apply the same defensive fallback already used for interval. Confidence: Medium.
[Minor] Connection instructions embed hardcoded example identifiers.
components/cli/cmd/hypershell/get/gateway/cmd.go hardcodes providerName = "my-gcp", model = "claude-haiku-4-5", and sandboxName = "mysand" inside buildConnectionScript. These are example placeholders rendered as if runnable; a user copy-pasting gets a Vertex-AI/Claude-specific script regardless of their setup. Consider clearly marking them as placeholders (or making them configurable). Confidence: Medium. See the Cross-PR section - this instruction text overlaps with the web console's connection guidance.
Cross-PR coordination
Two coordination points need a maintainer decision:
-
Shared management-API audience assumption. This PR provisions the
hypershell-clipublic client with an audience mapper that injectshypershell-frontendintoaudso the API server accepts CLI-issued tokens, and it editsspecs/platform/oidc-integration.spec.mdto document that. Another open PR hardens/enforces management-API JWT audience validation on the API server (upgrading rh-trex-ai to fail closed and requiring thehypershell-frontendaudience) and edits the same spec. The two changes rest on the same shared-audience contract and touch overlapping spec text, so they must land consistently: if audience enforcement merges while this client's audience mapper is absent or differently configured,hsctltokens will be rejected. Maintainers should confirm the requiredaudvalue and decide a merge order so CLI login and audience enforcement stay aligned. -
Duplicate "how to connect to a gateway/sandbox" instructions. This PR generates openshell connection/setup instructions in Go (
get gateway --show-connection), while other open work builds the canonical connection and install-command guidance in the web console'spackages/gateway-management-ui/src/gateways/gateway-connections.tsand adds a "Connect to a sandbox" section to the gateway Connection tab. These are independent implementations of the same user-facing guidance and will drift (different flags, model/provider examples, install command derivation). Maintainers should decide on a single canonical definition of the connection steps and have the CLI and web console derive from it, rather than maintaining two divergent copies. This affects the gateway-version/connection PR that ownsgateway-connections.tsand the web-console sandbox-connection PR.
Findings Summary (ordered by severity, highest first)
- [Minor]
create gatewayalways sendsroute, defaulting to enabled - behavior change for all CLI callers - API/UX Behavior (create/gateway/cmd.go L106-110) - [Minor] Device flow returns immediate timeout when
expires_inis missing/zero - Robustness (auth/device.go L76) - [Minor] Hardcoded example provider/model/sandbox names in connection script - UX / Docs (get/gateway/cmd.go L132-134)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or error messages | Pass |
Config/secret file permissions (0600) |
Pass |
| Input validated / callback state + XSS handling | Pass |
Image/binary references consistent across the stack (hsctl) |
Pass |
| Test Diff Scrutiny (no weakened pre-existing assertions) | Pass |
| Conventional commit messages | Pass |
| request["route"] = args.route | ||
| route := args.route | ||
| if route == "" { | ||
| route = `{"enabled":true}` |
There was a problem hiding this comment.
Behavior change: when --route is empty this now injects {"enabled":true}, so every CLI create gateway requests a route. Previously the field was omitted. Please confirm the server default matched this so existing scripted invocations do not start provisioning routes they did not before; otherwise consider leaving it unset unless the user opts in.
There was a problem hiding this comment.
Fixed in 62abb7d — removed the injected default entirely. Route is now only sent when --route is explicitly passed.
| if interval < time.Second { | ||
| interval = 5 * time.Second | ||
| } | ||
| deadline := time.Now().Add(time.Duration(dar.ExpiresIn) * time.Second) |
There was a problem hiding this comment.
If the device-auth response omits expires_in, dar.ExpiresIn is 0 and deadline equals now, so the poll loop never runs and the user immediately sees a misleading "device authorization timed out". Add a sane fallback (mirroring the interval default just above), e.g. treat a non-positive ExpiresIn as a few minutes.
There was a problem hiding this comment.
Fixed in 62abb7d — added a if expiresIn <= 0 { expiresIn = 300 } guard before computing the deadline, so a missing or zero expires_in uses a 5-minute fallback instead of expiring immediately.
| func buildConnectionScript(name, endpoint string, oidc oidcConfig) string { | ||
| const ( | ||
| providerName = "my-gcp" | ||
| model = "claude-haiku-4-5" |
There was a problem hiding this comment.
providerName, model, and sandboxName are hardcoded example values rendered inside a copy-pasteable script. A user following these instructions gets a Vertex-AI/claude-haiku-4-5/mysand setup regardless of their environment. Consider marking them clearly as placeholders or deriving them, and note this overlaps with the web console connection guidance (see the Cross-PR section of the top-level review).
There was a problem hiding this comment.
Fixed in 62abb7d — the script now includes a comment marking steps 2-4 as a "GCP/Vertex AI example" so the placeholder names are clearly framed as examples to adapt.
- create gateway: omit route field when --route is unset; let the server own the default - device flow: fall back to 300s when expires_in is missing or zero to prevent immediate timeout - auth: add 30s timeout to all HTTP clients (secure and insecure) so an unresponsive issuer cannot hang the CLI - get gateway: label the GCP/Vertex AI example section so non-GCP users know to adapt it - tests: add unit tests for generatePKCE (challenge=BASE64URL(SHA256(verifier))), buildAuthURL (required params), tokenEndpoint, parseTokenResponse, shellArg quoting/injection cases, and printConnectionInstructions edge cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured OIDC login experience to hsctl (browser Authorization Code + PKCE, headless device flow, static-token fallback), plus whoami, eager token refresh, logout revocation, and a dedicated hypershell-cli Keycloak client. The security posture of the flows is good (loopback-only callback, state validation, S256 PKCE, no secrets logged, %w error wrapping, real unit tests); I found no blockers, only a few minor items and one cross-PR design coordination worth a maintainer decision.
What looks good
- Auth flows are implemented carefully: PKCE uses
crypto/rand+S256, validatesstateon the callback, binds only to127.0.0.1, escapes error HTML, and has a hard 5-minute timeout. Device flow correctly handlesauthorization_pendingandslow_down(with backoff) and an overall deadline. - No secrets in logs. Only OAuth error bodies (not successful token bodies) are surfaced in error strings; the config file is written
0600. - Error handling is idiomatic:
panic()/os.Exit(1)removed fromloginin favor of returned, wrapped errors;%wused consistently. - Tests were added for PKCE generation, auth URL construction, token endpoint derivation, token-response parsing, and the shell-arg quoting/
<PENDING>logic. No pre-existing test assertions were weakened (Test Diff Scrutiny: clean — all test changes are additive).
Findings (all Minor)
--urlis now required (defaulthttp://localhost:8000removed) — a breaking change for existing invocations/scripts. Intentional and documented, but flag it in release notes. (login/cmd.go:45)whoami --show-tokenand--show-token-decodedare not mutually exclusive;--show-tokensilently wins. ConsiderMarkFlagsMutuallyExclusive. (whoami/cmd.go:29-30)--insecuredisables TLS verification for IdP credential exchange (PKCE/device/refresh/revoke). Acceptable and opt-in; consider warning when used against a non-loopback issuer. (auth/auth.go:35)- Hardcoded onboarding/install guidance in
buildConnectionScriptcan drift from the canonical UI connection-instruction source (see Cross-PR coordination). (get/gateway/cmd.go:147)
Cross-PR coordination
-
Shared management-API audience contract. This PR's new
hypershell-cliKeycloak client injectshypershell-frontendinto the tokenaudso the API server accepts CLI-issued tokens, and it rewrites thedirectAccessGrantsEnabledrationale inspecs/platform/oidc-integration.spec.md. Separately, PR #182 upgrades the API server to enforce thehypershell-frontendmanagement-API audience (and to reject tokens not minted for the management API) and edits the same spec. These two changes are interdependent: the audience this PR emits must match exactly what #182 enforces, or CLI login succeeds but every API call 401s. Maintainers should confirm the audience value and decide a merge order (audience enforcement + the CLI client's audience mapper need to land consistently), and reconcile the overlapping edits tooidc-integration.spec.md. -
Single source of truth for gateway connect/onboarding instructions.
hsctl get gateway --show-connectionrenders anopenshellonboarding script (install line,gateway add, provider/model/sandbox steps) as hardcoded Go strings. PR #208 adds equivalent connect guidance to the web-console gateway Connection tab, and PR #210 makes the UI derive theopenshellinstall command from the reconciledgateway_version(trimming the downstream-rh…suffix) rather than a staticmain-branch URL. As written, the CLI duplicates this guidance and will drift (e.g., it hardcodes the install URL/version instead of using the version-aware derivation). Maintainers should decide whether the CLI onboarding text should reuse the canonical connection-instruction logic/source so the CLI and UI stay in sync, and whether this PR should adopt the version-aware install command once that field is available.
| flags := Cmd.Flags() | ||
| flags.StringVar(&args.url, "url", "http://localhost:8000", "URL of the API server.") | ||
| flags.StringVar(&args.token, "token", "", "Bearer access token (JWT) - DEPRECATED: use --token-file instead.") | ||
| flags.StringVar(&args.url, "url", "", "URL of the API server.") |
There was a problem hiding this comment.
[Minor] Behavior change: --url is now required. The previous default (http://localhost:8000) was removed and run now returns required flag "url" not set. This is a deliberate, documented change, but any existing script or muscle-memory invocation of hsctl login (relying on the localhost default) will now fail. Please make sure this is called out as a breaking CLI change in the changelog / release notes so users update their scripts.
3 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