Skip to content

Commit ca72b74

Browse files
committed
fix(auth): bound OIDC CA bundle reads
Signed-off-by: Gordon Sim <gsim@redhat.com>
1 parent 83d0be7 commit ca72b74

8 files changed

Lines changed: 155 additions & 36 deletions

File tree

‎.agents/skills/helm-dev-environment/SKILL.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,8 @@ development certificate and trust anchor; redeploy the gateway afterward so it r
342342
the mounted CA bundle. The chart renders the mount path as
343343
`[openshell.gateway.oidc] ca_bundle`; the gateway adds that issuer CA to native roots
344344
for OIDC discovery and JWKS requests without changing trust for other HTTPS clients.
345+
The mounted bundle must be a regular file no larger than 1 MiB; the gateway rejects
346+
other file types and oversized bundles during startup.
345347

346348
Then activate OIDC in the OpenShell Helm chart:
347349
1. Uncomment `#- ci/values-keycloak.yaml` in `skaffold.yaml`

‎architecture/gateway.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ Supported auth modes:
291291
| Plaintext | Local development or a trusted reverse proxy boundary. |
292292
| Unauthenticated local users | Trusted Kubernetes dev or fully trusted proxy deployments only. |
293293
| Cloudflare JWT | Edge-authenticated deployments where Cloudflare Access supplies identity. |
294-
| OIDC | Bearer-token auth for users, with browser or device-code PKCE and client credentials login. Discovery and JWKS retrieval require HTTPS, reject redirects, and pin JWKS to the issuer origin or an explicit origin allowlist. An optional private issuer CA augments native roots for the OIDC client only. JWKS validation accepts RS256, RS384, RS512, PS256, PS384, PS512, ES256, ES384, and EdDSA (Ed25519) signing keys. |
294+
| OIDC | Bearer-token auth for users, with browser or device-code PKCE and client credentials login. Discovery and JWKS retrieval require HTTPS, reject redirects, and pin JWKS to the issuer origin or an explicit origin allowlist. An optional private issuer CA augments native roots for the OIDC client only; its bundle must be a regular file no larger than 1 MiB. JWKS validation accepts RS256, RS384, RS512, PS256, PS384, PS512, ES256, ES384, and EdDSA (Ed25519) signing keys. |
295295

296296
The CLI persists the scopes requested during OIDC login in gateway metadata and
297297
reuses them when refreshing an access token. This preserves the intended API

