fix(sdk): address client credentials review

Signed-off-by: Seth Jennings <sjenning@redhat.com>

Author: sjenning

architecture/gateway.md

@@ -193,8 +193,9 @@ Python and Go SDK client-credentials providers can use the same registered
 issuer, client ID, audience, and scope metadata; the TypeScript provider accepts
 those fields explicitly. All three own a separate in-memory lifecycle, repeat
 the grant before expiry, and never persist the client secret or acquired access
-token into the CLI token cache. This keeps non-interactive SDK authentication
-independent from refresh-token rotation and shared disk state.
+token into the CLI token cache. They require TLS when sending renewable bearer
+credentials to non-loopback gateways. This keeps non-interactive SDK
+authentication independent from refresh-token rotation and shared disk state.
 
 Gateway health and user authentication are separate probes. `OpenShell.Health`
 remains unauthenticated so deployment and load-balancer health checks do not

docs/reference/gateway-auth.mdx

@@ -127,7 +127,8 @@ authentication directly. Configure the service account at the identity provider
 with the audience, roles, scopes, and workspace membership required by the
 gateway. The SDKs discover the token endpoint, attach the bearer token to each
 RPC, and repeat the grant before expiry. They keep the client secret and access
-token in memory and do not update the CLI's `oidc_token.json`.
+token in memory and do not update the CLI's `oidc_token.json`. SDK clients
+require TLS when sending these credentials to a non-loopback gateway.
 
 <Tabs>
   <Tab title="Python">

docs/sandboxes/manage-sandboxes.mdx

@@ -269,7 +269,7 @@ with SandboxClient.from_active_cluster() as client:
 
 For non-interactive automation, pass a renewable client-credentials provider.
 Omitted issuer, client ID, audience, and scopes are read from the active
