Skip to content

feat(openshell): pure planConnection auth-mode resolver (S2) - #90

Merged
robbycochran merged 1 commit into
mainfrom
rc-pr1-s2-plan-connection
Aug 21, 2026
Merged

feat(openshell): pure planConnection auth-mode resolver (S2)#90
robbycochran merged 1 commit into
mainfrom
rc-pr1-s2-plan-connection

Conversation

@robbycochran

@robbycochran robbycochran commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

PR1 · Slice 2 — pure planConnection auth-mode resolver

Second slice of the PR1 SDK-foundation ladder (S1 merged in #89). This slice extracts the single highest-value seam: one pure function that turns gateway metadata into a dial decision, so every auth-mode branch is table-tested before any dial code depends on it.

What this adds

  • internal/openshell/sdkclient/auth.goplanConnection(cfg, env):
    • Resolves all five auth modes (mtls, none/plaintext, cloudflare_jwt, oidc) plus the SA-OIDC branch (oidc + OPENSHELL_OIDC_CLIENT_SECRET present).
    • Single owner of mTLS cert-path derivation (<cfg.Dir>/mtls/{tls.crt,tls.key,ca.crt}).
    • Pure: no disk, no network, EnvLookup is injected (prod passes os.Getenv). No secret material appears in any returned field or error.
    • Unknown auth mode → wrapped ErrConfig, never panics.
    • SA-OIDC live path is branch-selected only and marked // TODO(PR8) — unverifiable here (our gateways are mTLS, not OIDC).
  • internal/openshell/sdkclient/auth_test.go — exhaustive table tests over every matrix row (asserting the exact three cert paths for mTLS, branch/mode for each mode, errors.Is(err, ErrConfig) for the unknown case) plus a secret-non-leak assertion.
  • internal/openshell/sdkclient/client.goNew refactored to route its branch decision through planConnection. Only the mTLS branch is dialed today; the other branches return ErrConfig until S3 wires their dial paths.

Firewall invariant (unchanged)

internal/openshell/sdkclient remains the only package that imports the OpenShell SDK. internal/openshell (the firewall) still has zero SDK imports. Purely additive — apply, deploy, internal/gateway, and internal/k8s are untouched.

Validation

  • go build ./... — pass
  • CGO_ENABLED=0 go test ./... — pass (8 packages)
  • go vet ./... — pass
  • planConnection table tests — 7 matrix subtests + secret-non-leak, pass
  • S1 mTLS Health E2E gate — re-run through the refactored planConnection path: gateway healthy, v0.0.85
  • Config test suite (make test-suite) — 23/23

Next slice (S3) completes the working Client: dial for every branch, provider mapping, and the single translate() error-meaning seam.

Summary by CodeRabbit

  • New Features

    • Improved SDK gateway connection setup with support for mTLS authentication and automatic TLS certificate configuration.
    • Added handling for unauthenticated, plaintext, Cloudflare JWT, and OIDC connection modes.
  • Bug Fixes

    • Unsupported authentication configurations now return clear configuration errors.
    • Improved protection of OIDC client-secret information so it is not retained in connection settings.

Extract a pure, exhaustively-tested resolver that turns gateway metadata
into a dial decision. planConnection covers all five auth modes plus the
SA-OIDC branch and is the single owner of mTLS cert-path derivation. It
performs no I/O (env lookup is injected) and never places secret material
in its output or errors.

Refactor sdkclient.New to route its branch decision through planConnection
instead of inlining the mTLS branch. Only the mTLS branch is dialed today
(the auth mode all our managed gateways use); the remaining branches return
ErrConfig until S3 wires their dial paths. The S1 mTLS Health E2E gate still
passes through the refactored path.

Table tests cover every branch-selection matrix row, assert the exact three
derived cert paths, and assert the client secret never leaks into any
connPlan field.
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 88820c52-6851-48da-935e-90233fd9d327

📥 Commits

Reviewing files that changed from the base of the PR and between 2af957e and dc52d2b.

📒 Files selected for processing (3)
  • internal/openshell/sdkclient/auth.go
  • internal/openshell/sdkclient/auth_test.go
  • internal/openshell/sdkclient/client.go

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.


Walkthrough

The SDK adds pure connection planning for gateway authentication. New uses the plan to resolve gateway metadata and TLS settings, currently allowing only mTLS dialing. Tests cover supported modes, invalid modes, and OIDC secret non-leakage.

Changes

SDK connection authentication

Layer / File(s) Summary
Connection planning and validation
internal/openshell/sdkclient/auth.go, internal/openshell/sdkclient/auth_test.go
planConnection resolves connection metadata, authentication branches, mTLS certificate paths, and OIDC client-secret presence. Tests cover supported modes, unsupported modes, zero-value plans, and secret non-leakage.
Client dialing integration
internal/openshell/sdkclient/client.go
New obtains the connection plan through os.Getenv, rejects non-mTLS plans with ErrConfig, and dials with the resolved gateway name and TLS settings.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to dc52d

This PR adds a localized, pure authentication-mode planning step and routes connection setup through it; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant SDKCaller
  participant New
  participant planConnection
  participant EnvLookup
  participant GatewayDialer

  SDKCaller->>New: provide gateway configuration
  New->>planConnection: resolve configuration with environment lookup
  planConnection->>EnvLookup: read OIDC client-secret variable
  EnvLookup-->>planConnection: return secret value
  planConnection-->>New: return connection plan or ErrConfig
  New->>GatewayDialer: dial with gateway name and TLS settings
  GatewayDialer-->>New: return connection
  New-->>SDKCaller: return SDK client
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: adding a pure planConnection authentication-mode resolver.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rc-pr1-s2-plan-connection

Comment @coderabbitai help to get the list of available commands.

@robbycochran
robbycochran merged commit 7805fca into main Aug 21, 2026
7 checks passed
robbycochran added a commit that referenced this pull request Aug 21, 2026
…apping (PR1 S3) (#91)

* feat(sdkclient): complete Client with dial, translate, and provider mapping (PR1 S3)

Third slice of the PR1 SDK-foundation ladder (S1 #89, S2 #90). Completes the
working openshell.Client over the OpenShell SDK, behind the harness firewall.

- dial(connPlan) executes every auth branch: mTLS (verified live), default,
  and SA-OIDC (dialSAOIDC; compiled and branch-selected but // UNVERIFIED —
  no OIDC gateway available; gates PR8).
- errors.go: translate() is the single owner of SDK-error meaning, mapping
  v1.Is* typed errors to the openshell.Err* sentinels; unknown errors pass
  through unchanged. Callers branch via errors.Is, never on SDK codes.
- provider.go: fromSDKProvider maps the SDK provider view to the minimal
  harness Provider (least-exposure firewall).
- NewFromClient is the injection seam (used by white-box tests and S4
  testutil); New now routes through the general dial(plan) path.
- Health() and Providers() are live and route errors through translate().
- connPlan gains non-secret oidcIssuer/oidcClientID for the SA-OIDC dial; the
  client secret is still never stored in the plan (re-read at dial time only).

White-box tests via the SDK fake cover Health (healthy/unhealthy), Providers
(with/without), translate (all SDK codes + nil + passthrough), error
translation on a closed client, and Close idempotency.

Firewall unchanged: internal/openshell/sdkclient remains the only production
package importing the SDK; internal/openshell has zero SDK imports. Additive
only.

* fix(sdkclient): make SA-OIDC token source genuinely refreshing

Addresses CodeRabbit review on PR #91. oidc.ClientCredentials returns a
one-shot token that expires, and v1.RefreshableToken re-calls its source once
the cached token nears expiry — so oauth2.StaticTokenSource pinned a single
token and would break auth the moment it expired.

Replace it with an eager grant (preserving fast-fail ErrUnauthenticated
classification) wrapped in oauth2.ReuseTokenSource(tok, refresher), where the
refresher re-runs the client-credentials grant on a context.WithoutCancel
context so refreshes outlive the dial call while keeping context values. The
eager token is served first (no wasted grant). Secret is read fresh from the
environment inside the source at grant time and never stored on the plan.

Path remains // UNVERIFIED (no OIDC gateway); gates PR8.

* fix(sdkclient): bound each SA-OIDC grant with a timeout

Addresses follow-up CodeRabbit review on PR #91. context.WithoutCancel strips
the parent deadline as well as cancellation, so a stalled OIDC token endpoint
could block a background token refresh indefinitely.

Bound both the eager grant and every refresh grant with their own
context.WithTimeout(oidcGrantTimeout=30s), cancelled after the grant. Token
reuse and the existing clientCredentials invocation are preserved.

Path remains // UNVERIFIED (no OIDC gateway); gates PR8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant