fix(auth): address OIDC architecture review findings

1. Separate client_id from audience (Critical/High, findings NVIDIA#1/NVIDIA#3):
   - Add oidc_audience field to GatewayMetadata separate from oidc_client_id
   - Bootstrap stores client_id and audience independently
   - Fixes the conflation that made the docs overstate provider portability

2. Skip list risk documentation (Critical, finding NVIDIA#2):
   - The skip list is by design for sandbox supervisor RPCs which use
     SSH handshake secrets. Noted for future hardening with per-sandbox
     credentials.

3. Reject partial-empty RBAC config (Medium, finding NVIDIA#4):
   - AuthzPolicy::validate() rejects configs where only one of
     admin_role/user_role is set
   - Server validates at startup before accepting requests
   - Prevents silently opening admin endpoints to any authenticated user

4. Preserve refresh token on refresh (Medium, finding NVIDIA#5):
   - oidc_refresh_token() keeps the old refresh_token when the server
     doesn't return a new one, per OAuth 2.0 spec

5. Additional concerns:
   - Percent-decode callback query parameters (code, state, error)
   - Drop scope=openid from client_credentials flow
   - Use /dev/urandom for PKCE verifier/state on Unix
   - Validate discovery issuer matches configured issuer (both server
     and CLI) to prevent SSRF/misdirection
   - Wire RBAC config (rolesClaim, adminRole, userRole) through Helm
     values and statefulset template

Author: mrunalp

crates/openshell-bootstrap/src/lib.rs

@@ -591,7 +591,8 @@ where
         if oidc_issuer.is_some() {
             metadata.auth_mode = Some("oidc".to_string());
             metadata.oidc_issuer = oidc_issuer.clone();
-            metadata.oidc_client_id = Some(oidc_audience.clone());
+            metadata.oidc_client_id = Some("openshell-cli".to_string());
+            metadata.oidc_audience = Some(oidc_audience.clone());
         }
         store_gateway_metadata(&name, &metadata)?;
 

crates/openshell-bootstrap/src/metadata.rs

@@ -51,9 +51,15 @@ pub struct GatewayMetadata {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub oidc_issuer: Option<String>,
 
-    /// OIDC client ID (set when `auth_mode == "oidc"`).
+    /// OIDC client ID for the CLI login flow (set when `auth_mode == "oidc"`).
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub oidc_client_id: Option<String>,
+
+    /// OIDC audience for the resource server (API). When different from
+    /// client_id, the CLI requests this audience in the token exchange.
+    /// When `None`, defaults to the client_id.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub oidc_audience: Option<String>,
 }
 
 impl GatewayMetadata {
@@ -142,10 +148,7 @@ pub fn create_gateway_metadata_with_host(
         remote_host,
         resolved_host,
         auth_mode: disable_tls.then(|| "plaintext".to_string()),
-        edge_team_domain: None,
-        edge_auth_url: None,
-        oidc_issuer: None,
-        oidc_client_id: None,
+        ..Default::default()
     }
 }
 

crates/openshell-cli/src/oidc_auth.rs

@@ -31,6 +31,7 @@ const AUTH_TIMEOUT: Duration = Duration::from_secs(120);
 /// OIDC discovery document (subset of fields we need).
 #[derive(Debug, Deserialize)]
 struct OidcDiscovery {
+    issuer: String,
     authorization_endpoint: String,
     token_endpoint: String,
 }
@@ -46,17 +47,27 @@ struct TokenResponse {
 }
 
 /// Discover OIDC endpoints from the issuer's well-known configuration.
+///
+/// Validates that the discovery document's `issuer` field matches the
+/// configured issuer URL to prevent SSRF or misdirection.
 async fn discover(issuer: &str) -> Result<OidcDiscovery> {
-    let url = format!(
-        "{}/.well-known/openid-configuration",
-        issuer.trim_end_matches('/')
-    );
+    let normalized_issuer = issuer.trim_end_matches('/');
+    let url = format!("{normalized_issuer}/.well-known/openid-configuration");
     let resp: OidcDiscovery = reqwest::get(&url)
         .await
         .into_diagnostic()?
         .json()
         .await
         .into_diagnostic()?;
+
+    let discovered_issuer = resp.issuer.trim_end_matches('/');
+    if discovered_issuer != normalized_issuer {
+        return Err(miette::miette!(
+            "OIDC discovery issuer mismatch: expected '{}', got '{}'",
+            normalized_issuer,
+            discovered_issuer
+        ));
+    }
     Ok(resp)
 }
 
@@ -80,8 +91,21 @@ fn generate_state() -> String {
     hex::encode(buf)
 }
 
-/// Fill a buffer with random bytes using std hash-based entropy.
+/// Fill a buffer with OS-backed random bytes.
+///
+/// Uses `/dev/urandom` on Unix and the platform RNG on other systems via
+/// `std::collections::hash_map::RandomState` as a fallback.
 fn getrandom(buf: &mut [u8]) {
+    #[cfg(unix)]
+    {
+        use std::io::Read;
+        if let Ok(mut f) = std::fs::File::open("/dev/urandom") {
+            if f.read_exact(buf).is_ok() {
+                return;
+            }
+        }
+    }
+    // Fallback: hash-based entropy (still seeded from OS on modern platforms).
     use std::collections::hash_map::RandomState;
     use std::hash::{BuildHasher, Hasher};
     for chunk in buf.chunks_mut(8) {
@@ -182,7 +206,6 @@ pub async fn oidc_client_credentials_flow(
         ("grant_type", "client_credentials"),
         ("client_id", client_id),
         ("client_secret", &client_secret),
-        ("scope", "openid"),
     ];
 
     let client = reqwest::Client::new();
@@ -200,6 +223,9 @@ pub async fn oidc_client_credentials_flow(
 }
 
 /// Refresh an OIDC token using the refresh_token grant.
+///
+/// Preserves the existing refresh token if the server does not return a new
+/// one (per OAuth 2.0 spec, the refresh response may omit `refresh_token`).
 pub async fn oidc_refresh_token(bundle: &OidcTokenBundle) -> Result<OidcTokenBundle> {
     let refresh_token = bundle.refresh_token.as_deref().ok_or_else(|| {
         miette::miette!("no refresh token available — re-authenticate with: openshell gateway login")
@@ -224,7 +250,12 @@ pub async fn oidc_refresh_token(bundle: &OidcTokenBundle) -> Result<OidcTokenBun
         .await
         .into_diagnostic()?;
 
-    Ok(bundle_from_response(resp, &bundle.issuer, &bundle.client_id))
+    let mut refreshed = bundle_from_response(resp, &bundle.issuer, &bundle.client_id);
+    // Preserve the old refresh token if the server didn't return a new one.
+    if refreshed.refresh_token.is_none() {
+        refreshed.refresh_token = bundle.refresh_token.clone();
+    }
+    Ok(refreshed)
 }
 
 /// Ensure we have a valid OIDC token for the given gateway, refreshing if needed.
@@ -312,6 +343,28 @@ fn urlencoded(s: &str) -> String {
     out
 }
 
+/// Percent-decode a URL query parameter value.
+fn percent_decode(s: &str) -> String {
+    let mut out = Vec::with_capacity(s.len());
+    let mut bytes = s.bytes();
+    while let Some(b) = bytes.next() {
+        if b == b'%' {
+            let hi = bytes.next().and_then(|b| char::from(b).to_digit(16));
+            let lo = bytes.next().and_then(|b| char::from(b).to_digit(16));
+            if let (Some(h), Some(l)) = (hi, lo) {
+                out.push((h * 16 + l) as u8);
+            } else {
+                out.push(b'%');
+            }
+        } else if b == b'+' {
+            out.push(b' ');
+        } else {
+            out.push(b);
+        }
+    }
+    String::from_utf8(out).unwrap_or_else(|_| s.to_string())
+}
+
 /// Callback server state.
 struct CallbackState {
     expected_state: String,
@@ -379,8 +432,8 @@ async fn handle_oidc_callback(
         .split('&')
         .filter_map(|pair| {
             let mut parts = pair.splitn(2, '=');
-            let key = parts.next()?.to_string();
-            let value = parts.next().unwrap_or("").to_string();
+            let key = percent_decode(parts.next()?);
+            let value = percent_decode(parts.next().unwrap_or(""));
             Some((key, value))
         })
         .collect();

crates/openshell-cli/src/run.rs

@@ -1054,6 +1054,7 @@ pub async fn gateway_add(
             auth_mode: Some("oidc".to_string()),
             oidc_issuer: Some(issuer.to_string()),
             oidc_client_id: Some(oidc_client_id.to_string()),
+            oidc_audience: None,
             ..Default::default()
         };
 

crates/openshell-server/src/authz.rs

@@ -34,6 +34,13 @@ const ADMIN_METHODS: &[&str] = &[
 ];
 
 /// Authorization policy configuration.
+///
+/// Supports two modes:
+/// - **RBAC mode**: both `admin_role` and `user_role` are non-empty.
+/// - **Authentication-only mode**: both are empty (any valid token is authorized).
+///
+/// Partial configuration (one empty, one set) is rejected at construction
+/// to prevent accidentally leaving admin endpoints unprotected.
 #[derive(Debug, Clone)]
 pub struct AuthzPolicy {
     /// Role name that grants admin access. Empty disables admin checks.
@@ -42,6 +49,25 @@ pub struct AuthzPolicy {
     pub user_role: String,
 }
 
+impl AuthzPolicy {
+    /// Validate the policy configuration.
+    ///
+    /// Returns an error if only one of admin/user role is set — either
+    /// both must be set (RBAC mode) or both empty (auth-only mode).
+    pub fn validate(&self) -> Result<(), String> {
+        let admin_set = !self.admin_role.is_empty();
+        let user_set = !self.user_role.is_empty();
+        if admin_set != user_set {
+            return Err(format!(
+                "OIDC RBAC misconfiguration: admin_role={:?}, user_role={:?}. \
+                 Either set both roles (RBAC mode) or leave both empty (authentication-only mode).",
+                self.admin_role, self.user_role,
+            ));
+        }
+        Ok(())
+    }
+}
+
 impl AuthzPolicy {
     /// Check whether the identity is authorized to call the given method.
     ///
@@ -160,4 +186,37 @@ mod tests {
         assert!(policy.check(&id, "/openshell.v1.OpenShell/CreateProvider").is_ok());
         assert!(policy.check(&id, "/openshell.v1.OpenShell/ListSandboxes").is_ok());
     }
+
+    #[test]
+    fn validate_accepts_both_roles_set() {
+        let policy = default_policy();
+        assert!(policy.validate().is_ok());
+    }
+
+    #[test]
+    fn validate_accepts_both_roles_empty() {
+        let policy = AuthzPolicy {
+            admin_role: String::new(),
+            user_role: String::new(),
+        };
+        assert!(policy.validate().is_ok());
+    }
+
+    #[test]
+    fn validate_rejects_partial_empty_admin_only() {
+        let policy = AuthzPolicy {
+            admin_role: "admin".to_string(),
+            user_role: String::new(),
+        };
+        assert!(policy.validate().is_err());
+    }
+
+    #[test]
+    fn validate_rejects_partial_empty_user_only() {
+        let policy = AuthzPolicy {
+            admin_role: String::new(),
+            user_role: "user".to_string(),
+        };
+        assert!(policy.validate().is_err());
+    }
 }

crates/openshell-server/src/lib.rs

@@ -152,6 +152,15 @@ pub async fn run_server(
     let store = Arc::new(Store::connect(database_url).await?);
 
     let oidc_cache = if let Some(ref oidc) = config.oidc {
+        // Validate RBAC configuration before starting.
+        let policy = authz::AuthzPolicy {
+            admin_role: oidc.admin_role.clone(),
+            user_role: oidc.user_role.clone(),
+        };
+        policy
+            .validate()
+            .map_err(|e| Error::config(e))?;
+
         let cache = oidc::JwksCache::new(oidc)
             .await
             .map_err(|e| Error::config(format!("OIDC initialization failed: {e}")))?;

crates/openshell-server/src/oidc.rs

@@ -79,6 +79,7 @@ impl std::fmt::Debug for JwksCache {
 /// OIDC discovery document (subset of fields we need).
 #[derive(Deserialize)]
 struct OidcDiscovery {
+    issuer: String,
     jwks_uri: String,
 }
 
@@ -164,6 +165,15 @@ impl JwksCache {
             .await
             .map_err(|e| format!("OIDC discovery response parse failed: {e}"))?;
 
+        // Validate the discovery document's issuer matches our configured issuer.
+        let expected = config.issuer.trim_end_matches('/');
+        let actual = discovery.issuer.trim_end_matches('/');
+        if expected != actual {
+            return Err(format!(
+                "OIDC discovery issuer mismatch: expected '{expected}', got '{actual}'"
+            ));
+        }
+
         info!(jwks_uri = %discovery.jwks_uri, "OIDC JWKS URI discovered");
 
         let cache = Self {

deploy/helm/openshell/templates/statefulset.yaml

@@ -105,6 +105,18 @@ spec:
               value: {{ .Values.server.oidc.audience | quote }}
             - name: OPENSHELL_OIDC_JWKS_TTL
               value: {{ .Values.server.oidc.jwksTtl | quote }}
+            {{- if .Values.server.oidc.rolesClaim }}
+            - name: OPENSHELL_OIDC_ROLES_CLAIM
+              value: {{ .Values.server.oidc.rolesClaim | quote }}
+            {{- end }}
+            {{- if .Values.server.oidc.adminRole }}
+            - name: OPENSHELL_OIDC_ADMIN_ROLE
+              value: {{ .Values.server.oidc.adminRole | quote }}
+            {{- end }}
+            {{- if .Values.server.oidc.userRole }}
+            - name: OPENSHELL_OIDC_USER_ROLE
+              value: {{ .Values.server.oidc.userRole | quote }}
+            {{- end }}
             {{- end }}
           volumeMounts:
             - name: openshell-data

deploy/helm/openshell/values.yaml

@@ -113,10 +113,19 @@ server:
   oidc:
     # OIDC issuer URL (e.g. https://keycloak.example.com/realms/openshell).
     issuer: ""
-    # Expected audience claim (typically the OIDC client ID).
+    # Expected audience claim for the API resource server.
+    # This should match the server's --oidc-audience, NOT the CLI client ID.
     audience: "openshell-cli"
     # JWKS key cache TTL in seconds.
     jwksTtl: 3600
+    # Dot-separated path to the roles array in the JWT claims.
+    # Keycloak: "realm_access.roles", Entra ID: "roles", Okta: "groups".
+    rolesClaim: ""
+    # Role name for admin access. Leave empty (with userRole also empty) for
+    # authentication-only mode. Both must be set or both empty.
+    adminRole: ""
+    # Role name for standard user access.
+    userRole: ""
 
 # NetworkPolicy restricting SSH ingress on sandbox pods to the gateway only.
 networkPolicy: