fix(sdk/go): honor explicitly-set zero option values in oidc defaults

[rhuss]

Summary

oidc.loginConfig.applyDefaults() decided whether to apply a default by inspecting the field's value (len(c.scopes) == 0, c.timeout == 0) rather than whether the caller had set it. As a result, a caller who explicitly passed a zero value had it silently replaced:

• WithTimeout(0) became the 2-minute default.
• WithScopes() (explicit empty) became the default scopes, even though WithScopes already records scopesSet.

This makes applyDefaults consult the *Set sentinels instead, so defaults fill only genuinely-unset fields. Unset behavior and non-zero explicit values are unchanged.

Related Issue

Follow-up to a review comment on #3232 (raised by @elezar): #3232 (comment). This is a small, localized pre-existing bug fix, so no separate issue is filed.

Changes

• oidc/options.go: add a timeoutSet sentinel (set by WithTimeout); applyDefaults now checks !scopesSet / !timeoutSet instead of the zero value.
• oidc/credentials_auth.go: guard the client-credentials token exchange so a zero timeout means "no deadline" rather than an already-expired context (matching the existing guards in Login and DeviceFlow).
• Tests: explicit empty scopes and explicit zero timeout are honored; unset still defaults; a client-credentials exchange with WithTimeout(0) succeeds instead of failing on a born-expired context.

Behavior change: WithTimeout(0) now means "no timeout" instead of the 2-minute default.

Testing

• mise run go:ci green (build, golangci-lint, gofmt, full go test, proto-check, docs-check).
• New oidc unit tests plus an end-to-end client-credentials test covering the zero-timeout path.

Checklist

• Conventional Commit message, DCO sign-off
• Tests added and passing
• No public option-type or entry-point signature changed
• Behavior change (WithTimeout(0)) documented above

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[elezar]

Does context.WithTimeout() interpret a 0 value as no deadline, or does it explicitly need this handling?

[rhuss]

context.WithTimeout(parent, 0) doesn't mean "no deadline". It's defined as WithDeadline(parent, time.Now().Add(timeout)), so a 0 (or negative) timeout sets the deadline to now and the context is born already-expired; any call using it returns context.DeadlineExceeded immediately. So the explicit timeout > 0 guard is needed to actually get "no deadline" behavior, and it mirrors what Login and DeviceFlow already do. Without it, WithTimeout(0) would break the client-credentials exchange instead of disabling the timeout (covered by TestClientCredentialsAuthZeroTimeoutHasNoDeadline).

[elezar]

The explicit-empty-scope behavior is blocking: WithScopes() now leaves the interactive Login and DeviceLogin flows without the required openid scope. Please validate that interactive OIDC scopes contain openid (or otherwise reject/normalize an empty list), while preserving empty/non-OIDC scopes for client-credentials flows.

Two non-blocking points for clarification:

• Now that set-ness is tracked, is there a reason not to initialize a default config and then apply options? That would make option precedence explicit and may avoid sentinel-driven defaulting.
• Please define the complete WithTimeout contract. The new guard treats every non-positive duration as no deadline, whereas the PR description discusses only 0. Document and test d <= 0 as no deadline, or reject negative values.

[rhuss]

Pushed 89a0f9a addressing all three points. Reasoning below.

1. Explicit empty scopes on the interactive flows (blocking)

Confirmed, and there was already an answer in the repo that I should have mirrored from the start.

Login is affected. buildAuthURL sets the parameter unconditionally (authcode.go:67):

q.Set("scope", strings.Join(scopes, " "))

An explicitly-empty list emits a literal scope= in the authorization URL, since url.Values.Encode keeps empty values.

DeviceLogin is not affected the same way. requestDeviceCode already guards with if len(scopes) > 0 and omits the parameter entirely rather than sending an empty one.

The real gap is broader than the empty list. The Go SDK never had the normalization the Rust CLI already does. build_scopes in crates/openshell-cli/src/oidc_auth.rs prepends openid to every interactive request and drops a caller-supplied duplicate, while build_ci_scopes deliberately does not force it on the client-credentials path. That is the split you are asking for, already shipped; the Go SDK simply diverged from it.