-gateway's metadata:
+gateway's metadata. The client requires TLS for non-loopback gateways:
 
 ```python
 from openshell import ClientCredentialsAuth, SandboxClient

python/openshell/sandbox.py

@@ -130,6 +130,22 @@ def _normalize_bearer(
     return lambda: token
 
 
+def _is_loopback_host(hostname: str | None) -> bool:
+    if hostname is None:
+        return False
+    if hostname.lower() == "localhost":
+        return True
+    with contextlib.suppress(ValueError):
+        return ipaddress.ip_address(hostname).is_loopback
+    return False
+
+
+def _is_local_grpc_endpoint(endpoint: str) -> bool:
+    if endpoint.startswith("unix:"):
+        return True
+    return _is_loopback_host(urlparse(f"//{endpoint}").hostname)
+
+
 def _validate_oauth_url(name: str, raw: str) -> str:
     """Validate an OAuth endpoint without reflecting attacker-controlled URLs."""
     parsed = urlparse(raw)
@@ -139,11 +155,7 @@ def _validate_oauth_url(name: str, raw: str) -> str:
         raise SandboxError(f"OAuth {name} URL must not contain a fragment")
     if parsed.scheme == "https":
         return raw
-    loopback = parsed.hostname.lower() == "localhost"
-    if not loopback:
-        with contextlib.suppress(ValueError):
-            loopback = ipaddress.ip_address(parsed.hostname).is_loopback
-    if parsed.scheme == "http" and loopback:
+    if parsed.scheme == "http" and _is_loopback_host(parsed.hostname):
         return raw
     raise SandboxError(
         f"OAuth {name} URL must use HTTPS (HTTP is allowed only for loopback hosts)"
@@ -510,7 +522,8 @@ def __init__(
                 the gateway uses mTLS for transport identity and OIDC
                 for user identity.
             client_credentials: renewable OAuth client-credentials provider.
-                Mutually exclusive with `bearer_token`.
+                Mutually exclusive with `bearer_token`. A non-loopback endpoint
+                requires `tls` so the acquired bearer is never sent in cleartext.
             timeout: default per-call timeout in seconds.
             cluster_name: optional friendly name for error messages.
             _bearer_close: internal — wired by `from_active_cluster`
@@ -524,6 +537,14 @@ def __init__(
             raise SandboxError(
                 "bearer_token and client_credentials are mutually exclusive"
             )
+        if (
+            client_credentials is not None
+            and tls is None
+            and not _is_local_grpc_endpoint(endpoint)
+        ):
+            raise SandboxError(
+                "OAuth client credentials require TLS for non-loopback gateway endpoints"
+            )
         self._endpoint = endpoint
         self._timeout = timeout
         self._cluster_name = cluster_name
@@ -592,7 +613,7 @@ def from_active_cluster(
             client_credentials: renewable OAuth client-credentials provider.
                 Omitted issuer, client ID, audience, and scopes are filled from
                 the registered gateway metadata. This provider does not read or
-                write `oidc_token.json`.
+                write `oidc_token.json`. Remote plaintext gateways are rejected.
         """
         cluster_name = cluster or _resolve_active_cluster()
         gateway_dir = _xdg_config_home() / "openshell" / "gateways" / cluster_name
@@ -638,7 +659,6 @@ def from_active_cluster(
                     f"gateway '{cluster_name}' is not configured for OIDC"
                 )
             client_credentials._apply_gateway_metadata(metadata, insecure=insecure)
-            bearer_token = client_credentials
         elif metadata.get("auth_mode") == "oidc":
             bearer_token, bearer_close = _make_cluster_bearer_provider(
                 gateway_dir,
@@ -652,6 +672,7 @@ def from_active_cluster(
             endpoint,
             tls=tls,
             bearer_token=bearer_token,
+            client_credentials=client_credentials,
             timeout=timeout,
             cluster_name=cluster_name,
             _bearer_close=bearer_close,

python/openshell/sandbox_test.py

@@ -1565,6 +1565,68 @@ def test_from_active_cluster_fills_client_credentials_from_metadata(
         client.close()
 
 
+def test_sandbox_client_rejects_client_credentials_on_remote_plaintext(
+    monkeypatch: Any,
+) -> None:
+    channel_opened = False
+
+    def insecure_channel(_endpoint: str) -> Any:
+        nonlocal channel_opened
+        channel_opened = True
+        raise AssertionError("plaintext channel must not be opened")
+
+    monkeypatch.setattr(sandbox_module.grpc, "insecure_channel", insecure_channel)
+    auth = ClientCredentialsAuth(
+        issuer="https://issuer.example.com",
+        client_id="client",
+        client_secret="secret",
+    )
+    with pytest.raises(SandboxError, match="require TLS"):
+        SandboxClient("gateway.example.com:50051", client_credentials=auth)
+    assert not channel_opened
+
+
+@pytest.mark.parametrize(
+    "endpoint",
+    ["localhost:50051", "127.42.0.1:50051", "[::1]:50051"],
+)
+def test_sandbox_client_allows_client_credentials_on_plaintext_loopback(
+    endpoint: str,
+) -> None:
+    auth = ClientCredentialsAuth(
+        issuer="https://issuer.example.com",
+        client_id="client",
+        client_secret="secret",
+    )
+    client = SandboxClient(endpoint, client_credentials=auth)
+    client.close()
+
+
+def test_from_active_cluster_rejects_client_credentials_on_remote_plaintext(
+    tmp_path: Path,
+    monkeypatch: Any,
+) -> None:
+    gateway_dir = _setup_gateway_dir(
+        tmp_path,
+        monkeypatch,
+        endpoint="http://gateway.example.com:8080",
+        auth_mode="oidc",
+    )
+    metadata_path = gateway_dir / "metadata.json"
+    metadata = json.loads(metadata_path.read_text())
+    metadata.update(
+        {
+            "oidc_issuer": "https://issuer.example.com",
+            "oidc_client_id": "service-client",
+        }
+    )
+    metadata_path.write_text(json.dumps(metadata))
+
+    auth = ClientCredentialsAuth(client_secret="secret")
+    with pytest.raises(SandboxError, match="require TLS"):
+        SandboxClient.from_active_cluster(client_credentials=auth)
+
+
 def test_sandbox_client_rejects_ambiguous_bearer_configuration() -> None:
     auth = ClientCredentialsAuth(
         issuer="https://issuer.example.com",

sdk/go/openshell/v1/oidc/credentials.go

@@ -32,7 +32,7 @@ func ClientCredentials(ctx context.Context, opts ...LoginOption) (*oauth2.Token,
 	if err != nil {
 		return nil, err
 	}
-	return exchangeClientCredentials(ctx, cfg)
+	return exchangeClientCredentials(ctx, cfg, false)
 }
 
 func resolveClientCredentialsConfig(opts ...LoginOption) (*loginConfig, error) {
@@ -89,7 +89,7 @@ func resolveClientCredentialsConfig(opts ...LoginOption) (*loginConfig, error) {
 	return cfg, nil
 }
 
-func exchangeClientCredentials(ctx context.Context, cfg *loginConfig) (*oauth2.Token, error) {
+func exchangeClientCredentials(ctx context.Context, cfg *loginConfig, requirePositiveExpiry bool) (*oauth2.Token, error) {
 	secret := cfg.clientSecret
 	if cfg.secretProvider != nil {
 		value, err := cfg.secretProvider(ctx)
@@ -153,6 +153,10 @@ func exchangeClientCredentials(ctx context.Context, cfg *loginConfig) (*oauth2.T
 	if err := json.Unmarshal(body, &tokResp); err != nil {
 		return nil, fmt.Errorf("%w: invalid token response JSON", ErrClientCredentials)
 	}
+	var fields map[string]json.RawMessage
+	if err := json.Unmarshal(body, &fields); err != nil {
+		return nil, fmt.Errorf("%w: invalid token response JSON", ErrClientCredentials)
+	}
 
 	if resp.StatusCode != http.StatusOK || tokResp.Error != "" {
 		return nil, fmt.Errorf("%w: provider rejected the exchange (HTTP %d)", ErrClientCredentials, resp.StatusCode)
@@ -167,9 +171,16 @@ func exchangeClientCredentials(ctx context.Context, cfg *loginConfig) (*oauth2.T
 		RefreshToken: tokResp.RefreshToken,
 		TokenType:    tokResp.TokenType,
 	}
-	if tokResp.ExpiresIn <= 0 {
+	_, expiresInPresent := fields["expires_in"]
+	if expiresInPresent && tokResp.ExpiresIn <= 0 {
 		return nil, fmt.Errorf("%w: token response requires a positive expires_in", ErrClientCredentials)
 	}
+	if !expiresInPresent {
+		if requirePositiveExpiry {
+			return nil, fmt.Errorf("%w: token response requires a positive expires_in", ErrClientCredentials)
+		}
+		return tok, nil
+	}
 	tok.Expiry = time.Now().Add(time.Duration(tokResp.ExpiresIn) * time.Second)
 
 	return tok, nil

sdk/go/openshell/v1/oidc/credentials_auth.go

@@ -47,7 +47,7 @@ func (a *clientCredentialsAuth) GetRequestMetadata(ctx context.Context, _ ...str
 	result := a.group.DoChan("exchange", func() (any, error) {
 		exchangeCtx, cancel := context.WithTimeout(context.Background(), a.cfg.timeout)
 		defer cancel()
-		token, err := exchangeClientCredentials(exchangeCtx, a.cfg)
+		token, err := exchangeClientCredentials(exchangeCtx, a.cfg, true)
 		if err != nil {
 			return nil, err
 		}

sdk/go/openshell/v1/oidc/credentials_test.go

@@ -105,6 +105,39 @@ func TestClientCredentialsRejectsInvalidExpiryConformance(t *testing.T) {
 	}
 }
 
+func TestClientCredentialsMissingExpiryCompatibility(t *testing.T) {
+	resetDiscoveryCache()
+	var server *httptest.Server
+	server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/.well-known/openid-configuration" {
+			_ = json.NewEncoder(w).Encode(map[string]string{
+				"issuer":                 server.URL,
+				"authorization_endpoint": server.URL + "/authorize",
+				"token_endpoint":         server.URL + "/token",
+			})
+			return
+		}
+		_, _ = w.Write([]byte(`{"access_token":"token","token_type":"Bearer"}`))
+	}))
+	t.Cleanup(server.Close)
+
+	opts := []LoginOption{
+		WithIssuer(server.URL),
+		WithClientID("client"),
+		WithClientSecret("secret"),
+	}
+	token, err := ClientCredentials(context.Background(), opts...)
+	require.NoError(t, err)
+	assert.True(t, token.Expiry.IsZero())
+
+	auth, err := NewClientCredentialsAuth(opts...)
+	require.NoError(t, err)
+	metadata, err := auth.GetRequestMetadata(context.Background())
+	require.Error(t, err)
+	assert.Nil(t, metadata)
+	assert.Contains(t, err.Error(), "positive expires_in")
+}
+
 func TestClientCredentialsRefusesRedirectsConformance(t *testing.T) {
 	fixture := loadClientCredentialsFixture(t)
 	for _, status := range fixture.Discovery.RedirectStatuses {