Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions crates/openshell-bootstrap/src/oidc_token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,17 @@ mod tests {
assert!(is_token_actually_expired(&expired));
}

#[test]
fn conformance_fixture_matches_cli_serialization() {
const FIXTURE: &str = include_str!("../../../sdk/conformance/oidc-token-bundle.json");
let bundle: OidcTokenBundle = serde_json::from_str(FIXTURE).unwrap();
assert!(bundle.refresh_token.is_some() && bundle.expires_at.is_some());
assert_eq!(
serde_json::to_value(&bundle).unwrap(),
serde_json::from_str::<serde_json::Value>(FIXTURE).unwrap()
);
}

#[test]
fn oidc_login_prompt_marker_is_per_gateway() {
let tmp = tempfile::tempdir().unwrap();
Expand Down
1 change: 1 addition & 0 deletions scripts/update_license_headers.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
"deploy/deb/control.in",
"scripts/keycloak-realm.json",
"sdk/conformance/oauth-client-credentials.json",
"sdk/conformance/oidc-token-bundle.json",
"sdk/typescript/biome.json",
"sdk/typescript/tsconfig.build.json",
"sdk/typescript/tsconfig.json",
Expand Down
7 changes: 7 additions & 0 deletions sdk/conformance/oidc-token-bundle.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"access_token": "conformance-access-token",
"refresh_token": "conformance-refresh-token",
"expires_at": 4102444800,
"issuer": "https://issuer.example.com",
"client_id": "openshell-cli"
}
2 changes: 2 additions & 0 deletions sdk/conformance/oidc-token-bundle.json.license
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
SPDX-License-Identifier: Apache-2.0
3 changes: 2 additions & 1 deletion sdk/go/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ defer client.Close()
```

Concurrent callers share a single refresh call. If the token source fails, the
SDK falls back to the cached token with a logged warning. See the
SDK falls back to the cached token with a logged warning until that token
expires; after that, RPCs fail with the refresh error. See the
[Auth](https://ro14nd.de/openshell-sdk-go/api/auth.html) docs for details.

### With edge proxy headers
Expand Down
14 changes: 8 additions & 6 deletions sdk/go/docs/src/api/gateway.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ client, err := gateway.NewClient("staging",
)
```

**Errors**: `ErrGatewayNotFound`, `ErrConfigParse`, `ErrTokenLoad`,
**Errors**: `ErrGatewayNotFound`, `ErrConfigParse`,
`ErrUnsupportedAuthMode`, `ErrInvalidGatewayName`, `ErrNoActiveGateway`