So rather than adding validation, I ported that normalization:

• Login and DeviceLogin call a new loginConfig.requireOpenIDScope() after applyDefaults(). openid is placed first and a caller-supplied duplicate is dropped; remaining scopes keep their order. WithScopes() becomes openid, and WithScopes("sandbox:read") becomes openid sandbox:read.
• ClientCredentials and NewClientCredentialsAuth are untouched. That grant has no user and no ID token, so openid stays off unless the caller asks for it, matching build_ci_scopes.

This closes the empty-list case you raised and also WithScopes("profile"), which has produced an openid-less interactive request since the Go SDK landed in #2702. Because it aligns the SDK with shipped CLI behavior rather than inventing a new rule, I treated it as in scope here instead of splitting it out. Normalizing rather than rejecting also keeps the option total: there is no new error path for callers to handle.

Two supporting details that convinced me a missing openid is a real failure rather than a cosmetic one:

• The gateway requires a sub claim: validation.set_required_spec_claims(&["iss", "aud", "exp", "sub"]) in crates/openshell-server/src/auth/oidc.rs.
• OidcClaims::extract_scopes filters openid, profile, email and offline_access out before authorization runs, so forcing openid cannot interfere with application scopes like sandbox:read.

For context on why the scopes change is in this PR at all, which the description undersold: before it, WithScopes() set scopesSet = true, then applyDefaults inspected the value (len(c.scopes) == 0) and overwrote the slice with openid profile email, and then the guard at credentials.go:46 saw scopesSet == true and kept them. The client-credentials grant was sending exactly the interactive scopes the comment above that guard says it must not send. The same applied to the gateway OIDCScopes fallback further down, where an explicit empty list could not suppress the configured scopes.

Tests added: wire-level assertions that both Login and DeviceLogin send openid for unset, explicitly-empty, and application-only scope lists, plus a table on the normalization itself (dedupe, ordering) and one confirming the client-credentials path is not normalized.

2. Default config, then apply options

Partly agree, and it splits by field.

For timeout you are right. Seeding the config with timeout: defaultTimeout before options run would let WithTimeout(0) survive on its own and timeoutSet would be unnecessary.

For scopes it does not work. Two call sites in resolveClientCredentialsConfig need to distinguish "the caller chose this" from "a default is sitting here", independent of the value:

• The client-credentials default must be no scopes rather than the interactive defaults, so it has to undo a seeded default. Set-ness is the only signal that tells it to.
• The gateway's OIDCScopes must fill only genuinely-unset scopes. With a seeded default there is nothing left to test.

audienceSet already exists for exactly this reason.

There is a smaller cost too: seeding needs a newLoginConfig() constructor and makes the zero value of loginConfig invalid, whereas applyDefaults() today is explicit and idempotent. Three production sites plus the option tests construct it as a zero value.

So the choice is one mechanism for both fields, or pre-seeding for timeout and set-ness for scopes. I kept the uniform one. If you would rather have the split, I will seed timeout and drop timeoutSet in a follow-up commit here.

3. WithTimeout contract

Agreed that the guard is > 0 while the description only discussed 0. Worth noting the > 0 form is not new: Login and DeviceLogin have both gated on cfg.timeout > 0 already, so d <= 0 has meant "no deadline" on those paths for a while. This PR makes the client-credentials exchange consistent with them rather than letting it build an already-expired context.

I documented d <= 0 as "no deadline" on WithTimeout and added coverage for a negative duration, rather than rejecting negatives: LoginOption is func(*loginConfig) with no error return, so rejection would have to happen at resolve time in three separate places for a value that already has a coherent meaning.

Verification

mise run go:ci is green (build, golangci-lint 0 issues, gofmt, full go test -race, proto-check, docs-check), and mise run pre-commit passes.

One note: this branch predates #3232, so it still carries the old inline option loop. Happy to rebase onto main before merge if you prefer that over letting the merge resolve it.