‎crates/openshell-core/src/config.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -334,9 +334,9 @@ pub struct OidcConfig {
334334
/// OIDC issuer URL (e.g., `https://idp.example.com/realms/openshell`).
335335
pub issuer: String,
336336

337-
/// Optional PEM CA bundle for an issuer signed by a private CA. These
338-
/// certificates augment the platform trust roots for OIDC discovery and
339-
/// JWKS requests only.
337+
/// Optional PEM CA bundle for an issuer signed by a private CA. It must be
338+
/// a regular file no larger than 1 MiB. These certificates augment the
339+
/// platform trust roots for OIDC discovery and JWKS requests only.
340340
#[serde(default)]
341341
pub ca_bundle: Option<PathBuf>,
342342

‎crates/openshell-core/src/driver_utils.rs‎

Lines changed: 38 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ pub const MAX_UPSTREAM_PROXY_CREDENTIAL_BYTES: u64 = 4096;
389389
/// cannot be opened or stat'd, is not a regular file, or exceeds the size
390390
/// bound.
391391
pub fn read_upstream_proxy_credential_file(path: &str) -> Result<String, String> {
392-
read_regular_file_bounded(path, MAX_UPSTREAM_PROXY_CREDENTIAL_BYTES).map_err(|err| match err {
392+
read_regular_utf8_file_bounded(Path::new(path), MAX_UPSTREAM_PROXY_CREDENTIAL_BYTES).map_err(|err| match err {
393393
BoundedReadError::Open(e) => format!("failed to open proxy auth file '{path}': {e}"),
394394
BoundedReadError::Stat(e) => format!("failed to stat proxy auth file '{path}': {e}"),
395395
BoundedReadError::NotRegular => format!("proxy auth file '{path}' is not a regular file"),
@@ -430,19 +430,18 @@ pub const MAX_UPSTREAM_PROXY_CA_BUNDLE_BYTES: u64 = 1024 * 1024;
430430
/// cannot be read, is not a regular file, exceeds the size bound, or holds no
431431
/// usable certificate.
432432
pub fn read_upstream_proxy_ca_bundle_file(path: &str, label: &str) -> Result<String, String> {
433-
let pem = read_regular_file_bounded(path, MAX_UPSTREAM_PROXY_CA_BUNDLE_BYTES).map_err(
434-
|err| match err {
435-
BoundedReadError::Open(e) | BoundedReadError::Stat(e) | BoundedReadError::Read(e) => {
436-
format!("{label} '{path}' could not be read: {e}")
437-
}
438-
BoundedReadError::NotRegular => {
439-
format!("{label} '{path}' is not a regular file")
440-
}
441-
BoundedReadError::TooLarge => format!(
442-
"{label} '{path}' exceeds the {MAX_UPSTREAM_PROXY_CA_BUNDLE_BYTES}-byte limit"
443-
),
444-
},
445-
)?;
433+
let pem = read_regular_utf8_file_bounded(Path::new(path), MAX_UPSTREAM_PROXY_CA_BUNDLE_BYTES)
434+
.map_err(|err| match err {
435+
BoundedReadError::Open(e) | BoundedReadError::Stat(e) | BoundedReadError::Read(e) => {
436+
format!("{label} '{path}' could not be read: {e}")
437+
}
438+
BoundedReadError::NotRegular => {
439+
format!("{label} '{path}' is not a regular file")
440+
}
441+
BoundedReadError::TooLarge => {
442+
format!("{label} '{path}' exceeds the {MAX_UPSTREAM_PROXY_CA_BUNDLE_BYTES}-byte limit")
443+
}
444+
})?;
446445
validate_upstream_proxy_ca_bundle_pem(&pem, path, label)?;
447446
Ok(pem)
448447
}
@@ -491,21 +490,30 @@ pub fn validate_upstream_proxy_ca_bundle_pem(
491490

492491
/// Failure modes of [`read_regular_file_bounded`], so each caller can phrase
493492
/// them in terms of the operator setting it is reading.
494-
enum BoundedReadError {
493+
#[derive(Debug)]
494+
pub enum BoundedReadError {
495495
Open(std::io::Error),
496496
Stat(std::io::Error),
497497
NotRegular,
498498
TooLarge,
499499
Read(std::io::Error),
500500
}
501501

502-
/// Read a regular file into a `String`, rejecting anything larger than
502+
/// Read a regular file into memory, rejecting anything larger than
503503
/// `max_bytes` and anything that is not a regular file.
504504
///
505-
/// Backs the operator-supplied proxy file readers, which must never let a
506-
/// hostile or misconfigured path (`/dev/zero`, a FIFO, a directory, a huge
507-
/// file) exhaust memory or block the caller.
508-
fn read_regular_file_bounded(path: &str, max_bytes: u64) -> Result<String, BoundedReadError> {
505+
/// The file is opened nonblocking on Unix so a FIFO with no writer cannot hang
506+
/// the caller. The size is checked both before and during the read so a file
507+
/// that grows after it is opened cannot bypass the bound.
508+
///
509+
/// This is a blocking read. Async callers should run it with
510+
/// `tokio::task::spawn_blocking`.
511+
///
512+
/// # Errors
513+
///
514+
/// Returns the operation that failed, or a dedicated error when the path is
515+
/// not a regular file or exceeds `max_bytes`.
516+
pub fn read_regular_file_bounded(path: &Path, max_bytes: u64) -> Result<Vec<u8>, BoundedReadError> {
509517
use std::io::Read as _;
510518

511519
// Windows rejects opening a directory before a file handle is available,
@@ -544,16 +552,23 @@ fn read_regular_file_bounded(path: &str, max_bytes: u64) -> Result<String, Bound
544552
return Err(BoundedReadError::TooLarge);
545553
}
546554
// Bound the read even if the file grows between stat and read.
547-
let mut buf = String::new();
548-
file.take(max_bytes + 1)
549-
.read_to_string(&mut buf)
555+
let mut buf = Vec::new();
556+
file.take(max_bytes.saturating_add(1))
557+
.read_to_end(&mut buf)
550558
.map_err(BoundedReadError::Read)?;
551559
if buf.len() as u64 > max_bytes {
552560
return Err(BoundedReadError::TooLarge);
553561
}
554562
Ok(buf)
555563
}
556564

565+
fn read_regular_utf8_file_bounded(path: &Path, max_bytes: u64) -> Result<String, BoundedReadError> {
566+
let bytes = read_regular_file_bounded(path, max_bytes)?;
567+
String::from_utf8(bytes).map_err(|error| {
568+
BoundedReadError::Read(std::io::Error::new(std::io::ErrorKind::InvalidData, error))
569+
})
570+
}
571+
557572
/// Operator-supplied corporate upstream-proxy settings, as a borrowed view.
558573
///
559574
/// Compute drivers store these keys under their own

‎crates/openshell-server/src/auth/oidc.rs‎

Lines changed: 107 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use super::principal::{Principal, UserPrincipal};
1616
use async_trait::async_trait;
1717
use jsonwebtoken::{Algorithm, DecodingKey, Validation, decode, decode_header};
1818
use openshell_core::OidcConfig;
19+
use openshell_core::driver_utils::{BoundedReadError, read_regular_file_bounded};
1920
use reqwest::Client;
2021
use serde::Deserialize;
2122
use std::collections::{HashMap, HashSet};
@@ -58,6 +59,10 @@ const KID_MISS_REFRESH_COOLDOWN: Duration = Duration::from_secs(1);
5859
const OIDC_DISCOVERY_MAX_BYTES: usize = 64 * 1024;
5960
const JWKS_MAX_BYTES: usize = 1024 * 1024;
6061

62+
/// Hard upper bound for an operator-supplied OIDC CA bundle. This matches the
63+
/// Kubernetes `ConfigMap` size limit used to supply the bundle in Helm installs.
64+
const OIDC_CA_BUNDLE_MAX_BYTES: u64 = 1024 * 1024;
65+
6166
/// Cached JWKS key set fetched from the OIDC issuer.
6267
///
6368
/// A `refresh_mutex` ensures that only one refresh runs at a time,
@@ -583,12 +588,7 @@ impl JwksCache {
583588
.timeout(Duration::from_secs(10))
584589
.redirect(reqwest::redirect::Policy::none());
585590
if let Some(ca_bundle) = config.ca_bundle.as_deref() {
586-
let pem = std::fs::read(ca_bundle).map_err(|error| {
587-
format!(
588-
"failed to read OIDC CA bundle '{}': {error}",
589-
ca_bundle.display()
590-
)
591-
})?;
591+
let pem = read_oidc_ca_bundle(ca_bundle).await?;
592592
let certificates = reqwest::Certificate::from_pem_bundle(&pem).map_err(|error| {
593593
format!(
594594
"failed to parse OIDC CA bundle '{}': {error}",
@@ -895,6 +895,30 @@ impl JwksCache {
895895
}
896896
}
897897

898+
async fn read_oidc_ca_bundle(path: &std::path::Path) -> Result<Vec<u8>, String> {
899+
let path = path.to_path_buf();
900+
let display_path = path.display().to_string();
901+
let task_display_path = display_path.clone();
902+
tokio::task::spawn_blocking(move || {
903+
read_regular_file_bounded(&path, OIDC_CA_BUNDLE_MAX_BYTES).map_err(|error| match error {
904+
BoundedReadError::Open(error)
905+
| BoundedReadError::Stat(error)
906+
| BoundedReadError::Read(error) => {
907+
format!("failed to read OIDC CA bundle '{task_display_path}': {error}")
908+
}
909+
BoundedReadError::NotRegular => {
910+
format!("OIDC CA bundle '{task_display_path}' is not a regular file")
911+
}
912+
BoundedReadError::TooLarge => format!(
913+
"OIDC CA bundle '{task_display_path}' exceeds the \
914+
{OIDC_CA_BUNDLE_MAX_BYTES}-byte limit"
915+
),
916+
})
917+
})
918+
.await
919+
.map_err(|error| format!("failed to read OIDC CA bundle '{display_path}': {error}"))?
920+
}
921+
898922
/// Authenticator that validates `Authorization: Bearer <jwt>` headers against
899923
/// the configured OIDC issuer.
900924
///
@@ -978,6 +1002,83 @@ mod tests {
9781002
);
9791003
}
9801004

1005+
#[tokio::test]
1006+
async fn oidc_rejects_an_oversized_ca_bundle() {
1007+
let ca_bundle = tempfile::NamedTempFile::new().unwrap();
1008+
std::fs::write(
1009+
ca_bundle.path(),
1010+
vec![b'x'; usize::try_from(OIDC_CA_BUNDLE_MAX_BYTES + 1).unwrap()],
1011+
)
1012+
.unwrap();
1013+
let mut config = transport_test_config("https://issuer.example.com");
1014+
config.ca_bundle = Some(ca_bundle.path().to_path_buf());
1015+
1016+
let error = JwksCache::new(&config)
1017+
.await
1018+
.expect_err("an oversized CA bundle must fail before discovery");
1019+
1020+
assert!(
1021+
error.contains("OIDC CA bundle"),
1022+
"unexpected error: {error}"
1023+
);
1024+
assert!(error.contains("exceeds"), "unexpected error: {error}");
1025+
assert!(
1026+
error.contains(&OIDC_CA_BUNDLE_MAX_BYTES.to_string()),
1027+
"unexpected error: {error}"
1028+
);
1029+
}
1030+
1031+
#[cfg(unix)]
1032+
#[tokio::test]
1033+
async fn oidc_rejects_a_fifo_ca_bundle_without_blocking() {
1034+
let dir = tempfile::tempdir().unwrap();
1035+
let fifo = dir.path().join("oidc-ca-fifo");
1036+
nix::unistd::mkfifo(&fifo, nix::sys::stat::Mode::S_IRUSR).unwrap();
1037+
let mut config = transport_test_config("https://issuer.example.com");
1038+
config.ca_bundle = Some(fifo);
1039+
1040+
let start = Instant::now();
1041+
let error = JwksCache::new(&config)
1042+
.await
1043+
.expect_err("a FIFO CA bundle must fail before discovery");
1044+
1045+
assert!(
1046+
error.contains("OIDC CA bundle"),
1047+
"unexpected error: {error}"
1048+
);
1049+
assert!(
1050+
error.contains("not a regular file"),
1051+
"unexpected error: {error}"
1052+
);
1053+
assert!(
1054+
start.elapsed() < Duration::from_secs(5),
1055+
"reading a FIFO must not block"
1056+
);
1057+
}
1058+
1059+
#[cfg(unix)]
1060+
#[tokio::test]
1061+
async fn oidc_rejects_a_device_ca_bundle() {
1062+
if !std::path::Path::new("/dev/zero").exists() {
1063+
return;
1064+
}
1065+
let mut config = transport_test_config("https://issuer.example.com");
1066+
config.ca_bundle = Some("/dev/zero".into());
1067+
1068+
let error = JwksCache::new(&config)
1069+
.await
1070+
.expect_err("a device CA bundle must fail before discovery");
1071+
1072+
assert!(
1073+
error.contains("OIDC CA bundle"),
1074+
"unexpected error: {error}"
1075+
);
1076+
assert!(
1077+
error.contains("not a regular file"),
1078+
"unexpected error: {error}"
1079+
);
1080+
}
1081+
9811082
#[tokio::test]
9821083
async fn oidc_rejects_non_loopback_http_even_when_acknowledged() {
9831084
let mut config = transport_test_config("http://192.0.2.1/issuer");

‎crates/openshell-server/src/cli.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,8 @@ struct RunArgs {
163163
oidc_issuer: Option<String>,
164164

165165
/// Path to a PEM CA bundle for an OIDC issuer signed by a private CA.
166-
/// The certificates augment platform trust roots for OIDC requests only.
166+
/// Must be a regular file no larger than 1 MiB. The certificates augment
167+
/// platform trust roots for OIDC requests only.
167168
#[arg(long, env = "OPENSHELL_OIDC_CA_BUNDLE")]
168169
oidc_ca_bundle: Option<PathBuf>,
169170

‎docs/reference/gateway-config.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ Local Docker, Podman, and VM gateways can also set `[openshell.gateway.mtls_auth
265265

266266
The client-certificate handshake policy is derived and has no `require_client_auth` TOML field. This preserves bearer-only OIDC clients and prevents a file setting from silently weakening CA-only gateways.
267267

268-
`[openshell.gateway.oidc] ca_bundle` points to a certificate-only PEM bundle for an issuer signed by a private CA. The bundle augments platform trust roots for OIDC discovery and JWKS requests only; it does not replace native CA discovery or change trust for provider refresh, token exchange, telemetry, Vault, or other gateway HTTPS clients. Set the same value with `--oidc-ca-bundle` or `OPENSHELL_OIDC_CA_BUNDLE`. For Helm deployments, `server.oidc.caConfigMapName` mounts the ConfigMap's `ca.crt` key and renders this path automatically.
268+
`[openshell.gateway.oidc] ca_bundle` points to a certificate-only PEM bundle for an issuer signed by a private CA. The path must resolve to a regular file no larger than 1 MiB (1,048,576 bytes). The bundle augments platform trust roots for OIDC discovery and JWKS requests only; it does not replace native CA discovery or change trust for provider refresh, token exchange, telemetry, Vault, or other gateway HTTPS clients. Set the same value with `--oidc-ca-bundle` or `OPENSHELL_OIDC_CA_BUNDLE`. For Helm deployments, `server.oidc.caConfigMapName` mounts the ConfigMap's `ca.crt` key and renders this path automatically.
269269

270270
`[openshell.gateway.tls]` supports optional SNI-based dual-certificate mode for deployments that need separate internal and external server certificates. Set `external_cert_path` and `external_key_path` to point at the external (e.g. ACME/publicly-trusted) certificate and key. List the hostnames that should be served with the external certificate in `external_server_names`. Connections whose TLS SNI hostname matches one of those names receive the external certificate; all other connections (including those with no SNI) receive the primary internal certificate from `cert_path`/`key_path`. Both fields must be set together — providing only one is a configuration error. On Kubernetes with the Helm chart, the external certificate is managed automatically when `certManager.serverIssuerRef.name` is set; the chart populates these fields from the cert-manager-issued external server certificate.
271271

‎skills/debug-openshell-cluster/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -910,7 +910,7 @@ credential failures.
910910
| Vault credential driver returns HTTP 403 / `Vault Kubernetes auth denied the configured role` on provider create | Vault's `auth/kubernetes` method or the gateway login role is not provisioned, or the role is not bound to the gateway service account and namespace | In Vault: `bao auth enable kubernetes` and `bao write auth/kubernetes/config kubernetes_host=... kubernetes_ca_cert=@...`; ensure the login role's `bound_service_account_names`/`bound_service_account_namespaces` match the gateway SA and namespace and its policy grants the credential paths |
911911
| CLI TLS error | Local mTLS bundle does not match server cert/CA | Check `~/.config/openshell/gateways/<name>/mtls/` |
912912
| Edge or OIDC gateway returns `Unauthenticated` | Stored login expired, audience/scopes mismatch, or gateway auth configuration changed | `openshell gateway info`, `openshell gateway login <name>`, gateway auth logs |
913-
| Gateway exits during OIDC initialization | Issuer is not HTTPS, discovery redirected, metadata used a non-JSON media type or exceeded its size limit, the configured `ca_bundle` is missing or invalid, or `jwks_uri` uses an untrusted origin | Use an HTTPS issuer; mount a private CA with `server.oidc.caConfigMapName` and confirm `[openshell.gateway.oidc] ca_bundle` names the mounted `ca.crt`; keep JWKS on the issuer origin or explicitly add its HTTPS origin to `server.oidc.jwksAllowedOrigins`. The issuer CA augments platform roots for OIDC only. Numeric-loopback HTTP is development-only and also requires `server.oidc.dangerouslyAllowInsecureHttp=true` |
913+
| Gateway exits during OIDC initialization | Issuer is not HTTPS, discovery redirected, metadata used a non-JSON media type or exceeded its size limit, the configured `ca_bundle` is missing, invalid, non-regular, or larger than 1 MiB, or `jwks_uri` uses an untrusted origin | Use an HTTPS issuer; mount a private CA with `server.oidc.caConfigMapName` and confirm `[openshell.gateway.oidc] ca_bundle` names the mounted regular-file `ca.crt` and is no larger than 1 MiB; keep JWKS on the issuer origin or explicitly add its HTTPS origin to `server.oidc.jwksAllowedOrigins`. The issuer CA augments platform roots for OIDC only. Numeric-loopback HTTP is development-only and also requires `server.oidc.dangerouslyAllowInsecureHttp=true` |
914914
| Gateway fails before serving health after enabling an interceptor | Interceptor endpoint unavailable or manifest/binding validation failed | Gateway and interceptor logs; interceptor socket; `binding_policy`, phases, and failure policy |
915915
| Authenticated interceptor or middleware rejects gateway calls | Private CA or hostname mismatch, expected audience or issuer mismatch, stale/unknown `kid`, or malformed extension token | `tls_ca_cert_path`, registration `audience`, service verifier config and logs; fetch well-known metadata only through the already-trusted gateway TLS endpoint |
916916
| Provider profiles disappear after enabling an interceptor catalog | `provider_profile_sources` selected only an authoritative interceptor or returned invalid/duplicate IDs | Inspect source list and interceptor `Describe`/catalog logs; include `user` when composition with imported profiles is intended |

0 commit comments

Comments
 (0)