Repository navigation
fix(sdk/go): use the CLI oidc_token.json format - #4830
lunarwhite wants to merge 1 commit into
Conversation
|
Hi @TaylorMutch, would you please take a look at this PR when you have a moment? I'd like to hear your thoughts as you triaged it. Thanks in advance! |
b752b4f to
86d7f48
Compare
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
- Genuine cross-language anti-drift: the single fixture
sdk/conformance/oidc-token-bundle.jsonis asserted from both Rust (oidc_token.rs:197) and Go (readoidc/token_test.go:28, write:43), so a field change on either side breaks that language's test. internal/oidctokencentralizes the on-diskBundletype and converters, removing the two divergent private structs (the root-cause fix for #4508).- Missing/null
expires_atis handled consistently as non-expiring on every path (Go and Rust), with dedicated nil-vs-zero tests. - The
RefreshableTokenbackoff path is nil-error safe:nextRetry/lastErrare set together on failure and cleared together on success. - Go token persistence is atomic and 0600 (
CreateTemp+Chmod+Sync+Rename) with temp-file cleanup on every error path.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Notable | sdk/go/openshell/v1/auth_refresh.go:140 | Fail-closed RefreshableToken change applies to every token source | goal-alignment |
| Notable | sdk/go/openshell/v1/gateway/token.go:120 | Upgrade invalidates older Go-SDK-written token files (missing issuer/client_id) | production |
| Notable | sdk/go/openshell/v1/gateway/token.go:120 | Runtime path does not match stored issuer/client_id against gateway config | security |
| Minor | sdk/go/openshell/v1/gateway/token.go:120 | Load-bearing issuer/client_id guard is undocumented | gator |
| Minor | crates/openshell-bootstrap/src/oidc_token.rs:54 | Rust writes oidc_token.json non-atomically (perm window + partial-read race) | security |
| Notable | sdk/go/openshell/v1/auth_refresh.go:82 | No-expiry cached token is never re-read (won't pick up a later expiring token) | gator |
Two findings are summary-only because they fall on lines outside this PR's diff:
- crates/openshell-bootstrap/src/oidc_token.rs:54 (Minor, pre-existing
store_oidc_token): non-atomicstd::fs::write+ separate chmod 0600 leaves a brief wider-permission window and a truncating in-place write that the Go reader can observe mid-write. The Go writer this PR adds does it correctly. In the issue's stated scope; mitigated by the 0700 parent dir and the cached-token fallback. - sdk/go/openshell/v1/auth_refresh.go:82 (Notable): a no-expiry cached token makes
isTokenValid()always true, so a long-lived process never re-reads the file and won't pick up a later CLI-written expiring token until restart. Consistent with the Rust side; the only defensible behavior without expiry metadata.
Review Details
- Findings posted: 6 (4 inline, 2 summary-only)
- Gate outcome: PASS (0 Critical, 0 Important)
- Participating reviewers: goal-alignment, production, security, gator (architecture, correctness, test-quality ran with no surviving findings; codex and coderabbit skipped on this fork PR)
| AccessToken: bundle.AccessToken, | ||
| RefreshToken: bundle.RefreshToken, | ||
| TokenType: "Bearer", | ||
| if bundle.Issuer == "" || bundle.ClientID == "" { |
There was a problem hiding this comment.
Notable: The runtime diskTokenSource.Token() checks only that the stored issuer/client_id are non-empty; it never matches them against the gateway’s configured OIDC issuer/client_id.
Why this matters: If a gateway is reconfigured to a new issuer but a stale oidc_token.json for the old issuer remains, the runtime path accepts and sends it. The interactive path (oidc.go:60-62) does the stronger normalizeIssuer(...) == cfg.issuer && ClientID == cfg.clientID check; the runtime path omits it (OIDCIssuer/OIDCClientID are available but unused). Impact is a confusing client-side error, not a true bypass, since the gateway validates server-side and returns 401.
Suggested fix: Mirror the oidc.go match check in the runtime path and fail with the login hint instead of sending a token the gateway will reject.
Source: security agent
There was a problem hiding this comment.
I found that the CLI also sends the stored bundle without matching issuer/client_id against metadata. Would you suggest updating both sides here, or leave it as a follow-up?
There was a problem hiding this comment.
Good catch that the CLI has the same gap. I'd leave it as a follow-up and fix both sides together rather than make the Go SDK stricter than the CLI here, since this PR is about getting the two aligned. A mismatch is also hard to hit today: gateway add refuses an existing name and gateway remove/logout delete the token file, so it takes a hand-edited or system-provided metadata.json changing underneath an existing token. When it does happen, the gateway rejects the token server-side, so the cost is a confusing 401 rather than a bypass.
Since #4514 reworks the gateway refresh path anyway, that looks like a good place for it: one shared check in openshell-bootstrap that the stored bundle's issuer and client_id match the gateway metadata (failing with the login hint), used by the CLI, the TUI and the completers, and mirrored in diskTokenSource. Worth noting there too that the CLI refreshes against the issuer stored in the bundle, not the one in the metadata.
|
Re: review comments in the body
I'll keep this for #4514 with the locking. I checked it's a small
Re-reading those breaks |
86d7f48 to
6054443
Compare
|
Appreciate your review @rhuss! I've replied to each review comment from my perspective. Please let me know what you think. I'm open to addressing your feedback, as you write the Go sdk mostly :) |
There was a problem hiding this comment.
Here's is another round of reviews on the latest changes (with updated agents, so there might be findings that the first round hasn't found). Although there are some findings (hopefully easy to fix by aganets), I think we are close to converge.
cc-review Summary
New round with fresh agent eyes on 6054443. The earlier threads are either addressed or continue in their existing threads (including the open issuer/client_id matching question), so this round lists only new observations. The findings come from an agent-assisted review; the failure scenarios were derived from the code and not executed.
What Went Well
sdk/go/openshell/v1/internal/oidctoken/oidctoken.go: one internalBundletype withFromToken/Tokenreplaces the two divergentoidcBundlecopies ingateway/token.goandoidc/token.go. Its field tags (omitemptyonrefresh_token,*uint64expires_at, requiredissuer/client_id) mirror the serde attributes ofOidcTokenBundleinopenshell-bootstrap.sdk/conformance/oidc-token-bundle.jsonis checked from both sides: Rustconformance_fixture_matches_cli_serialization(crates/openshell-bootstrap/src/oidc_token.rs) and GoTestWriteToken_MatchesConformanceFixtureandTestReadToken_ConformanceFixture, so a format drift between CLI and SDK fails CI.sdk/go/openshell/v1/gateway/token.godiskTokenSource: bundles withoutissuer/client_idare rejected instead of being treated as never-expiring, the expiry check fails closed at the exact expiry instant, andloadErrorgives every failure branch theErrTokenLoadwrap and theopenshell gateway login <name>hint without putting token material in the message.sdk/go/openshell/v1/auth_refresh.gousableCached/lastErr: after a failed refresh, an expired cached bearer is no longer sent, and the backoff path returns the real refresh error.lastErris read and written underr.muand cleared on success.TestGetRequestMetadata_RefreshFailureWithExpiredCachedTokenReturnsErrorcovers it withrequire.ErrorIs, a check that no "using cached token" log was emitted, and a source call count.sdk/go/openshell/v1/oidc/oidc.goLoginreuses a stored token only when the normalized issuer and the client ID match, andTestLogin_ReplacesMismatchedStoredTokencovers the client-id, issuer, and legacy-format cases in one table.TestDiskTokenSource_ExpiredTokenandTestDiskTokenSource_NoExpirytable-test the boundary valuesexpires_at=0, an absent field, and explicit null.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Minor | sdk/go/openshell/v1/auth_refresh.go:102 | During backoff the cached lastErr is replayed, so a fresh login is not picked up for up to 30s |
gator |
| Minor | sdk/go/openshell/v1/gateway/gateway_test.go:93 | TestNewClient_AuthModeOIDC seeds a token file in a format the SDK now rejects |
test-quality |
| Minor | sdk/go/openshell/v1/gateway/gateway_test.go:345 | No gateway-level test for the ErrTokenLoad plus login hint path through resolveAuthProvider |
test-quality |
| Minor | sdk/go/openshell/v1/oidc/oidc_test.go:204 | TestLogin_ReplacesMismatchedStoredToken asserts only access_token on disk |
test-quality |
| Minor | sdk/go/docs/src/api/gateway.md:160 | ErrTokenLoad row and related comments do not mention expired tokens |
architecture |
| Notable | sdk/go/openshell/v1/gateway/token.go:107 | Reading and parsing oidc_token.json is still duplicated in the gateway and oidc packages |
architecture |
Not Posted Inline
sdk/go/docs/src/api/gateway.md:160: Location is outside the PR diff (line 160).
Minor: The ErrTokenLoad row in the error table still says "Token file missing or unreadable", and so does gateway/doc.go:64. The new paragraph right above the table (lines 151-154) lists a missing, malformed, or expired oidc_token.json, and the comment in gateway/errors.go:19-20 says "missing, unreadable, or malformed". None of these reference spots mention expired tokens.
Why this matters: The table introduces errors.Is classification, so a reader could wrap an RPC in errors.Is(err, gateway.ErrTokenLoad) to catch an expired OIDC token. That check never matches: the sentinel comes from GetRequestMetadata, gRPC turns it into a status, and the SDK StatusError does not keep the wrap chain. The paragraph above already points to v1.IsUnauthenticated, so this is about keeping the reference row in line with it. Expired tokens returned ErrTokenLoad before this PR too, so the wording gap is older than this change.
Suggested fix: In the table and in doc.go, either drop ErrTokenLoad or describe it as "returned by the auth provider's GetRequestMetadata for missing, malformed, expired, or pre-CLI-format token files; on RPCs it surfaces as Unauthenticated". Add the expired case to the errors.go comment.
Source: architecture agent
Review Details
- Findings posted: 6
- Findings reviewed and not posted: 1
- Gate outcome: PASS
- Participating reviewers: architecture, gator, production, test-quality
| r.mu.Unlock() | ||
| return map[string]string{"authorization": "Bearer " + tok}, nil | ||
| } | ||
| err := r.lastErr |
There was a problem hiding this comment.
Minor: During refresh backoff (up to 30s), GetRequestMetadata replays the cached lastErr. After a user follows the new "run openshell gateway login <name>" hint, a long-running client keeps failing with the stale "expired" message until the backoff window ends, instead of re-reading the freshly written oidc_token.json.
Why this matters: The backoff delay predates this PR, but the PR adds the login hint and the lastErr replay, and together they make the stale message visible to users. Scenario: a client created with gateway.NewClient (auth_mode=oidc) keeps issuing RPCs after the token in oidc_token.json expires. Each failed diskTokenSource.Token() doubles the backoff (1s, 2s, 4s, up to the 30s cap, auth_refresh.go:122-136), and the error reads "OIDC token for gateway "prod" expired at ...; run openshell gateway login prod" (gateway/token.go:117-139). The user runs the CLI login, which rewrites oidc_token.json with a valid token. The next RPC lands inside nextRetry, takes the branch at lines 96-104, finds usableCached() false, and returns the same "expired ... run login" error for up to 30s. To reproduce: take the TestResolveAuthProvider_OIDCPicksUpReplacedTokenNearExpiry setup with an expired token, call GetRequestMetadata about 6 times so the backoff grows, rewrite the file with a fresh token, and call again. Going by the code, that call returns the stale ErrTokenLoad error instead of Bearer token-b.
Suggested fix: Since you plan to move the gateway path to refresh in #4514, take this mainly as input for that change. Any one of these closes the gap: skip backoff for the disk source, because re-reading a local file is cheap (for example RefreshableToken(src, v1.WithMaxBackoff(0)) or an internal option); cap the disk-source backoff at about 1s; or mark the replayed error as cached, for example fmt.Errorf("%w (retrying in %s)", r.lastErr, time.Until(r.nextRetry).Round(time.Second)).
Source: gator agent (also flagged by: production agent)
There was a problem hiding this comment.
Thanks for your suggestion, I've noted this and will include it in #4514. Re-reading the file will prevent the backoff, meaning that only the IdP refresh will be affected. Probably also worth adding this case as a test.
| setupGatewayWithTokens(t, "oidc-gw", | ||
| `{"gateway_endpoint":"localhost:50051","auth_mode":"oidc"}`, | ||
| map[string]string{oidcTokenFile: `{"access_token":"test-oidc-token"}`}, | ||
| map[string]string{oidctoken.FileName: `{"access_token":"test-oidc-token"}`}, |
There was a problem hiding this comment.
Minor: TestNewClient_AuthModeOIDC still seeds oidc_token.json with {"access_token":"test-oidc-token"}. The new diskTokenSource rejects that shape because issuer and client_id are missing.
Why this matters: The fixture is never read. WithAuth(&mockAuth{}) overrides resolveAuthProvider, and token loading is lazy anyway, so the test passes either way, and its comment already defers auth resolution to TestResolveAuthProvider_OIDC. Nothing is broken today, but the test now carries a token file in a format the SDK refuses, which is easy to copy into the next OIDC test.
Suggested fix: Build the fixture with oidcBundleJSON("test-oidc-token", "") so it matches the CLI format, the same way TestResolveAuthProvider_OIDC does.
Source: test-quality agent
There was a problem hiding this comment.
Updated TestNewClient_AuthModeOIDC to seed a token file in the CLI format.
| @@ -341,9 +344,9 @@ func TestResolveAuthProvider_CloudflareJWT(t *testing.T) { | |||
|
|
|||
| func TestResolveAuthProvider_OIDC(t *testing.T) { | |||
There was a problem hiding this comment.
Minor: No gateway-level test drives an unusable oidc_token.json through resolveAuthProvider and GetRequestMetadata. Nothing checks that callers of the composed provider get ErrTokenLoad plus the openshell gateway login <name> hint, on the first call and again during backoff.
Why this matters: The pieces are covered on their own. token_test.go asserts the error and the hint for missing, malformed, expired and legacy bundles, but it constructs the source directly with "prod", and auth_refresh_test.go asserts the replay during backoff with a generic error. The OIDC tests in gateway_test.go (TestResolveAuthProvider_OIDC, ...PicksUpReplacedTokenNearExpiry) cover only success paths. What stays untested is the one-line wiring in gateway.go:172 that passes cfg.Name into newDiskTokenSource: if it passed an empty name or cfg.Dir instead, the hint would name the wrong gateway and every test would still pass.
Suggested fix: Add TestResolveAuthProvider_OIDCLegacyFileReturnsLoginHint. Write a legacy bundle such as {"access_token":"a","expiry":"..."} into the gateway dir, call resolveAuthProvider(&Config{AuthMode: AuthModeOIDC, Dir: dir, Name: "prod"}), then call GetRequestMetadata twice and assert ErrorIs(err, ErrTokenLoad) and Contains(err.Error(), "openshell gateway login prod") both times. The second call takes the backoff branch and returns r.lastErr.
Source: test-quality agent
There was a problem hiding this comment.
Added tests as per your suggestion
| path := filepath.Join(d.dir, oidcTokenFile) | ||
|
|
||
| data, err := os.ReadFile(path) | ||
| data, err := os.ReadFile(filepath.Join(d.dir, oidctoken.FileName)) |
There was a problem hiding this comment.
Notable: internal/oidctoken now owns the type and the filename, but the read-and-unmarshal of oidc_token.json still exists twice: in the gateway diskTokenSource.Token and in the oidc readToken (oidc/token.go:58-75). The write path stays in the oidc package.
Why this matters: Both readers do os.ReadFile(filepath.Join(dir, oidctoken.FileName)) followed by json.Unmarshal into oidctoken.Bundle (gateway/token.go:107-115, oidc/token.go:59-72). That is about eight lines each, and the two handle a missing file differently on purpose, so this is a refactoring idea rather than a defect. A future change to how the file is read (a version field, stricter validation of the bundle) would still have to land in both places.
Suggested fix: Add oidctoken.Read(dir) (*Bundle, error) that returns fs.ErrNotExist unwrapped, and optionally oidctoken.Write(dir, Bundle) for the atomic 0600 write. Each package then wraps the raw error with its own sentinel (ErrTokenLoad or ErrTokenPersist) and keeps its own not-exist handling, while the shared package owns the whole on-disk contract.
Source: architecture agent
There was a problem hiding this comment.
Noted, I'll move read and write into oidctoken in #4514. The gateway refresher has to write the file there and can't import oidc, so it fits with the locking change.
| diskTok, err := readToken(tokenDir) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, diskTok) | ||
| assert.Equal(t, "login-access-token", diskTok.AccessToken) |
There was a problem hiding this comment.
Minor: TestLogin_ReplacesMismatchedStoredToken says Login rewrites the file in the CLI format, but on disk it only asserts access_token. It does not show that issuer, client_id, or expires_at were written.
Why this matters: readToken also parses a legacy-shaped file (it ignores unknown expiry/expires_in and leaves Issuer/ClientID empty). If the write after login dropped Issuer/ClientID, diskTok.AccessToken would still equal "login-access-token" and every subtest would pass. Login has a single write site (oidc.go:114) that TestLogin_GatewayResolution already checks, so the behavior is covered today; the gap is that this test's comment promises more than it asserts.
Suggested fix: In each subtest assert diskTok.Issuer == provider.URL, diskTok.ClientID == "test-client", and require.NotNil(diskTok.ExpiresAt), as TestLogin_GatewayResolution already does at line 325.
Source: test-quality agent
There was a problem hiding this comment.
Updated TestLogin_ReplacesMismatchedStoredToken to also assert the rewritten issuer and client ID.
Signed-off-by: Yuedong Wu <dwcn22@outlook.com>
6054443 to
edfcb47
Compare
Summary
The Go SDK now reads and writes
oidc_token.jsonin the CLI's format (expires_at,issuer,client_id), so one login works for both the CLI and Go programs.gateway.NewClientstops sending an expired token and fails with arun `openshell gateway login <name>`hint instead.Related Issue
Closes #4508
Changes
oidctoken.Bundlemirrors the CLI'sOidcTokenBundleand replaces the two private Go structs.gatewayrejects token files that are expired or lackissuerorclient_id, including files from earlier Go SDK builds. Every load error carries the login hint.oidc.Loginwrites the CLI format and reuses a stored token only for the same issuer and client ID.v1.RefreshableTokenno longer returns an expired cached token after a failed refresh. It returns the refresh error, which surfaces asUnauthenticated. This applies to every token source and matches the CLI.sdk/conformance/oidc-token-bundle.json. A Rust test checks it against the CLI's serialization, and Go tests check reading and writing it.Not covered: Go
Loginstill ignores the gateway'soidc_scopesandoidc_audience(#4513). Refresh and file locking stay in #4514.Release Notes
The Go SDK now shares
oidc_token.jsonwith the CLI, andv1.RefreshableTokenstops returning an expired cached token for any token source; runopenshell gateway loginonce to replace token files from earlier releases.Testing
mise run go:ci,cargo test -p openshell-bootstrap --lib oidc_token,cargo fmtandcargo clippyforopenshell-bootstrap, andmise run license:checkKey tests: the shared fixture in Rust and Go, the issue reproduction (an expired CLI-written file fails with the hint), and
RefreshableTokenfailing closed during backoff.Checklist