### LoadConfig
Expand Down Expand Up @@ -124,7 +124,7 @@ type Info struct {
| `""` or `"none"` | `AuthModeNone` | `v1.NoAuth()` |
| `"plaintext"` | `AuthModePlaintext` | `v1.NoAuth()` + insecure TLS |
| `"cloudflare_jwt"` | `AuthModeCloudflareJWT` | Lazy edge token auth |
| `"oidc"` | `AuthModeOIDC` | `v1.RefreshableToken` with disk source |
| `"oidc"` | `AuthModeOIDC` | `v1.RefreshableToken` that reads `oidc_token.json` near expiry and fails once the token has expired |
| `"mtls"` | `AuthModeMTLS` | Unsupported (use `WithAuth`) |

### ClientOption
Expand All @@ -146,16 +146,18 @@ client, err := gateway.NewClient("my-gateway")
if errors.Is(err, gateway.ErrGatewayNotFound) {
fmt.Println("Gateway not configured. Run: openshell gateway add my-gateway")
}
if errors.Is(err, gateway.ErrTokenLoad) {
fmt.Println("Token expired or missing. Run: openshell gateway login my-gateway")
}
```

Tokens load lazily, so `NewClient` does not return `ErrTokenLoad`. For `oidc`
gateways, a missing, malformed, or expired `oidc_token.json` surfaces on RPCs as
an error for which `v1.IsUnauthenticated(err)` is true, and its message includes
``run `openshell gateway login <name>` ``.

| Error | Meaning |
|-------|---------|
| `ErrGatewayNotFound` | No gateway directory in user or system paths |
| `ErrConfigParse` | metadata.json missing or malformed |
| `ErrTokenLoad` | Token file missing or unreadable |
| `ErrTokenLoad` | Token file missing, unreadable, malformed, or expired; RPCs report it as Unauthenticated |
| `ErrUnsupportedAuthMode` | Unrecognized auth_mode value |
| `ErrInvalidGatewayName` | Name fails validation |
| `ErrNoActiveGateway` | No active gateway configured |
Expand Down
5 changes: 4 additions & 1 deletion sdk/go/docs/src/api/oidc.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ PKCE. If the provider does not support PKCE (S256), the flow proceeds
without it.

Tokens are persisted to the gateway's config directory as
`oidc_token.json`. Subsequent calls reuse a valid cached token.
`oidc_token.json`, in the same format as `openshell gateway login`, so CLI
commands against that gateway can use them. Subsequent calls reuse the stored
token only when it is valid and was issued for the gateway's issuer and client
ID; otherwise `Login` runs the flow again and rewrites the file.

For standalone use (no gateway), pass an empty `gatewayName` with
`WithIssuer` and `WithClientID`. Combine with `WithInMemory` to skip
Expand Down
18 changes: 15 additions & 3 deletions sdk/go/openshell/v1/auth_refresh.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ type refreshableAuth struct {
logger types.Logger
nextRetry time.Time
backoff time.Duration
lastErr error
}

// usableCached reports whether the cached token has not yet expired.
// Callers must hold r.mu.
func (r *refreshableAuth) usableCached() bool {
return r.tok != nil && (r.tok.Expiry.IsZero() || time.Now().Before(r.tok.Expiry))
}

func (r *refreshableAuth) isTokenValid() bool {
Expand All @@ -87,13 +94,14 @@ func (r *refreshableAuth) GetRequestMetadata(ctx context.Context, _ ...string) (
}

if !r.nextRetry.IsZero() && time.Now().Before(r.nextRetry) {
if r.tok != nil {
if r.usableCached() {
tok := r.tok.AccessToken
r.mu.Unlock()
return map[string]string{"authorization": "Bearer " + tok}, nil
}
err := r.lastErr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

r.mu.Unlock()
return nil, errors.New("openshell: token refresh failed and backoff is active")
return nil, err
}
r.mu.Unlock()

Expand Down Expand Up @@ -127,8 +135,9 @@ func (r *refreshableAuth) GetRequestMetadata(ctx context.Context, _ ...string) (
r.backoff = bo
r.nextRetry = time.Now().Add(bo)
}
r.lastErr = err

if r.tok != nil {
if r.usableCached() {
Comment thread
lunarwhite marked this conversation as resolved.
Resolved
if r.logger != nil {
r.logger.Error(err, "token refresh failed, using cached token")
}
Expand All @@ -144,6 +153,7 @@ func (r *refreshableAuth) GetRequestMetadata(ctx context.Context, _ ...string) (
r.tok = tok
r.backoff = 0
r.nextRetry = time.Time{}
r.lastErr = nil
return map[string]string{"authorization": "Bearer " + r.tok.AccessToken}, nil
}

Expand All @@ -155,6 +165,8 @@ func (r *refreshableAuth) RequireTransportSecurity() bool {
// and refreshes them before expiry. Concurrent callers share a single
// in-flight refresh via singleflight. Failed refreshes trigger exponential
// backoff (1s, 2s, 4s, ..., 30s cap) to avoid amplifying token endpoint outages.
// After a failed refresh, the cached token is used only until it expires;
// after that, requests fail with the last refresh error.
func RefreshableToken(src oauth2.TokenSource, opts ...RefreshOption) (AuthProvider, error) {
if src == nil {
return nil, errNilTokenSource
Expand Down
70 changes: 60 additions & 10 deletions sdk/go/openshell/v1/auth_refresh_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package v1

import (
"context"
"errors"
"fmt"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -194,38 +195,69 @@ func TestGetRequestMetadata_RefreshFailureReturnsStaleCachedToken(t *testing.T)
tokenFunc: func() (*oauth2.Token, error) {
n := callNum.Add(1)
if n == 1 {
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(-time.Minute)}, nil
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(5 * time.Second)}, nil
}
return nil, fmt.Errorf("idp unavailable")
},
}
provider, err := RefreshableToken(src, WithLeeway(0))
provider, err := RefreshableToken(src) // default 10s leeway
require.NoError(t, err)

// First call succeeds but returns already-expired token.
// First call succeeds with a token inside the leeway window.
_, err = provider.GetRequestMetadata(context.Background())
require.NoError(t, err)

// Second call: refresh fails, should return stale token.
// Second call: refresh fails, should return the unexpired stale token.
md, err := provider.GetRequestMetadata(context.Background())
require.NoError(t, err)
assert.Equal(t, "Bearer stale", md["authorization"])
}

func TestGetRequestMetadata_RefreshFailureWithExpiredCachedTokenReturnsError(t *testing.T) {
errIDP := errors.New("idp unavailable")
var callNum atomic.Int32
src := &mockTokenSource{
tokenFunc: func() (*oauth2.Token, error) {
n := callNum.Add(1)
if n == 1 {
return &oauth2.Token{AccessToken: "expired", Expiry: time.Now().Add(-time.Minute)}, nil
}
return nil, errIDP
},
}
logger := &captureLogger{}
provider, err := RefreshableToken(src, WithLeeway(0), WithLogger(logger))
require.NoError(t, err)

_, err = provider.GetRequestMetadata(context.Background())
require.NoError(t, err)

md, err := provider.GetRequestMetadata(context.Background())
require.ErrorIs(t, err, errIDP)
assert.Nil(t, md)
assert.Empty(t, logger.errors)

// During backoff, the same error is returned without calling the source.
md, err = provider.GetRequestMetadata(context.Background())
require.ErrorIs(t, err, errIDP)
assert.Nil(t, md)
assert.Equal(t, 2, src.calls())
}

func TestGetRequestMetadata_RefreshFailureLogsWarning(t *testing.T) {
var callNum atomic.Int32
src := &mockTokenSource{
tokenFunc: func() (*oauth2.Token, error) {
n := callNum.Add(1)
if n == 1 {
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(-time.Minute)}, nil
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(5 * time.Second)}, nil
}
return nil, fmt.Errorf("idp unavailable")
},
}

logger := &captureLogger{}
provider, err := RefreshableToken(src, WithLeeway(0), WithLogger(logger))
provider, err := RefreshableToken(src, WithLogger(logger))
require.NoError(t, err)

_, _ = provider.GetRequestMetadata(context.Background())
Expand Down Expand Up @@ -255,12 +287,12 @@ func TestGetRequestMetadata_RefreshFailureNoLoggerNoPanic(t *testing.T) {
tokenFunc: func() (*oauth2.Token, error) {
n := callNum.Add(1)
if n == 1 {
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(-time.Minute)}, nil
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(5 * time.Second)}, nil
}
return nil, fmt.Errorf("idp unavailable")
},
}
provider, err := RefreshableToken(src, WithLeeway(0))
provider, err := RefreshableToken(src)
require.NoError(t, err)

_, _ = provider.GetRequestMetadata(context.Background())
Expand Down Expand Up @@ -348,12 +380,12 @@ func TestGetRequestMetadata_BackoffSkipsRefreshDuringWindow(t *testing.T) {
tokenFunc: func() (*oauth2.Token, error) {
n := callNum.Add(1)
if n == 1 {
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(-time.Minute)}, nil
return &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(5 * time.Second)}, nil
}
return nil, fmt.Errorf("idp down")
},
}
provider, err := RefreshableToken(src, WithLeeway(0))
provider, err := RefreshableToken(src)
require.NoError(t, err)

_, err = provider.GetRequestMetadata(context.Background())
Expand All @@ -374,6 +406,24 @@ func TestGetRequestMetadata_BackoffSkipsRefreshDuringWindow(t *testing.T) {
assert.Equal(t, beforeCount, src.calls(), "should not call Token() during backoff window")
}

func TestGetRequestMetadata_BackoffWithoutCachedTokenReturnsLastError(t *testing.T) {
errIDP := errors.New("idp down")
src := &mockTokenSource{
tokenFunc: func() (*oauth2.Token, error) {
return nil, errIDP
},
}
provider, err := RefreshableToken(src)
require.NoError(t, err)

_, err = provider.GetRequestMetadata(context.Background())
require.ErrorIs(t, err, errIDP)

_, err = provider.GetRequestMetadata(context.Background())
require.ErrorIs(t, err, errIDP)
assert.Equal(t, 1, src.calls(), "should not call Token() during backoff window")
}

func TestGetRequestMetadata_BackoffResetsOnSuccess(t *testing.T) {
var callNum atomic.Int32
src := &mockTokenSource{
Expand Down
2 changes: 1 addition & 1 deletion sdk/go/openshell/v1/gateway/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
//
// - [ErrGatewayNotFound]: no gateway directory found
// - [ErrConfigParse]: metadata.json missing or malformed
// - [ErrTokenLoad]: token file missing or unreadable
// - [ErrTokenLoad]: token file missing, unreadable, malformed, or expired; RPCs report it as Unauthenticated
// - [ErrUnsupportedAuthMode]: unrecognized auth_mode value
// - [ErrInvalidGatewayName]: name fails validation
// - [ErrNoActiveGateway]: no active gateway configured
Expand Down
2 changes: 1 addition & 1 deletion sdk/go/openshell/v1/gateway/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ var (
ErrConfigParse = errors.New("gateway: config parse error")

// ErrTokenLoad is returned when a token file (edge_token,
// oidc_token.json) is missing, unreadable, or malformed.
// oidc_token.json) is missing, unreadable, malformed, or expired.
ErrTokenLoad = errors.New("gateway: token load error")

// ErrUnsupportedAuthMode is returned when the auth_mode value in
Expand Down
2 changes: 1 addition & 1 deletion sdk/go/openshell/v1/gateway/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func resolveAuthProvider(cfg *Config) (types.AuthProvider, error) {
return &lazyEdgeAuth{loader: &edgeTokenLoader{dir: cfg.Dir}}, nil

case AuthModeOIDC:
src := newDiskTokenSource(cfg.Dir)
src := newDiskTokenSource(cfg.Dir, cfg.Name)
return v1.RefreshableToken(src)

case AuthModeMTLS:
Expand Down
Loading
Loading