diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d9018dd72..ce4206f8b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -305,6 +305,14 @@ jobs: KRISHIV_TEST_UDF_PYTHON: ${{ runner.temp }}/udf-python/bin/python KRISHIV_UDF_POOL_REQUIRED: "1" - run: cargo test --workspace --doc --locked + # The feature-gated runs below rebuild their crates' test executables beside the workspace's, which + # have already run: drop those first, or the krishiv-server link runs out of disk (ld Bus error, #769). + # Libraries (`.rlib`, `.rmeta`, `.so`) stay, so nothing is compiled again. + - name: Drop the workspace's test executables + run: | + df -h / + find target/debug/deps -maxdepth 1 -type f -perm -u+x ! -name '*.so' ! -name '*.d' -delete + df -h / - run: cargo nextest run -p krishiv-session --features telemetry-otlp --test otlp_collector --locked # Server TLS (`tls`) and OIDC (`oidc`), both off by default: the in-memory rustls handshakes, # and the JWT verifier against keys generated per run. diff --git a/Cargo.lock b/Cargo.lock index 67ad3cc06..95f59dfb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2362,6 +2362,7 @@ dependencies = [ "libmimalloc-sys", "mimalloc", "reedline", + "rustls", "serde_json", "tempfile", "tokio", @@ -2403,6 +2404,7 @@ dependencies = [ "bytes", "futures", "getrandom 0.4.3", + "hyper-util", "krishiv-common", "krishiv-connect", "krishiv-memory", @@ -2416,8 +2418,10 @@ dependencies = [ "sha2", "tempfile", "tokio", + "tokio-rustls", "tonic", "tonic-prost", + "tower", "tracing", "zstd", ] @@ -2750,6 +2754,7 @@ dependencies = [ "serde_json", "tempfile", "tokio", + "tokio-rustls", "tracing", ] diff --git a/Cargo.toml b/Cargo.toml index ae526fa57..8842f75b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -179,10 +179,14 @@ rdkafka = { version = "0.39", default-features = false } wasmtime = { version = "49", default-features = false } axum = { version = "0.8.9", default-features = false } tower = { version = "0.5.3", default-features = false } +# Test clients only: `TokioIo` for a tonic channel over a TLS stream. +hyper-util = { version = "0.1.21", default-features = false } rust-embed = { version = "8.13", default-features = false } utoipa = { version = "6.0", default-features = false } rmcp = { version = "3.5.1", default-features = false } rustls = { version = "0.23", default-features = false } +# Server TLS on the listeners (P3-T002 design §3.5): the async acceptor over `rustls`. +tokio-rustls = { version = "0.26.6", default-features = false } # OIDC JWT signatures (krishiv-session `oidc`); already in the tree as rustls's crypto provider. aws-lc-rs = { version = "1.18.2", default-features = false, features = ["aws-lc-sys"] } tracing = { version = "0.1.44", default-features = false } diff --git a/changelog.d/769.added.md b/changelog.d/769.added.md new file mode 100644 index 000000000..e1f533300 --- /dev/null +++ b/changelog.d/769.added.md @@ -0,0 +1 @@ +`krishiv server` authenticates every Spark Connect RPC once authentication is configured (static tokens, client certificates or OIDC): each session belongs to its caller, Connect requests follow the viewer/operator/admin role matrix, both ports serve TLS, and refusals are counted on `/metrics` (`krishiv_auth_failed_total`, `krishiv_auth_denied_total`). diff --git a/crates/krishiv-cli/CLAUDE.md b/crates/krishiv-cli/CLAUDE.md index b7479dcf7..a741964e8 100644 --- a/crates/krishiv-cli/CLAUDE.md +++ b/crates/krishiv-cli/CLAUDE.md @@ -10,7 +10,7 @@ The `krishiv` binary: sql, server, coordinator, worker, mcp, ui, doctor, migrate Paths are relative to this crate. - `src/main.rs` subcommand dispatch and the allocator/memory hook set-up - `src/sql.rs` the `krishiv sql` subcommand: input (`-e`, `-f`, stdin), output (`-o`), `--conf`, `--table`, `--timing`, `--agent`, `--max-rows`, `--max-seconds` -- `src/server.rs` `krishiv server` (feature `server`; `ui` implies it; design P3-T001): host checks before binding (non-loopback refused until P3-T002, root refused without `--allow-root`, core dumps off unless `krishiv.server.allowCoreDumps`), one tokio runtime for the Spark Connect listener (`krishiv-connect-server`, `--connect-port`, default 15002) and the admin listener (`--port` 8080: `/healthz`, `/readyz`, `/metrics` and the UI with `ui`; with `krishiv.auth.*` tokens configured they are guarded by `SessionGuard` over one `ServerAuth`, else by a random token printed once; `krishiv.auth.*` settings never reach the engine configuration), SIGTERM/SIGINT drain then exit 0 after the engine runtime is dropped, a second signal exits 130; `tests/server.rs` +- `src/server.rs` `krishiv server` (feature `server`; `ui` implies it; design P3-T001): host checks before binding (`check_listener`: non-loopback needs `krishiv.auth.*`, bearer methods there need TLS, client certificates need TLS; root refused without `--allow-root`, core dumps off unless `krishiv.server.allowCoreDumps`), one tokio runtime for the Spark Connect listener (`krishiv-connect-server`, `--connect-port`, default 15002) and the admin listener (`--port` 8080: `/healthz`, `/readyz`, `/metrics` and the UI with `ui`; with `krishiv.auth.*` tokens configured they are guarded by `SessionGuard` over one `ServerAuth`, else by a random token printed once; `krishiv.auth.*` and `krishiv.server.tls.*` settings never reach the engine configuration; with `krishiv.server.tls.certFile`/`keyFile` both listeners speak TLS through one `ServerTls`, reloaded on SIGHUP (handler installed before the addresses print), and the printed addresses are `sc://...;use_ssl=true` / `https://`). With authentication on, the same `ServerAuth` authenticates every Connect RPC (`ConnectAuth`) and each caller's session is `Session::new_session_as(principal, PathPrefixPolicy(RolePolicy(role)))` (`krishiv_connect_server::role_sessions`; client code admin-only until T014b, ADR 0046 N1; the path confinement is checked at startup; a caller with no role is refused before any session), SIGTERM/SIGINT drain then exit 0 after the engine runtime is dropped, a second signal exits 130; `tests/server.rs` - `src/check_compat/` (P3-T029) `krishiv check-compat `: `mod.rs` args, file walk, exit codes (0 none, 1 findings, 2 read errors), JSON/text report; `scan.rs` SQL / Python / notebook-cell scanners; `alternatives.json` workarounds for not-yet functions (names only supported functions; a unit test checks), `functions.json` the function matrix (generated from the SQL reference pages by `tests/scripts/gen_compat_matrix.py`); corpus `tests/check_compat_corpus/` (`handwritten/` with a hand-checked `expected.json`, `supported_only/` must give no findings, `generated/` from `tests/scripts/gen_check_compat_corpus.py`); JSON report is `{schema_version, files_scanned, findings, errors}` - `src/repl.rs` the interactive loop (no `-e`/`-f` on a terminal): `.timer`, `.format`, `.complete`, `.history`, history persistence - `src/complete.rs` completion sources (keywords, function registry, session catalog, config registry) diff --git a/crates/krishiv-cli/Cargo.toml b/crates/krishiv-cli/Cargo.toml index 580d691e4..20092fc83 100644 --- a/crates/krishiv-cli/Cargo.toml +++ b/crates/krishiv-cli/Cargo.toml @@ -36,6 +36,7 @@ serde_json = { workspace = true } mimalloc = { workspace = true, optional = true } [dev-dependencies] +rustls = { workspace = true, features = ["aws_lc_rs", "std", "tls12"] } tempfile.workspace = true libmimalloc-sys = { workspace = true, features = ["extended"] } @@ -62,7 +63,9 @@ server = [ "dep:krishiv-server", "krishiv-server/session", "krishiv-server/metrics-prometheus", + "krishiv-server/tls", "dep:krishiv-session", + "krishiv-session/tls", "dep:krishiv-connect-server", "dep:axum", "dep:tokio", diff --git a/crates/krishiv-cli/src/server.rs b/crates/krishiv-cli/src/server.rs index 76de5eb55..04484a2c7 100644 --- a/crates/krishiv-cli/src/server.rs +++ b/crates/krishiv-cli/src/server.rs @@ -6,10 +6,14 @@ //! `krishiv.auth.tokens.*` configured the admin routes take those principals //! and the ADR 0046 roles (P3-T002); without, a random token printed once. //! -//! Before anything binds: a non-loopback address is refused (no -//! authentication exists until P3-T002), uid 0 is refused without -//! `--allow-root`, and core dumps are turned off unless -//! `krishiv.server.allowCoreDumps`. On SIGTERM or SIGINT the server drains +//! Before anything binds: the address, authentication and TLS must fit +//! together (`check_listener`, design P3-T002 §3.5: a non-loopback address +//! needs an authentication method, a bearer method there needs TLS, client +//! certificates need TLS), uid 0 is refused without `--allow-root`, and +//! core dumps are turned off unless `krishiv.server.allowCoreDumps`. With +//! `krishiv.server.tls.certFile`/`keyFile` both listeners speak TLS; on +//! SIGHUP (Unix) the certificate and key are read again, and a failed read +//! keeps the old ones serving. On SIGTERM or SIGINT the server drains //! (`/readyz` 503, new RPCs `UNAVAILABLE`), gives running queries //! `krishiv.server.shutdownTimeout`, closes every session, and returns; the //! engine runtime is dropped before `main` exits, which removes the spill @@ -25,11 +29,14 @@ use krishiv_common::config::{ SERVER_METRICS_PUBLIC, UI_ENABLED, }; use krishiv_connect_server::{ - ConnectServer, ConnectServerConfig, SessionLimits, SessionManager, host, + ConnectAuth, ConnectServer, ConnectServerConfig, SessionLimits, SessionManager, host, + role_sessions, }; use krishiv_server::guard::{Guard, TokenGuard}; use krishiv_server::session_guard::SessionGuard; -use krishiv_session::auth::{AuthConfig, ServerAuth, is_auth_key}; +use krishiv_session::auth::{ + AuthConfig, ServerAuth, ServerTls, TlsConfig, check_listener, is_auth_key, +}; use krishiv_session::{EngineRuntime, Session}; use tokio::net::TcpListener; use tokio::sync::watch; @@ -44,7 +51,8 @@ pub(crate) struct ServerArgs { #[arg(long = "connect-port")] connect_port: Option, /// Address both listeners bind; default `krishiv.connect.address`, loopback. A - /// non-loopback address needs authentication, which this build cannot configure yet. + /// non-loopback address needs authentication (`krishiv.auth.*`), and TLS + /// (`krishiv.server.tls.*`) when that is a bearer token or OIDC. #[arg(long)] host: Option, /// Run even as uid 0. @@ -70,18 +78,18 @@ pub(crate) fn run(args: &ServerArgs) -> i32 { fn serve(args: &ServerArgs) -> Result<(), String> { let mut config = Config::new(); let mut builder = EngineRuntime::builder(); - // Authentication settings (secret references included) are read once here and never become - // engine configuration: a session could otherwise list them with `SET`. - let auth = AuthConfig::from_settings( + // Authentication and TLS settings (secret references and key files included) are read once + // here and never become engine configuration: a session could otherwise list them with `SET`. + let listener_settings = || { args.conf .iter() .map(|(k, v)| (k.as_str(), v.as_str())) - .filter(|(k, _)| is_auth_key(k)), - ) - .and_then(|c| ServerAuth::new(&c)) - .map_err(|e| e.to_string())?; + .filter(|(k, _)| is_listener_key(k)) + }; + let auth_config = AuthConfig::from_settings(listener_settings()).map_err(|e| e.to_string())?; + let tls_config = TlsConfig::from_settings(listener_settings()).map_err(|e| e.to_string())?; for (k, v) in &args.conf { - if is_auth_key(k) { + if is_listener_key(k) { continue; } let warning = config.set(k, v).map_err(|e| e.to_string())?; @@ -106,14 +114,16 @@ fn serve(args: &ServerArgs) -> Result<(), String> { .parse() .map_err(|_| "krishiv.connect.address is not an IP address".to_owned())?, }; - // ADR 0046 §6: before either listener binds. - if host::needs_auth(host) { - return Err(format!( - "refusing to serve on {host} without authentication: a non-loopback address needs \ - authentication configured (P3-T002, not in this build). Bind a loopback address \ - (--host 127.0.0.1) and put an authenticated tunnel or proxy in front of it." - )); - } + // ADR 0046 §6, design §3.5: before either listener binds (both bind `host`). + check_listener(SocketAddr::new(host, 0), &auth_config, &tls_config).map_err(|e| { + format!( + "refusing to serve on {host}: {e}. Bind a loopback address (--host 127.0.0.1), or \ + configure authentication (krishiv.auth.*) and, for tokens or OIDC, TLS \ + (krishiv.server.tls.certFile and keyFile)." + ) + })?; + let tls = ServerTls::new(&tls_config, &auth_config.mtls).map_err(|e| e.to_string())?; + let auth = ServerAuth::new(&auth_config).map_err(|e| e.to_string())?; if host::is_root() && !args.allow_root { return Err("refusing to run as root; pass --allow-root to override".to_owned()); } @@ -140,6 +150,7 @@ fn serve(args: &ServerArgs) -> Result<(), String> { runtime, &config, auth, + tls, SocketAddr::new(host, connect_port), SocketAddr::new(host, args.port), ) @@ -151,6 +162,12 @@ fn serve(args: &ServerArgs) -> Result<(), String> { result } +/// A setting `krishiv server` reads itself: authentication (`krishiv.auth.*`, Spark's token key) +/// and the listeners' TLS (`krishiv.server.tls.*`). +fn is_listener_key(key: &str) -> bool { + is_auth_key(key) || key.starts_with("krishiv.server.tls.") +} + fn flag(config: &Config, entry: &krishiv_common::config::ConfigEntry, default: bool) -> bool { config .get(entry) @@ -160,13 +177,13 @@ fn flag(config: &Config, entry: &krishiv_common::config::ConfigEntry, default: b #[expect( clippy::print_stdout, - clippy::print_stderr, - reason = "CLI binary: the addresses, token and warnings are its output" + reason = "CLI binary: the addresses and the token are its output" )] async fn run_listeners( runtime: Arc, config: &Config, auth: ServerAuth, + tls: Option, connect_addr: SocketAddr, admin_addr: SocketAddr, ) -> Result<(), String> { @@ -174,10 +191,19 @@ async fn run_listeners( .runtime(Arc::clone(&runtime)) .build() .map_err(|e| e.to_string())?; - let manager = Arc::new(SessionManager::from_base( - base, - SessionLimits::from_config(config), - )); + let authenticated = auth.is_enabled(); + let auth = Arc::new(auth); + // One denial counter for the guards and every caller's session (ADR 0046 §8). + let denials = auth.denials(); + let limits = SessionLimits::from_config(config); + let manager = Arc::new(if authenticated { + SessionManager::new( + role_sessions(base, config, Arc::clone(&denials)).map_err(|e| e.to_string())?, + limits, + ) + } else { + SessionManager::from_base(base, limits) + }); let connect_listener = TcpListener::bind(connect_addr) .await .map_err(|e| format!("cannot bind the Spark Connect port {connect_addr}: {e}"))?; @@ -189,10 +215,9 @@ async fn run_listeners( // With authentication configured the admin routes take the same principals as Connect; // without it, a random token printed once (loopback only). - let authenticated = auth.is_enabled(); let token = krishiv_server::internal::random_token().map_err(|e| e.to_string())?; let guard: Arc = if authenticated { - Arc::new(SessionGuard::new(Arc::new(auth))) + Arc::new(SessionGuard::new(Arc::clone(&auth) as Arc<_>).with_denials(Arc::clone(&denials))) } else { Arc::new(TokenGuard::new(&token)) }; @@ -201,14 +226,20 @@ async fn run_listeners( &runtime, config, guard, - authenticated, + authenticated.then(|| Arc::clone(&auth)), Arc::clone(&ready), admin_bound, )?; - let connect = ConnectServer::new( + let mut connect = ConnectServer::new( Arc::clone(&manager), ConnectServerConfig::from_config(config), ); + if authenticated { + connect = connect.with_auth(ConnectAuth::new(auth).with_denials(denials)); + } + if let Some(tls) = &tls { + connect = connect.with_tls(&tls.server_config()); + } let (stop_tx, stop_rx) = watch::channel(false); // Installed before the addresses are printed: a SIGTERM right after @@ -219,25 +250,49 @@ async fn run_listeners( let stopped = |mut rx: watch::Receiver| async move { let _ = rx.wait_for(|stop| *stop).await; }; - let admin = tokio::spawn(krishiv_server::admin::serve_capped( - admin_listener, - app, - krishiv_server::admin::HttpLimits::default().max_connections, - stopped(stop_rx.clone()), - )); - println!("krishiv server: Spark Connect on sc://{connect_bound}"); - println!("krishiv server: admin on http://{admin_bound}"); + let max_admin = krishiv_server::admin::HttpLimits::default().max_connections; + let admin = match &tls { + Some(tls) => { + let config = tls.server_config(); + let shutdown = stopped(stop_rx.clone()); + tokio::spawn(async move { + krishiv_server::tls::serve_capped_tls( + admin_listener, + app, + max_admin, + &config, + shutdown, + ) + .await + }) + } + None => tokio::spawn(krishiv_server::admin::serve_capped( + admin_listener, + app, + max_admin, + stopped(stop_rx.clone()), + )), + }; + // The SIGHUP handler is installed here, before the addresses are printed: a SIGHUP after + // readiness must reload, not end the process (SIGHUP's default action). + let reloads = tls + .clone() + .map(|tls| tokio::spawn(reload_on_hangup(tls, Hangups::install()))); + let (sc_suffix, http) = if tls.is_some() { + ("/;use_ssl=true", "https") + } else { + ("", "http") + }; + println!("krishiv server: Spark Connect on sc://{connect_bound}{sc_suffix}"); + println!("krishiv server: admin on {http}://{admin_bound}"); let ui = flag(config, &UI_ENABLED, true) && cfg!(feature = "ui"); if authenticated { - println!("krishiv server: admin routes need a configured token (krishiv.auth.tokens)"); - // The Connect listener is authenticated in a later unit; until then it serves any - // local client, which the loopback-only rule above keeps to this machine. - eprintln!( - "krishiv server: warning: Spark Connect does not check tokens yet; it accepts any \ - client on {connect_bound}" + println!( + "krishiv server: Spark Connect and the admin routes need a configured credential \ + (krishiv.auth.*)" ); } else if ui { - println!("open http://{admin_bound}/?token={token}"); + println!("open {http}://{admin_bound}/?token={token}"); } else if metrics_need_token(config, admin_bound) { // `/metrics` is behind the admin token: print it once. println!("krishiv server: admin token {token}"); @@ -249,6 +304,9 @@ async fn run_listeners( .map_err(|e| e.to_string()); ready.store(false, Ordering::Release); signals.abort(); + if let Some(reloads) = reloads { + reloads.abort(); + } // The admin listener stopped accepting with the same signal; requests in // flight get `ADMIN_DRAIN` to finish. Open event streams never end on // their own, so whatever is left then is aborted. @@ -266,10 +324,11 @@ fn admin_router( runtime: &Arc, config: &Config, guard: Arc, - authenticated: bool, + auth: Option>, ready: Arc, bound: SocketAddr, ) -> Result { + let authenticated = auth.is_some(); let metrics_on = flag(config, &METRICS_ENABLED, false); // With authentication configured every scrape authenticates (ADR 0046 §3c: a viewer // service principal); `krishiv.server.metrics.public` opens it only without. @@ -277,7 +336,14 @@ fn admin_router( let snapshot_runtime = Arc::clone(runtime); let metrics = krishiv_server::metrics::metrics_route_guarded( metrics_on, - Arc::new(move || snapshot_runtime.metrics()), + // With authentication on, the auth counters ride along (ADR 0046 §8). + Arc::new(move || { + let mut snapshot = snapshot_runtime.metrics(); + if let Some(auth) = &auth { + snapshot.merge(&auth.metrics()); + } + snapshot + }), (!public).then(|| Arc::clone(&guard)), ); let base = if cfg!(feature = "ui") && flag(config, &UI_ENABLED, true) { @@ -306,6 +372,61 @@ fn metrics_need_token(config: &Config, bound: SocketAddr) -> bool { /// How long admin requests in flight may take once the server stops. const ADMIN_DRAIN: Duration = Duration::from_secs(2); +/// SIGHUP, registered when installed (tokio registers a handler only when its stream is +/// created). `None` elsewhere than Unix, or when it cannot be installed. +struct Hangups { + #[cfg(unix)] + signal: Option, +} + +impl Hangups { + #[expect(clippy::print_stderr, reason = "CLI binary: stderr is its interface")] + fn install() -> Self { + #[cfg(unix)] + { + use tokio::signal::unix::{SignalKind, signal}; + let signal = signal(SignalKind::hangup()).ok(); + if signal.is_none() { + eprintln!("krishiv server: cannot install the SIGHUP handler; TLS reload is off"); + } + Self { signal } + } + #[cfg(not(unix))] + { + Self {} + } + } +} + +/// Reads the TLS certificate and key again on every SIGHUP (Unix; elsewhere it waits forever). +/// New handshakes get the new material; a failed read is logged and the old material serves on. +#[expect(clippy::print_stderr, reason = "CLI binary: stderr is its interface")] +async fn reload_on_hangup(tls: ServerTls, hangups: Hangups) { + #[cfg(unix)] + { + let Some(mut hangups) = hangups.signal else { + return; + }; + while hangups.recv().await.is_some() { + let tls = tls.clone(); + match tokio::task::spawn_blocking(move || tls.reload()).await { + Ok(Ok(())) => eprintln!("krishiv server: TLS certificate reloaded"), + Ok(Err(e)) => { + eprintln!( + "krishiv server: TLS reload failed, the old certificate serves on: {e}" + ); + } + Err(e) => eprintln!("krishiv server: TLS reload did not finish: {e}"), + } + } + } + #[cfg(not(unix))] + { + let _ = (tls, hangups); + std::future::pending::<()>().await; + } +} + /// The first SIGTERM or SIGINT starts the drain; a second exits at once. #[expect(clippy::print_stderr, reason = "CLI binary: stderr is its interface")] async fn watch_signals(mut signals: Signals, ready: Arc, stop: watch::Sender) { diff --git a/crates/krishiv-cli/tests/server.rs b/crates/krishiv-cli/tests/server.rs index 15d288848..11e5ce057 100644 --- a/crates/krishiv-cli/tests/server.rs +++ b/crates/krishiv-cli/tests/server.rs @@ -70,7 +70,7 @@ fn non_loopback_requires_auth() { .expect("run krishiv server"); assert_eq!(out.status.code(), Some(1), "{host}"); let err = String::from_utf8_lossy(&out.stderr); - assert!(err.contains("without authentication"), "{err}"); + assert!(err.contains("needs an authentication method"), "{err}"); assert!(err.contains("--host"), "{err}"); assert!(!String::from_utf8_lossy(&out.stdout).contains("Spark Connect on")); } @@ -101,7 +101,7 @@ fn non_loopback_requires_auth() { .output() .expect("run krishiv server"); assert_eq!(out.status.code(), Some(1)); - assert!(String::from_utf8_lossy(&out.stderr).contains("without authentication")); + assert!(String::from_utf8_lossy(&out.stderr).contains("needs an authentication method")); } /// A running server and the addresses it printed; killed and reaped on @@ -151,11 +151,14 @@ fn start_with(spill: &std::path::Path, conf: &[String]) -> Running { .next() .expect("server printed its addresses") .expect("utf-8"); - if let Some(a) = line.strip_prefix("krishiv server: admin on http://") { + if let Some(a) = line + .strip_prefix("krishiv server: admin on http://") + .or_else(|| line.strip_prefix("krishiv server: admin on https://")) + { admin = Some(a.to_owned()); } if let Some(a) = line.strip_prefix("krishiv server: Spark Connect on sc://") { - connect = Some(a.to_owned()); + connect = Some(a.trim_end_matches("/;use_ssl=true").to_owned()); } } Running { @@ -418,4 +421,174 @@ fn configured_tokens_guard_the_admin_routes() { assert!(ui.starts_with(expected), "{ui}"); let probe = http_get(&server.admin, "/healthz"); assert!(probe.starts_with("HTTP/1.1 200"), "{probe}"); + // ADR 0046 §8: the refusals above are on `/metrics` (the counters ride on the engine's). + let scrape = http_get_as(&server.admin, "/metrics", Some(token)); + assert!( + scrape.contains(r#"krishiv_auth_failed_total{surface="metrics",method="none"} 1"#), + "{scrape}" + ); + if cfg!(feature = "ui") { + assert!( + scrape.contains(r#"krishiv_auth_denied_total{surface="ui",privilege="MONITOR"} 1"#), + "{scrape}" + ); + } +} + +#[cfg(unix)] +#[test] +fn tokens_on_a_public_address_need_tls() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("t"); + std::fs::write(&path, "public-address-token-0123456789abcdef").expect("write secret"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + let out = krishiv() + .args([ + "server", + "--port", + "0", + "--connect-port", + "0", + "--host", + "0.0.0.0", + ]) + .arg("--conf") + .arg(format!( + "krishiv.auth.tokens.a.secret=file:{}", + path.display() + )) + .args(["--conf", "krishiv.auth.tokens.a.principal=a"]) + .args(["--conf", "krishiv.auth.tokens.a.role=viewer"]) + .output() + .expect("run krishiv server"); + assert_eq!(out.status.code(), Some(1)); + let err = String::from_utf8_lossy(&out.stderr); + assert!(err.contains("need TLS"), "{err}"); + assert!(!String::from_utf8_lossy(&out.stdout).contains("Spark Connect on")); +} + +#[cfg(unix)] +/// The TLS fixtures of `krishiv-session` (`tests/fixtures/tls/README.md`). +fn tls_fixture(name: &str) -> std::path::PathBuf { + std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../krishiv-session/tests/fixtures/tls") + .join(name) +} + +/// A TLS connection to `addr`, trusting the test CA, with its handshake done. +#[cfg(unix)] +fn tls_connect(addr: &str) -> rustls::StreamOwned { + use rustls::pki_types::pem::PemObject as _; + use rustls::pki_types::{CertificateDer, ServerName}; + let mut roots = rustls::RootCertStore::empty(); + roots + .add(CertificateDer::from_pem_file(tls_fixture("ca.pem")).expect("ca")) + .expect("add ca"); + let config = rustls::ClientConfig::builder_with_provider(std::sync::Arc::new( + rustls::crypto::aws_lc_rs::default_provider(), + )) + .with_safe_default_protocol_versions() + .expect("versions") + .with_root_certificates(roots) + .with_no_client_auth(); + let name = ServerName::try_from("localhost").expect("name"); + let mut conn = + rustls::ClientConnection::new(std::sync::Arc::new(config), name).expect("client"); + let mut tcp = TcpStream::connect(addr).expect("connect"); + tcp.set_read_timeout(Some(Duration::from_secs(10))) + .expect("timeout"); + while conn.is_handshaking() { + conn.complete_io(&mut tcp).expect("handshake"); + } + rustls::StreamOwned::new(conn, tcp) +} + +/// The leaf certificate the server presented on `tls`, and the response to `GET /healthz` over +/// that connection. +#[cfg(unix)] +fn tls_healthz_on( + mut tls: rustls::StreamOwned, +) -> (Vec, String) { + tls.write_all(b"GET /healthz HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n") + .expect("write"); + let mut out = Vec::new(); + // The server may close without `close_notify`; keep what came. + let _ = tls.read_to_end(&mut out); + let leaf = tls + .conn + .peer_certificates() + .and_then(<[_]>::first) + .expect("server certificate") + .to_vec(); + (leaf, String::from_utf8_lossy(&out).into_owned()) +} + +/// [`tls_healthz_on`] over a new connection to `addr`. +#[cfg(unix)] +fn tls_healthz(addr: &str) -> (Vec, String) { + tls_healthz_on(tls_connect(addr)) +} + +#[cfg(unix)] +#[test] +fn tls_serves_the_admin_listener_and_reloads_on_sighup() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().expect("tempdir"); + let (cert, key) = (dir.path().join("server.pem"), dir.path().join("server.key")); + std::fs::copy(tls_fixture("server.pem"), &cert).expect("cert"); + std::fs::copy(tls_fixture("server.key"), &key).expect("key"); + std::fs::set_permissions(&key, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + let conf = [ + format!("krishiv.server.tls.certFile={}", cert.display()), + format!("krishiv.server.tls.keyFile={}", key.display()), + ]; + let mut server = start_with(dir.path(), &conf); + let (leaf, answer) = tls_healthz(&server.admin); + assert!(answer.starts_with("HTTP/1.1 200"), "{answer}"); + let first = std::fs::read(tls_fixture("server.der")).expect("der"); + assert_eq!(leaf, first); + // Plain HTTP on the TLS port gets no HTTP answer. + let mut plain = TcpStream::connect(&server.admin).expect("connect"); + plain + .set_read_timeout(Some(Duration::from_secs(10))) + .expect("timeout"); + let _ = plain.write_all(b"GET /healthz HTTP/1.1\r\nHost: x\r\n\r\n"); + let mut out = Vec::new(); + let _ = plain.read_to_end(&mut out); + assert!( + !out.starts_with(b"HTTP/"), + "{:?}", + String::from_utf8_lossy(&out) + ); + // SIGHUP: the second certificate is served to new connections; one opened before keeps + // its handshake and is served. + let before = tls_connect(&server.admin); + std::fs::copy(tls_fixture("server2.pem"), &cert).expect("cert 2"); + std::fs::copy(tls_fixture("server2.key"), &key).expect("key 2"); + // `copy` gives the file the fixture's mode; a key readable by others is refused. + std::fs::set_permissions(&key, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + let second = std::fs::read(tls_fixture("server2.der")).expect("der 2"); + let hup = Command::new("kill") + .args(["-HUP", &server.child.id().to_string()]) + .status() + .expect("kill -HUP"); + assert!(hup.success()); + let start = Instant::now(); + loop { + let (leaf, answer) = tls_healthz(&server.admin); + assert!(answer.starts_with("HTTP/1.1 200"), "{answer}"); + if leaf == second { + break; + } + assert!( + start.elapsed() < Duration::from_secs(10), + "no reload after SIGHUP" + ); + std::thread::sleep(Duration::from_millis(50)); + } + let (leaf, answer) = tls_healthz_on(before); + assert!(answer.starts_with("HTTP/1.1 200"), "{answer}"); + assert_eq!(leaf, first, "the old connection keeps its certificate"); + let _ = server.child.kill(); } diff --git a/crates/krishiv-common/src/config/registry/server.rs b/crates/krishiv-common/src/config/registry/server.rs index b4df373ba..f5b489c03 100644 --- a/crates/krishiv-common/src/config/registry/server.rs +++ b/crates/krishiv-common/src/config/registry/server.rs @@ -251,7 +251,9 @@ pub static CONNECT_HANDSHAKE_TIMEOUT: ConfigEntry = startup( ValueType::Duration, ConfigValue::Duration(Duration::from_secs(10)), "A new Spark Connect connection that sends no first byte for this long is closed, so silent \ - sockets cannot hold `krishiv.connect.maxConnections` for longer. `0s` means no deadline.", + sockets cannot hold `krishiv.connect.maxConnections` for longer; with TLS the whole handshake \ + must also finish within it. `0s` means no deadline for either, so a client that stalls \ + mid-handshake holds its connection slot until it disconnects.", None, ); diff --git a/crates/krishiv-connect-server/CLAUDE.md b/crates/krishiv-connect-server/CLAUDE.md index 83efc0420..136c0a52a 100644 --- a/crates/krishiv-connect-server/CLAUDE.md +++ b/crates/krishiv-connect-server/CLAUDE.md @@ -4,8 +4,8 @@ **Real code since:** P3-T001 (design `../krishiv-internal/design/P3-T001-server.md`, approved 2026-11-12). ## Purpose -The Spark Connect gRPC server: the tonic service, the session manager keyed by (authenticated principal, session -id), the limits applied before admission, and graceful shutdown. Decoding and translation stay in `krishiv-connect`; +The Spark Connect gRPC server: the tonic service, the authentication of every RPC, the session manager keyed by +(authenticated caller, session id), the limits applied before admission, and graceful shutdown. Decoding and translation stay in `krishiv-connect`; the admin HTTP server (`/healthz`, `/readyz`, `/metrics`, UI) is `krishiv-server`; the process around both is `krishiv server` in `krishiv-cli`, which runs Krishiv single-node (deployment modes: embedded, single-node, distributed; clients connect over Spark Connect). @@ -25,10 +25,21 @@ Paths are relative to this crate. `parseDatatypeString` - `src/config.rs` `Config` (design §3.6) over `RuntimeConf`: the seven operations, `silent`, Spark's per-key warnings, redaction for both profiles (Spark's pattern plus `krishiv_common::secret`'s classifier and URL - credentials), `SettableBy` (`check_settable`; `role_of` makes every principal admin until T002) + credentials), who may `set`/`unset` a key (a `Startup` key is `CANNOT_MODIFY_STATIC_CONFIG` for everyone, `check_static`; any other key asks the session's policy as SQL `SET` does, `Session::authorize_set_key`: admin keys and server-scoped store keys need `Manage`, refusals audited and counted; no listener-local role check, ADR 0046) - `src/interrupt.rs` `Interrupt` (design §3.7): `ALL` and `OPERATION_ID` through the session's operations and `Session::cancel` per reported id; an interrupted operation stays registered (Spark keeps the `ExecuteHolder`), so a reattach gets `OPERATION_CANCELED`; `TAG` is `UNSUPPORTED_FEATURE.INTERRUPT_TYPE` until T064 +- `src/auth.rs` (P3-T002, design `design/P3-T002-authentication.md` §3.1/§3.6, ADR 0046 §2/§3b/§5) `ConnectAuth`; + `src/auth/layer.rs` `AuthLayer`, the tower layer in front of the router: every RPC (the unimplemented ones too) + authenticates from its headers before the body is decoded, through the server's one `Authenticator` + (`ServerAuth`, shared with the admin listener): the `authorization` header (two entries or a non-`Bearer` scheme + are malformed) and the peer address; failures `UNAUTHENTICATED` with Spark's messages, throttled or unavailable + `UNAVAILABLE`, no peer address refused, a caller with no role `PERMISSION_DENIED` (42501, audited) before any + session; the result rides in the request extensions (`ConnectAuth::caller` fails closed without it); auth off = + the local user. `role_sessions` is the production session factory (`PathPrefixPolicy(RolePolicy(role))`; each session counts its denials into the server's `DeniedCounts` under `connect`, as does the no-role refusal with `ConnectAuth::with_denials`). `SessionOwner` (method, kind, tenant, name, + no groups) is the session key. `check_rpc` / `check_command`: the §3b class of every RPC and `Command` arm, + no wildcard arm (a new proto arm fails to compile); `enforce` asks the session's policy for a + `Check::Privilege` (`Session::authorize_privilege`, one `krishiv::audit` record per denial); `tests/auth.rs` - `src/operation.rs` `Operation` (one per `ExecutePlan`: id, plan digest, tags, reattachable, its `Replay`, one atomic state: running, then finished or cancelled by compare-and-swap; `close` = Spark's `ExecuteHolder.close`) and `Operations`, the per-session map in `SessionEntry` (`krishiv.connect.maxOperationsPerSession`, @@ -54,22 +65,34 @@ Paths are relative to this crate. `src/execute/schema.rs` the `schema` response (`DataTypeProtoConverter.toConnectProtoType`) - `src/codec.rs` `WireCodec`: requests decode only through `wire::decode_request` (bounded size, count, depth, fields) - `src/status.rs` `status_of`: a `KrishivError` as Spark's gRPC status (`INTERNAL`, `google.rpc.ErrorInfo` with - `classes`, `sqlState`, `errorClass`, `messageParameters`; `ErrorUtils.buildStatusFromThrowable`, v4.3.0); + `classes`, `sqlState`, `errorClass`, `messageParameters`; `ErrorUtils.buildStatusFromThrowable`, v4.3.0; + `KRISHIV_INSUFFICIENT_PERMISSIONS` is `PERMISSION_DENIED`, ADR 0046 §8); `exception_classes` picks the Java class list PySpark's `convert_exception` reads (Spark's own for the Connect classes, else the Python facade's class/SQLSTATE rules); `KRISHIV_SESSION_LIMIT` and `KRISHIV_OPERATION_LIMIT` are `RESOURCE_EXHAUSTED` -- `src/manager.rs` `SessionManager` (Spark's `SparkConnectSessionManager`): get-or-create / lookup / release, tombstones - (LRU, no expiry), abandoned-operation tombstones and `abandon_detached` (Spark's execution maintenance), `SESSION_CHANGED` on a changed server-side id, the idle sweep (skips a session running a query or +- `src/manager.rs` `SessionManager` (Spark's `SparkConnectSessionManager`; its factory gets the `Authenticated` + caller; a new session records transport `connect`, the client's `user_id` (unverified, ADR 0046 §5) and its + server-side id for `system.sessions`): get-or-create / lookup / release, tombstones + (LRU, no expiry), abandoned-operation tombstones and `abandon_detached` (Spark's execution maintenance), + `SESSION_CHANGED` on a changed server-side id, a `krishiv::audit` record (not counted) for a + session or operation id another owner holds (ADR 0046 §3b; `audit_foreign_operation`, `tests/foreign_handles_audit.rs`), the idle sweep (skips a session running a query or holding a live operation; `busy()` is what shutdown waits for), `krishiv.connect.maxSessions` / `maxSessionsPerPrincipal` -- `src/server.rs` `ConnectServer::serve`: HTTP/2 limits, connection cap, sweeper and abandon tasks, shutdown order +- `src/server.rs` `ConnectServer::serve`: HTTP/2 limits, connection cap, sweeper and abandon tasks, shutdown order; `with_tls` (P3-T002 §3.5): the TLS handshake (ALPN `h2`) on the connection's own task within `handshakeTimeout`, `PeerInfo` (address and verified client leaf) as the connection's `ConnectInfo`, which `AuthLayer` passes to the authenticator; `tests/tls.rs` - `tests/server.rs`, `tests/execute.rs`, `tests/rpcs.rs` (AnalyzePlan, Config, Interrupt), `tests/reattach.rs` - (replay, reattach, caps, detach, races) end to end over a raw tonic client + (replay, reattach, caps, detach, races), `tests/auth.rs`, `tests/tls.rs` end to end over a raw tonic client ## Invariants -- **Sessions are keyed by the authenticated principal**, never by `UserContext.user_id` (ADR 0046 §5, threat model - T-A8); a session id another principal holds is `INVALID_HANDLE.SESSION_NOT_FOUND`, the same answer as unknown. - Until T002 the principal is a request extension set by the auth layer, else the local user (loopback only). +- **Sessions are keyed by the authenticated caller's `SessionOwner`**, never by `UserContext.user_id` (ADR 0046 + §5, threat model T-A8); a session id another owner holds is `INVALID_HANDLE.SESSION_NOT_FOUND`, the same answer + as unknown. Groups are not in the key; the method is (a token and an OIDC subject of one name are two owners). +- **A session's policy is its caller's role at creation, for life**: a caller whose role differs never gets that + session; it is closed without a tombstone and the lookup goes on as for a free id (a create builds a new one, + a client that saw the old id gets `INVALID_HANDLE.SESSION_CHANGED`, an RPC that creates nothing + `SESSION_NOT_FOUND`). `ReleaseSession` ends it whatever the role. +- **Every RPC authenticates before its body is decoded** (`AuthLayer`), once per RPC (tokens expire, connections + outlive them); a class `check_rpc` gives that needs more than the session lookup or the analyzer is refused + until its RPC applies it (fail closed). - **No lock across an `await`**, and none while a session is built or closed: the manager's mutex guards map lookups and inserts only. A racing create of the same key keeps the first and closes the loser. - **Decode only through `wire`** (`#![deny(clippy::disallowed_methods)]`): the codec is the one decode path. diff --git a/crates/krishiv-connect-server/Cargo.toml b/crates/krishiv-connect-server/Cargo.toml index 74a71574c..245e8fd56 100644 --- a/crates/krishiv-connect-server/Cargo.toml +++ b/crates/krishiv-connect-server/Cargo.toml @@ -21,7 +21,7 @@ krishiv-common = { path = "../krishiv-common", version = "=0.2.0" } krishiv-connect = { path = "../krishiv-connect", version = "=0.2.0" } krishiv-memory = { path = "../krishiv-memory", version = "=0.2.0" } krishiv-plan = { path = "../krishiv-plan", version = "=0.2.0" } -krishiv-session = { path = "../krishiv-session", version = "=0.2.0", features = ["unstable-ir"] } +krishiv-session = { path = "../krishiv-session", version = "=0.2.0", features = ["unstable-ir", "tls"] } krishiv-types = { path = "../krishiv-types", version = "=0.2.0" } getrandom.workspace = true prost = { workspace = true, features = ["derive", "std"] } @@ -30,11 +30,15 @@ rustix.workspace = true serde_json = { workspace = true, features = ["std"] } sha2.workspace = true tokio = { workspace = true, features = ["net", "rt", "sync", "time", "macros"] } +tokio-rustls.workspace = true tonic = { workspace = true, features = ["server", "router", "codegen"] } +tower.workspace = true tracing.workspace = true zstd.workspace = true [dev-dependencies] +hyper-util = { workspace = true, features = ["tokio"] } +tower = { workspace = true, features = ["util"] } tempfile.workspace = true tokio = { workspace = true, features = ["rt-multi-thread", "macros", "net", "time"] } tonic = { workspace = true, features = ["channel", "server", "router", "codegen"] } diff --git a/crates/krishiv-connect-server/src/auth.rs b/crates/krishiv-connect-server/src/auth.rs new file mode 100644 index 000000000..3d9e34bce --- /dev/null +++ b/crates/krishiv-connect-server/src/auth.rs @@ -0,0 +1,469 @@ +// SPDX-License-Identifier: Apache-2.0 +//! Authentication and request classes of the Spark Connect listener (P3-T002; design +//! `design/P3-T002-authentication.md` §3.1 and §3.6, ADR 0046 §2, §3b and §5). +//! +//! - [`AuthLayer`] authenticates **every RPC** from its headers alone, in front of the tonic +//! service, so nothing of an unauthenticated request's body is read or decoded. Once per +//! RPC, not per connection: tokens expire and an HTTP/2 connection outlives them. +//! [`ConnectAuth::authenticate`] hands the bearer token of the `authorization` header and +//! the peer address (the failure throttle's key) to the server's one [`Authenticator`]. A +//! failure is `UNAUTHENTICATED` with Spark's messages; a throttled peer or an unreachable +//! OIDC provider is `UNAVAILABLE`, which clients retry; a request without a peer address is +//! refused (fail closed). A caller that authenticates but maps to **no role** is refused +//! before any session exists (`PERMISSION_DENIED`, `KRISHIV_INSUFFICIENT_PERMISSIONS`, ADR +//! 0046 §2), with one `krishiv::audit` record. The result rides in the request extensions; +//! [`ConnectAuth::caller`] reads it there and fails closed when it is missing. With +//! authentication off every request is the server's local user, which the loopback-only +//! listener rule keeps to this machine. +//! - [`SessionOwner`] is what a Connect session is bound to: the method, the principal's kind, +//! tenant and name, **without its groups** (one OIDC issuer per server, so the method stands +//! for it). An OIDC user whose group claim changes between two tokens keeps its sessions, +//! and a static token and an OIDC subject with the same name never share one (design §7, +//! open question; this is the proposed answer). A session's policy is fixed for its life, +//! so a caller whose **role** differs from the role its session was built with does not get +//! that session: the session manager replaces it (`crate::manager`). A session keeps the +//! principal (groups included) of the caller that created it. +//! - [`check_rpc`] and [`check_command`] give every RPC and every `Command` arm its ADR 0046 +//! §3b class. Neither has a wildcard arm, so a proto update that adds an RPC or a command +//! does not compile until the new arm is classified. +//! - [`role_sessions`] builds each caller's session as `krishiv server` does: +//! `PathPrefixPolicy(RolePolicy(role))` (design §3.6). + +mod layer; + +use std::net::SocketAddr; +use std::sync::Arc; + +use krishiv_common::config::Config; +use krishiv_common::error::KrishivError; +use krishiv_common::principal::{Principal, PrincipalKind}; +use krishiv_connect::proto::v4_3 as pb; +use krishiv_connect::proto::v4_3::command::CommandType; +use krishiv_connect::proto::v4_3::ml_command::Command as MlCommandType; +use krishiv_plan::analyzer::insufficient_permissions; +use krishiv_session::auth::{ + AuthFailure, Authenticated, Authenticator, Credentials, DeniedCounts, Method, Role, RolePolicy, + Surface, +}; +use krishiv_session::{ + PathPrefixPolicy, PolicyVersion, Privilege, Securable, Session, StatementClass, +}; +use tonic::codegen::http::HeaderMap; +use tonic::codegen::http::header::AUTHORIZATION; +use tonic::{Request, Status}; + +pub use layer::{AuthLayer, AuthService}; + +use crate::manager::SessionFactory; +use crate::status::status_of; + +/// The `system` securable a principal needs `Usage` on to send code the server runs (ADR +/// 0046 §4). +const SYSTEM_UDF: &str = "udf"; + +/// The catalog the `global_temp` namespace belongs to, whatever catalog is current. +const SESSION_CATALOG: &str = "spark_catalog"; + +/// The namespace global temporary views live in. +const GLOBAL_TEMP: &str = "global_temp"; + +/// Who each Connect request acts for. +pub struct ConnectAuth { + authenticator: Option>, + local: Principal, + denials: Option>, +} + +impl std::fmt::Debug for ConnectAuth { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ConnectAuth") + .field("enabled", &self.is_enabled()) + .field("local", &self.local) + .finish() + } +} + +impl Default for ConnectAuth { + fn default() -> Self { + Self::off() + } +} + +impl ConnectAuth { + /// Authentication off: every request is the server's local user (loopback only). + #[must_use] + pub fn off() -> Self { + Self { + authenticator: None, + local: Principal::local_user(), + denials: None, + } + } + + /// Every request authenticated by `authenticator` (the server's one `ServerAuth`, shared + /// with the admin listener). + #[must_use] + pub fn new(authenticator: Arc) -> Self { + Self { + authenticator: Some(authenticator), + local: Principal::local_user(), + denials: None, + } + } + + /// The same, counting its refusals of callers with no role into `denials` + /// (`krishiv_auth_denied_total`, ADR 0046 §8; normally `ServerAuth::denials`). + #[must_use] + pub fn with_denials(mut self, denials: Arc) -> Self { + self.denials = Some(denials); + self + } + + /// Whether requests are authenticated. + #[must_use] + pub fn is_enabled(&self) -> bool { + self.authenticator.is_some() + } + + fn local(&self) -> Authenticated { + Authenticated::new(self.local.clone(), None, Method::None) + } + + /// Authenticates one request from its `headers`, `peer` address and, over TLS, the client + /// certificate rustls verified (`peer_leaf`) ([`AuthLayer`] calls + /// it before the body is read). + /// + /// # Errors + /// `UNAUTHENTICATED` for a missing, malformed, wrong or expired credential (and for a + /// request without a peer address); `UNAVAILABLE` for a throttled peer or a credential that + /// cannot be checked now; `PERMISSION_DENIED` (`KRISHIV_INSUFFICIENT_PERMISSIONS`) for a + /// caller with no role. + pub async fn authenticate( + &self, + headers: &HeaderMap, + peer: Option, + peer_leaf: Option<&[u8]>, + ) -> Result { + let Some(authenticator) = &self.authenticator else { + return Ok(self.local()); + }; + let Some(peer) = peer else { + tracing::warn!( + target: "krishiv::auth", + surface = Surface::Connect.as_str(), + "refused a Connect request without a peer address" + ); + return Err(failure_status(AuthFailure::Missing)); + }; + let mut credentials = Credentials::new(peer, Surface::Connect); + credentials.peer_leaf = peer_leaf; + let mut values = headers.get_all(AUTHORIZATION).iter(); + match (values.next(), values.next()) { + (None, _) => {} + // One header, readable as text: its `Bearer` token, if it is one. + (Some(value), None) => match value.to_str() { + Ok(text) => credentials = credentials.with_authorization_header(text), + Err(_) => credentials.malformed = true, + }, + // Two `authorization` entries: which one a proxy or the client meant is unknown. + (Some(_), Some(_)) => credentials.malformed = true, + } + let caller = authenticator + .authenticate(&credentials) + .await + .map_err(failure_status)?; + if caller.role.is_none() { + // ADR 0046 §2: no role, no session. `Browse` is the least any role holds. + let error = insufficient_permissions( + &caller.principal, + &Securable::System("cluster".to_owned()), + Privilege::Browse, + ); + tracing::warn!( + target: "krishiv::audit", + principal = %caller.principal.name, + method = caller.method.as_str(), + surface = Surface::Connect.as_str(), + error_class = error.error_class().name(), + "refused a Connect caller with no role" + ); + if let Some(d) = &self.denials { + d.record(Surface::Connect, Privilege::Browse); + } + return Err(status_of(&error)); + } + Ok(caller) + } + + /// The caller of `request`: what [`AuthLayer`] attached to it, or the local user with + /// authentication off. + /// + /// # Errors + /// `UNAUTHENTICATED` when authentication is on and nothing was attached (the service runs + /// without the layer): fail closed. + pub fn caller(&self, request: &Request) -> Result { + if !self.is_enabled() { + return Ok(self.local()); + } + request + .extensions() + .get::() + .cloned() + .ok_or_else(|| { + tracing::error!( + target: "krishiv::auth", + surface = Surface::Connect.as_str(), + "a Connect request reached the service without authentication" + ); + failure_status(AuthFailure::Missing) + }) + } +} + +/// The gRPC status of an authentication failure (the failure is logged and counted by the +/// authenticator; the status never carries the credential). +fn failure_status(failure: AuthFailure) -> Status { + if failure.is_retryable() { + Status::unavailable(failure.message()) + } else { + Status::unauthenticated(failure.message()) + } +} + +/// The session factory of an authenticated server (design §3.6): each caller's session acts +/// for its principal under `PathPrefixPolicy(RolePolicy(role))` on `base`'s runtime. Client +/// code needs admin until the T014b sandbox merges (ADR 0046 N1). The path confinement of +/// `config` (`krishiv.io.local.allowedRoots`, `krishiv.io.objectStore.allowedPrefixes`) is +/// checked once here, so a bad root fails at startup. Each session counts its denials into +/// `denials` under the `connect` surface (`krishiv_auth_denied_total`, ADR 0046 §8). +/// +/// # Errors +/// What `PathPrefixPolicy::from_config` refuses (`KRISHIV_INVALID_CONFIG_VALUE`). +pub fn role_sessions( + base: Session, + config: &Config, + denials: Arc, +) -> Result, KrishivError> { + let config = config.clone(); + let policy = move |role: Option| { + let roles = RolePolicy::new(role, Role::Admin, PolicyVersion(0)); + PathPrefixPolicy::from_config(&config, Arc::new(roles)) + }; + policy(None)?; + Ok(Box::new(move |caller: &Authenticated| { + let policy = policy(caller.role)?; + let session = base.new_session_as(caller.principal.clone(), Arc::new(policy)); + session.count_denials(Arc::clone(&denials), Surface::Connect); + Ok(session) + })) +} + +/// What a Connect session is bound to (module docs): the session manager's key. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct SessionOwner { + method: Method, + kind: PrincipalKind, + tenant: Option, + name: String, +} + +impl SessionOwner { + /// The owner `caller`'s sessions belong to. + #[must_use] + pub fn of(caller: &Authenticated) -> Self { + Self { + method: caller.method, + kind: caller.principal.kind, + tenant: caller.principal.tenant.clone(), + name: caller.principal.name.clone(), + } + } +} + +/// A Spark Connect RPC (`spark.connect.SparkConnectService`, Spark 4.3.0). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Rpc { + /// `ExecutePlan`; its commands are classified by [`check_command`]. + ExecutePlan, + /// `AnalyzePlan`: `Browse` on what the plan names for most analyses; its `persist` / + /// `unpersist` subtypes are [`Check::SideEffect`] and `input_files` is `Select` on the + /// paths (ADR 0046 §3b), classified per subtype when PR-11b implements the RPC. + AnalyzePlan, + /// `Config`. + Config, + /// `AddArtifacts`. + AddArtifacts, + /// `ArtifactStatus`. + ArtifactStatus, + /// `Interrupt`. + Interrupt, + /// `ReattachExecute`. + ReattachExecute, + /// `ReleaseExecute`. + ReleaseExecute, + /// `ReleaseSession`. + ReleaseSession, + /// `FetchErrorDetails`. + FetchErrorDetails, + /// `CloneSession`. + CloneSession, + /// `GetStatus`. + GetStatus, +} + +/// What a request is checked as (ADR 0046 §3b). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Check { + /// Nothing beyond the session lookup, which is keyed by the caller: its own sessions and + /// operations, every role. + OwnSession, + /// The securables of the resolved plan, which the session's analyzer asks the policy + /// about when it runs the plan (and, for SQL text, the statement classifier, statement by + /// statement). + Plan, + /// A non-durable side effect (`checkpoint_command`; `AnalyzePlan` `persist` / `unpersist`): + /// the plan's securables plus the operator floor of the ADR 0046 §3 side-effect row + /// (a viewer is refused even for a plan that reads no table), applied with the SQL + /// `CACHE TABLE` path when P3-T012 / PR-11b run them. + SideEffect, + /// The `Write` class: `Modify` on the target, plus `Create` when the save mode makes one, + /// plus the source plan's securables, through the session's write path (P3-T012 runs it). + Write, + /// `privilege` on `securable`, recorded as `class` when denied, before anything runs. + Privilege { + /// What is asked about. + securable: Securable, + /// The privilege needed. + privilege: Privilege, + /// The statement class an audit record names. + class: StatementClass, + }, + /// Refused for every role: not supported in P3 (ADR 0046 §3b); `what` names it. + Unsupported(&'static str), +} + +fn client_code() -> Check { + Check::Privilege { + securable: Securable::System(SYSTEM_UDF.to_owned()), + privilege: Privilege::Usage, + class: StatementClass::Session, + } +} + +/// The class of `rpc` (ADR 0046 §3b). `ExecutePlan` and `AnalyzePlan` are [`Check::Plan`]: a +/// command is classified further by [`check_command`]. +#[must_use] +pub fn check_rpc(rpc: Rpc) -> Check { + match rpc { + Rpc::ExecutePlan | Rpc::AnalyzePlan => Check::Plan, + // Code the server may run: `Usage` on `System("udf")`. + Rpc::AddArtifacts => client_code(), + // A `Config.set` of an admin key needs `Manage` on the cluster: the key's + // `SettableBy`, checked by the same path as SQL `SET`. + Rpc::Config + | Rpc::ArtifactStatus + | Rpc::Interrupt + | Rpc::ReattachExecute + | Rpc::ReleaseExecute + | Rpc::ReleaseSession + | Rpc::FetchErrorDetails + | Rpc::CloneSession + | Rpc::GetStatus => Check::OwnSession, + } +} + +/// The class of `command` (ADR 0046 §3b). A command without a type is [`Check::Plan`]: the +/// translation refuses it. +#[must_use] +pub fn check_command(command: &pb::Command) -> Check { + let Some(command) = &command.command_type else { + return Check::Plan; + }; + match command { + CommandType::SqlCommand(_) => Check::Plan, + CommandType::CheckpointCommand(_) => Check::SideEffect, + CommandType::WriteOperation(_) + | CommandType::WriteOperationV2(_) + | CommandType::MergeIntoTableCommand(_) => Check::Write, + CommandType::CreateDataframeView(view) => { + if view.is_global { + // A shared object: the `Ddl` class, as `CREATE GLOBAL TEMPORARY VIEW`. + Check::Privilege { + securable: Securable::Namespace(vec![ + SESSION_CATALOG.to_owned(), + GLOBAL_TEMP.to_owned(), + ]), + privilege: Privilege::Create, + class: StatementClass::Ddl, + } + } else { + // A local temporary view: its plan is checked when it is read. + Check::OwnSession + } + } + CommandType::RegisterFunction(_) + | CommandType::RegisterTableFunction(_) + | CommandType::RegisterDataSource(_) => client_code(), + CommandType::GetResourcesCommand(_) | CommandType::RemoveCachedRemoteRelationCommand(_) => { + Check::OwnSession + } + CommandType::MlCommand(ml) => match &ml.command { + // A stock client sends it before every test and at exit: nothing to clean. + Some(MlCommandType::CleanCache(_)) => Check::OwnSession, + Some( + MlCommandType::Fit(_) + | MlCommandType::Fetch(_) + | MlCommandType::Delete(_) + | MlCommandType::Write(_) + | MlCommandType::Read(_) + | MlCommandType::Evaluate(_) + | MlCommandType::GetCacheInfo(_) + | MlCommandType::CreateSummary(_) + | MlCommandType::GetModelSize(_), + ) + | None => Check::Unsupported("ml_command"), + }, + CommandType::CreateResourceProfileCommand(_) => { + Check::Unsupported("create_resource_profile_command") + } + CommandType::ExecuteExternalCommand(_) => Check::Unsupported("execute_external_command"), + CommandType::PipelineCommand(_) => Check::Unsupported("pipeline_command"), + CommandType::WriteStreamOperationStart(_) + | CommandType::StreamingQueryCommand(_) + | CommandType::StreamingQueryManagerCommand(_) + | CommandType::StreamingQueryListenerBusCommand(_) => { + Check::Unsupported("streaming commands") + } + // No extension is registered until P3-T015; one without a declared class is refused. + CommandType::Extension(_) => Check::Unsupported("extension"), + } +} + +/// Applies the part of `check` that is decided before a request runs: a +/// [`Check::Privilege`] is asked of `session`'s policy. The other classes are applied where +/// the request runs (the session lookup, the analyzer, the write path) or refused by it. +/// +/// # Errors +/// `KRISHIV_INSUFFICIENT_PERMISSIONS` (42501) when the session's principal lacks the +/// privilege. +pub async fn enforce(check: &Check, session: &Session) -> Result<(), KrishivError> { + match check { + Check::Privilege { + securable, + privilege, + class, + } => { + session + .authorize_privilege(securable, *privilege, *class) + .await + } + Check::OwnSession + | Check::Plan + | Check::SideEffect + | Check::Write + | Check::Unsupported(_) => Ok(()), + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/krishiv-connect-server/src/auth/layer.rs b/crates/krishiv-connect-server/src/auth/layer.rs new file mode 100644 index 000000000..4449ea080 --- /dev/null +++ b/crates/krishiv-connect-server/src/auth/layer.rs @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: Apache-2.0 +//! [`AuthLayer`]: the tower layer in front of the Connect service that authenticates each +//! request from its headers and peer address before tonic reads or decodes the body (design +//! P3-T002 §3.1). A refusal is answered here as a gRPC status; an accepted request carries its +//! `Authenticated` caller in the request extensions, where [`super::ConnectAuth::caller`] reads +//! it. + +use std::future::Future; +use std::pin::Pin; +use std::sync::Arc; +use std::task::{Context, Poll}; + +use tonic::codegen::http::{Request, Response}; +use tower::Service; + +use super::ConnectAuth; +use crate::server::PeerInfo; + +/// Authenticates every request before the wrapped service sees it. +#[derive(Debug, Clone)] +pub struct AuthLayer { + auth: Arc, +} + +impl AuthLayer { + /// The layer of `auth`. + #[must_use] + pub fn new(auth: Arc) -> Self { + Self { auth } + } +} + +impl tower::Layer for AuthLayer { + type Service = AuthService; + + fn layer(&self, inner: S) -> AuthService { + AuthService { + inner, + auth: Arc::clone(&self.auth), + } + } +} + +/// The service [`AuthLayer`] wraps around the Connect router. +#[derive(Debug, Clone)] +pub struct AuthService { + inner: S, + auth: Arc, +} + +type BoxFuture = Pin + Send + 'static>>; + +impl Service> for AuthService +where + S: Service, Response = Response> + Clone + Send + 'static, + S::Future: Send + 'static, + S::Error: Send + 'static, + ReqBody: Send + 'static, + ResBody: Default + Send + 'static, +{ + type Response = Response; + type Error = S::Error; + type Future = BoxFuture>; + + fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + self.inner.poll_ready(cx) + } + + fn call(&mut self, mut request: Request) -> Self::Future { + // The service that was polled ready serves this request; a fresh clone stays behind + // for the next one (tower's usual clone-and-swap). + let clone = self.inner.clone(); + let mut inner = std::mem::replace(&mut self.inner, clone); + let auth = Arc::clone(&self.auth); + Box::pin(async move { + let info = request.extensions().get::().cloned(); + let peer = info.as_ref().and_then(|i| i.remote); + let leaf = info.as_ref().and_then(|i| i.leaf.as_deref()); + match auth.authenticate(request.headers(), peer, leaf).await { + Ok(caller) => { + request.extensions_mut().insert(caller); + inner.call(request).await + } + Err(status) => Ok(status.into_http()), + } + }) + } +} diff --git a/crates/krishiv-connect-server/src/auth/tests.rs b/crates/krishiv-connect-server/src/auth/tests.rs new file mode 100644 index 000000000..401f268d3 --- /dev/null +++ b/crates/krishiv-connect-server/src/auth/tests.rs @@ -0,0 +1,331 @@ +// SPDX-License-Identifier: Apache-2.0 +use std::io::Write; +use std::net::{IpAddr, Ipv4Addr, SocketAddr}; +use std::path::Path; +use std::sync::Arc; + +use krishiv_common::principal::PrincipalKind; +use krishiv_connect::proto::v4_3 as pb; +use krishiv_connect::proto::v4_3::command::CommandType; +use krishiv_connect::proto::v4_3::ml_command::Command as MlCommandType; +use krishiv_session::auth::Authenticated; +use krishiv_session::auth::{AuthConfig, Method, Role, ServerAuth}; +use krishiv_session::{Privilege, Securable, StatementClass}; +use tonic::codegen::http::HeaderMap; +use tonic::{Code, Request}; + +use super::{Check, ConnectAuth, Rpc, SessionOwner, check_command, check_rpc}; + +const TOKEN: &str = "cccccccccccccccccccccccccccccccccccccccc"; + +#[expect( + clippy::disallowed_methods, + reason = "test setup: the secret file a token config reads" +)] +fn secret_file(dir: &Path, contents: &str) -> String { + let path = dir.join("token"); + let mut f = std::fs::File::create(&path).expect("create"); + f.write_all(contents.as_bytes()).expect("write"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).expect("mode"); + } + format!("file:{}", path.display()) +} + +/// A `ConnectAuth` over a real `ServerAuth` with one token: `carol`, viewer. +fn token_auth(dir: &Path) -> ConnectAuth { + let secret = secret_file(dir, TOKEN); + let config = AuthConfig::from_settings([ + ("krishiv.auth.tokens.c.secret", secret.as_str()), + ("krishiv.auth.tokens.c.principal", "carol"), + ("krishiv.auth.tokens.c.role", "viewer"), + ]) + .expect("config"); + ConnectAuth::new(Arc::new(ServerAuth::new(&config).expect("auth"))) +} + +fn peer(last: u8) -> SocketAddr { + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(10, 1, 2, last)), 50_000) +} + +/// The headers of a request with the `authorization` values `values`. +fn headers(values: &[&str]) -> HeaderMap { + let mut h = HeaderMap::new(); + for v in values { + h.append("authorization", v.parse().expect("ascii")); + } + h +} + +/// Authenticates a request from `peer` with the `authorization` values `values`. +fn authenticate( + auth: &ConnectAuth, + peer: Option, + values: &[&str], +) -> Result { + block_on(auth.authenticate(&headers(values), peer, None)) +} + +fn block_on(f: F) -> F::Output { + futures::executor::block_on(f) +} + +#[test] +fn off_is_the_local_user_without_a_credential() { + let auth = ConnectAuth::off(); + assert!(!auth.is_enabled()); + let caller = auth.caller(&Request::new(())).expect("local"); + assert_eq!( + caller.principal.name, + krishiv_common::principal::Principal::local_user().name + ); + assert_eq!(caller.method, Method::None); + assert_eq!(caller.role, None); +} + +#[test] +fn a_token_names_its_principal_and_role() { + let dir = tempfile::tempdir().expect("dir"); + let auth = token_auth(dir.path()); + assert!(auth.is_enabled()); + let caller = + authenticate(&auth, Some(peer(1)), &[&format!("Bearer {TOKEN}")]).expect("authenticated"); + assert_eq!(caller.principal.name, "carol"); + assert_eq!(caller.principal.kind, PrincipalKind::User); + assert_eq!(caller.role, Some(Role::Viewer)); + assert_eq!(caller.method, Method::Token); +} + +#[test] +fn failures_are_spark_unauthenticated_messages() { + let dir = tempfile::tempdir().expect("dir"); + let auth = token_auth(dir.path()); + let status = |values: &[&str]| authenticate(&auth, Some(peer(2)), values).expect_err("refused"); + let missing = status(&[]); + assert_eq!(missing.code(), Code::Unauthenticated); + assert_eq!(missing.message(), "No authentication token provided"); + let wrong = status(&["Bearer not-the-token"]); + assert_eq!(wrong.code(), Code::Unauthenticated); + assert_eq!(wrong.message(), "Invalid authentication token"); + // Another scheme, and two `authorization` entries, are malformed: invalid, not missing. + let basic = status(&["Basic dXNlcjpwdw=="]); + assert_eq!(basic.message(), "Invalid authentication token"); + let two = status(&[&format!("Bearer {TOKEN}"), &format!("Bearer {TOKEN}")]); + assert_eq!(two.code(), Code::Unauthenticated); + assert_eq!(two.message(), "Invalid authentication token"); + // The credential never appears in the status. + for s in [&missing, &wrong, &basic, &two] { + assert!(!s.message().contains(TOKEN)); + } +} + +#[test] +fn a_request_without_a_peer_address_is_refused() { + let dir = tempfile::tempdir().expect("dir"); + let auth = token_auth(dir.path()); + let s = authenticate(&auth, None, &[&format!("Bearer {TOKEN}")]).expect_err("fails closed"); + assert_eq!(s.code(), Code::Unauthenticated); +} + +#[test] +fn a_throttled_peer_is_unavailable() { + let dir = tempfile::tempdir().expect("dir"); + let auth = token_auth(dir.path()); + let mut last = None; + // The default throttle blocks a peer after 20 failures a minute. + for _ in 0..25 { + last = authenticate(&auth, Some(peer(3)), &["Bearer wrong"]).err(); + } + let s = last.expect("refused"); + assert_eq!(s.code(), Code::Unavailable); + // Even the right token waits out the block, and another peer is unaffected. + let blocked = authenticate(&auth, Some(peer(3)), &[&format!("Bearer {TOKEN}")]); + assert_eq!(blocked.expect_err("blocked").code(), Code::Unavailable); + authenticate(&auth, Some(peer(4)), &[&format!("Bearer {TOKEN}")]).expect("other peer"); +} + +#[test] +fn the_owner_leaves_out_groups_and_keeps_the_method() { + let dir = tempfile::tempdir().expect("dir"); + let auth = token_auth(dir.path()); + let caller = + authenticate(&auth, Some(peer(5)), &[&format!("Bearer {TOKEN}")]).expect("authenticated"); + let mut grouped = caller.clone(); + grouped.principal.groups = vec!["bi".to_owned()]; + assert_eq!(SessionOwner::of(&caller), SessionOwner::of(&grouped)); + let mut oidc = caller.clone(); + oidc.method = Method::Oidc; + assert_ne!(SessionOwner::of(&caller), SessionOwner::of(&oidc)); +} + +fn command(c: CommandType) -> pb::Command { + pb::Command { + command_type: Some(c), + } +} + +fn udf_usage() -> Check { + Check::Privilege { + securable: Securable::System("udf".to_owned()), + privilege: Privilege::Usage, + class: StatementClass::Session, + } +} + +#[test] +fn every_rpc_has_its_class() { + for rpc in [Rpc::ExecutePlan, Rpc::AnalyzePlan] { + assert_eq!(check_rpc(rpc), Check::Plan, "{rpc:?}"); + } + assert_eq!(check_rpc(Rpc::AddArtifacts), udf_usage()); + for rpc in [ + Rpc::Config, + Rpc::ArtifactStatus, + Rpc::Interrupt, + Rpc::ReattachExecute, + Rpc::ReleaseExecute, + Rpc::ReleaseSession, + Rpc::FetchErrorDetails, + Rpc::CloneSession, + Rpc::GetStatus, + ] { + assert_eq!(check_rpc(rpc), Check::OwnSession, "{rpc:?}"); + } +} + +#[test] +fn every_command_has_its_class() { + assert_eq!(check_command(&pb::Command::default()), Check::Plan); + let cases: Vec<(CommandType, Check)> = vec![ + (CommandType::SqlCommand(Box::default()), Check::Plan), + ( + CommandType::CheckpointCommand(Box::default()), + Check::SideEffect, + ), + (CommandType::WriteOperation(Box::default()), Check::Write), + (CommandType::WriteOperationV2(Box::default()), Check::Write), + ( + CommandType::MergeIntoTableCommand(Box::default()), + Check::Write, + ), + (CommandType::RegisterFunction(Box::default()), udf_usage()), + ( + CommandType::RegisterTableFunction(Box::default()), + udf_usage(), + ), + (CommandType::RegisterDataSource(Box::default()), udf_usage()), + ( + CommandType::CreateDataframeView(Box::default()), + Check::OwnSession, + ), + ( + CommandType::CreateDataframeView(Box::new(pb::CreateDataFrameViewCommand { + is_global: true, + ..Default::default() + })), + Check::Privilege { + securable: Securable::Namespace(vec![ + "spark_catalog".to_owned(), + "global_temp".to_owned(), + ]), + privilege: Privilege::Create, + class: StatementClass::Ddl, + }, + ), + ( + CommandType::GetResourcesCommand(Box::default()), + Check::OwnSession, + ), + ( + CommandType::RemoveCachedRemoteRelationCommand(Box::default()), + Check::OwnSession, + ), + ( + CommandType::MlCommand(Box::new(pb::MlCommand { + command: Some(MlCommandType::CleanCache(Default::default())), + })), + Check::OwnSession, + ), + ( + CommandType::MlCommand(Box::new(pb::MlCommand { + command: Some(MlCommandType::GetCacheInfo(Default::default())), + })), + Check::Unsupported("ml_command"), + ), + ( + CommandType::MlCommand(Box::default()), + Check::Unsupported("ml_command"), + ), + ( + CommandType::PipelineCommand(Box::default()), + Check::Unsupported("pipeline_command"), + ), + ( + CommandType::ExecuteExternalCommand(Box::default()), + Check::Unsupported("execute_external_command"), + ), + ( + CommandType::CreateResourceProfileCommand(Box::default()), + Check::Unsupported("create_resource_profile_command"), + ), + ( + CommandType::StreamingQueryCommand(Box::default()), + Check::Unsupported("streaming commands"), + ), + ( + CommandType::Extension(Box::default()), + Check::Unsupported("extension"), + ), + ]; + for (c, want) in cases { + assert_eq!(check_command(&command(c.clone())), want, "{c:?}"); + } +} + +#[test] +fn enforce_asks_the_session_policy() { + use krishiv_session::auth::RolePolicy; + let base = krishiv_session::Session::new().expect("session"); + let viewer = base.new_session_as( + krishiv_common::principal::Principal::new("v", PrincipalKind::User), + Arc::new(RolePolicy::new( + Some(Role::Viewer), + Role::Admin, + krishiv_session::PolicyVersion(1), + )), + ); + let e = block_on(super::enforce(&udf_usage(), &viewer)).expect_err("denied"); + assert_eq!(e.error_class().name(), "KRISHIV_INSUFFICIENT_PERMISSIONS"); + for allowed in [ + Check::OwnSession, + Check::Plan, + Check::Write, + Check::Unsupported("x"), + ] { + block_on(super::enforce(&allowed, &viewer)).expect("decided elsewhere"); + } + let admin = base.new_session_as( + krishiv_common::principal::Principal::new("a", PrincipalKind::User), + Arc::new(RolePolicy::new( + Some(Role::Admin), + Role::Admin, + krishiv_session::PolicyVersion(1), + )), + ); + block_on(super::enforce(&udf_usage(), &admin)).expect("admin sends client code"); +} + +#[test] +fn with_authentication_on_a_request_without_the_layers_caller_is_refused() { + // The service reads the caller the layer attached; without one it fails closed. + let dir = tempfile::tempdir().expect("dir"); + let auth = token_auth(dir.path()); + let s = auth.caller(&Request::new(())).expect_err("no layer ran"); + assert_eq!(s.code(), Code::Unauthenticated); + let mut attached = Request::new(()); + let carol = authenticate(&auth, Some(peer(6)), &[&format!("Bearer {TOKEN}")]).expect("token"); + attached.extensions_mut().insert(carol.clone()); + assert_eq!(auth.caller(&attached).expect("attached"), carol); +} diff --git a/crates/krishiv-connect-server/src/config.rs b/crates/krishiv-connect-server/src/config.rs index 0d48f123e..a27a5fdda 100644 --- a/crates/krishiv-connect-server/src/config.rs +++ b/crates/krishiv-connect-server/src/config.rs @@ -5,10 +5,12 @@ //! - `set(pairs, silent)`: per pair, in order; with `silent` a failure is //! the warning `Failed to set to Some() due to ` //! (Spark prints the Scala `Option`) and the next pair is tried. Who may -//! set a key is the registry's `SettableBy` (ADR 0046 §8, threat model -//! T-Z3): a `Startup` key is `CANNOT_MODIFY_STATIC_CONFIG` for every role, -//! an `Admin` key `KRISHIV_INSUFFICIENT_PERMISSIONS` (42501) for a viewer or -//! operator (`check_settable`). +//! set or unset a key (ADR 0046 §3, §8, threat model T-Z3): a `Startup` +//! key is `CANNOT_MODIFY_STATIC_CONFIG` for everyone (`check_static`); +//! any other key is the session's policy, asked as SQL `SET` asks it +//! (`Session::authorize_set_key`: an `Admin` key or a server-scoped store +//! key needs `Manage` on the cluster, else `KRISHIV_INSUFFICIENT_PERMISSIONS` +//! (42501), audited and counted). The listener keeps no role check of its own. //! - `get(keys)`: an unset key without a default is `SQL_CONF_NOT_FOUND` //! (Spark's `SQLConf.getConfString`, `SQLConf.scala:9801-9808`). //! - `get_with_default`: a set value, else the caller's default (a @@ -33,9 +35,8 @@ use std::borrow::Cow; use krishiv_common::config::{SettableBy, lookup, resolve_alternative}; use krishiv_common::error::{ CannotModifyStaticConfig, ConnectInvalidPlanInvalidOneOfFieldNotSet, - InvalidConfValueRequirement, KrishivError, KrishivInsufficientPermissions, SqlConfNotFound, + InvalidConfValueRequirement, KrishivError, SqlConfNotFound, }; -use krishiv_common::principal::Principal; use krishiv_common::redact::redact; use krishiv_common::secret::is_sensitive_key; use krishiv_connect::proto::v4_3 as pb; @@ -44,68 +45,25 @@ use krishiv_session::RuntimeConf; use crate::manager::SessionEntry; -/// What a principal may do with configuration (ADR 0046 §8). -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -#[cfg_attr( - not(test), - expect( - dead_code, - reason = "viewer and operator arrive with authentication (P3-T002)" - ) -)] -pub(crate) enum ConfigRole { - /// Reads and sets `User` keys only. - Viewer, - /// Reads and sets `User` keys only. - Operator, - /// Sets `Admin` keys too. - Admin, -} - -/// The configuration role of `principal`. -/// -/// TODO(P3-T002): roles arrive with authentication; until then every -/// principal is the local user on a loopback listener (design P3-T001 -/// §3.6) and is treated as the admin. T002 must also: -/// - flip this default (an unknown principal is a viewer); -/// - move [`check_settable`] into the session's `AccessPolicy` path, so -/// SQL `SET` (`Session::sql`) and Connect's `SqlCommand` (T012) take the -/// same gate as this RPC (ADR 0046 rejects listener-local checks); -/// - hide `Startup` and `Admin` keys from `get_all` for viewers and -/// operators. -#[must_use] -pub(crate) fn role_of(_principal: &Principal) -> ConfigRole { - ConfigRole::Admin -} - -/// Whether `role` may set or unset `key` (the registry's `SettableBy`). +/// A `Startup` key cannot change in a running session, for anyone. The key is looked up as +/// `Session::authorize_set_key` looks it up (its Spark alias or legacy alternative too). /// /// # Errors -/// `CANNOT_MODIFY_STATIC_CONFIG` for a `Startup` key (every role); -/// `KRISHIV_INSUFFICIENT_PERMISSIONS` for an `Admin` key and a role other -/// than [`ConfigRole::Admin`]. -pub(crate) fn check_settable( - role: ConfigRole, - principal: &Principal, - key: &str, -) -> Result<(), KrishivError> { - match lookup(key).map(|e| e.settable_by) { - Some(SettableBy::Startup) => Err(CannotModifyStaticConfig { +/// `CANNOT_MODIFY_STATIC_CONFIG` for a `Startup` key. +pub(crate) fn check_static(key: &str) -> Result<(), KrishivError> { + let entry = lookup(key).or_else(|| lookup(resolve_alternative(key))); + if entry.is_some_and(|e| e.settable_by == SettableBy::Startup) { + return Err(CannotModifyStaticConfig { key: format!("\"{key}\""), } - .into()), - Some(SettableBy::Admin) if role != ConfigRole::Admin => { - Err(KrishivInsufficientPermissions { - principal: principal.name.clone(), - privilege: "SET".to_owned(), - securable: format!("configuration `{key}`"), - } - .into()) - } - _ => Ok(()), + .into()); } + Ok(()) } +// TODO(P3-T002 follow-up): hide `Startup` and `Admin` keys from `get_all` for callers the +// session's policy would not let set them. + /// The two SQL configs Spark Connect warns about /// (`SparkConnectConfigHandler.unsupportedConfigurations`, line 217-218). const UNSUPPORTED: [&str; 2] = [ @@ -263,11 +221,9 @@ fn message(e: &KrishivError) -> String { /// module docs); `CONNECT_INVALID_PLAN.INVALID_ONE_OF_FIELD_NOT_SET` for a /// request without an operation (Spark throws a classless /// `UnsupportedOperationException`: a recorded difference). -pub(crate) fn config( +pub(crate) async fn config( entry: &SessionEntry, request: &pb::ConfigRequest, - principal: &Principal, - role: ConfigRole, ) -> Result { let conf = entry.session().conf(); let mut out = pb::ConfigResponse { @@ -288,7 +244,7 @@ pub(crate) fn config( Some(OpType::Set(set)) => { let silent = set.silent.unwrap_or(false); for kv in &set.pairs { - match set_one(&conf, principal, role, &kv.key, kv.value.as_deref()) { + match set_one(entry, &kv.key, kv.value.as_deref()).await { Ok(()) => out.warnings.extend(warning(&kv.key)), Err(e) if silent => { let shown = kv @@ -370,7 +326,7 @@ pub(crate) fn config( } Some(OpType::Unset(u)) => { for key in &u.keys { - check_settable(role, principal, key)?; + authorize(entry, key).await?; conf.unset(key)?; out.warnings.extend(warning(key)); } @@ -386,14 +342,18 @@ pub(crate) fn config( Ok(out) } -fn set_one( - conf: &RuntimeConf, - principal: &Principal, - role: ConfigRole, - key: &str, - value: Option<&str>, -) -> Result<(), KrishivError> { - check_settable(role, principal, key)?; +/// The checks of a `set` or `unset` of `key`: a `Startup` key is refused for everyone first +/// (no policy call, nothing counted: not an auth denial, ADR 0046 §3), then the session asks +/// its policy as SQL `SET` does (`Session::authorize_set_key`: an `Admin` key or a +/// server-scoped store key needs `Manage` on the cluster; a denial is audited and counted). +async fn authorize(entry: &SessionEntry, key: &str) -> Result<(), KrishivError> { + check_static(key)?; + entry.session().authorize_set_key(key).await +} + +async fn set_one(entry: &SessionEntry, key: &str, value: Option<&str>) -> Result<(), KrishivError> { + authorize(entry, key).await?; + let conf = entry.session().conf(); // Spark's `require(value != null)` is a classless // `IllegalArgumentException`: a recorded difference. let Some(value) = value else { diff --git a/crates/krishiv-connect-server/src/config/tests.rs b/crates/krishiv-connect-server/src/config/tests.rs index c3ef5def2..1d29eefcd 100644 --- a/crates/krishiv-connect-server/src/config/tests.rs +++ b/crates/krishiv-connect-server/src/config/tests.rs @@ -1,31 +1,21 @@ // SPDX-License-Identifier: Apache-2.0 //! Unit tests of `SettableBy`, redaction and the per-key warnings. -use krishiv_common::principal::{Principal, PrincipalKind}; - -use super::{ConfigRole, check_settable, is_redacted, role_of, warning}; +use super::{check_static, is_redacted, warning}; const STARTUP: &str = "krishiv.resultCache.maxBytes"; const ADMIN: &str = "krishiv.resultCache.ttl"; const USER: &str = "spark.sql.ansi.enabled"; +/// A `Startup` key is refused here for everyone; an `Admin` key is left to the session's +/// policy (`tests/auth.rs` `config_refusals_are_counted_like_sql_set`). #[test] fn config_settable_by() { - let p = Principal::new("ann", PrincipalKind::User); - for role in [ConfigRole::Viewer, ConfigRole::Operator, ConfigRole::Admin] { - let e = check_settable(role, &p, STARTUP).unwrap_err(); - assert_eq!(e.error_class().name(), "CANNOT_MODIFY_STATIC_CONFIG"); - assert_eq!(e.error_class().sql_state(), Some("46110")); - check_settable(role, &p, USER).unwrap(); - check_settable(role, &p, "my.app.flag").unwrap(); - } - for role in [ConfigRole::Viewer, ConfigRole::Operator] { - let e = check_settable(role, &p, ADMIN).unwrap_err(); - assert_eq!(e.error_class().name(), "KRISHIV_INSUFFICIENT_PERMISSIONS"); - assert_eq!(e.error_class().sql_state(), Some("42501")); - } - check_settable(ConfigRole::Admin, &p, ADMIN).unwrap(); - // Until T002 the local principal is the admin. - assert_eq!(role_of(&Principal::local_user()), ConfigRole::Admin); + let e = check_static(STARTUP).unwrap_err(); + assert_eq!(e.error_class().name(), "CANNOT_MODIFY_STATIC_CONFIG"); + assert_eq!(e.error_class().sql_state(), Some("46110")); + check_static(ADMIN).unwrap(); + check_static(USER).unwrap(); + check_static("my.app.flag").unwrap(); } #[test] diff --git a/crates/krishiv-connect-server/src/execute.rs b/crates/krishiv-connect-server/src/execute.rs index 40e7a9b74..a4593e4c7 100644 --- a/crates/krishiv-connect-server/src/execute.rs +++ b/crates/krishiv-connect-server/src/execute.rs @@ -28,7 +28,9 @@ //! Errors end the stream with the gRPC status of [`crate::status`] after the //! responses before them. An interrupted operation ends with //! `OPERATION_CANCELED`. Commands answer `CONNECT_INVALID_PLAN` (56K00) -//! until P3-T012. +//! until P3-T012, after the check of their ADR 0046 §3b class +//! (`crate::auth::check_command`): a caller without the privilege a command +//! needs is refused with 42501 first. //! //! The core is protocol-neutral where it is cheap: the plan is decoded into //! a `krishiv_plan::Plan`, run as a `DataFrame` and encoded as Arrow IPC; @@ -40,6 +42,7 @@ pub mod schema; use std::io::Read; use std::sync::Arc; +use crate::auth::{Check, check_command}; use arrow_schema::SchemaRef; use futures::StreamExt; use krishiv_common::error::{ @@ -304,11 +307,36 @@ pub async fn execute_plan( Ok(stream) } +/// What [`translate_plan`] made of a plan. +enum Translated { + /// A ROOT relation as the engine's IR. + Relation(krishiv_plan::Plan), + /// A command (from a compressed operation too): its ADR 0046 §3b class, + /// then the refusal it gets until P3-T012 runs commands. The command + /// itself is not kept: it stays in the scrubbed request. + Command { + /// The command's class. + check: Check, + /// `CONNECT_INVALID_PLAN` naming the command. + refusal: KrishivError, + }, +} + +impl Translated { + fn command(command: &pb::Command) -> Self { + Self::Command { + check: check_command(command), + refusal: command_not_supported(command), + } + } +} + /// The engine plan of a request's `Plan`: a compressed one decompressed /// (off the async workers, bounded by `maxPlanSize`), then translated with /// the session's `spark.sql.sources.default`. Shared by `ExecutePlan` and /// `AnalyzePlan` (Spark's `RequestDecompressionInterceptor` then -/// `SparkConnectPlanner`). +/// `SparkConnectPlanner`). A command gets its class checked, then its refusal (until +/// P3-T012 runs commands). /// /// # Errors /// The decompression, decoding and translation errors. @@ -340,24 +368,35 @@ pub(crate) async fn request_plan( } _ => None, }; - translate_plan(plan, decompressed.as_deref(), &translate) + match translate_plan(plan, decompressed.as_deref(), &translate)? { + Translated::Relation(logical) => Ok(logical), + // A command's ADR 0046 §3b class is checked before it is refused, on every RPC that + // takes a plan. + Translated::Command { check, refusal } => { + crate::auth::enforce(&check, session).await?; + Err(refusal) + } + } } /// The plan as the engine's IR: a ROOT relation translated, a compressed -/// one decompressed first; commands refused until P3-T012. +/// one decompressed first; a command is returned as decoded. fn translate_plan( plan: &pb::Plan, decompressed: Option<&[u8]>, conf: &TranslateConf, -) -> Result { +) -> Result { match &plan.op_type { - Some(OpType::Root(relation)) => translate_relation(relation, conf), - Some(OpType::Command(command)) => Err(command_not_supported(command)), + Some(OpType::Root(relation)) => { + translate_relation(relation, conf).map(Translated::Relation) + } + Some(OpType::Command(command)) => Ok(Translated::command(command)), Some(OpType::CompressedOperation(c)) => { let bytes = decompressed.unwrap_or_default(); match CompressedOpType::try_from(c.op_type) { Ok(CompressedOpType::Relation) => { translate_relation(&*wire::decode_relation(bytes)?, conf) + .map(Translated::Relation) } Ok(CompressedOpType::Command) => { // Decoded through the bounded decoder as the `command` @@ -371,8 +410,8 @@ fn translate_plan( prost::encoding::encode_varint(bytes.len() as u64, &mut wrapped); wrapped.extend_from_slice(bytes); match &wire::decode_plan(&wrapped)?.op_type { - Some(OpType::Command(command)) => Err(command_not_supported(command)), - _ => Err(command_not_supported(&pb::Command::default())), + Some(OpType::Command(command)) => Ok(Translated::command(command)), + _ => Ok(Translated::command(&pb::Command::default())), } } _ => { diff --git a/crates/krishiv-connect-server/src/host.rs b/crates/krishiv-connect-server/src/host.rs index b51c8a267..2a173b1ce 100644 --- a/crates/krishiv-connect-server/src/host.rs +++ b/crates/krishiv-connect-server/src/host.rs @@ -1,19 +1,9 @@ // SPDX-License-Identifier: Apache-2.0 //! Host hygiene of a server process (design P3-T001 §3.6, threat model -//! T-A1, T-H1, T-H3): which addresses need authentication, the root check -//! and core dumps off. Unix only; elsewhere the checks pass and core dumps +//! T-H1, T-H3): the root check and core dumps off. Which addresses need +//! authentication is `krishiv_session::auth::check_listener`. Unix only; elsewhere the checks pass and core dumps //! are left as they are. -use std::net::IpAddr; - -/// Whether binding `address` without authentication is refused: any address -/// that is not loopback (ADR 0046 §6). `0.0.0.0` and `::` (every interface) -/// are not loopback. -#[must_use] -pub fn needs_auth(address: IpAddr) -> bool { - !address.is_loopback() -} - /// Whether the process runs with effective uid 0. #[must_use] pub fn is_root() -> bool { @@ -69,16 +59,6 @@ pub fn core_dump_limit() -> Option { mod tests { use super::*; - #[test] - fn only_loopback_binds_without_auth() { - for a in ["127.0.0.1", "127.1.2.3", "::1"] { - assert!(!needs_auth(a.parse().unwrap()), "{a}"); - } - for a in ["0.0.0.0", "::", "10.0.0.5", "192.168.1.1", "fe80::1"] { - assert!(needs_auth(a.parse().unwrap()), "{a}"); - } - } - // nextest runs each test in a process of its own, so this changes no // other test's limit. #[test] diff --git a/crates/krishiv-connect-server/src/lib.rs b/crates/krishiv-connect-server/src/lib.rs index 5d78971d4..56a0390e3 100644 --- a/crates/krishiv-connect-server/src/lib.rs +++ b/crates/krishiv-connect-server/src/lib.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 //! The Spark Connect gRPC server (layer L6, design P3-T001): the tonic -//! service over the generated stub, the session manager keyed by principal -//! and session id, the limits applied before admission, and graceful +//! service over the generated stub, the authentication of every RPC, the session manager keyed +//! by the authenticated caller and session id, the limits applied before admission, and graceful //! shutdown. Decoding and translation are `krishiv-connect`'s; the admin //! HTTP server is `krishiv-server`'s; the process around both is //! `krishiv server` (`krishiv-cli`). @@ -11,6 +11,7 @@ #![deny(clippy::disallowed_methods)] pub mod analyze; +pub mod auth; pub mod codec; pub mod config; pub mod execute; @@ -23,6 +24,9 @@ pub mod server; pub mod service; pub mod status; +pub use auth::{ + AuthLayer, Check, ConnectAuth, Rpc, SessionOwner, check_command, check_rpc, role_sessions, +}; pub use manager::{ManagerCounts, SessionEntry, SessionLimits, SessionManager}; pub use operation::{ExecuteLimits, Operation, Operations}; pub use server::{ConnectServer, ConnectServerConfig}; diff --git a/crates/krishiv-connect-server/src/manager.rs b/crates/krishiv-connect-server/src/manager.rs index 7d01a20c7..2016a7859 100644 --- a/crates/krishiv-connect-server/src/manager.rs +++ b/crates/krishiv-connect-server/src/manager.rs @@ -6,11 +6,28 @@ //! admission. //! //! Spark keys by `(UserContext.user_id, session_id)`. Krishiv keys by the -//! authenticated principal instead (ADR 0046 §5, threat model T-A8): the +//! authenticated caller instead (ADR 0046 §5, threat model T-A8): the //! client's `user_id` is recorded, unverified, and never decides anything. -//! A session id another principal holds is refused as unknown -//! (`INVALID_HANDLE.SESSION_NOT_FOUND`), the answer a lookup of an unknown -//! id gets. +//! The key is the caller's [`SessionOwner`] (method, principal kind, tenant +//! and name, without groups). A session id another owner holds is refused as +//! unknown (`INVALID_HANDLE.SESSION_NOT_FOUND`), the answer a lookup of an +//! unknown id gets, and writes a `krishiv::audit` denial record (ADR 0046 +//! §3b: no existence oracle, but the denial is recorded); so does a release +//! of such an id (a no-op) and, through +//! [`SessionManager::audit_foreign_operation`], an operation id another +//! owner's session holds. These records are not counted in +//! `krishiv_auth_denied_total`, which counts `KRISHIV_INSUFFICIENT_PERMISSIONS` +//! refusals only (§8). +//! +//! A session's policy is fixed when it is built, from the caller's role. A +//! caller whose role differs from the role its live session was built with +//! (a role list changed, an OIDC group claim changed) never gets that +//! session: it is closed, without a tombstone, and the lookup goes on as if +//! the id were free, so a create builds a new session under the new role and +//! a client that saw the old one gets `INVALID_HANDLE.SESSION_CHANGED`; an +//! RPC that creates nothing gets `INVALID_HANDLE.SESSION_NOT_FOUND`. Two +//! clients of one owner using one session id under different roles at once +//! replace each other's session on every request (each keeps its own role). //! //! No lock is held across an `await`, nor while a session is built or //! closed: the map lock guards lookups and inserts only. @@ -28,13 +45,15 @@ use krishiv_common::error::{ InvalidHandleSessionChanged, InvalidHandleSessionClosed, InvalidHandleSessionNotFound, KrishivError, KrishivSessionLimit, }; -use krishiv_common::principal::Principal; +use krishiv_session::auth::{Authenticated, Role, Surface}; use krishiv_session::{QueryId, Session}; +use crate::auth::SessionOwner; use crate::operation::{Operation, Operations}; -/// Builds the session a principal gets for a new session id. -pub type SessionFactory = dyn Fn(&Principal) -> Result + Send + Sync; +/// Builds the session an authenticated caller gets for a new session id: +/// its principal, and the policy of its role. +pub type SessionFactory = dyn Fn(&Authenticated) -> Result + Send + Sync; /// The manager's limits and timers. #[derive(Debug, Clone)] @@ -107,6 +126,7 @@ impl SessionLimits { #[derive(Debug)] pub struct SessionEntry { session: Session, + caller: Authenticated, client_user_id: String, operations: Arc, /// Milliseconds since the manager's epoch at the last lookup. @@ -128,6 +148,12 @@ impl SessionEntry { self.session.id() } + /// The caller the session was built for (its principal and role). + #[must_use] + pub fn caller(&self) -> &Authenticated { + &self.caller + } + /// The `UserContext.user_id` the client sent when it created the /// session: recorded, never trusted. #[must_use] @@ -161,7 +187,7 @@ pub struct ManagerCounts { #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct Key { - principal: Principal, + owner: SessionOwner, session_id: String, } @@ -175,9 +201,9 @@ struct OpKey { #[derive(Default)] struct Inner { live: HashMap>, - /// The principal holding each live session id. - owners: HashMap, - per_principal: HashMap, + /// The owner holding each live session id. + owners: HashMap, + per_principal: HashMap, closed: Tombstones, /// Abandoned operations (Spark's `abandonedTombstones`). abandoned: Tombstones, @@ -266,8 +292,9 @@ impl SessionManager { /// A manager whose sessions are new sessions of `base` (same runtime, /// profile and function registry; Spark's `baseSession.newSession()`). - /// Every principal gets `base`'s principal until authentication (T002) - /// supplies a factory that sets it. + /// Every caller gets `base`'s principal and policy: the server without + /// authentication (loopback only); with it, [`Self::new`] takes a factory + /// that builds each caller's session (`Session::new_session_as`). #[must_use] pub fn from_base(base: Session, limits: SessionLimits) -> Self { Self::new(Box::new(move |_| Ok(base.new_session())), limits) @@ -287,53 +314,95 @@ impl SessionManager { u64::try_from(self.epoch.elapsed().as_millis()).unwrap_or(u64::MAX) } - fn key(principal: &Principal, session_id: &str) -> Key { + fn key(caller: &Authenticated, session_id: &str) -> Key { Key { - principal: principal.clone(), + owner: SessionOwner::of(caller), session_id: session_id.to_owned(), } } - /// The session `session_id` of `principal`, created when missing (the + /// The live entry for `key` if it was built for `caller`'s role, stamped + /// as an access (under the lock, so a sweep cannot release it in + /// between). One built for another role leaves the map without a + /// tombstone and goes to `retired`, to be closed after the lock is + /// released (module docs). + fn live_locked( + &self, + inner: &mut Inner, + key: &Key, + caller: &Authenticated, + retired: &mut Option>, + ) -> Option> { + let entry = inner.live.get(key).map(Arc::clone)?; + if entry.caller.role == caller.role { + entry.last_access.store(self.now_ms(), Ordering::Relaxed); + return Some(entry); + } + *retired = Self::remove_locked(inner, key); + None + } + + /// Closes a session [`Self::live_locked`] retired. + fn close_retired(retired: Option>) { + if let Some(entry) = retired { + tracing::info!( + target: "krishiv::connect", + session = entry.server_side_id(), + "closed a Connect session: its caller's role changed" + ); + entry.close(); + } + } + + /// The session `session_id` of `caller`, created when missing (the /// `ExecutePlan`, `AnalyzePlan` and `Config` RPCs). /// /// # Errors /// `INVALID_HANDLE.FORMAT` for an id that is not a UUID (on create), /// `INVALID_HANDLE.SESSION_CLOSED` for a released id, - /// `INVALID_HANDLE.SESSION_NOT_FOUND` for an id another principal + /// `INVALID_HANDLE.SESSION_NOT_FOUND` for an id another owner /// holds, `INVALID_HANDLE.SESSION_CHANGED` when `observed` is not the /// session's server-side id, `KRISHIV_SESSION_LIMIT` past a session /// cap, and whatever building the session fails with. pub fn get_or_create( &self, - principal: &Principal, + caller: &Authenticated, session_id: &str, observed: Option<&str>, client_user_id: &str, ) -> Result, KrishivError> { - let key = Self::key(principal, session_id); - if let Some(entry) = self.lookup(&key, observed)? { + let key = Self::key(caller, session_id); + if let Some(entry) = self.lookup(&key, caller, observed)? { return Ok(entry); } // Validate before building: Spark's `validateSessionCreate`. QueryId::parse(session_id)?; - if let Some(existing) = self.check_create(&key)? { + if let Some(existing) = self.check_create(&key, caller)? { Self::check_observed(&key, &existing, observed)?; return Ok(existing); } // Built without the lock; a racing create of the same key wins and // this one is closed unused. - let session = (self.factory)(principal)?; + let session = (self.factory)(caller)?; + // `system.sessions` shows the client's `user_id` as reported (ADR 0046 §5: recorded, + // unverified, never the identity). + session.set_client_info( + krishiv_session::Transport::Connect, + None, + Some(client_user_id), + Some(session.id()), + ); let entry = Arc::new(SessionEntry { session, + caller: caller.clone(), client_user_id: client_user_id.to_owned(), operations: Arc::default(), last_access: AtomicU64::new(self.now_ms()), }); + let mut retired = None; let raced = { let mut inner = self.lock(); - if let Some(existing) = inner.live.get(&key).map(Arc::clone) { - existing.last_access.store(self.now_ms(), Ordering::Relaxed); + if let Some(existing) = self.live_locked(&mut inner, &key, caller, &mut retired) { Ok(Some(existing)) } else { match Self::check_create_locked(&mut inner, &key, &self.limits) { @@ -342,21 +411,19 @@ impl SessionManager { inner.live.insert(key.clone(), Arc::clone(&entry)); inner .owners - .insert(key.session_id.clone(), key.principal.clone()); - *inner - .per_principal - .entry(key.principal.clone()) - .or_insert(0) += 1; + .insert(key.session_id.clone(), key.owner.clone()); + *inner.per_principal.entry(key.owner.clone()).or_insert(0) += 1; Ok(None) } } } }; + Self::close_retired(retired); match raced { Err(e) => { // Built, then refused by a check that changed meanwhile. entry.session.close(); - Err(e) + Err(audit_foreign(caller, "session", e)) } Ok(Some(existing)) => { entry.session.close(); @@ -370,22 +437,23 @@ impl SessionManager { } } - /// The live session `session_id` of `principal` (every RPC that does not + /// The live session `session_id` of `caller` (every RPC that does not /// create one). /// /// # Errors /// `INVALID_HANDLE.SESSION_CLOSED` for a released id, /// `INVALID_HANDLE.SESSION_NOT_FOUND` for an unknown one (or one another - /// principal holds), `INVALID_HANDLE.SESSION_CHANGED` when `observed` - /// differs from its server-side id. + /// owner holds, or one built for another role of the caller), + /// `INVALID_HANDLE.SESSION_CHANGED` when `observed` differs from its + /// server-side id. pub fn get( &self, - principal: &Principal, + caller: &Authenticated, session_id: &str, observed: Option<&str>, ) -> Result, KrishivError> { - let key = Self::key(principal, session_id); - if let Some(entry) = self.lookup(&key, observed)? { + let key = Self::key(caller, session_id); + if let Some(entry) = self.lookup(&key, caller, observed)? { return Ok(entry); } let closed = self.lock().closed.contains(&key); @@ -407,31 +475,39 @@ impl SessionManager { #[must_use] pub fn get_if_present( &self, - principal: &Principal, + caller: &Authenticated, session_id: &str, ) -> Option> { - let key = Self::key(principal, session_id); - let inner = self.lock(); - let entry = inner.live.get(&key).map(Arc::clone)?; - // Stamped under the lock, so a sweep cannot release it in between. - entry.last_access.store(self.now_ms(), Ordering::Relaxed); - Some(entry) + let key = Self::key(caller, session_id); + let mut retired = None; + let entry = self.live_locked(&mut self.lock(), &key, caller, &mut retired); + Self::close_retired(retired); + entry } - /// Releases session `session_id` of `principal` (`ReleaseSession`): it + /// Releases session `session_id` of `caller` (`ReleaseSession`): it /// leaves the map, its id is tombstoned unless `allow_reconnect`, its /// operations are cancelled and it is closed (its queries cancelled). An unknown id is a no-op. Returns /// the released session's server-side id. pub fn release( &self, - principal: &Principal, + caller: &Authenticated, session_id: &str, allow_reconnect: bool, ) -> Option { - let key = Self::key(principal, session_id); + // Keyed by the owner alone: releasing ends the session whatever role + // it was built with. + let key = Self::key(caller, session_id); let entry = { let mut inner = self.lock(); - let entry = Self::remove_locked(&mut inner, &key)?; + let Some(entry) = Self::remove_locked(&mut inner, &key) else { + let foreign = Self::foreign_locked(&inner, &key); + drop(inner); + if foreign { + write_foreign_record(caller, "session"); + } + return None; + }; if !allow_reconnect { inner.closed.insert(key.clone(), self.limits.tombstones); } @@ -460,7 +536,7 @@ impl SessionManager { #[must_use] pub fn is_abandoned( &self, - principal: &Principal, + caller: &Authenticated, session_id: &str, operation_id: &str, ) -> bool { @@ -468,7 +544,7 @@ impl SessionManager { return false; }; let key = OpKey { - session: Self::key(principal, session_id), + session: Self::key(caller, session_id), operation, }; self.lock().abandoned.contains(&key) @@ -617,41 +693,68 @@ impl SessionManager { } } - /// The live entry for `key`, checked against `observed`. + /// The live entry for `key` built for `caller`'s role, checked against + /// `observed`. fn lookup( &self, key: &Key, + caller: &Authenticated, observed: Option<&str>, ) -> Result>, KrishivError> { - let entry = { - let inner = self.lock(); - match inner.live.get(key) { - Some(e) => { - // Stamped under the lock, so a sweep cannot release it - // in between. - e.last_access.store(self.now_ms(), Ordering::Relaxed); - Arc::clone(e) - } - None => { - // Held by another principal: refused as unknown. - if inner - .owners - .get(&key.session_id) - .is_some_and(|p| *p != key.principal) - { - return Err(InvalidHandleSessionNotFound { - handle: key.session_id.clone(), - } - .into()); - } - return Ok(None); + let mut retired = None; + let found = { + let mut inner = self.lock(); + match self.live_locked(&mut inner, key, caller, &mut retired) { + Some(e) => Ok(Some(e)), + // Held by another owner: refused as unknown. + None if Self::foreign_locked(&inner, key) => { + Err(KrishivError::from(InvalidHandleSessionNotFound { + handle: key.session_id.clone(), + })) } + None => Ok(None), } }; + Self::close_retired(retired); + let Some(entry) = found.map_err(|e| audit_foreign(caller, "session", e))? else { + return Ok(None); + }; Self::check_observed(key, &entry, observed)?; Ok(Some(entry)) } + /// Whether another owner holds `key`'s session id. + fn foreign_locked(inner: &Inner, key: &Key) -> bool { + inner + .owners + .get(&key.session_id) + .is_some_and(|o| *o != key.owner) + } + + /// Writes the denial record of ADR 0046 §3b when `operation_id`, unknown in + /// `caller`'s session, is a live operation of another owner's session (the + /// caller is answered as for an unknown id either way: `ReattachExecute`, + /// `ReleaseExecute`, `Interrupt` by id). It looks through every live + /// session (at most `krishiv.connect.maxSessions`), and runs on every + /// unknown operation id, which includes a normal client's reattach or + /// release after its operation finished. + pub fn audit_foreign_operation(&self, caller: &Authenticated, operation_id: &str) { + let owner = SessionOwner::of(caller); + let others: Vec> = self + .lock() + .live + .iter() + .filter(|(k, _)| k.owner != owner) + .map(|(_, e)| Arc::clone(e)) + .collect(); + if others + .iter() + .any(|e| e.operations().get(operation_id).is_some()) + { + write_foreign_record(caller, "operation"); + } + } + fn check_observed( key: &Key, entry: &SessionEntry, @@ -669,13 +772,21 @@ impl SessionManager { /// The caps, checked before a session is built. A racing create that /// inserted the key since the lookup is returned instead: reusing it /// takes no slot, so the caps must not refuse it. - fn check_create(&self, key: &Key) -> Result>, KrishivError> { - let mut inner = self.lock(); - if let Some(existing) = inner.live.get(key).map(Arc::clone) { - existing.last_access.store(self.now_ms(), Ordering::Relaxed); - return Ok(Some(existing)); - } - Self::check_create_locked(&mut inner, key, &self.limits).map(|()| None) + fn check_create( + &self, + key: &Key, + caller: &Authenticated, + ) -> Result>, KrishivError> { + let mut retired = None; + let checked = { + let mut inner = self.lock(); + match self.live_locked(&mut inner, key, caller, &mut retired) { + Some(existing) => Ok(Some(existing)), + None => Self::check_create_locked(&mut inner, key, &self.limits).map(|()| None), + } + }; + Self::close_retired(retired); + checked.map_err(|e| audit_foreign(caller, "session", e)) } fn check_create_locked( @@ -691,11 +802,7 @@ impl SessionManager { } .into()); } - if inner - .owners - .get(&key.session_id) - .is_some_and(|p| *p != key.principal) - { + if Self::foreign_locked(inner, key) { return Err(InvalidHandleSessionNotFound { handle: key.session_id.clone(), } @@ -708,11 +815,7 @@ impl SessionManager { } .into()); } - if inner - .per_principal - .get(&key.principal) - .copied() - .unwrap_or(0) + if inner.per_principal.get(&key.owner).copied().unwrap_or(0) >= limits.max_sessions_per_principal { return Err(KrishivSessionLimit { @@ -727,15 +830,43 @@ impl SessionManager { fn remove_locked(inner: &mut Inner, key: &Key) -> Option> { let entry = inner.live.remove(key)?; inner.owners.remove(&key.session_id); - if let Some(n) = inner.per_principal.get_mut(&key.principal) { + if let Some(n) = inner.per_principal.get_mut(&key.owner) { *n = n.saturating_sub(1); if *n == 0 { - inner.per_principal.remove(&key.principal); + inner.per_principal.remove(&key.owner); } } Some(entry) } } +/// `error` unchanged, after the ADR 0046 §3b denial record when it is the +/// `INVALID_HANDLE.SESSION_NOT_FOUND` of an id another owner holds (the only +/// `SESSION_NOT_FOUND` [`SessionManager::lookup`] and the create checks give). +fn audit_foreign( + caller: &Authenticated, + handle: &'static str, + error: KrishivError, +) -> KrishivError { + if error.error_class().name() == "INVALID_HANDLE.SESSION_NOT_FOUND" { + write_foreign_record(caller, handle); + } + error +} + +/// The `krishiv::audit` record of a refused id another principal holds: who +/// asked and what kind of id, never the id's owner. +fn write_foreign_record(caller: &Authenticated, handle: &'static str) { + tracing::warn!( + target: "krishiv::audit", + principal = %caller.principal.name, + method = caller.method.as_str(), + role = caller.role.map_or("none", Role::as_str), + surface = Surface::Connect.as_str(), + handle, + "refused a Connect id another principal holds, answered as unknown" + ); +} + #[cfg(test)] mod tests; diff --git a/crates/krishiv-connect-server/src/manager/tests.rs b/crates/krishiv-connect-server/src/manager/tests.rs index d4171bc05..f726ba5c0 100644 --- a/crates/krishiv-connect-server/src/manager/tests.rs +++ b/crates/krishiv-connect-server/src/manager/tests.rs @@ -4,18 +4,27 @@ use std::time::Duration; use krishiv_common::principal::{Principal, PrincipalKind}; use krishiv_session::Session; +use krishiv_session::auth::{Authenticated, Method, Role}; use super::{SessionLimits, SessionManager}; const ID: &str = "8f0e4c55-5ad0-4d1e-9a3c-3a1b0b2c7d10"; const ID2: &str = "1d6a3c1e-77b2-4c55-8d0f-0a6c2e9b4f21"; -fn alice() -> Principal { - Principal::new("alice", PrincipalKind::User) +fn user(name: &str) -> Authenticated { + Authenticated::new( + Principal::new(name, PrincipalKind::User), + Some(Role::Operator), + Method::Token, + ) +} + +fn alice() -> Authenticated { + user("alice") } -fn bob() -> Principal { - Principal::new("bob", PrincipalKind::User) +fn bob() -> Authenticated { + user("bob") } fn manager(limits: SessionLimits) -> SessionManager { @@ -167,7 +176,7 @@ fn session_caps_refuse_before_building() { ); m.get_or_create(&bob(), ID2, None, "u") .expect("bob's first"); - let carol = Principal::new("carol", PrincipalKind::User); + let carol = user("carol"); let e = m .get_or_create(&carol, "2b1c6e0a-0f3d-4f7e-9a51-7c6d5e4f3a21", None, "u") .unwrap_err(); @@ -360,6 +369,117 @@ fn a_session_with_a_live_operation_is_busy_and_never_swept() { assert_eq!(m.sweep(), 1); } +fn with_groups(mut caller: Authenticated, groups: &[&str]) -> Authenticated { + caller.principal.groups = groups.iter().map(|g| (*g).to_owned()).collect(); + caller +} + +fn with_role(caller: &Authenticated, role: Option) -> Authenticated { + Authenticated::new(caller.principal.clone(), role, caller.method) +} + +#[test] +fn sessions_are_bound_without_groups() { + // An OIDC user whose group claim changes between two tokens keeps its session (design + // P3-T002 §7): groups are not part of the owner. + let m = manager(SessionLimits::default()); + let first = with_groups(alice(), &["etl"]); + let a = m.get_or_create(&first, ID, None, "u").expect("create"); + let b = m + .get( + &with_groups(alice(), &["etl", "bi"]), + ID, + Some(a.server_side_id()), + ) + .expect("same owner, other groups"); + assert!(Arc::ptr_eq(&a, &b)); + assert_eq!(m.counts().live, 1); +} + +#[test] +fn the_method_separates_owners_with_the_same_name() { + // A static token and an OIDC subject both named `alice` are two owners: neither reaches + // the other's session. + let m = manager(SessionLimits::default()); + let token = alice(); + let oidc = Authenticated::new(token.principal.clone(), token.role, Method::Oidc); + m.get_or_create(&token, ID, None, "u").expect("create"); + let e = m.get(&oidc, ID, None).unwrap_err(); + assert_eq!(class(&e), "INVALID_HANDLE.SESSION_NOT_FOUND"); + let e = m.get_or_create(&oidc, ID, None, "u").unwrap_err(); + assert_eq!(class(&e), "INVALID_HANDLE.SESSION_NOT_FOUND"); + assert_eq!(m.counts().live, 1); +} + +#[test] +fn a_changed_role_replaces_the_session() { + // The policy is fixed when a session is built: a caller whose role changed never gets the + // old session. A create builds a new one; a client that saw the old one is told it changed. + let m = manager(SessionLimits::default()); + let operator = alice(); + let old = m.get_or_create(&operator, ID, None, "u").expect("create"); + let viewer = with_role(&operator, Some(Role::Viewer)); + let e = m + .get_or_create(&viewer, ID, Some(old.server_side_id()), "u") + .unwrap_err(); + assert_eq!(class(&e), "INVALID_HANDLE.SESSION_CHANGED"); + assert!(old.session().is_closed(), "the old session is closed"); + let new = m.get(&viewer, ID, None).expect("the new session"); + assert_ne!(new.server_side_id(), old.server_side_id()); + assert_eq!(new.caller().role, Some(Role::Viewer)); + assert_eq!(m.counts().live, 1); + assert_eq!( + m.counts().tombstones, + 0, + "a replaced session leaves no tombstone" + ); +} + +#[test] +fn a_lookup_with_a_changed_role_finds_nothing() { + let m = manager(SessionLimits::default()); + let old = m.get_or_create(&alice(), ID, None, "u").expect("create"); + let e = m.get(&with_role(&alice(), None), ID, None).unwrap_err(); + assert_eq!(class(&e), "INVALID_HANDLE.SESSION_NOT_FOUND"); + assert!(old.session().is_closed()); + assert_eq!(m.counts().live, 0); + assert!(m.get_if_present(&alice(), ID).is_none()); +} + +#[test] +fn release_ends_the_session_whatever_its_role() { + let m = manager(SessionLimits::default()); + let s = m.get_or_create(&alice(), ID, None, "u").expect("create"); + let released = m.release(&with_role(&alice(), Some(Role::Admin)), ID, false); + assert_eq!(released.as_deref(), Some(s.server_side_id())); + assert!(s.session().is_closed()); + assert_eq!(m.counts().live, 0); +} + +#[test] +fn the_factory_gets_the_caller() { + let seen = Arc::new(std::sync::Mutex::new(Vec::new())); + let base = Session::new().expect("session"); + let log = Arc::clone(&seen); + let m = SessionManager::new( + Box::new(move |caller: &Authenticated| { + log.lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push((caller.principal.name.clone(), caller.role)); + Ok(base.new_session()) + }), + SessionLimits::default(), + ); + let s = m.get_or_create(&bob(), ID, None, "u").expect("create"); + assert_eq!(s.caller(), &bob()); + assert_eq!( + *seen + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner), + vec![("bob".to_owned(), Some(Role::Operator))] + ); +} + #[test] fn an_ended_detached_operation_does_not_keep_shutdown_waiting() { // Review M1 (PR-15a): a reattachable operation that finished and whose diff --git a/crates/krishiv-connect-server/src/server.rs b/crates/krishiv-connect-server/src/server.rs index 7c0d6b67e..f24b515a4 100644 --- a/crates/krishiv-connect-server/src/server.rs +++ b/crates/krishiv-connect-server/src/server.rs @@ -7,6 +7,12 @@ //! RPCs get `UNAVAILABLE`, no new connection is accepted), running queries //! get up to [`ConnectServerConfig::shutdown_timeout`] to finish, then every //! session is closed, which cancels what is left. +//! +//! TLS ([`ConnectServer::with_tls`], design P3-T002 §3.5): each accepted +//! connection finishes its handshake on a task of its own within +//! [`ConnectServerConfig::handshake_timeout`] before tonic sees it; the +//! verified client certificate rides on the connection ([`PeerInfo`]) for +//! the authenticator. use std::future::Future; use std::io; @@ -28,9 +34,13 @@ use krishiv_common::error::KrishivError; use tokio::io::{AsyncRead, AsyncWrite, ReadBuf}; use tokio::net::{TcpListener, TcpStream}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; +use tokio_rustls::TlsAcceptor; +use tokio_rustls::rustls::ServerConfig; +use tokio_rustls::server::TlsStream; use tonic::transport::Server; -use tonic::transport::server::{Connected, TcpConnectInfo}; +use tonic::transport::server::Connected; +use crate::auth::{AuthLayer, ConnectAuth}; use crate::execute::ResponseBudget; use crate::manager::SessionManager; use crate::operation::ExecuteLimits; @@ -167,11 +177,14 @@ pub struct ConnectServer { manager: Arc, draining: Arc, config: ConnectServerConfig, + auth: Arc, + tls: Option>, budget: ResponseBudget, } impl ConnectServer { - /// A server over `manager`. + /// A server over `manager`, with authentication off (loopback only: + /// `krishiv server` refuses another address without it). #[must_use] pub fn new(manager: Arc, config: ConnectServerConfig) -> Self { Self { @@ -179,9 +192,30 @@ impl ConnectServer { draining: Arc::new(AtomicBool::new(false)), budget: ResponseBudget::new(config.execute.max_response_bytes), config, + auth: Arc::new(ConnectAuth::off()), + tls: None, } } + /// This server speaking TLS with `config` (`krishiv_session::auth::ServerTls`): every + /// connection completes a TLS handshake before its first request. The ALPN list is set to + /// `h2` (gRPC); the certificate resolver is shared, so a reload of the server's TLS + /// material reaches new handshakes here too. + #[must_use] + pub fn with_tls(mut self, config: &ServerConfig) -> Self { + let mut config = config.clone(); + config.alpn_protocols = vec![b"h2".to_vec()]; + self.tls = Some(Arc::new(config)); + self + } + + /// This server authenticating every RPC with `auth` (P3-T002). + #[must_use] + pub fn with_auth(mut self, auth: ConnectAuth) -> Self { + self.auth = Arc::new(auth); + self + } + /// The server's response budget (`krishiv.connect.maxResponseBytes`), /// shared with the service it serves. #[must_use] @@ -222,10 +256,13 @@ impl ConnectServer { manager, draining, config, + auth, + tls, budget, } = self; config.execute.validate()?; let service = ConnectService::new(Arc::clone(&manager), Arc::clone(&draining)) + .with_auth(Arc::clone(&auth)) .with_limits(config.execute.clone()) .with_budget(budget); let service = SparkConnectServiceServer::from_arc(Arc::new(service)) @@ -243,6 +280,8 @@ impl ConnectServer { .http2_keepalive_interval(keepalive_interval) .http2_keepalive_timeout(keepalive_timeout) .tcp_nodelay(true) + // Authentication runs on the headers, before the body is decoded. + .layer(AuthLayer::new(Arc::clone(&auth))) .add_service(service); // Aborted on drop, so a caller that cancels `serve` leaves no task @@ -256,7 +295,12 @@ impl ConnectServer { config.execute.maintenance_interval, config.execute.detached_timeout, ))); - let incoming = capped(listener, config.max_connections, config.handshake_timeout); + let incoming = capped( + listener, + config.max_connections, + config.handshake_timeout, + tls.map(TlsAcceptor::from), + ); let (stop_tx, stop_rx) = tokio::sync::oneshot::channel::<()>(); let server = router.serve_with_incoming_shutdown(incoming, async { let _ = stop_rx.await; @@ -362,15 +406,19 @@ async fn abandon_loop(manager: Arc, every: Duration, timeout: Du /// first byte only: a client that sends one byte and stalls keeps its slot /// until the established-connection idle timeout exists (T011a). Accepting runs on a task of its own, aborted when the /// stream is dropped; each new connection waits for its first byte on a -/// task of its own, so one slow client delays no other. +/// task of its own, so one slow client delays no other. With `tls`, the +/// whole TLS handshake must also finish within `handshake`. fn capped( listener: TcpListener, max: usize, handshake: Duration, + tls: Option, ) -> impl futures::Stream> + Send + 'static { let permits = Arc::new(Semaphore::new(max.clamp(1, Semaphore::MAX_PERMITS))); let (tx, rx) = tokio::sync::mpsc::channel::>(64); - let acceptor = AbortOnDrop(tokio::spawn(accept_loop(listener, permits, tx, handshake))); + let acceptor = AbortOnDrop(tokio::spawn(accept_loop( + listener, permits, tx, handshake, tls, + ))); futures::stream::unfold((rx, acceptor), |(mut rx, acceptor)| async move { rx.recv().await.map(|item| (item, (rx, acceptor))) }) @@ -381,15 +429,16 @@ async fn accept_loop( permits: Arc, tx: tokio::sync::mpsc::Sender>, handshake: Duration, + tls: Option, ) { let mut backoff = Duration::from_millis(10); loop { let Ok(permit) = Arc::clone(&permits).acquire_owned().await else { return; }; - let stream = loop { + let (stream, remote) = loop { match listener.accept().await { - Ok((stream, _)) => break stream, + Ok(accepted) => break accepted, // A failed accept (the peer reset before it was taken, too // many open files) does not stop the server; back off, as // every retry fails while file descriptors are out. @@ -403,26 +452,53 @@ async fn accept_loop( backoff = Duration::from_millis(10); let _ = stream.set_nodelay(true); let sender = tx.clone(); + let tls = tls.clone(); tokio::spawn(async move { - let mut first = [0_u8; 1]; + let opened = async { + let mut first = [0_u8; 1]; + match stream.peek(&mut first).await { + Ok(n) if n > 0 => {} + // Closed or failed. + _ => return None, + } + Some(match tls { + None => (Conn::Plain(stream), None), + Some(acceptor) => match acceptor.accept(stream).await { + Ok(tls) => { + let leaf = verified_leaf(&tls); + (Conn::Tls(Box::new(tls)), leaf) + } + Err(e) => { + tracing::debug!( + target: "krishiv::connect", + peer = %remote, + error = %e, + "TLS handshake failed" + ); + return None; + } + }, + }) + }; // A zero deadline means none (as `0s` for the session idle // timeout). - let peeked = if handshake.is_zero() { - Ok(stream.peek(&mut first).await) + let opened = if handshake.is_zero() { + Ok(opened.await) } else { - tokio::time::timeout(handshake, stream.peek(&mut first)).await + tokio::time::timeout(handshake, opened).await }; - match peeked { - Ok(Ok(n)) if n > 0 => { - let _ = sender - .send(Ok(CappedStream { - inner: stream, - _permit: permit, - })) - .await; - } - // Silent, closed or failed: drop it and its permit. - _ => {} + // Silent, closed, failed or a failed handshake: drop it and its permit. + if let Ok(Some((inner, leaf))) = opened { + let _ = sender + .send(Ok(CappedStream { + inner, + info: PeerInfo { + remote: Some(remote), + leaf, + }, + _permit: permit, + })) + .await; } }); if tx.is_closed() { @@ -431,28 +507,71 @@ async fn accept_loop( } } +/// The verified client leaf certificate of a TLS connection (rustls checked +/// its chain against the client CA at the handshake), if the client sent one. +fn verified_leaf(tls: &TlsStream) -> Option> { + tls.get_ref() + .1 + .peer_certificates() + .and_then(<[_]>::first) + .map(|c| Arc::from(c.as_ref())) +} + +/// What a connection tells the authenticator: the peer's address (the +/// throttle's key) and, over TLS, the client certificate rustls verified. +#[derive(Debug, Clone, Default)] +pub struct PeerInfo { + /// The peer's address. + pub remote: Option, + /// The verified client leaf certificate (DER), if the client sent one. + pub leaf: Option>, +} + +/// A plain or TLS connection. +#[derive(Debug)] +enum Conn { + Plain(TcpStream), + Tls(Box>), +} + /// A connection that holds one of the listener's permits until it closes. #[derive(Debug)] pub struct CappedStream { - inner: TcpStream, + inner: Conn, + info: PeerInfo, _permit: OwnedSemaphorePermit, } impl Connected for CappedStream { - type ConnectInfo = TcpConnectInfo; + type ConnectInfo = PeerInfo; - fn connect_info(&self) -> TcpConnectInfo { - self.inner.connect_info() + fn connect_info(&self) -> PeerInfo { + self.info.clone() } } +macro_rules! conn { + ($self:expr, $s:ident => $e:expr) => { + match &mut $self.inner { + Conn::Plain($s) => { + let $s = Pin::new($s); + $e + } + Conn::Tls($s) => { + let $s = Pin::new($s.as_mut()); + $e + } + } + }; +} + impl AsyncRead for CappedStream { fn poll_read( mut self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut ReadBuf<'_>, ) -> Poll> { - Pin::new(&mut self.inner).poll_read(cx, buf) + conn!(self, s => s.poll_read(cx, buf)) } } @@ -462,15 +581,15 @@ impl AsyncWrite for CappedStream { cx: &mut Context<'_>, buf: &[u8], ) -> Poll> { - Pin::new(&mut self.inner).poll_write(cx, buf) + conn!(self, s => s.poll_write(cx, buf)) } fn poll_flush(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - Pin::new(&mut self.inner).poll_flush(cx) + conn!(self, s => s.poll_flush(cx)) } fn poll_shutdown(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - Pin::new(&mut self.inner).poll_shutdown(cx) + conn!(self, s => s.poll_shutdown(cx)) } fn poll_write_vectored( @@ -478,10 +597,13 @@ impl AsyncWrite for CappedStream { cx: &mut Context<'_>, bufs: &[io::IoSlice<'_>], ) -> Poll> { - Pin::new(&mut self.inner).poll_write_vectored(cx, bufs) + conn!(self, s => s.poll_write_vectored(cx, bufs)) } fn is_write_vectored(&self) -> bool { - self.inner.is_write_vectored() + match &self.inner { + Conn::Plain(s) => s.is_write_vectored(), + Conn::Tls(s) => s.is_write_vectored(), + } } } diff --git a/crates/krishiv-connect-server/src/service.rs b/crates/krishiv-connect-server/src/service.rs index c90e60b65..825df807f 100644 --- a/crates/krishiv-connect-server/src/service.rs +++ b/crates/krishiv-connect-server/src/service.rs @@ -4,7 +4,8 @@ //! implementation of it, [`ConnectService`]. //! //! T001 implements the session RPCs (`ReleaseSession`) and what every RPC -//! shares (the principal, the draining refusal). T011a (PR-11a) adds +//! shares (the draining refusal); T002 authenticates every RPC, the +//! unimplemented ones too (`crate::auth`). T011a (PR-11a) adds //! `ExecutePlan` (`crate::execute`), `ReattachExecute` and `ReleaseExecute` //! (with the replay buffer, sender caps and detach of PR-15a, design //! P3-T011a §3.8). PR-11b adds `AnalyzePlan` (`crate::analyze`), `Config` @@ -18,13 +19,14 @@ use krishiv_common::error::{ ConnectInvalidPlanInvalidOneOfFieldNotSet, InvalidCursorNotReattachable, InvalidHandleOperationAbandoned, InvalidHandleOperationNotFound, KrishivError, }; -use krishiv_common::principal::Principal; use krishiv_connect::proto::v4_3 as pb; use krishiv_connect::proto::v4_3::release_execute_request::Release; use krishiv_connect::wire::Decoded; +use krishiv_session::auth::Authenticated; use tonic::codegen::BoxStream; use tonic::{Request, Response, Status, Streaming}; +use crate::auth::{Check, ConnectAuth, Rpc, check_rpc}; use crate::codec::WireCodec; use crate::execute::ResponseBudget; use crate::manager::{SessionEntry, SessionManager}; @@ -45,41 +47,37 @@ mod generated { pub use generated::{SparkConnectService, SparkConnectServiceServer}; -/// The principal a request runs as: set by the authentication layer (T002) -/// as a request extension; until then the server's local user, on loopback -/// only (design P3-T001 §3.6). -fn principal_of(request: &Request, default: &Principal) -> Principal { - request - .extensions() - .get::() - .cloned() - .unwrap_or_else(|| default.clone()) -} - /// Krishiv's Spark Connect service over a [`SessionManager`]. #[derive(Debug)] pub struct ConnectService { manager: Arc, draining: Arc, - local: Principal, + auth: Arc, limits: ExecuteLimits, budget: ResponseBudget, } impl ConnectService { - /// A service over `manager`; new RPCs are refused with `UNAVAILABLE` - /// once `draining` is set (shutdown). + /// A service over `manager`, with authentication off; new RPCs are + /// refused with `UNAVAILABLE` once `draining` is set (shutdown). #[must_use] pub fn new(manager: Arc, draining: Arc) -> Self { Self { manager, draining, - local: Principal::local_user(), + auth: Arc::new(ConnectAuth::off()), budget: ResponseBudget::new(ExecuteLimits::default().max_response_bytes), limits: ExecuteLimits::default(), } } + /// This service authenticating every RPC with `auth`. + #[must_use] + pub fn with_auth(mut self, auth: Arc) -> Self { + self.auth = auth; + self + } + /// This service with the `ExecutePlan` limits `limits` (and a response /// budget of `limits.max_response_bytes`). #[must_use] @@ -119,26 +117,48 @@ impl ConnectService { } } - /// The session `session_id` of `principal`, created if needed, its - /// server-side id checked against the client's (the get-or-create RPCs - /// of design P3-T011a §2.1). + /// Admits `request` and returns its caller (authenticated by + /// [`crate::auth::AuthLayer`] before the body was read), after the + /// ADR 0046 §3b class of `rpc`. A class that needs more than the + /// session lookup or the analyzer (a privilege, the write path, a side + /// effect) is refused until the RPC that needs it applies it: fail + /// closed, so a newly implemented RPC cannot skip its check. + fn caller(&self, request: &Request, rpc: Rpc) -> Result { + self.admit()?; + let caller = self.auth.caller(request)?; + match check_rpc(rpc) { + Check::OwnSession | Check::Plan => Ok(caller), + Check::Privilege { .. } | Check::Write | Check::SideEffect | Check::Unsupported(_) => { + Err(Status::unimplemented(format!( + "{rpc:?} is not supported by this server yet" + ))) + } + } + } + + /// The session `session_id` of `caller`, created if needed, its server-side id checked + /// against the client's (the get-or-create RPCs of design P3-T011a §2.1). The client's + /// `user_id` is recorded, never trusted (ADR 0046 §5). fn session( &self, - principal: &Principal, + caller: &Authenticated, session_id: &str, observed: Option<&str>, user: Option<&pb::UserContext>, ) -> Result, Status> { let user = user.map_or("", |u| u.user_id.as_str()); self.manager - .get_or_create(principal, session_id, observed, user) + .get_or_create(caller, session_id, observed, user) .map_err(|e| status_of(&e)) } - fn unimplemented(&self, rpc: &str) -> Status { - match self.admit() { + /// `UNIMPLEMENTED`, once `request` is admitted and authenticated: an + /// unauthenticated client learns nothing about what the server + /// implements. + fn unimplemented(&self, request: &Request, rpc: Rpc) -> Status { + match self.caller(request, rpc) { Err(s) => s, - Ok(()) => Status::unimplemented(format!("{rpc} is not supported by this server yet")), + Ok(_) => Status::unimplemented(format!("{rpc:?} is not supported by this server yet")), } } } @@ -154,11 +174,10 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::ExecutePlan)?; let request = request.into_inner(); let entry = self.session( - &principal, + &caller, &request.session_id, request.client_observed_server_side_session_id.as_deref(), request.user_context.as_ref(), @@ -167,9 +186,7 @@ impl SparkConnectService for ConnectService { // lines 105-120); a live one is attached to by `execute_plan`. if let Some(id) = &request.operation_id && entry.operations().get(id).is_none() - && self - .manager - .is_abandoned(&principal, &request.session_id, id) + && self.manager.is_abandoned(&caller, &request.session_id, id) { return Err(status_of( &InvalidHandleOperationAbandoned { handle: id.clone() }.into(), @@ -187,11 +204,10 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::AnalyzePlan)?; let request = request.into_inner(); let entry = self.session( - &principal, + &caller, &request.session_id, request.client_observed_server_side_session_id.as_deref(), request.user_context.as_ref(), @@ -208,32 +224,32 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::Config)?; let request = request.into_inner(); let entry = self.session( - &principal, + &caller, &request.session_id, request.client_observed_server_side_session_id.as_deref(), request.user_context.as_ref(), )?; - config::config(&entry, &request, &principal, config::role_of(&principal)) + config::config(&entry, &request) + .await .map(Response::new) .map_err(|e| status_of(&e)) } async fn add_artifacts( &self, - _request: Request>>, + request: Request>>, ) -> Result, Status> { - Err(self.unimplemented("AddArtifacts")) + Err(self.unimplemented(&request, Rpc::AddArtifacts)) } async fn artifact_status( &self, - _request: Request>, + request: Request>, ) -> Result, Status> { - Err(self.unimplemented("ArtifactStatus")) + Err(self.unimplemented(&request, Rpc::ArtifactStatus)) } /// Spark's `SparkConnectInterruptHandler`: the session is looked up or @@ -242,18 +258,24 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::Interrupt)?; let request = request.into_inner(); let entry = self.session( - &principal, + &caller, &request.session_id, request.client_observed_server_side_session_id.as_deref(), request.user_context.as_ref(), )?; - interrupt::interrupt(&entry, &request) - .map(Response::new) - .map_err(|e| status_of(&e)) + let response = interrupt::interrupt(&entry, &request).map_err(|e| status_of(&e))?; + // An id it did not interrupt that is no operation of this session may be another + // owner's: answered the same (an empty list), recorded (ADR 0046 §3b). + if let Some(pb::interrupt_request::Interrupt::OperationId(id)) = &request.interrupt + && response.interrupted_ids.is_empty() + && entry.operations().get(id).is_none() + { + self.manager.audit_foreign_operation(&caller, id); + } + Ok(Response::new(response)) } /// Spark's `SparkConnectReattachExecuteHandler` (v4.3.0 lines 30-66) @@ -269,13 +291,12 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::ReattachExecute)?; let request = request.into_inner(); let entry = self .manager .get( - &principal, + &caller, &request.session_id, request.client_observed_server_side_session_id.as_deref(), ) @@ -285,10 +306,13 @@ impl SparkConnectService for ConnectService { let error: KrishivError = if self .manager - .is_abandoned(&principal, &request.session_id, &request.operation_id) + .is_abandoned(&caller, &request.session_id, &request.operation_id) { InvalidHandleOperationAbandoned { handle }.into() } else { + // Answered as unknown, recorded if another owner's (ADR 0046 §3b). + self.manager + .audit_foreign_operation(&caller, &request.operation_id); InvalidHandleOperationNotFound { handle }.into() }; return Err(status_of(&error)); @@ -321,19 +345,25 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::ReleaseExecute)?; let request = request.into_inner(); let entry = self .manager - .get(&principal, &request.session_id, None) + .get(&caller, &request.session_id, None) .map_err(|e| status_of(&e))?; let mut response = pb::ReleaseExecuteResponse { session_id: request.session_id.clone(), server_side_session_id: entry.server_side_id().to_owned(), operation_id: None, }; - if let Some(op) = entry.operations().get(&request.operation_id) { + let found = entry.operations().get(&request.operation_id); + if found.is_none() { + // Answered as unknown (success, no operation id), recorded if another owner's + // (ADR 0046 §3b). + self.manager + .audit_foreign_operation(&caller, &request.operation_id); + } + if let Some(op) = found { if !op.reattachable() { return Err(status_of(&InvalidCursorNotReattachable {}.into())); } @@ -370,12 +400,11 @@ impl SparkConnectService for ConnectService { &self, request: Request>, ) -> Result, Status> { - self.admit()?; - let principal = principal_of(&request, &self.local); + let caller = self.caller(&request, Rpc::ReleaseSession)?; let request = request.into_inner(); let server_side = self.manager - .release(&principal, &request.session_id, request.allow_reconnect); + .release(&caller, &request.session_id, request.allow_reconnect); Ok(Response::new(pb::ReleaseSessionResponse { session_id: request.session_id.clone(), server_side_session_id: server_side.unwrap_or_default(), @@ -384,22 +413,22 @@ impl SparkConnectService for ConnectService { async fn fetch_error_details( &self, - _request: Request>, + request: Request>, ) -> Result, Status> { - Err(self.unimplemented("FetchErrorDetails")) + Err(self.unimplemented(&request, Rpc::FetchErrorDetails)) } async fn clone_session( &self, - _request: Request>, + request: Request>, ) -> Result, Status> { - Err(self.unimplemented("CloneSession")) + Err(self.unimplemented(&request, Rpc::CloneSession)) } async fn get_status( &self, - _request: Request>, + request: Request>, ) -> Result, Status> { - Err(self.unimplemented("GetStatus")) + Err(self.unimplemented(&request, Rpc::GetStatus)) } } diff --git a/crates/krishiv-connect-server/tests/auth.rs b/crates/krishiv-connect-server/tests/auth.rs new file mode 100644 index 000000000..a09c199e4 --- /dev/null +++ b/crates/krishiv-connect-server/tests/auth.rs @@ -0,0 +1,853 @@ +// SPDX-License-Identifier: Apache-2.0 +//! The authenticated Connect listener end to end over a raw tonic client (P3-T002, design +//! §3.1, §3.6 and §4; ADR 0046 §3b): every RPC authenticates, sessions are the caller's, and a +//! command's class is checked against the caller's role before it is refused as unsupported. +#![cfg(unix)] +#![expect( + clippy::expect_used, + reason = "integration-test helpers are outside #[test] fns" +)] + +use std::collections::BTreeMap; +use std::path::Path; +use std::sync::Arc; + +use krishiv_common::config::Config; +use krishiv_common::principal::{Principal, PrincipalKind}; +use krishiv_connect::proto::v4_3 as pb; +use krishiv_connect::proto::v4_3::command::CommandType; +use krishiv_connect::proto::v4_3::plan::OpType; +use krishiv_connect::proto::v4_3::relation::RelType; +use krishiv_connect_server::{ + ConnectAuth, ConnectServer, ConnectServerConfig, SessionLimits, SessionManager, role_sessions, +}; +use krishiv_session::BoxFuture; +use krishiv_session::Privilege; +use krishiv_session::Session; +use krishiv_session::auth::{ + AuthConfig, AuthFailure, Authenticated, Authenticator, Credentials, Method, Role, ServerAuth, + Surface, +}; +use prost::Message; +use tokio::net::TcpListener; +use tokio::sync::oneshot; +use tonic::Code; +use tonic::transport::Channel; + +const SESSION: &str = "8f0e4c55-5ad0-4d1e-9a3c-3a1b0b2c7d10"; +const VIEWER: &str = "viewer-token-0123456789abcdefghijklmnopqrstuv"; +const ADMIN: &str = "admin-token-0123456789abcdefghijklmnopqrstuvw"; +const OPERATOR: &str = "operator-token-0123456789abcdefghijklmnopqrstu"; + +struct Running { + manager: Arc, + /// The denial counter the sessions and the no-role refusal count into. + denials: Arc, + channel: Channel, + _stop: oneshot::Sender<()>, + _dir: tempfile::TempDir, +} + +fn secret(dir: &Path, name: &str, token: &str) -> String { + use std::os::unix::fs::PermissionsExt; + let path = dir.join(name); + std::fs::write(&path, token).expect("write secret"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + format!("file:{}", path.display()) +} + +/// A server with three tokens, `carol` (viewer), `olga` (operator) and `dana` (admin), whose +/// sessions are built by the factory `krishiv server` uses (`role_sessions`), with local paths +/// confined to `/data`. +async fn start() -> Running { + let dir = tempfile::tempdir().expect("tempdir"); + let v = secret(dir.path(), "v", VIEWER); + let o = secret(dir.path(), "o", OPERATOR); + let a = secret(dir.path(), "a", ADMIN); + let config = AuthConfig::from_settings([ + ("krishiv.auth.tokens.v.secret", v.as_str()), + ("krishiv.auth.tokens.v.principal", "carol"), + ("krishiv.auth.tokens.v.role", "viewer"), + ("krishiv.auth.tokens.o.secret", o.as_str()), + ("krishiv.auth.tokens.o.principal", "olga"), + ("krishiv.auth.tokens.o.role", "operator"), + ("krishiv.auth.tokens.a.secret", a.as_str()), + ("krishiv.auth.tokens.a.principal", "dana"), + ("krishiv.auth.tokens.a.role", "admin"), + ]) + .expect("auth config"); + let auth = Arc::new(ServerAuth::new(&config).expect("auth")); + start_with(auth, dir).await +} + +async fn start_with(auth: Arc, dir: tempfile::TempDir) -> Running { + let data = dir.path().join("data"); + std::fs::create_dir(&data).expect("data dir"); + let mut config = Config::new(); + config + .set( + "krishiv.io.local.allowedRoots", + &data + .canonicalize() + .expect("canonical") + .display() + .to_string(), + ) + .expect("allowed roots"); + let denials = Arc::new(krishiv_session::auth::DeniedCounts::new()); + let factory = role_sessions( + Session::new().expect("session"), + &config, + Arc::clone(&denials), + ) + .expect("factory"); + let manager = Arc::new(SessionManager::new(factory, SessionLimits::default())); + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let server = ConnectServer::new(Arc::clone(&manager), ConnectServerConfig::default()) + .with_auth(ConnectAuth::new(auth).with_denials(Arc::clone(&denials))); + let (stop, stopped) = oneshot::channel::<()>(); + tokio::spawn(server.serve(listener, async { + let _ = stopped.await; + })); + let channel = Channel::from_shared(format!("http://{addr}")) + .expect("uri") + .connect() + .await + .expect("connect"); + Running { + manager, + denials, + channel, + _stop: stop, + _dir: dir, + } +} + +fn path(rpc: &str) -> tonic::codegen::http::uri::PathAndQuery { + format!("/spark.connect.SparkConnectService/{rpc}") + .parse() + .expect("path") +} + +fn with_token(request: T, token: Option<&str>) -> tonic::Request { + let mut r = tonic::Request::new(request); + if let Some(t) = token { + r.metadata_mut().insert( + "authorization", + format!("Bearer {t}").parse().expect("ascii"), + ); + } + r +} + +async fn unary( + channel: &Channel, + rpc: &str, + request: Req, + token: Option<&str>, +) -> Result +where + Req: prost::Message + Send + Sync + 'static, + Resp: prost::Message + Default + Send + Sync + 'static, +{ + let mut grpc = tonic::client::Grpc::new(channel.clone()); + grpc.ready().await.expect("ready"); + grpc.unary( + with_token(request, token), + path(rpc), + tonic_prost::ProstCodec::::default(), + ) + .await + .map(tonic::Response::into_inner) +} + +/// Every response of an `ExecutePlan`, or the status it ended with. +async fn execute( + channel: &Channel, + plan: pb::Plan, + token: Option<&str>, +) -> Result, tonic::Status> { + let request = pb::ExecutePlanRequest { + session_id: SESSION.to_owned(), + user_context: Some(pb::UserContext { + // Never identity: the token decides who this is. + user_id: "dana".to_owned(), + ..Default::default() + }), + plan: Some(plan), + ..Default::default() + }; + let mut grpc = tonic::client::Grpc::new(channel.clone()); + grpc.ready().await.expect("ready"); + let mut s = grpc + .server_streaming( + with_token(request, token), + path("ExecutePlan"), + tonic_prost::ProstCodec::::default(), + ) + .await? + .into_inner(); + let mut out = Vec::new(); + while let Some(r) = s.message().await? { + out.push(r); + } + Ok(out) +} + +fn range(end: i64) -> pb::Plan { + pb::Plan { + op_type: Some(OpType::Root(Box::new(pb::Relation { + common: None, + rel_type: Some(RelType::Range(pb::Range { + start: Some(0), + end, + step: 1, + num_partitions: None, + })), + }))), + } +} + +fn command(c: CommandType) -> pb::Plan { + pb::Plan { + op_type: Some(OpType::Command(Box::new(pb::Command { + command_type: Some(c), + }))), + } +} + +#[derive(Clone, PartialEq, prost::Message)] +struct RpcStatus { + #[prost(int32, tag = "1")] + code: i32, + #[prost(string, tag = "2")] + message: String, + #[prost(message, repeated, tag = "3")] + details: Vec, +} + +#[derive(Clone, PartialEq, prost::Message)] +struct ErrorInfo { + #[prost(string, tag = "1")] + reason: String, + #[prost(string, tag = "2")] + domain: String, + #[prost(btree_map = "string, string", tag = "3")] + metadata: BTreeMap, +} + +fn error_class(status: &tonic::Status) -> String { + let rpc = RpcStatus::decode(status.details()).expect("google.rpc.Status"); + ErrorInfo::decode(rpc.details[0].value.as_slice()) + .expect("ErrorInfo") + .metadata + .get("errorClass") + .cloned() + .unwrap_or_default() +} + +fn release() -> pb::ReleaseSessionRequest { + pb::ReleaseSessionRequest { + session_id: SESSION.to_owned(), + ..Default::default() + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn every_rpc_needs_a_credential() { + let run = start().await; + // A session RPC, an implemented streaming RPC and an unimplemented one: all are + // `UNAUTHENTICATED` without a token (Spark's message), so an unauthenticated client learns + // nothing about the server. + let s = unary::<_, pb::ReleaseSessionResponse>(&run.channel, "ReleaseSession", release(), None) + .await + .expect_err("no token"); + assert_eq!(s.code(), Code::Unauthenticated); + assert_eq!(s.message(), "No authentication token provided"); + let s = execute(&run.channel, range(3), None) + .await + .expect_err("no token"); + assert_eq!(s.code(), Code::Unauthenticated); + let s = + unary::<_, pb::ConfigResponse>(&run.channel, "Config", pb::ConfigRequest::default(), None) + .await + .expect_err("no token"); + assert_eq!(s.code(), Code::Unauthenticated); + let s = unary::<_, pb::ConfigResponse>( + &run.channel, + "Config", + pb::ConfigRequest::default(), + Some("not-a-configured-token"), + ) + .await + .expect_err("wrong token"); + assert_eq!(s.code(), Code::Unauthenticated); + assert_eq!(s.message(), "Invalid authentication token"); + assert_eq!(run.manager.counts().live, 0, "nothing was created"); + // With a token, the same RPCs get past authentication. + let s = unary::<_, pb::ConfigResponse>( + &run.channel, + "Config", + pb::ConfigRequest::default(), + Some(VIEWER), + ) + .await + .expect_err("an empty session id"); + assert_ne!(s.code(), Code::Unauthenticated, "{s:?}"); + assert_ne!(s.code(), Code::PermissionDenied, "{s:?}"); + unary::<_, pb::ReleaseSessionResponse>(&run.channel, "ReleaseSession", release(), Some(VIEWER)) + .await + .expect("an unknown session is a no-op"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn sessions_belong_to_the_authenticated_principal() { + let run = start().await; + let responses = execute(&run.channel, range(3), Some(VIEWER)) + .await + .expect("a viewer runs a plan that reads no table"); + assert!(!responses.is_empty()); + // The session is carol's, whatever `user_id` said; its principal is carol. + let carol = run + .manager + .get_if_present( + &Authenticated::new( + Principal::new("carol", PrincipalKind::User), + Some(Role::Viewer), + Method::Token, + ), + SESSION, + ) + .expect("carol's session"); + assert_eq!(carol.caller().principal.name, "carol"); + assert_eq!(carol.client_user_id(), "dana", "recorded, never trusted"); + // dana's token does not reach carol's session id: it is not found, as an unknown id is. + let s = execute(&run.channel, range(3), Some(ADMIN)) + .await + .expect_err("another principal's session id"); + assert_eq!(error_class(&s), "INVALID_HANDLE.SESSION_NOT_FOUND"); + assert_eq!(run.manager.counts().live, 1); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_command_is_checked_against_the_role_before_it_is_refused() { + let run = start().await; + let register = || command(CommandType::RegisterFunction(Box::default())); + // Client code needs `Usage` on `System("udf")`: admin only until T014b (ADR 0046 N1). + let s = execute(&run.channel, register(), Some(VIEWER)) + .await + .expect_err("a viewer sends no client code"); + assert_eq!(error_class(&s), "KRISHIV_INSUFFICIENT_PERMISSIONS"); + // The admin passes the check and meets the refusal every command gets until P3-T012. + release_as(&run, VIEWER).await; + let s = execute(&run.channel, register(), Some(ADMIN)) + .await + .expect_err("commands are not run yet"); + assert!( + error_class(&s).starts_with("CONNECT_INVALID_PLAN"), + "{}", + error_class(&s) + ); + // A global temporary view is a shared object: the `Ddl` class. + release_as(&run, ADMIN).await; + let global = command(CommandType::CreateDataframeView(Box::new( + pb::CreateDataFrameViewCommand { + name: "v".to_owned(), + is_global: true, + ..Default::default() + }, + ))); + let s = execute(&run.channel, global, Some(VIEWER)) + .await + .expect_err("a viewer creates no shared object"); + assert_eq!(error_class(&s), "KRISHIV_INSUFFICIENT_PERMISSIONS"); +} + +async fn release_as(run: &Running, token: &str) { + unary::<_, pb::ReleaseSessionResponse>( + &run.channel, + "ReleaseSession", + pb::ReleaseSessionRequest { + allow_reconnect: true, + ..release() + }, + Some(token), + ) + .await + .expect("release"); +} + +fn sql(query: &str) -> pb::Plan { + pb::Plan { + op_type: Some(OpType::Root(Box::new(pb::Relation { + common: None, + rel_type: Some(RelType::Sql(pb::Sql { + query: query.to_owned(), + ..Default::default() + })), + }))), + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn sessions_get_the_role_policy_and_the_path_confinement() { + let run = start().await; + // A viewer reads no rows, even of a file inside the allowed roots. + let s = execute(&run.channel, sql("SELECT * FROM range(1)"), Some(VIEWER)) + .await + .expect("a plan that reads no table needs no privilege"); + assert!(!s.is_empty()); + release_as(&run, VIEWER).await; + // An operator may read rows, but not by a path outside `krishiv.io.local.allowedRoots`. + let outside = run._dir.path().join("outside.parquet"); + let s = execute( + &run.channel, + sql(&format!("SELECT * FROM parquet.`{}`", outside.display())), + Some(OPERATOR), + ) + .await + .expect_err("confined"); + assert_eq!(s.code(), Code::PermissionDenied); + assert_eq!(error_class(&s), "KRISHIV_INSUFFICIENT_PERMISSIONS"); + // ADR 0046 §8: the session counted it once, under `connect` and the privilege asked for. + assert_eq!(denied(&run), [(Surface::Connect, Privilege::Select, 1)]); + // Inside the roots the same read gets past the policy (the file does not exist). + let inside = run._dir.path().join("data").join("missing.parquet"); + let s = execute( + &run.channel, + sql(&format!("SELECT * FROM parquet.`{}`", inside.display())), + Some(OPERATOR), + ) + .await + .expect_err("no such file"); + assert_ne!(error_class(&s), "KRISHIV_INSUFFICIENT_PERMISSIONS"); +} + +/// An authenticator whose callers map to no role (an OIDC subject in no role list, a +/// certificate principal in none). +struct NoRole; + +impl Authenticator for NoRole { + fn authenticate<'a>( + &'a self, + _credentials: &'a Credentials<'a>, + ) -> BoxFuture<'a, Result> { + Box::pin(async { + Ok(Authenticated::new( + Principal::new("nobody", PrincipalKind::User), + None, + Method::Oidc, + )) + }) + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_caller_with_no_role_gets_no_session() { + let run = start_with(Arc::new(NoRole), tempfile::tempdir().expect("tempdir")).await; + // ADR 0046 §2: refused when the session would be created, before anything is built. + let s = execute(&run.channel, range(3), Some("any")) + .await + .expect_err("no role"); + assert_eq!(s.code(), Code::PermissionDenied); + assert_eq!(error_class(&s), "KRISHIV_INSUFFICIENT_PERMISSIONS"); + let s = unary::<_, pb::ReleaseSessionResponse>( + &run.channel, + "ReleaseSession", + release(), + Some("any"), + ) + .await + .expect_err("no role"); + assert_eq!(s.code(), Code::PermissionDenied); + assert_eq!(run.manager.counts().live, 0); + // Each refusal is one denial of `BROWSE` on `connect`. + assert_eq!(denied(&run), [(Surface::Connect, Privilege::Browse, 2)]); +} + +/// The non-zero denial counters. +fn denied(run: &Running) -> Vec<(Surface, Privilege, u64)> { + run.denials + .counts() + .into_iter() + .filter(|c| c.count > 0) + .map(|c| (c.surface, c.privilege, c.count)) + .collect() +} + +/// Sends `body` (a message that is no `ExecutePlanRequest`: field 1 as a varint where a +/// string is expected) as an `ExecutePlan`. +async fn undecodable(channel: &Channel, token: Option<&str>) -> tonic::Status { + let body = prost_types::Duration { + seconds: 5, + nanos: 0, + }; + let mut grpc = tonic::client::Grpc::new(channel.clone()); + grpc.ready().await.expect("ready"); + match grpc + .server_streaming( + with_token(body, token), + path("ExecutePlan"), + tonic_prost::ProstCodec::::default(), + ) + .await + { + Ok(mut s) => s.get_mut().message().await.expect_err("refused"), + Err(status) => status, + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn authentication_comes_before_the_body_is_decoded() { + let run = start().await; + // Without a credential the body is never decoded: the answer is about the credential. + let s = undecodable(&run.channel, None).await; + assert_eq!(s.code(), Code::Unauthenticated, "{s:?}"); + // With one, the same body reaches the decoder and fails there. + let s = undecodable(&run.channel, Some(VIEWER)).await; + assert_ne!(s.code(), Code::Unauthenticated, "{s:?}"); + assert_ne!(s.code(), Code::Ok); +} + +/// ADR 0046 §5 (threat model T-R3): the client's `UserContext.user_id` is never the identity, +/// but its session's `system.sessions` row records it as the client reported it. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn the_clients_user_id_is_recorded_not_trusted() { + use arrow_array::Array as _; + let run = start().await; + // The helper's requests say `user_id = "dana"`; the token is carol's (a viewer, who sees + // her own rows of `system.sessions`). + let responses = execute( + &run.channel, + sql("SELECT principal, client_user_id, transport FROM system.sessions"), + Some(VIEWER), + ) + .await + .expect("a viewer reads her own sessions"); + let rows: Vec<(String, String, String)> = responses + .iter() + .filter_map(|r| match &r.response_type { + Some(pb::execute_plan_response::ResponseType::ArrowBatch(b)) => Some(b), + _ => None, + }) + .flat_map(|b| { + arrow_ipc::reader::StreamReader::try_new(std::io::Cursor::new(b.data.clone()), None) + .expect("ipc") + .collect::, _>>() + .expect("batches") + }) + .flat_map(|batch| { + let text = |i: usize| { + batch + .column(i) + .as_any() + .downcast_ref::() + .expect("string column") + .clone() + }; + let (p, u, t) = (text(0), text(1), text(2)); + (0..batch.num_rows()) + .map(|r| { + let get = |a: &arrow_array::StringArray| { + if a.is_null(r) { + String::new() + } else { + a.value(r).to_owned() + } + }; + (get(&p), get(&u), get(&t)) + }) + .collect::>() + }) + .collect(); + assert_eq!( + rows, + [("carol".to_owned(), "dana".to_owned(), "connect".to_owned())] + ); +} + +/// The `krishiv::audit` events of this thread, as `field=value` lines. +#[derive(Clone, Default)] +struct AuditCapture(Arc>>); + +impl AuditCapture { + fn take(&self) -> Vec { + std::mem::take( + &mut *self + .0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner), + ) + } +} + +struct AuditFields(String); + +impl tracing::field::Visit for AuditFields { + fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { + use std::fmt::Write as _; + let _ = write!(self.0, "{}={value:?} ", field.name()); + } + + fn record_str(&mut self, field: &tracing::field::Field, value: &str) { + use std::fmt::Write as _; + let _ = write!(self.0, "{}={value} ", field.name()); + } +} + +impl tracing::Subscriber for AuditCapture { + fn enabled(&self, _: &tracing::Metadata<'_>) -> bool { + true + } + + fn new_span(&self, _: &tracing::span::Attributes<'_>) -> tracing::span::Id { + tracing::span::Id::from_u64(1) + } + + fn record(&self, _: &tracing::span::Id, _: &tracing::span::Record<'_>) {} + + fn record_follows_from(&self, _: &tracing::span::Id, _: &tracing::span::Id) {} + + fn event(&self, event: &tracing::Event<'_>) { + if event.metadata().target() == "krishiv::audit" { + let mut fields = AuditFields(String::new()); + event.record(&mut fields); + self.0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push(fields.0); + } + } + + fn enter(&self, _: &tracing::span::Id) {} + + fn exit(&self, _: &tracing::span::Id) {} +} + +/// Review N2 (ADR 0046 §3b): `ReattachExecute` of another principal's operation id, end to +/// end. It is answered as an unknown id and writes one audit record naming the caller only. +/// A current-thread runtime, so the handler runs on this thread and the thread's subscriber +/// sees its record. +#[tokio::test(flavor = "current_thread")] +async fn reattaching_another_principals_operation_is_unknown_and_recorded() { + use krishiv_connect::proto::v4_3::execute_plan_request::request_option::RequestOption; + const OP: &str = "1d6a3c1e-77b2-4c55-8d0f-0a6c2e9b4f21"; + const UNKNOWN: &str = "5b1f7a2e-0c3d-4e8f-9a6b-2d4c6e8f0a1b"; + const OTHER_SESSION: &str = "2c4e6a8b-0d1f-4a3c-8e5b-7d9f1b3d5f70"; + let capture = AuditCapture::default(); + let _guard = tracing::subscriber::set_default(capture.clone()); + let run = start().await; + + // carol's reattachable operation, read to its end: it stays registered until released. + let request = pb::ExecutePlanRequest { + session_id: SESSION.to_owned(), + operation_id: Some(OP.to_owned()), + plan: Some(range(3)), + request_options: vec![pb::execute_plan_request::RequestOption { + request_option: Some(RequestOption::ReattachOptions(pb::ReattachOptions { + reattachable: true, + })), + }], + ..Default::default() + }; + let mut grpc = tonic::client::Grpc::new(run.channel.clone()); + grpc.ready().await.expect("ready"); + let mut stream = grpc + .server_streaming( + with_token(request, Some(VIEWER)), + path("ExecutePlan"), + tonic_prost::ProstCodec::::default(), + ) + .await + .expect("execute") + .into_inner(); + while stream.message().await.expect("response").is_some() {} + assert!( + capture.take().is_empty(), + "carol's own operation writes nothing" + ); + + let reattach = |operation_id: &str| pb::ReattachExecuteRequest { + session_id: OTHER_SESSION.to_owned(), + operation_id: operation_id.to_owned(), + ..Default::default() + }; + let reattach_as_dana = async |operation_id: &str| { + let mut grpc = tonic::client::Grpc::new(run.channel.clone()); + grpc.ready().await.expect("ready"); + grpc.server_streaming( + with_token(reattach(operation_id), Some(ADMIN)), + path("ReattachExecute"), + tonic_prost::ProstCodec::::default( + ), + ) + .await + .expect_err("not dana's operation") + }; + + // Dana (an admin) creates her own session first, so the reattach finds it. + execute_in(&run.channel, OTHER_SESSION, ADMIN).await; + capture.take(); + + let unknown = reattach_as_dana(UNKNOWN).await; + assert_eq!(error_class(&unknown), "INVALID_HANDLE.OPERATION_NOT_FOUND"); + assert!(capture.take().is_empty(), "an unknown id writes nothing"); + + let foreign = reattach_as_dana(OP).await; + assert_eq!( + error_class(&foreign), + "INVALID_HANDLE.OPERATION_NOT_FOUND", + "answered as the unknown id is" + ); + let records = capture.take(); + assert_eq!(records.len(), 1, "{records:?}"); + let r = &records[0]; + assert!(r.contains("principal=dana "), "{r}"); + assert!(r.contains("handle=operation "), "{r}"); + assert!(r.contains("surface=connect "), "{r}"); + assert!(!r.contains("carol"), "the owner is never named: {r}"); +} + +/// Runs a small plan in `session_id` as the holder of `token`, to create that session. +async fn execute_in(channel: &Channel, session_id: &str, token: &str) { + let request = pb::ExecutePlanRequest { + session_id: session_id.to_owned(), + plan: Some(range(1)), + ..Default::default() + }; + let mut grpc = tonic::client::Grpc::new(channel.clone()); + grpc.ready().await.expect("ready"); + let mut s = grpc + .server_streaming( + with_token(request, Some(token)), + path("ExecutePlan"), + tonic_prost::ProstCodec::::default(), + ) + .await + .expect("execute") + .into_inner(); + while s.message().await.expect("response").is_some() {} +} + +/// A `Config` request of `op` in [`SESSION`] as `token`. +async fn config_as( + run: &Running, + token: &str, + op: krishiv_connect::proto::v4_3::config_request::operation::OpType, +) -> Result { + unary( + &run.channel, + "Config", + pb::ConfigRequest { + session_id: SESSION.to_owned(), + operation: Some(pb::config_request::Operation { op_type: Some(op) }), + ..Default::default() + }, + Some(token), + ) + .await +} + +/// `Config` set and unset of an admin-only key or a server-scoped store key ask the session's +/// policy as SQL `SET` does: a refusal is `PERMISSION_DENIED`, audited and counted under +/// `connect` and `MANAGE` (ADR 0046 §8), a silent set turns it into a warning (still counted) +/// and changes nothing, an admin-only key named by its Spark alias is refused the same way, and +/// an admin may. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_refusals_are_counted_like_sql_set() { + use krishiv_connect::proto::v4_3::config_request::{self as creq, operation::OpType as Op}; + let kv = |key: &str, value: &str| pb::KeyValue { + key: key.to_owned(), + value: Some(value.to_owned()), + }; + let run = start().await; + let set = |silent| { + Op::Set(creq::Set { + pairs: vec![kv("krishiv.resultCache.ttl", "1m")], + silent: Some(silent), + }) + }; + let s = config_as(&run, OPERATOR, set(false)) + .await + .expect_err("admin key"); + assert_eq!(s.code(), Code::PermissionDenied); + assert_eq!(error_class(&s), "KRISHIV_INSUFFICIENT_PERMISSIONS"); + assert_eq!(denied(&run), [(Surface::Connect, Privilege::Manage, 1)]); + + let quiet = config_as(&run, OPERATOR, set(true)).await.expect("silent"); + assert!( + quiet + .warnings + .iter() + .any(|w| w.contains("krishiv.resultCache.ttl")), + "{:?}", + quiet.warnings + ); + let s = config_as( + &run, + OPERATOR, + Op::Unset(creq::Unset { + keys: vec!["krishiv.resultCache.ttl".to_owned()], + }), + ) + .await + .expect_err("admin key"); + assert_eq!(s.code(), Code::PermissionDenied); + let s = config_as( + &run, + OPERATOR, + Op::Set(creq::Set { + pairs: vec![kv("fs.s3a.endpoint", "http://169.254.169.254")], + silent: None, + }), + ) + .await + .expect_err("server-scoped store key"); + assert_eq!(s.code(), Code::PermissionDenied); + // `krishiv.sql.autoBroadcastJoinThreshold` (admin) by its Spark name. + let s = config_as( + &run, + OPERATOR, + Op::Set(creq::Set { + pairs: vec![kv("spark.sql.autoBroadcastJoinThreshold", "1")], + silent: None, + }), + ) + .await + .expect_err("admin key by its Spark alias"); + assert_eq!(s.code(), Code::PermissionDenied); + assert_eq!(denied(&run), [(Surface::Connect, Privilege::Manage, 5)]); + let got = config_as( + &run, + OPERATOR, + Op::GetOption(creq::GetOption { + keys: vec!["krishiv.resultCache.ttl".to_owned()], + }), + ) + .await + .expect("get"); + assert_ne!( + got.pairs[0].value.as_deref(), + Some("1min"), + "a refused set changes nothing" + ); + release_as(&run, OPERATOR).await; + + config_as(&run, ADMIN, set(false)).await.expect("admin may"); + let got = config_as( + &run, + ADMIN, + Op::GetOption(creq::GetOption { + keys: vec!["krishiv.resultCache.ttl".to_owned()], + }), + ) + .await + .expect("get"); + assert_eq!( + got.pairs[0].value.as_deref(), + Some("1min"), + "the admin's set holds (1m, read back normalized)" + ); + assert_eq!(denied(&run), [(Surface::Connect, Privilege::Manage, 5)]); +} diff --git a/crates/krishiv-connect-server/tests/execute.rs b/crates/krishiv-connect-server/tests/execute.rs index 0183d7ae6..ff45002e0 100644 --- a/crates/krishiv-connect-server/tests/execute.rs +++ b/crates/krishiv-connect-server/tests/execute.rs @@ -26,6 +26,7 @@ use krishiv_connect_server::{ ConnectServer, ConnectServerConfig, ExecuteLimits, SessionLimits, SessionManager, }; use krishiv_session::Session; +use krishiv_session::auth::{Authenticated, Method}; use prost::Message; use tokio::net::TcpListener; use tokio::sync::oneshot; @@ -242,7 +243,7 @@ async fn execute_response_order() { // small engine batches are not merged (design P3-T011a §3.2), so the // engine's batch size decides how many come below the limits. run.manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session") .session() .conf() @@ -292,7 +293,7 @@ async fn execute_response_order() { )); let entry = run .manager - .get(&Principal::local_user(), SESSION, None) + .get(&local_caller(), SESSION, None) .expect("session"); let ids: HashSet<&str> = responses.iter().map(|r| r.response_id.as_str()).collect(); assert_eq!(ids.len(), responses.len(), "fresh response ids"); @@ -359,7 +360,7 @@ async fn strings_are_utf8_and_chunks_reassemble() { }); let entry = run .manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session"); entry .session() @@ -402,7 +403,7 @@ async fn compressed_ipc_buffers_follow_the_session_codec() { let run = start().await; let entry = run .manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session"); for (codec, expected) in [ ("zstd", arrow_ipc::CompressionType::ZSTD), @@ -593,7 +594,7 @@ async fn invalid_requests_are_refused_with_sparks_classes() { ); let entry = run .manager - .get(&Principal::local_user(), SESSION, None) + .get(&local_caller(), SESSION, None) .expect("session"); assert!( entry.operations().is_empty(), @@ -686,7 +687,7 @@ async fn live_operations_reattach_release_and_cap() { assert_eq!(error_class(&e), "INVALID_HANDLE.OPERATION_NOT_FOUND"); // Another session never sees the operation (operation_bound_to_session). run.manager - .get_or_create(&Principal::local_user(), OTHER_SESSION, None, "u") + .get_or_create(&local_caller(), OTHER_SESSION, None, "u") .expect("other"); let e = stream(&run.channel, "ReattachExecute", reattach(OTHER_SESSION, OP)) .await @@ -758,7 +759,7 @@ async fn live_operations_reattach_release_and_cap() { // (Spark's `ExecuteHolder.close` interrupts the attached sender). let entry = run .manager - .get(&Principal::local_user(), SESSION, None) + .get(&local_caller(), SESSION, None) .expect("session"); let session = entry.session().clone(); let _: pb::ReleaseExecuteResponse = unary( @@ -858,7 +859,7 @@ async fn a_dropped_stream_cancels_the_query_and_frees_its_memory() { .await; let entry = run .manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session"); let session = entry.session().clone(); session @@ -899,7 +900,7 @@ async fn a_dropped_stream_cancels_a_query_that_has_not_produced_yet() { let run = start().await; let entry = run .manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session"); let session = entry.session().clone(); // An aggregate sends nothing until it has read its whole input. @@ -934,7 +935,7 @@ async fn a_released_operation_does_not_wait_for_a_client_that_reads_nothing() { .await; let entry = run .manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session"); let session = entry.session().clone(); session @@ -998,7 +999,7 @@ async fn large_row_result() { assert_eq!(batch.num_chunks_in_batch, None); } run.manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create(&local_caller(), SESSION, None, "u") .expect("session") .session() .conf() @@ -1032,3 +1033,8 @@ async fn large_row_result() { .expect("utf8"); assert_eq!(s.value(0).len(), 65_536); } + +/// The caller of every request to a server without authentication. +fn local_caller() -> Authenticated { + Authenticated::new(Principal::local_user(), None, Method::None) +} diff --git a/crates/krishiv-connect-server/tests/foreign_handles_audit.rs b/crates/krishiv-connect-server/tests/foreign_handles_audit.rs new file mode 100644 index 000000000..f3e32e236 --- /dev/null +++ b/crates/krishiv-connect-server/tests/foreign_handles_audit.rs @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: Apache-2.0 +//! ADR 0046 §3b: a session or operation id another principal holds answers as +//! a missing one and still writes one `krishiv::audit` denial record; an +//! unknown id writes none. One test per binary: the subscriber is +//! process-wide. +#![cfg(test)] + +use std::fmt::Write as _; +use std::sync::{Arc, Mutex, OnceLock, Weak}; + +use krishiv_common::principal::{Principal, PrincipalKind}; +use krishiv_connect_server::manager::{SessionLimits, SessionManager}; +use krishiv_connect_server::operation::{Operation, plan_digest}; +use krishiv_session::auth::{Authenticated, Method, Role}; +use krishiv_session::{QueryId, Session}; +use tracing::field::{Field, Visit}; +use tracing::span::{Attributes, Id, Record}; +use tracing::{Event, Metadata, Subscriber}; + +static RECORDS: Mutex> = Mutex::new(Vec::new()); + +struct Capture; + +struct Fields(String); + +impl Visit for Fields { + fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { + let _ = write!(self.0, "{}={value:?} ", field.name()); + } + + fn record_str(&mut self, field: &Field, value: &str) { + let _ = write!(self.0, "{}={value} ", field.name()); + } +} + +impl Subscriber for Capture { + fn enabled(&self, _: &Metadata<'_>) -> bool { + true + } + + fn new_span(&self, _: &Attributes<'_>) -> Id { + Id::from_u64(1) + } + + fn record(&self, _: &Id, _: &Record<'_>) {} + + fn record_follows_from(&self, _: &Id, _: &Id) {} + + fn event(&self, event: &Event<'_>) { + if event.metadata().target() == "krishiv::audit" { + let mut fields = Fields(String::new()); + event.record(&mut fields); + RECORDS.lock().unwrap().push(fields.0); + } + } + + fn enter(&self, _: &Id) {} + + fn exit(&self, _: &Id) {} +} + +const ID: &str = "8f0e4c55-5ad0-4d1e-9a3c-3a1b0b2c7d10"; +const OP: &str = "1d6a3c1e-77b2-4c55-8d0f-0a6c2e9b4f21"; +const UNKNOWN: &str = "5b1f7a2e-0c3d-4e8f-9a6b-2d4c6e8f0a1b"; + +fn user(name: &str) -> Authenticated { + Authenticated::new( + Principal::new(name, PrincipalKind::User), + Some(Role::Operator), + Method::Token, + ) +} + +#[test] +fn foreign_ids_are_answered_as_unknown_and_recorded() { + tracing::subscriber::set_global_default(Capture).unwrap(); + let take = || std::mem::take(&mut *RECORDS.lock().unwrap()); + let m = SessionManager::from_base(Session::new().unwrap(), SessionLimits::default()); + let (alice, bob) = (user("alice"), user("bob")); + let entry = m.get_or_create(&alice, ID, None, "u").unwrap(); + let op = Arc::new(Operation::new( + QueryId::parse(OP).unwrap(), + plan_digest(b"p"), + Arc::from(Vec::new()), + false, + )); + entry.operations().register(&op, 10).unwrap(); + m.get_or_create(&bob, UNKNOWN, None, "u").unwrap(); + assert!(take().is_empty(), "own sessions write nothing"); + + // Unknown ids: no record. + let unknown = "0a0b0c0d-0e0f-4a1b-8c2d-3e4f5a6b7c8d"; + m.get(&bob, unknown, None).unwrap_err(); + assert_eq!(m.release(&bob, unknown, false), None); + m.audit_foreign_operation(&bob, unknown); + assert!(take().is_empty(), "unknown ids write nothing"); + + // Alice's ids, asked for by bob: one record each, naming bob and the kind of id. + for e in [ + m.get(&bob, ID, None).unwrap_err(), + m.get_or_create(&bob, ID, None, "alice").unwrap_err(), + ] { + assert_eq!(e.error_class().name(), "INVALID_HANDLE.SESSION_NOT_FOUND"); + } + assert_eq!(m.release(&bob, ID, false), None); + m.audit_foreign_operation(&bob, OP); + let records = take(); + assert_eq!(records.len(), 4, "{records:?}"); + for (r, handle) in records + .iter() + .zip(["session", "session", "session", "operation"]) + { + assert!(r.contains("principal=bob "), "{r}"); + assert!(r.contains("surface=connect "), "{r}"); + assert!(r.contains("role=operator "), "{r}"); + assert!(r.contains(&format!("handle={handle} ")), "{r}"); + assert!(!r.contains("alice"), "the owner is never named: {r}"); + } + + // Alice's own operation, from her own session: nothing. + m.audit_foreign_operation(&alice, OP); + assert!(take().is_empty()); + m.get(&alice, ID, None).unwrap(); + + // The race after the build (review N2): bob's lookup and caps check find the id free, and + // alice creates it while bob's session is being built. Bob's create is refused as unknown, + // its session closed unused, and exactly one record is written. + let racing = racing_manager(&alice); + let e = racing.get_or_create(&bob, ID, None, "u").unwrap_err(); + assert_eq!(e.error_class().name(), "INVALID_HANDLE.SESSION_NOT_FOUND"); + let records = take(); + assert_eq!(records.len(), 1, "{records:?}"); + assert!(records[0].contains("principal=bob "), "{}", records[0]); + assert!(records[0].contains("handle=session "), "{}", records[0]); + assert!(!records[0].contains("alice"), "{}", records[0]); + assert_eq!( + racing.counts().live, + 1, + "alice's session stays, bob's is dropped" + ); + assert!(!racing.get(&alice, ID, None).unwrap().session().is_closed()); +} + +/// A manager whose factory, building a session for anyone but `winner`, first creates +/// `winner`'s session of the same id: the create that is being built loses the race. +fn racing_manager(winner: &Authenticated) -> Arc { + let base = Session::new().unwrap(); + let this: Arc>> = Arc::new(OnceLock::new()); + let (hook, winner) = (Arc::clone(&this), winner.clone()); + let m = Arc::new(SessionManager::new( + Box::new(move |caller: &Authenticated| { + if caller.principal.name != winner.principal.name + && let Some(m) = hook.get().and_then(Weak::upgrade) + { + m.get_or_create(&winner, ID, None, "u").unwrap(); + } + Ok(base.new_session()) + }), + SessionLimits::default(), + )); + this.set(Arc::downgrade(&m)).unwrap(); + m +} diff --git a/crates/krishiv-connect-server/tests/reattach.rs b/crates/krishiv-connect-server/tests/reattach.rs index edec085f2..1c0f2cb44 100644 --- a/crates/krishiv-connect-server/tests/reattach.rs +++ b/crates/krishiv-connect-server/tests/reattach.rs @@ -30,6 +30,7 @@ use krishiv_connect_server::{ ConnectServer, ConnectServerConfig, ExecuteLimits, SessionEntry, SessionLimits, SessionManager, }; use krishiv_session::Session; +use krishiv_session::auth::{Authenticated, Method}; use prost::Message; use tokio::net::TcpListener; use tokio::sync::oneshot; @@ -51,7 +52,12 @@ struct Running { impl Running { fn entry(&self) -> Arc { self.manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create( + &Authenticated::new(Principal::local_user(), None, Method::None), + SESSION, + None, + "u", + ) .expect("session") } diff --git a/crates/krishiv-connect-server/tests/rpcs.rs b/crates/krishiv-connect-server/tests/rpcs.rs index 900ea20bb..ddc083541 100644 --- a/crates/krishiv-connect-server/tests/rpcs.rs +++ b/crates/krishiv-connect-server/tests/rpcs.rs @@ -27,6 +27,7 @@ use krishiv_connect_server::{ ConnectServer, ConnectServerConfig, SessionEntry, SessionLimits, SessionManager, }; use krishiv_session::Session; +use krishiv_session::auth::{Authenticated, Method}; use prost::Message; use tokio::net::TcpListener; use tokio::sync::oneshot; @@ -46,7 +47,12 @@ struct Running { impl Running { fn entry(&self) -> Arc { self.manager - .get_or_create(&Principal::local_user(), SESSION, None, "u") + .get_or_create( + &Authenticated::new(Principal::local_user(), None, Method::None), + SESSION, + None, + "u", + ) .expect("session") } } @@ -808,6 +814,61 @@ async fn config_set_silent_and_static_keys() { ); } +/// Authentication off: the local user of the loopback listener sets and unsets admin-only keys +/// (by name and by Spark alias) and server-scoped store keys, as SQL `SET` lets it (the base +/// session's policy allows everything). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_the_local_user_sets_admin_keys() { + let run = start().await; + config( + &run, + OpType::Set(creq::Set { + pairs: vec![ + kv("krishiv.resultCache.ttl", Some("1m")), + kv("spark.sql.autoBroadcastJoinThreshold", Some("1")), + kv("fs.s3a.endpoint", Some("http://minio.local:9000")), + ], + silent: None, + }), + ) + .await + .expect("the local user may set admin keys"); + let got = config( + &run, + OpType::GetOption(creq::GetOption { + keys: keys(&[ + "krishiv.resultCache.ttl", + "spark.sql.autoBroadcastJoinThreshold", + "fs.s3a.endpoint", + ]), + }), + ) + .await + .expect("get"); + let values: Vec<_> = got.pairs.iter().map(|p| p.value.as_deref()).collect(); + assert_eq!( + values, + [Some("1min"), Some("1"), Some("http://minio.local:9000")] + ); + config( + &run, + OpType::Unset(creq::Unset { + keys: keys(&["krishiv.resultCache.ttl"]), + }), + ) + .await + .expect("the local user may unset an admin key"); + assert_eq!( + run.entry() + .session() + .conf() + .get("krishiv.resultCache.ttl") + .as_deref(), + Some("1d"), + "unset: back to the default" + ); +} + /// T-C8: a planted secret never comes back from `Config`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn config_redaction() { diff --git a/crates/krishiv-connect-server/tests/server.rs b/crates/krishiv-connect-server/tests/server.rs index f9a739031..8dd1e0ba2 100644 --- a/crates/krishiv-connect-server/tests/server.rs +++ b/crates/krishiv-connect-server/tests/server.rs @@ -15,6 +15,7 @@ use krishiv_common::principal::Principal; use krishiv_connect::proto::v4_3 as pb; use krishiv_connect_server::{ConnectServer, ConnectServerConfig, SessionLimits, SessionManager}; use krishiv_session::Session; +use krishiv_session::auth::{Authenticated, Method}; use tokio::net::TcpListener; use tokio::sync::oneshot; use tonic::Code; @@ -95,7 +96,7 @@ fn release(session_id: &str, allow_reconnect: bool) -> pb::ReleaseSessionRequest #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn release_session_frees_state() { let run = start(ConnectServerConfig::default(), SessionLimits::default()).await; - let local = Principal::local_user(); + let local = local_caller(); let entry = run .manager .get_or_create(&local, ID, None, "someone") @@ -230,7 +231,7 @@ async fn shutdown_cancels_a_stuck_query_at_the_timeout_and_closes_every_session( SessionLimits::default(), ) .await; - let local = Principal::local_user(); + let local = local_caller(); let entry = run .manager .get_or_create(&local, ID, None, "u") @@ -269,7 +270,7 @@ async fn shutdown_cancels_a_stuck_query_at_the_timeout_and_closes_every_session( async fn shutdown_with_nothing_running_returns_at_once() { let mut run = start(ConnectServerConfig::default(), SessionLimits::default()).await; run.manager - .get_or_create(&Principal::local_user(), ID, None, "u") + .get_or_create(&local_caller(), ID, None, "u") .expect("create"); run.stop.take().expect("stop").send(()).expect("send"); // Well inside the 30 s grace period: no query to wait for. @@ -296,7 +297,7 @@ async fn the_idle_sweeper_runs_on_the_maintenance_interval() { .await; let entry = run .manager - .get_or_create(&Principal::local_user(), ID, None, "u") + .get_or_create(&local_caller(), ID, None, "u") .expect("create"); let deadline = tokio::time::Instant::now() + Duration::from_secs(10); while run.manager.counts().live > 0 { @@ -316,7 +317,7 @@ async fn a_session_with_a_running_query_is_not_swept() { }, ); let entry = manager - .get_or_create(&Principal::local_user(), ID, None, "u") + .get_or_create(&local_caller(), ID, None, "u") .expect("create"); let session = entry.session().clone(); let df = session @@ -412,7 +413,7 @@ async fn shutdown_then_dropping_the_runtime_leaves_no_spill_directory() { .expect("session"); let manager = Arc::new(SessionManager::from_base(base, SessionLimits::default())); let entry = manager - .get_or_create(&Principal::local_user(), ID, None, "u") + .get_or_create(&local_caller(), ID, None, "u") .expect("create"); // A sort larger than the pool spills, which makes the spill directory. let rows = entry @@ -649,3 +650,8 @@ async fn a_connection_cap_past_the_semaphore_limit_is_clamped() { .expect("release"); assert_eq!(response.session_id, ID); } + +/// The caller of every request to a server without authentication. +fn local_caller() -> Authenticated { + Authenticated::new(Principal::local_user(), None, Method::None) +} diff --git a/crates/krishiv-connect-server/tests/tls.rs b/crates/krishiv-connect-server/tests/tls.rs new file mode 100644 index 000000000..5530de4cc --- /dev/null +++ b/crates/krishiv-connect-server/tests/tls.rs @@ -0,0 +1,186 @@ +// SPDX-License-Identifier: Apache-2.0 +//! The Connect listener over TLS (P3-T002 design §3.4, §3.5): `ConnectServer::with_tls` with the +//! server's `ServerTls`; a bearer token over TLS, a client certificate that authenticates by +//! itself with the role its name is listed under, and a client in clear refused. Fixtures: +//! `crates/krishiv-session/tests/fixtures/tls/README.md`. +#![cfg(unix)] +#![expect( + clippy::expect_used, + reason = "integration-test helpers are outside #[test] fns" +)] + +use std::net::SocketAddr; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use hyper_util::rt::TokioIo; +use krishiv_common::config::Config; +use krishiv_connect::proto::v4_3 as pb; +use krishiv_connect_server::{ + ConnectAuth, ConnectServer, ConnectServerConfig, SessionLimits, SessionManager, role_sessions, +}; +use krishiv_session::Session; +use krishiv_session::auth::{AuthConfig, ServerAuth, ServerTls, TlsConfig}; +use tokio::net::{TcpListener, TcpStream}; +use tokio::sync::oneshot; +use tokio_rustls::TlsConnector; +use tokio_rustls::rustls::pki_types::pem::PemObject as _; +use tokio_rustls::rustls::pki_types::{CertificateDer, PrivateKeyDer, ServerName}; +use tokio_rustls::rustls::{ClientConfig, RootCertStore}; +use tonic::Code; +use tonic::transport::{Channel, Endpoint}; + +const SESSION: &str = "8f0e4c55-5ad0-4d1e-9a3c-3a1b0b2c7d10"; +const VIEWER: &str = "viewer-token-0123456789abcdefghijklmnopqrstuv"; + +fn fixture(name: &str) -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join(format!("../krishiv-session/tests/fixtures/tls/{name}")) +} + +/// `src` copied into `dir` with mode 0600 (git keeps no modes). +fn private_copy(dir: &Path, src: &Path, name: &str) -> String { + let path = dir.join(name); + std::fs::copy(src, &path).expect("copy"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + path.to_str().expect("utf-8 path").to_owned() +} + +/// A TLS server: a viewer token (`carol`), client certificates `optional` with the principal +/// from the CN, `etl-job` an operator. +async fn start() -> (SocketAddr, oneshot::Sender<()>, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("tempdir"); + let token = dir.path().join("token"); + std::fs::write(&token, VIEWER).expect("write"); + let secret = format!("file:{}", private_copy(dir.path(), &token, "v")); + let key = private_copy(dir.path(), &fixture("server.key"), "server.key"); + let cert = fixture("server.pem"); + let ca = format!("file:{}", fixture("ca.pem").display()); + let settings = [ + ("krishiv.auth.tokens.v.secret", secret.as_str()), + ("krishiv.auth.tokens.v.principal", "carol"), + ("krishiv.auth.tokens.v.role", "viewer"), + ("krishiv.auth.clientCertificates", "optional"), + ("krishiv.auth.clientCa", ca.as_str()), + ("krishiv.auth.mtls.principalFrom", "cn"), + ("krishiv.auth.roles.operator", "etl-job"), + ("krishiv.server.tls.certFile", cert.to_str().expect("utf-8")), + ("krishiv.server.tls.keyFile", key.as_str()), + ]; + let auth_config = AuthConfig::from_settings(settings).expect("auth config"); + let tls_config = TlsConfig::from_settings(settings).expect("tls config"); + let tls = ServerTls::new(&tls_config, &auth_config.mtls) + .expect("server tls") + .expect("tls on"); + let auth = Arc::new(ServerAuth::new(&auth_config).expect("auth")); + let factory = role_sessions( + Session::new().expect("session"), + &Config::new(), + auth.denials(), + ) + .expect("factory"); + let manager = Arc::new(SessionManager::new(factory, SessionLimits::default())); + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let server = ConnectServer::new(manager, ConnectServerConfig::default()) + .with_auth(ConnectAuth::new(auth)) + .with_tls(&tls.server_config()); + let (stop, stopped) = oneshot::channel::<()>(); + tokio::spawn(server.serve(listener, async { + let _ = stopped.await; + })); + (addr, stop, dir) +} + +/// A channel over TLS to `addr`, trusting the test CA and presenting the fixture `identity`. +async fn tls_channel(addr: SocketAddr, identity: Option<&str>) -> Channel { + let mut roots = RootCertStore::empty(); + roots + .add(CertificateDer::from_pem_file(fixture("ca.pem")).expect("ca")) + .expect("add ca"); + let provider = Arc::new(tokio_rustls::rustls::crypto::aws_lc_rs::default_provider()); + let builder = ClientConfig::builder_with_provider(provider) + .with_safe_default_protocol_versions() + .expect("versions") + .with_root_certificates(roots); + let mut config = match identity { + None => builder.with_no_client_auth(), + Some(name) => builder + .with_client_auth_cert( + vec![CertificateDer::from_pem_file(fixture(&format!("{name}.pem"))).expect("cert")], + PrivateKeyDer::from_pem_file(fixture(&format!("{name}.key"))).expect("key"), + ) + .expect("client auth"), + }; + config.alpn_protocols = vec![b"h2".to_vec()]; + let connector = TlsConnector::from(Arc::new(config)); + Endpoint::from_static("http://localhost") + .connect_with_connector(tower::service_fn(move |_| { + let connector = connector.clone(); + async move { + let tcp = TcpStream::connect(addr).await?; + let name = ServerName::try_from("localhost").map_err(std::io::Error::other)?; + let tls = connector.connect(name, tcp).await?; + Ok::<_, std::io::Error>(TokioIo::new(tls)) + } + })) + .await + .expect("connect") +} + +async fn release(channel: &Channel, token: Option<&str>) -> Result<(), tonic::Status> { + let mut grpc = tonic::client::Grpc::new(channel.clone()); + grpc.ready() + .await + .map_err(|e| tonic::Status::unavailable(e.to_string()))?; + let mut request = tonic::Request::new(pb::ReleaseSessionRequest { + session_id: SESSION.to_owned(), + user_context: Some(pb::UserContext::default()), + ..Default::default() + }); + if let Some(t) = token { + request.metadata_mut().insert( + "authorization", + format!("Bearer {t}").parse().expect("ascii"), + ); + } + grpc.unary( + request, + "/spark.connect.SparkConnectService/ReleaseSession" + .parse() + .expect("path"), + tonic_prost::ProstCodec::::default(), + ) + .await + .map(|_| ()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_token_and_a_certificate_authenticate_over_tls() { + let (addr, _stop, _dir) = start().await; + let plain_tls = tls_channel(addr, None).await; + release(&plain_tls, Some(VIEWER)) + .await + .expect("token over TLS"); + let s = release(&plain_tls, None).await.expect_err("no credential"); + assert_eq!(s.code(), Code::Unauthenticated); + // The certificate alone: `etl-job` holds the operator role. + let with_cert = tls_channel(addr, Some("client")).await; + release(&with_cert, None).await.expect("certificate"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_client_in_clear_gets_no_answer() { + let (addr, _stop, _dir) = start().await; + // h2 in clear on the TLS port: the handshake fails, so no RPC gets through. + let failed = match Channel::from_shared(format!("http://{addr}")) + .expect("uri") + .connect() + .await + { + Err(_) => true, + Ok(channel) => release(&channel, Some(VIEWER)).await.is_err(), + }; + assert!(failed, "an RPC in clear reached a TLS listener"); +} diff --git a/crates/krishiv-exec/src/lib.rs b/crates/krishiv-exec/src/lib.rs index 2e1dafbf7..5a6bade85 100644 --- a/crates/krishiv-exec/src/lib.rs +++ b/crates/krishiv-exec/src/lib.rs @@ -126,11 +126,12 @@ pub use join::{ }; pub use metrics::prometheus; pub use metrics::registry::{ - BUCKETS, CACHE_LABEL, CATALOGUE, CLASS_LABEL, CounterSample, ERROR_CLASS_LABEL, GIT_SHA_LABEL, - GaugeSample, HistogramBucket, HistogramSample, LABEL_ALLOWLIST, LabelDef, - MAX_ERROR_CLASS_VALUES, MetricDef, MetricDict, MetricKind, MetricSource, MetricsRegistry, - MetricsSnapshot, OP_LABEL, OTHER_ERROR_CLASS, QueryOutcome, STATUS_LABEL, STORE_KIND_LABEL, - Status, VERSION_LABEL, cpu_secs, output_rows, scan_rows, spill_bytes, spill_files, + AUTH_METHOD_LABEL, BUCKETS, CACHE_LABEL, CATALOGUE, CLASS_LABEL, CounterSample, + ERROR_CLASS_LABEL, GIT_SHA_LABEL, GaugeSample, HistogramBucket, HistogramSample, + LABEL_ALLOWLIST, LabelDef, MAX_ERROR_CLASS_VALUES, MetricDef, MetricDict, MetricKind, + MetricSource, MetricsRegistry, MetricsSnapshot, OP_LABEL, OTHER_ERROR_CLASS, PRIVILEGE_LABEL, + QueryOutcome, STATUS_LABEL, STORE_KIND_LABEL, SURFACE_LABEL, Status, VERSION_LABEL, cpu_secs, + output_rows, scan_rows, spill_bytes, spill_files, }; pub use metrics::{ DriverStats, ExtraMetrics, Metric, OperatorEntry, OperatorMetrics, OperatorRole, diff --git a/crates/krishiv-exec/src/metrics/prometheus.rs b/crates/krishiv-exec/src/metrics/prometheus.rs index 3c290ced6..20760ec9c 100644 --- a/crates/krishiv-exec/src/metrics/prometheus.rs +++ b/crates/krishiv-exec/src/metrics/prometheus.rs @@ -507,7 +507,26 @@ mod tests { #[test] fn output_round_trips_and_covers_every_sourced_metric() { - let text = render(&full_registry().unwrap().snapshot()); + let mut snapshot = full_registry().unwrap().snapshot(); + // The authentication counters are the server's (`ServerAuth::metrics`), merged into + // the engine's snapshot by `krishiv server`. + let server = |name: &'static str, second: (&str, &str)| CounterSample { + name, + unit: "{request}", + labels: vec![ + ("surface".to_owned(), "connect".to_owned()), + (second.0.to_owned(), second.1.to_owned()), + ], + value: 1.0, + }; + snapshot.merge(&MetricsSnapshot { + counters: vec![ + server("krishiv.auth.failed", ("method", "token")), + server("krishiv.auth.denied", ("privilege", "MODIFY")), + ], + ..MetricsSnapshot::default() + }); + let text = render(&snapshot); let parsed = parse(&text).unwrap(); for def in CATALOGUE { if def.source == MetricSource::NoSource { diff --git a/crates/krishiv-exec/src/metrics/registry.rs b/crates/krishiv-exec/src/metrics/registry.rs index 55aeb6bcf..c3cb33126 100644 --- a/crates/krishiv-exec/src/metrics/registry.rs +++ b/crates/krishiv-exec/src/metrics/registry.rs @@ -174,6 +174,35 @@ pub const ERROR_CLASS_LABEL: LabelDef = LabelDef { values: &[], }; +/// `surface`: the server listener a request came in on (ADR 0046 §8). +pub const SURFACE_LABEL: LabelDef = LabelDef { + name: "surface", + values: &["connect", "admin", "metrics", "ui"], +}; + +/// `method`: how a request tried to authenticate. +pub const AUTH_METHOD_LABEL: LabelDef = LabelDef { + name: "method", + values: &["token", "oidc", "mtls", "throttle", "none"], +}; + +/// `privilege`: the privilege an access check asked for. +pub const PRIVILEGE_LABEL: LabelDef = LabelDef { + name: "privilege", + values: &[ + "SELECT", + "MODIFY", + "CREATE", + "DROP", + "MONITOR", + "USAGE", + "BROWSE", + "OPERATE", + "MANAGE", + "IMPERSONATE", + ], +}; + /// `cache`: which cache. pub const CACHE_LABEL: LabelDef = LabelDef { name: "cache", @@ -216,6 +245,9 @@ pub const LABEL_ALLOWLIST: &[&str] = &[ "store_kind", "version", "git_sha", + "surface", + "method", + "privilege", ]; /// The most distinct `error_class` label values kept per metric; further @@ -473,6 +505,29 @@ pub const CATALOGUE: &[MetricDef] = &[ since: "0.1.0", source: MetricSource::Scrape, }, + MetricDef { + name: "krishiv.auth.failed", + kind: MetricKind::Counter, + unit: "{request}", + labels: &[SURFACE_LABEL, AUTH_METHOD_LABEL], + description: "Requests to `krishiv server` refused as unauthenticated (a missing, \ + malformed, wrong or expired credential) or throttled, by listener and \ + the method tried. Served only by `krishiv server` with authentication \ + configured.", + since: "0.2.0", + source: MetricSource::Scrape, + }, + MetricDef { + name: "krishiv.auth.denied", + kind: MetricKind::Counter, + unit: "{request}", + labels: &[SURFACE_LABEL, PRIVILEGE_LABEL], + description: "Access checks of `krishiv server` callers refused with \ + `KRISHIV_INSUFFICIENT_PERMISSIONS`, by listener and the privilege asked \ + for. Served only by `krishiv server` with authentication configured.", + since: "0.2.0", + source: MetricSource::Scrape, + }, ]; /// The query's outcome, as [`Status::from_error`] classifies it. diff --git a/crates/krishiv-exec/src/metrics/snapshots/krishiv_exec__metrics__registry__tests__report_lists_every_definition_for_the_snapshot_test.snap b/crates/krishiv-exec/src/metrics/snapshots/krishiv_exec__metrics__registry__tests__report_lists_every_definition_for_the_snapshot_test.snap index 9df1021e2..6ae310a7e 100644 --- a/crates/krishiv-exec/src/metrics/snapshots/krishiv_exec__metrics__registry__tests__report_lists_every_definition_for_the_snapshot_test.snap +++ b/crates/krishiv-exec/src/metrics/snapshots/krishiv_exec__metrics__registry__tests__report_lists_every_definition_for_the_snapshot_test.snap @@ -27,3 +27,5 @@ krishiv.io.bytes.read counter By [op,store_kind] -> krishiv_io_bytes_read_bytes_ krishiv.io.request.duration histogram s [op,store_kind] -> krishiv_io_request_duration_seconds krishiv.exec.driver.stalls counter 1 [] -> krishiv_exec_driver_stalls_total krishiv.build.info gauge 1 [version,git_sha] -> krishiv_build_info +krishiv.auth.failed counter {request} [surface=connect|admin|metrics|ui,method=token|oidc|mtls|throttle|none] -> krishiv_auth_failed_total +krishiv.auth.denied counter {request} [surface=connect|admin|metrics|ui,privilege=SELECT|MODIFY|CREATE|DROP|MONITOR|USAGE|BROWSE|OPERATE|MANAGE|IMPERSONATE] -> krishiv_auth_denied_total diff --git a/crates/krishiv-server/CLAUDE.md b/crates/krishiv-server/CLAUDE.md index f5ca05b00..80817eecc 100644 --- a/crates/krishiv-server/CLAUDE.md +++ b/crates/krishiv-server/CLAUDE.md @@ -9,13 +9,14 @@ Admin and UI API server (REST, OpenAPI), serves the web UI. Today: `/api/v1/vers ## Key files Paths are relative to this crate. - `src/lib.rs` `router()`, `serve` and `run_blocking` -- `src/guard.rs` (P3-T002) `Guard`, the one check every guarded admin-listener route asks: `GuardRequest` (presented token, malformed flag, peer, `GuardSurface`, `Access`: `Authenticated`, `Metrics` = `Monitor` on `System("metrics")`, `Monitor` on `System("system")`, `Operate` on `System("queries")`), `Refusal` (401 / 503 / 403 / 500 fail closed), `TokenGuard` (the start-up token of a server without authentication), `PeerAddr` (the peer `serve_capped` records as `ConnectInfo` for the throttle) -- `src/session_guard.rs` (feature `session`) `SessionGuard`: the server's `ServerAuth`, then the ADR 0046 matrix through `RolePolicy` (no role table of its own); a denial writes one `krishiv::audit` record -- `src/internal.rs` the UI backend (P2-T133 part a): `router_with_ui(source, token)` and `router_with_ui_guard(source, guard)` (GET needs `Access::Monitor`, anything else `Access::Operate`: operator), the `UiSource` trait (system-table rows as JSON, cancel), `/api/internal/{health,queries,queries/{id},queries/{id}/cancel,history,compare,resources,events}`; every route needs the token (`Authorization: Bearer` for any method; the `krishiv_ui_token` cookie or `?token=`, percent-decoded, only for GET), query text is shown with its literals masked, at most 16 event streams share one snapshot a period, every response of that router carries the CSP / `frame-ancestors 'none'` headers, columns are allowlisted per endpoint, `events` is SSE at one event a second +- `src/guard.rs` (P3-T002) `Guard`, the one check every guarded admin-listener route asks: `GuardRequest` (presented token, malformed flag, peer, `GuardSurface`, `Access`: `Authenticated`, `Metrics` = `Monitor` on `System("metrics")`, `Monitor` on `System("system")`, `Operate` on `System("queries")`), `Refusal` (401 / 503 / 403 / 500 fail closed), `TokenGuard` (the start-up token of a server without authentication), `PeerAddr` (the peer `serve_capped` records as `ConnectInfo`: the address for the throttle and, over TLS, the verified client leaf; `peer_of` / `peer_leaf_of`, passed on as `GuardRequest::with_peer_leaf` to `Credentials::peer_leaf`) +- `src/session_guard.rs` (feature `session`) `SessionGuard`: the server's `ServerAuth`, then the ADR 0046 matrix through `RolePolicy` (no role table of its own); a denial writes one `krishiv::audit` record and, with `with_denials`, counts into the server's `DeniedCounts` +- `src/internal.rs` the UI backend (P2-T133 part a): `router_with_ui(source, token)` and `router_with_ui_guard(source, guard)` (GET needs `Access::Monitor`, anything else `Access::Operate`: operator), the `UiSource` trait (system-table rows as JSON, cancel), `/api/internal/{health,queries,queries/{id},queries/{id}/cancel,history,compare,resources,events}`; every route needs the token (`Authorization: Bearer` for any method; the `krishiv_ui_token` cookie or `?token=` only for GET), query text is shown with its literals masked, at most 16 event streams share one snapshot a period, every response of that router carries the CSP / `frame-ancestors 'none'` headers, columns are allowlisted per endpoint, `events` is SSE at one event a second - `src/admin.rs` `krishiv server`'s admin endpoints (design P3-T001 §3.5): `health_routes` (`/healthz` open, `/readyz` 200/503 from a ready flag, fixed bodies; never guarded), `with_limits` (request timeout 30 s, SSE `/events` exempt; body limit 2 MiB), `serve_capped` (connection cap, graceful shutdown); `tests/admin.rs` - `src/session_source.rs` `SessionSource` (UI over a runtime) and `EmbeddedUi` (serves it from a host process, token + stop on drop) - `src/metrics.rs` `router_with_metrics`, `router_with_metrics_auth` (with a token, `/metrics` needs `Authorization: Bearer `, compared in constant time; a refusal is a JSON 401 and takes no snapshot; an empty token refuses everyone; a disabled `/metrics` stays a 404 for every caller) and `render` (feature `metrics-prometheus` only). `metrics_route` is the route alone, `metrics_route_guarded` the same behind a `Guard` (`Access::Metrics`, every role), merged into `krishiv server`'s admin router - `examples/console_capture.rs` runs TPC-H queries through an embedded runtime and serves the UI over it; the website's console captures (`site/capture-console.sh` in the docs repo) use it +- `src/tls.rs` (feature `tls`, P3-T002 design §3.5) `serve_capped_tls`: the admin listener over rustls (`tokio-rustls`), ALPN `http/1.1`, each handshake on a task of its own within `HANDSHAKE_TIMEOUT` (10 s) holding a connection slot, the verified client leaf on `TlsCappedStream::peer_leaf`; `tests/tls.rs` (features `session` + `metrics-prometheus` + `tls`) - `tests/serve.rs` server tests - `tests/auth.rs` (features `session` + `metrics-prometheus`) the admin listener with configured tokens: per-role access, 401 / throttle 503, open probes - `tests/metrics.rs` `/metrics` exposition tests (feature `metrics-prometheus` only) diff --git a/crates/krishiv-server/Cargo.toml b/crates/krishiv-server/Cargo.toml index 654021800..25f0c4e74 100644 --- a/crates/krishiv-server/Cargo.toml +++ b/crates/krishiv-server/Cargo.toml @@ -20,6 +20,7 @@ futures = { workspace = true, features = ["alloc"] } getrandom.workspace = true serde_json = { workspace = true, features = ["std"] } tokio = { workspace = true, features = ["rt-multi-thread", "net", "time", "sync"] } +tokio-rustls = { workspace = true, optional = true } tracing.workspace = true [features] @@ -29,6 +30,8 @@ tracing.workspace = true metrics-prometheus = ["dep:krishiv-exec"] # `SessionSource`: the UI backend over an engine runtime (embedded mode), reading the system tables. session = ["dep:krishiv-session", "dep:krishiv-common", "dep:arrow-array", "dep:arrow-schema"] +# The admin listener over TLS (`tls::serve_capped_tls`, P3-T002 design §3.5). +tls = ["dep:tokio-rustls", "krishiv-session?/tls"] [dev-dependencies] krishiv-session = { path = "../krishiv-session" } diff --git a/crates/krishiv-server/src/guard.rs b/crates/krishiv-server/src/guard.rs index f47560fc6..3e27284d1 100644 --- a/crates/krishiv-server/src/guard.rs +++ b/crates/krishiv-server/src/guard.rs @@ -50,7 +50,7 @@ pub enum GuardSurface { } /// What a surface extracted from one request. Never logged: `Debug` hides the token. -#[derive(Clone, Copy)] +#[derive(Clone)] #[non_exhaustive] pub struct GuardRequest<'a> { /// The presented token, if the surface found one. @@ -59,6 +59,8 @@ pub struct GuardRequest<'a> { pub malformed: bool, /// The peer's address, when the listener records it ([`PeerAddr`]). pub peer: Option, + /// The client certificate rustls verified at the TLS handshake, if any. + pub peer_leaf: Option>, /// The surface. pub surface: GuardSurface, /// What the route needs. @@ -73,6 +75,7 @@ impl<'a> GuardRequest<'a> { token: None, malformed: false, peer: None, + peer_leaf: None, surface, access, } @@ -98,6 +101,13 @@ impl<'a> GuardRequest<'a> { self.peer = peer; self } + + /// The same request with the connection's verified client certificate. + #[must_use] + pub fn with_peer_leaf(mut self, leaf: Option>) -> Self { + self.peer_leaf = leaf; + self + } } impl fmt::Debug for GuardRequest<'_> { @@ -182,20 +192,45 @@ pub(crate) fn same_secret(a: &[u8], b: &[u8]) -> bool { std::hint::black_box(diff) == 0 } -/// The peer address of an admin connection, recorded by [`crate::admin::serve_capped`] as -/// `ConnectInfo` for the failure throttle. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct PeerAddr(pub SocketAddr); +/// The peer of an admin connection, recorded by [`crate::admin::serve_capped`] as +/// `ConnectInfo`: its address (the failure throttle's key) and, over TLS, the client +/// certificate rustls verified. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PeerAddr { + /// The peer's address. + pub addr: SocketAddr, + /// The verified client leaf certificate (DER), if the connection is TLS and the client + /// sent one. + pub leaf: Option>, +} + +impl PeerAddr { + /// A peer at `addr` with no client certificate. + #[must_use] + pub fn new(addr: SocketAddr) -> Self { + Self { addr, leaf: None } + } +} impl Connected> for PeerAddr { fn connect_info(stream: IncomingStream<'_, crate::admin::CappedListener>) -> Self { - Self(*stream.remote_addr()) + Self::new(*stream.remote_addr()) + } +} + +#[cfg(feature = "tls")] +impl Connected> for PeerAddr { + fn connect_info(stream: IncomingStream<'_, crate::tls::TlsCappedListener>) -> Self { + Self { + addr: *stream.remote_addr(), + leaf: stream.io().peer_leaf(), + } } } impl Connected for PeerAddr { fn connect_info(addr: SocketAddr) -> Self { - Self(addr) + Self::new(addr) } } @@ -204,7 +239,15 @@ impl Connected for PeerAddr { pub fn peer_of(extensions: &axum::http::Extensions) -> Option { extensions .get::>() - .map(|c| c.0.0) + .map(|c| c.0.addr) +} + +/// The verified client certificate recorded on `extensions`, if the connection carried one. +#[must_use] +pub fn peer_leaf_of(extensions: &axum::http::Extensions) -> Option> { + extensions + .get::>() + .and_then(|c| c.0.leaf.clone()) } #[cfg(test)] diff --git a/crates/krishiv-server/src/internal.rs b/crates/krishiv-server/src/internal.rs index 298877986..e043dfc5b 100644 --- a/crates/krishiv-server/src/internal.rs +++ b/crates/krishiv-server/src/internal.rs @@ -25,7 +25,9 @@ use axum::response::{IntoResponse, Response}; use axum::routing::{get, post}; use serde_json::{Map, Value, json}; -use crate::guard::{Access, Guard, GuardRequest, GuardSurface, Refusal, TokenGuard, peer_of}; +use crate::guard::{ + Access, Guard, GuardRequest, GuardSurface, Refusal, TokenGuard, peer_leaf_of, peer_of, +}; /// One system-table row as JSON (column name to value). pub type Row = Map; @@ -277,7 +279,8 @@ async fn require_token( let request = GuardRequest::new(GuardSurface::Ui, access) .with_token(token.as_deref()) .with_malformed(malformed) - .with_peer(peer_of(req.extensions())); + .with_peer(peer_of(req.extensions())) + .with_peer_leaf(peer_leaf_of(req.extensions())); match s.guard.check(request).await { Ok(()) => next.run(req).await, Err(Refusal::Unauthenticated) => { diff --git a/crates/krishiv-server/src/lib.rs b/crates/krishiv-server/src/lib.rs index 24895fe4f..996d87fcc 100644 --- a/crates/krishiv-server/src/lib.rs +++ b/crates/krishiv-server/src/lib.rs @@ -27,6 +27,8 @@ pub mod metrics; pub mod admin; pub mod guard; pub mod internal; +#[cfg(feature = "tls")] +pub mod tls; #[cfg(feature = "session")] pub mod session_guard; diff --git a/crates/krishiv-server/src/metrics.rs b/crates/krishiv-server/src/metrics.rs index 932b24548..081e9d14e 100644 --- a/crates/krishiv-server/src/metrics.rs +++ b/crates/krishiv-server/src/metrics.rs @@ -21,7 +21,9 @@ use krishiv_exec::{MetricsSnapshot, prometheus}; #[cfg(test)] use crate::guard::same_secret; -use crate::guard::{Access, Guard, GuardRequest, GuardSurface, Refusal, TokenGuard, peer_of}; +use crate::guard::{ + Access, Guard, GuardRequest, GuardSurface, Refusal, TokenGuard, peer_leaf_of, peer_of, +}; /// Provides the snapshot a scrape renders: called once per `GET /// /metrics`, on the request task. Must be cheap: [`MetricsSnapshot`] is @@ -155,7 +157,8 @@ pub fn metrics_route_guarded( let check = GuardRequest::new(GuardSurface::Metrics, Access::Metrics) .with_token(token) .with_malformed(malformed) - .with_peer(peer_of(request.extensions())); + .with_peer(peer_of(request.extensions())) + .with_peer_leaf(peer_leaf_of(request.extensions())); if let Err(refusal) = guard.check(check).await { return refused(refusal); } diff --git a/crates/krishiv-server/src/session_guard.rs b/crates/krishiv-server/src/session_guard.rs index 37cb8cbef..7ca42b8f4 100644 --- a/crates/krishiv-server/src/session_guard.rs +++ b/crates/krishiv-server/src/session_guard.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use futures::future::BoxFuture; -use krishiv_session::auth::{Authenticator, Credentials, Role, RolePolicy, Surface}; +use krishiv_session::auth::{Authenticator, Credentials, DeniedCounts, Role, RolePolicy, Surface}; use krishiv_session::{PolicyVersion, Privilege, Securable}; use crate::guard::{Access, Guard, GuardRequest, GuardSurface, Refusal}; @@ -20,6 +20,7 @@ use crate::guard::{Access, Guard, GuardRequest, GuardSurface, Refusal}; #[derive(Clone)] pub struct SessionGuard { auth: Arc, + denials: Option>, } impl std::fmt::Debug for SessionGuard { @@ -32,7 +33,18 @@ impl SessionGuard { /// A guard over `auth` (normally the server's `ServerAuth`). #[must_use] pub fn new(auth: Arc) -> Self { - Self { auth } + Self { + auth, + denials: None, + } + } + + /// The same, counting its denials into `denials` (`krishiv_auth_denied_total`, ADR 0046 + /// §8; normally `ServerAuth::denials`). + #[must_use] + pub fn with_denials(mut self, denials: Arc) -> Self { + self.denials = Some(denials); + self } } @@ -69,6 +81,7 @@ impl Guard for SessionGuard { }; let mut credentials = Credentials::new(peer, surface); credentials.malformed = request.malformed; + credentials.peer_leaf = request.peer_leaf.as_deref(); if let Some(token) = request.token { credentials = credentials.with_bearer(token); } @@ -96,6 +109,9 @@ impl Guard for SessionGuard { error_class = "KRISHIV_INSUFFICIENT_PERMISSIONS", "access denied" ); + if let Some(d) = &self.denials { + d.record(surface, privilege); + } Err(Refusal::Denied) }) } diff --git a/crates/krishiv-server/src/tls.rs b/crates/krishiv-server/src/tls.rs new file mode 100644 index 000000000..e9a0aa004 --- /dev/null +++ b/crates/krishiv-server/src/tls.rs @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: Apache-2.0 +//! The admin listener over TLS (P3-T002 design §3.5; feature `tls`): [`serve_capped_tls`] is +//! [`crate::admin::serve_capped`] with each connection's TLS handshake run on a task of its own, +//! within [`HANDSHAKE_TIMEOUT`], so a slow or silent client delays no other and holds its slot for +//! at most that long. The client certificate rustls verified rides on the connection +//! ([`crate::guard::PeerAddr`]) for the authenticator. + +use std::future::Future; +use std::io; +use std::net::SocketAddr; +use std::pin::Pin; +use std::sync::Arc; +use std::task::{Context, Poll}; +use std::time::Duration; + +use axum::Router; +use tokio::io::{AsyncRead, AsyncWrite, ReadBuf}; +use tokio::net::{TcpListener, TcpStream}; +use tokio::sync::{OwnedSemaphorePermit, Semaphore, mpsc}; +use tokio::task::JoinHandle; +use tokio_rustls::TlsAcceptor; +use tokio_rustls::rustls::ServerConfig; +use tokio_rustls::server::TlsStream; + +/// The longest a connection may take from accept to a finished TLS handshake. +pub const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(10); + +/// Serves `app` on `listener` over TLS with `config` (the ALPN list is set to `http/1.1`: the +/// admin server speaks HTTP/1.1 only; the certificate resolver is shared, so a reload of the +/// server's TLS material reaches new handshakes here too), at most `max_connections` open at +/// once (a connection in its handshake holds a slot), until `shutdown` resolves. +/// +/// # Errors +/// An I/O error of the server. +pub async fn serve_capped_tls( + listener: TcpListener, + app: Router, + max_connections: usize, + config: &ServerConfig, + shutdown: impl Future + Send + 'static, +) -> io::Result<()> { + let mut config = config.clone(); + config.alpn_protocols = vec![b"http/1.1".to_vec()]; + let local = listener.local_addr()?; + let permits = Arc::new(Semaphore::new( + max_connections.clamp(1, Semaphore::MAX_PERMITS), + )); + let (tx, rx) = mpsc::channel(64); + let acceptor = tokio::spawn(accept_loop( + listener, + TlsAcceptor::from(Arc::new(config)), + permits, + tx, + )); + let listener = TlsCappedListener { + rx, + local, + _acceptor: AbortOnDrop(acceptor), + }; + axum::serve( + listener, + app.into_make_service_with_connect_info::(), + ) + .with_graceful_shutdown(shutdown) + .await +} + +async fn accept_loop( + listener: TcpListener, + acceptor: TlsAcceptor, + permits: Arc, + tx: mpsc::Sender<(TlsCappedStream, SocketAddr)>, +) { + let mut backoff = Duration::from_millis(10); + loop { + let Ok(permit) = Arc::clone(&permits).acquire_owned().await else { + return; + }; + let (stream, addr) = match listener.accept().await { + Ok(accepted) => accepted, + // Out of file descriptors every retry fails: log and back off. + Err(e) => { + tracing::warn!(target: "krishiv::server", error = %e, "admin accept failed"); + tokio::time::sleep(backoff).await; + backoff = (backoff * 2).min(Duration::from_secs(1)); + continue; + } + }; + backoff = Duration::from_millis(10); + let (acceptor, sender) = (acceptor.clone(), tx.clone()); + tokio::spawn(async move { + match tokio::time::timeout(HANDSHAKE_TIMEOUT, acceptor.accept(stream)).await { + Ok(Ok(tls)) => { + let leaf = tls + .get_ref() + .1 + .peer_certificates() + .and_then(<[_]>::first) + .map(|c| Arc::from(c.as_ref())); + let stream = TlsCappedStream { + inner: Box::new(tls), + leaf, + _permit: permit, + }; + let _ = sender.send((stream, addr)).await; + } + Ok(Err(e)) => { + tracing::debug!( + target: "krishiv::server", + peer = %addr, + error = %e, + "admin TLS handshake failed" + ); + } + // Too slow: drop it and its permit. + Err(_) => {} + } + }); + if tx.is_closed() { + return; + } + } +} + +/// Aborts its task when dropped, so a stopped server leaves no accept loop behind. +#[derive(Debug)] +struct AbortOnDrop(JoinHandle<()>); + +impl Drop for AbortOnDrop { + fn drop(&mut self) { + self.0.abort(); + } +} + +/// The TLS admin listener: the connections whose handshake finished. +#[derive(Debug)] +pub struct TlsCappedListener { + rx: mpsc::Receiver<(TlsCappedStream, SocketAddr)>, + local: SocketAddr, + _acceptor: AbortOnDrop, +} + +impl axum::serve::Listener for TlsCappedListener { + type Io = TlsCappedStream; + type Addr = SocketAddr; + + async fn accept(&mut self) -> (TlsCappedStream, SocketAddr) { + match self.rx.recv().await { + Some(accepted) => accepted, + // The accept loop ended (it never does while the listener lives): wait as a + // closed listener would. + None => std::future::pending().await, + } + } + + fn local_addr(&self) -> io::Result { + Ok(self.local) + } +} + +/// A TLS admin connection holding one of the listener's permits. +#[derive(Debug)] +pub struct TlsCappedStream { + inner: Box>, + leaf: Option>, + _permit: OwnedSemaphorePermit, +} + +impl TlsCappedStream { + /// The client's leaf certificate (DER) rustls verified at the handshake, if it sent one. + #[must_use] + pub fn peer_leaf(&self) -> Option> { + self.leaf.clone() + } +} + +impl AsyncRead for TlsCappedStream { + fn poll_read( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + Pin::new(self.inner.as_mut()).poll_read(cx, buf) + } +} + +impl AsyncWrite for TlsCappedStream { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + Pin::new(self.inner.as_mut()).poll_write(cx, buf) + } + + fn poll_flush(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + Pin::new(self.inner.as_mut()).poll_flush(cx) + } + + fn poll_shutdown(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + Pin::new(self.inner.as_mut()).poll_shutdown(cx) + } +} diff --git a/crates/krishiv-server/tests/metrics.rs b/crates/krishiv-server/tests/metrics.rs index 142d8bab3..c1f7cda3f 100644 --- a/crates/krishiv-server/tests/metrics.rs +++ b/crates/krishiv-server/tests/metrics.rs @@ -320,7 +320,30 @@ fn label<'a>(sample: &'a Sample, name: &str) -> Option<&'a str> { #[tokio::test] async fn metrics_serves_every_sourced_catalogue_metric() { - let addr = serve(router_with_metrics(true, provider(&full_registry()))).await; + // `krishiv server` merges its authentication counters into the engine's snapshot + // (`ServerAuth::metrics`); a sample of each stands in for them here. + let registry = full_registry(); + let with_auth: SnapshotProvider = Arc::new(move || { + let mut snapshot = registry.snapshot(); + let sample = |name: &'static str, second: (&str, &str)| krishiv_exec::CounterSample { + name, + unit: "{request}", + labels: vec![ + ("surface".to_owned(), "metrics".to_owned()), + (second.0.to_owned(), second.1.to_owned()), + ], + value: 1.0, + }; + snapshot.merge(&krishiv_exec::MetricsSnapshot { + counters: vec![ + sample("krishiv.auth.failed", ("method", "none")), + sample("krishiv.auth.denied", ("privilege", "MONITOR")), + ], + ..krishiv_exec::MetricsSnapshot::default() + }); + snapshot + }); + let addr = serve(router_with_metrics(true, with_auth)).await; let response = get(addr, "/metrics").await; assert!(response.starts_with("HTTP/1.1 200"), "{response}"); assert!( diff --git a/crates/krishiv-server/tests/tls.rs b/crates/krishiv-server/tests/tls.rs new file mode 100644 index 000000000..d7f27963f --- /dev/null +++ b/crates/krishiv-server/tests/tls.rs @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: Apache-2.0 +//! The admin listener over TLS (P3-T002 design §3.4, §3.5): `serve_capped_tls` with the server's +//! `ServerTls`, a bearer token over TLS, plain HTTP refused, and a client certificate that +//! authenticates by itself with the role its name is listed under. Fixtures: +//! `crates/krishiv-session/tests/fixtures/tls/README.md`. +#![cfg(all( + unix, + feature = "session", + feature = "metrics-prometheus", + feature = "tls" +))] +#![expect( + clippy::expect_used, + reason = "integration-test helpers are outside #[test] fns" +)] + +use std::net::SocketAddr; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::sync::atomic::AtomicBool; + +use krishiv_exec::MetricsSnapshot; +use krishiv_server::admin::{HttpLimits, health_routes, with_limits}; +use krishiv_server::guard::Guard; +use krishiv_server::internal::{Row, SystemTable, UiFuture, UiSource, router_with_ui_guard}; +use krishiv_server::metrics::metrics_route_guarded; +use krishiv_server::session_guard::SessionGuard; +use krishiv_server::tls::serve_capped_tls; +use krishiv_session::auth::{AuthConfig, ServerAuth, ServerTls, TlsConfig}; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; +use tokio::net::{TcpListener, TcpStream}; +use tokio_rustls::TlsConnector; +use tokio_rustls::rustls::pki_types::pem::PemObject as _; +use tokio_rustls::rustls::pki_types::{CertificateDer, PrivateKeyDer, ServerName}; +use tokio_rustls::rustls::{ClientConfig, RootCertStore}; + +const VIEWER: &str = "viewer-token-0123456789abcdef0123456789"; + +struct Empty; + +impl UiSource for Empty { + fn rows(&self, _: SystemTable) -> UiFuture<'_, Vec> { + Box::pin(async { Ok(Vec::new()) }) + } + fn cancel<'a>(&'a self, _: &'a str) -> UiFuture<'a, bool> { + Box::pin(async { Ok(false) }) + } +} + +fn fixture(name: &str) -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join(format!("../krishiv-session/tests/fixtures/tls/{name}")) +} + +/// `src` copied into `dir` with mode 0600 (git keeps no modes), as a path string. +fn private_copy(dir: &Path, src: &Path, name: &str) -> String { + let path = dir.join(name); + std::fs::copy(src, &path).expect("copy"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + path.to_str().expect("utf-8 path").to_owned() +} + +/// The admin router of `krishiv server` over TLS: a viewer token, client certificates +/// `optional` with the principal from the CN, `etl-job` an operator. +async fn start() -> (SocketAddr, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("tempdir"); + let secret = dir.path().join("v"); + std::fs::write(&secret, VIEWER).expect("write secret"); + let secret = private_copy(dir.path(), &secret, "viewer"); + let cert = fixture("server.pem"); + let key = private_copy(dir.path(), &fixture("server.key"), "server.key"); + let ca = format!("file:{}", fixture("ca.pem").display()); + let secret_ref = format!("file:{secret}"); + let settings = [ + ("krishiv.auth.tokens.prom.secret", secret_ref.as_str()), + ("krishiv.auth.tokens.prom.principal", "prometheus"), + ("krishiv.auth.tokens.prom.role", "viewer"), + ("krishiv.auth.clientCertificates", "optional"), + ("krishiv.auth.clientCa", ca.as_str()), + ("krishiv.auth.mtls.principalFrom", "cn"), + ("krishiv.auth.roles.operator", "etl-job"), + ("krishiv.server.tls.certFile", cert.to_str().expect("utf-8")), + ("krishiv.server.tls.keyFile", key.as_str()), + ]; + let auth_config = AuthConfig::from_settings(settings).expect("auth config"); + let tls_config = TlsConfig::from_settings(settings).expect("tls config"); + let tls = ServerTls::new(&tls_config, &auth_config.mtls) + .expect("server tls") + .expect("tls on"); + let auth = ServerAuth::new(&auth_config).expect("server auth"); + let guard: Arc = Arc::new(SessionGuard::new(Arc::new(auth))); + let metrics = metrics_route_guarded( + true, + Arc::new(MetricsSnapshot::default), + Some(Arc::clone(&guard)), + ); + let app = router_with_ui_guard(Arc::new(Empty), guard) + .merge(health_routes(Arc::new(AtomicBool::new(true)))) + .merge(metrics); + let limits = HttpLimits::default(); + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("addr"); + let config = tls.server_config(); + tokio::spawn(async move { + serve_capped_tls( + listener, + with_limits(app, limits), + limits.max_connections, + &config, + std::future::pending(), + ) + .await + }); + (addr, dir) +} + +/// A client trusting the test CA, presenting the fixture `identity` if given. +fn client(identity: Option<&str>) -> TlsConnector { + let mut roots = RootCertStore::empty(); + roots + .add(CertificateDer::from_pem_file(fixture("ca.pem")).expect("ca")) + .expect("add ca"); + let provider = Arc::new(tokio_rustls::rustls::crypto::aws_lc_rs::default_provider()); + let builder = ClientConfig::builder_with_provider(provider) + .with_safe_default_protocol_versions() + .expect("versions") + .with_root_certificates(roots); + let config = match identity { + None => builder.with_no_client_auth(), + Some(name) => { + let chain = + vec![CertificateDer::from_pem_file(fixture(&format!("{name}.pem"))).expect("cert")]; + let key = PrivateKeyDer::from_pem_file(fixture(&format!("{name}.key"))).expect("key"); + builder + .with_client_auth_cert(chain, key) + .expect("client auth") + } + }; + TlsConnector::from(Arc::new(config)) +} + +async fn exchange( + mut s: S, + path: &str, + token: Option<&str>, +) -> String { + let auth = token.map_or(String::new(), |t| format!("Authorization: Bearer {t}\r\n")); + let request = format!("GET {path} HTTP/1.1\r\nHost: x\r\n{auth}Connection: close\r\n\r\n"); + s.write_all(request.as_bytes()).await.expect("write"); + let mut out = Vec::new(); + // A server that closes without `close_notify` ends the read with an error; keep what came. + let _ = s.read_to_end(&mut out).await; + String::from_utf8_lossy(&out).into_owned() +} + +/// The response to `GET path` over TLS, presenting `identity` and `token`. +async fn get(addr: SocketAddr, path: &str, identity: Option<&str>, token: Option<&str>) -> String { + let tcp = TcpStream::connect(addr).await.expect("connect"); + let name = ServerName::try_from("localhost").expect("name"); + let tls = client(identity) + .connect(name, tcp) + .await + .expect("handshake"); + exchange(tls, path, token).await +} + +fn status(response: &str) -> &str { + response.get(9..12).unwrap_or("") +} + +#[tokio::test] +async fn a_bearer_token_works_over_tls_and_plain_http_gets_nothing() { + let (addr, _dir) = start().await; + assert_eq!(status(&get(addr, "/healthz", None, None).await), "200"); + assert_eq!( + status(&get(addr, "/metrics", None, Some(VIEWER)).await), + "200" + ); + assert_eq!(status(&get(addr, "/metrics", None, None).await), "401"); + // HTTP in clear on the TLS port: the handshake fails and no HTTP answer comes back. + let plain = TcpStream::connect(addr).await.expect("connect"); + let answer = exchange(plain, "/healthz", None).await; + assert!(!answer.starts_with("HTTP/"), "{answer:?}"); +} + +#[tokio::test] +async fn a_client_certificate_authenticates_with_its_listed_role() { + let (addr, _dir) = start().await; + let queries = "/api/internal/queries"; + // `etl-job` (the CN) is an operator: it monitors other principals' queries, no token. + assert_eq!( + status(&get(addr, queries, Some("client"), None).await), + "200" + ); + // The viewer token alone does not reach them (403), and nothing at all is 401. + assert_eq!(status(&get(addr, queries, None, Some(VIEWER)).await), "403"); + assert_eq!(status(&get(addr, queries, None, None).await), "401"); + // `reporting` holds no role: authenticated, but `/metrics` needs one. + assert_eq!( + status(&get(addr, "/metrics", Some("nosan"), None).await), + "403" + ); +} diff --git a/crates/krishiv-session/CLAUDE.md b/crates/krishiv-session/CLAUDE.md index c5fd9dfbe..c77f38f3c 100644 --- a/crates/krishiv-session/CLAUDE.md +++ b/crates/krishiv-session/CLAUDE.md @@ -12,7 +12,7 @@ re-exports it; the Python facade and the Connect and Flight SQL servers (L6) wra Paths are relative to this crate. - `src/lib.rs` public entry and re-exports; `public-api.txt` the reviewed public surface - `src/session.rs` `Session` and the query pipeline entry -- `src/runtime.rs` `EngineRuntime` (design P2-E1): what one process shares — executor, memory root, spill directory, the `IoScheduler` with its file-metadata and block caches and the local store (R3), metrics and format registries, query runner, running-query registry (`reserve`/`attach`/`cancel_where`). Sessions are light children (`SessionBuilder::runtime`; `Session::new_session`, `Session::clone_session`, `Session::close`, `Session::sql_with` in `src/session/lifecycle.rs`; `Session::from_plan_with` beside `from_plan`); a session built alone gets a private runtime, so embedded use is unchanged. Runtime-owned keys (`runtime_owned`: cluster scope, `krishiv.memory.*`, `krishiv.spill.*`, `krishiv.io.*`) on a session builder of a supplied runtime are `CANNOT_MODIFY_STATIC_CONFIG`, as `SET` of one is; `close` and `attach` share the registry lock, so no query registers on a closed session. `krishiv.session.memoryLimit` gives a session its own pool under the root (`TreePool::tenant`). A query's I/O goes through one `IoContext` (`QueryEnv::io`), so `system.queries.bytes_read/written` are its own, not the runtime's totals; `tests/runtime.rs`, `tests/runtime_memory.rs`, `tests/runtime_io.rs` +- `src/runtime.rs` `EngineRuntime` (design P2-E1): what one process shares — executor, memory root, spill directory, the `IoScheduler` with its file-metadata and block caches and the local store (R3), metrics and format registries, query runner, running-query registry (`reserve`/`attach`/`cancel_where`). Sessions are light children (`SessionBuilder::runtime`; `Session::new_session`, `Session::new_session_as` (another principal and policy: a server's per-caller sessions, P3-T002), `Session::clone_session`, `Session::close`, `Session::sql_with` in `src/session/lifecycle.rs`; `Session::from_plan_with` beside `from_plan`); a session built alone gets a private runtime, so embedded use is unchanged. Runtime-owned keys (`runtime_owned`: cluster scope, `krishiv.memory.*`, `krishiv.spill.*`, `krishiv.io.*`) on a session builder of a supplied runtime are `CANNOT_MODIFY_STATIC_CONFIG`, as `SET` of one is; `close` and `attach` share the registry lock, so no query registers on a closed session. `krishiv.session.memoryLimit` gives a session its own pool under the root (`TreePool::tenant`). A query's I/O goes through one `IoContext` (`QueryEnv::io`), so `system.queries.bytes_read/written` are its own, not the runtime's totals; `tests/runtime.rs`, `tests/runtime_memory.rs`, `tests/runtime_io.rs` - `src/cancel.rs` (design P2-E1 §4) `CancelReason` (`user`, `timeout`, `admin`, `quota`, `drain`, `client_gone`) and its error (`OPERATION_CANCELED`, `KRISHIV_QUERY_TIMEOUT`, `KRISHIV_QUERY_KILLED`, all HY008), applied through `QueryContext::cancel_with` (first reason wins; drivers and the stream end with it; `system.queries.state` stays `cancelled`, `cancel_reason` names it), and `Deadlines`, the runtime's one timer task (min-heap of weak queries, `krishiv.query.maxExecutionTime` and `QueryOptions::deadline`, the shorter wins). The runtime's registry entry exists from the query's row on (`EngineRuntime::reserve`/`attach`), so a cancel before the context is kept and applied at attach. APIs: `Session::cancel` (own session), `Session::interrupt_all`, `EngineRuntime::cancel` (any session, not reachable from SQL); `tests/runtime_cancel.rs`. By tag (P3-T064 part 2, design §4.3, §4.5): `QueryOptions::tags` are request tags (opaque, validated by the caller; stored as `(tag, "")` beside the session's tags in the row, the admission request and the action), `Session::interrupt_tag` (own session, exact match on request tags only) and `EngineRuntime::cancel_by_tag` (a selector `k` or `k=v` over every tag, a caller-supplied session filter, ids returned and no skipped count) pick through `Actions::cancel_tagged`, so a statement still analyzing or queued is reached too; `tests/request_tags.rs` - `src/action.rs` (P3-T021 follow-up, ruling R1) the cancellation of an action before its query exists: every `Session::on_executor` action registers in `EngineRuntime::actions` while it runs, so `cancel`/`interrupt_all` end it during analysis and planning (its future is dropped, the reason's error); from its first `reserve` it hands over (a reason already taken is the reservation's pending one; later cancels go to the reserved rows), so a running query ends as before; `action::commit_point()` goes before a side effect a drop would leave half done (`saveAsTable` overwrite's drop, `DirectoryWrite::begin`), after which a cancel waits for the reservation; nested actions record their parent, so a reservation hands over the whole chain; `action::cancelled()` is the wait for planning-time readers that hold their own cancellation (the Delta log reads). Limits: a later statement of a handed-over action (a script's next statement) is not dropped during its analysis; `interrupt_all` also ends non-query actions (`schema`, `explain`), unlike Spark Connect's `interruptAll` (executions only), to settle before Connect wires `Interrupt`; a statement ended before its query existed (analysis, admission) gets its `system.queries.cancel_reason` from the action (`on_executor_reasoned`, `QueryLog::close_unrun` via `action::current_reason`); `tests/analysis_cancel.rs` - `src/history.rs` the runtime-wide `system.queries` rows (`QueryHistory`: UUIDv7 minting; at 1,000 rows a principal loses its oldest finished row; `krishiv.system.queries.capacity` evicts the oldest finished row of the principal holding the most, and so does going past 64 MiB of rows (`QueryRow::heap_bytes`); running rows are never evicted), the public `QueryId` (parsed as Java's `UUID.fromString(id).toString`, as Spark Connect does) and `QueryOptions` (`Session::sql_with`: a caller operation id held by the statement's handles, `Operations` per session as Spark Connect's `SessionHolder`: `INVALID_HANDLE.FORMAT`, `OPERATION_ALREADY_EXISTS` while held, `OPERATION_ABANDONED` for 30 minutes after its last use); `system.rs` `QueryLog` is a session's view of it (tags, trace, principal filter; `Monitor` asks the policy for `MONITOR` on `system` to show every principal's rows); `tests/runtime_queries.rs` @@ -27,8 +27,8 @@ Paths are relative to this crate. - `src/admission.rs` the admission seam (E5): `AdmissionController::admit(&QueryRequest) -> Permit` asked once per query in `run_on_query_logged`; default `Unlimited`; a waiting `admit` shows the query `queued` in `system.queries`, a refusal closes the row `failed`, a drop while queued `cancelled`; the permit (slot, optional per-query memory budget) lives in the query's `QueryFold` and releases on drop; `tests/admission.rs` - `src/store_catalog.rs` (P3-T020a) `StoreCatalog`: the in-memory `CatalogStore` as a `krishiv_plan` `Catalog` and `CatalogWrite` (namespaces, create/alter/drop table; `alter_table` runs `apply_changes` under file-source rules and is refused with `INTERNAL_ERROR` when the table's `CatalogTable::version` (the store's `stamp` of the entry, taken from its epoch under its lock) is not the one the caller read; create checks names, provider, partition columns (moved last) and the managed location and makes its directory; a drop deletes a managed table's directory only after the canonical containment and link checks, before the metadata changes; a view is no table to drop or alter; `rename_table` renames within a namespace like `ALTER TABLE .. RENAME TO`: a managed table's directory moves first (checked: inside the warehouse, no link), then the metadata, swapped in one store call (`CatalogStore::rename_table_entry`: both names or neither); an existing target directory is `LOCATION_ALREADY_EXISTS`, and a failed swap puts the directory back; a table never changes namespace (`_LEGACY_ERROR_TEMP_1073`, as Spark's session catalog)). No existing caller uses it yet; the DDL commands move onto it in later commits - `src/resource_groups/` (P3-T003, design `P3-T003-workload-management.md`) `ResourceGroups`, an `AdmissionController` over a Trino-shaped group file (`config.rs`: subset only, unknown/CPU/template keys are `KRISHIV_RESOURCE_GROUP_CONFIG_INVALID`), selectors (`select.rs`: user, userGroup, source, clientTags `k`/`k=v`, queryType; first match, else `defaultGroup`), and the tree with its dispatcher (`tree.rs`: one `Mutex`, nothing sent or dropped under it; `weighted_fair` by running/weight, `query_priority`; hard concurrency, `maxQueued` refusal `KRISHIV_RESOURCE_GROUP_QUEUE_FULL`, soft memory from `GroupSlot::set_reserved`, scan quota windows charged at slot drop, `KRISHIV_RESOURCE_GROUP_QUOTA_EXHAUSTED`); `Permit::slot::()` reaches the guard. `krishiv.workload.resourceGroups.file` builds one controller per `EngineRuntime` (`ResourceGroups::from_file`; a session's own `admission_controller` wins), so the limits hold across its sessions (`tests/resource_groups_file.rs`). The permit carries the group (`system.queries.resource_group`, `queued_ns`), the group's workload class and its abort priority (`MemoryPool::child_with_priority`); scanned bytes reach the scan quota. Not wired yet: `source`/`statement_class` on the session's `QueryRequest`, `softMemoryLimit` reporting -- `src/access.rs` `StatementInfo` / `Session::statement_info` (class + read securables with columns), write/DDL checks (`authorize_statement`), the `krishiv::audit` denial record, `PathPrefixPolicy` (local roots; object-store prefixes by store and whole key segments, `with_object_prefixes`; `from_config` is the server-mode confinement of ADR 0046 §3a from `krishiv.io.local.allowedRoots` / `krishiv.io.objectStore.allowedPrefixes`, default empty, every role, absolute roots; Azure prefixes name their account; with `allowedRoots` set the runtime's local store refuses listings that follow a link out of the roots, `SettledLocalFileSystem::confined`; `tests/path_confinement.rs`). Server-scoped store options (P3-T067b; `krishiv_io::is_server_scoped_option`: every `fs.*` key but an inline key, secret, token or SAS) need `Manage` on `System("cluster")` per read (`bind_files`, table `WITH`), per write (`authorize_write_options`, before the target store is built; a table's own stored options are not asked again), on CREATE TABLE `OPTIONS` / `SET SERDEPROPERTIES`, on SQL `SET`, and per query while the session changed one from its builder config (`ConfStore::store_settings_changed`); `tests/server_egress.rs` -- `src/auth/` (P3-T002, design `design/P3-T002-authentication.md`, ADR 0046) server authentication: `Role` (viewer < operator < admin), `Credentials` / `Authenticated` / `AuthFailure` (Spark's two `UNAUTHENTICATED` messages), the `Authenticator` trait and `ServerAuth` (throttle, then static tokens; one `krishiv::auth` warn record and one `(surface, method)` counter per failure, never the credential); `config.rs` `AuthConfig::from_settings` (`krishiv.auth.tokens..{secret,principal,kind,role}`, `spark.connect.authenticate.token` = one admin token named `spark`, `krishiv.auth.failureThrottle.*`; unknown `krishiv.auth.*` keys refused); `secret_ref.rs` `env:`/`file:` references (a file readable by group or others refuses startup); `tokens.rs` SHA-256 digests compared in constant time against every entry (secrets >= 32 bytes, no two tokens share one); `throttle.rs` per-address failure blocks, bounded at 65,536 addresses; `mtls.rs` client certificates (`krishiv.auth.clientCertificates` off/optional/required, `clientCa`, `mtls.principalFrom` san-uri/san-dns/cn: exactly one value of that kind or refused, `mtls.userPrincipals`; roles from `krishiv.auth.roles.{viewer,operator,admin}` lists via `config.rs` `RoleLists`; a bearer token next to a certificate must name the same principal; `required` never falls back to a token); `der.rs` `CertNames::from_der`, the bounded no-recursion DER walk of the leaf's subject CN and DNS/URI SANs (fuzz target `fuzz/fuzz_targets/cert_names.rs` includes it by path); `tls_config.rs` `TlsConfig` (`krishiv.server.tls.{certFile,keyFile,minVersion}`) and `check_listener` (startup refusals: non-loopback without auth, non-loopback tokens without TLS, client certificates without TLS); `tls.rs` (feature `tls`) `ServerTls`, the rustls `ServerConfig` on aws-lc-rs with h2/http1.1 ALPN, WebPKI client verifier, a key file checked like a secret file and `reload()` swapping the certified key (a failed reload keeps the old one); `mtls_tests.rs` with in-memory handshakes against `tests/fixtures/tls/`; `oidc_config.rs` `OidcConfig` (`krishiv.auth.oidc.{issuer,audience,jwksUri,algorithms,principalClaim,roleClaim,clockSkew,jwksRefresh,allowInsecureIssuer}` and `krishiv.auth.oidc.roles.{viewer,operator,admin}`, the role-claim values by role, a namespace separate from the certificate names of `krishiv.auth.roles.*`; refused without feature `oidc`; locations https only unless the test flag, with a host, no userinfo, no fragment); `jose.rs` the pure JWS/JWK set/discovery parsers (16 KiB token cap, canonical unpadded base64url, duplicate JSON members refused, `alg` only RS256/384/512 and ES256/384, `crit` refused, header key members ignored; `check_claims`: `exp`/`nbf` with skew, exact `iss`, `aud`, principal and groups; fuzz target `fuzz/fuzz_targets/jwt_parse.rs` includes it by path); `oidc.rs` (feature `oidc`) signatures on aws-lc-rs (no `none`, no HMAC), `JwksCache` (an unknown `kid` asks for one fetch per minute, the first such request waits for it, others fail at once; a token without `kid` needs exactly one fitting key; a failed fetch or a set with no usable key keeps the keys; no keys at all is `Unavailable`) and the `krishiv-jwks` fetch thread (`HttpKeySource`: blocking reqwest, no redirects, 1 MiB cap, one 10 s deadline per fetch); the role of an OIDC principal is the highest any of its role-claim values has in the OIDC role lists; only an invalid token counts toward the throttle; `jose_tests.rs` (every build), `oidc_tests.rs` with keys generated per run, `oidc_http_tests.rs` against a loopback HTTP server; `roles.rs` `RolePolicy`, the ADR 0046 §3 matrix as an `AccessPolicy` (viewer: `Browse`, own `system.*` rows, `/metrics`; operator: `Select`, `Usage`, `Monitor`, `Operate`; admin: `Modify`/`Create`/`Drop`/`Manage`; client code at `krishiv.auth.udf.minRole`; `Impersonate` never; no role denies all), `tests/roles_matrix.rs`. Metadata statements need `Browse` (`authorize` falls back to `Select` for providers that know only `Select`); `ADMIN`-class statements need `Manage` on `System("cluster")` +- `src/access.rs` `StatementInfo` / `Session::statement_info` (class + read securables with columns), write/DDL checks (`authorize_statement`), `Session::authorize_privilege` (one privilege on one securable outside a statement, audited: the Connect request classes of ADR 0046 §3b), the `krishiv::audit` denial record, `PathPrefixPolicy` (local roots; object-store prefixes by store and whole key segments, `with_object_prefixes`; `from_config` is the server-mode confinement of ADR 0046 §3a from `krishiv.io.local.allowedRoots` / `krishiv.io.objectStore.allowedPrefixes`, default empty, every role, absolute roots; Azure prefixes name their account; with `allowedRoots` set the runtime's local store refuses listings that follow a link out of the roots, `SettledLocalFileSystem::confined`; `tests/path_confinement.rs`). Server-scoped store options (P3-T067b; `krishiv_io::is_server_scoped_option`: every `fs.*` key but an inline key, secret, token or SAS) need `Manage` on `System("cluster")` per read (`bind_files`, table `WITH`), per write (`authorize_write_path`; a table's own stored options are not asked again), on CREATE TABLE `OPTIONS` / `SET SERDEPROPERTIES`, on SQL `SET`, and per query while the session changed one from its builder config (`ConfStore::store_settings_changed`); `tests/server_egress.rs` +- `src/auth/` (P3-T002, design `design/P3-T002-authentication.md`, ADR 0046) server authentication: `Role` (viewer < operator < admin), `Credentials` / `Authenticated` / `AuthFailure` (Spark's two `UNAUTHENTICATED` messages), the `Authenticator` trait and `ServerAuth` (throttle, then static tokens; one `krishiv::auth` warn record and one `(surface, method)` counter per failure, never the credential; `ServerAuth::metrics` gives `krishiv.auth.failed` / `krishiv.auth.denied` as counter samples for `/metrics`); `denials.rs` `DeniedCounts`, the server's `(surface, privilege)` denial counters (ADR 0046 §8), shared by its guards and its sessions (`Session::count_denials`: every `KRISHIV_INSUFFICIENT_PERMISSIONS` the session's `log_denial` writes is counted, by the error's `privilege` parameter; the denial record also carries `client_user_id_unverified`, ADR 0046 §5); `config.rs` `AuthConfig::from_settings` (`krishiv.auth.tokens..{secret,principal,kind,role}`, `spark.connect.authenticate.token` = one admin token named `spark`, `krishiv.auth.failureThrottle.*`; unknown `krishiv.auth.*` keys refused); `secret_ref.rs` `env:`/`file:` references (a file readable by group or others refuses startup); `tokens.rs` SHA-256 digests compared in constant time against every entry (secrets >= 32 bytes, no two tokens share one); `throttle.rs` per-address failure blocks, bounded at 65,536 addresses; `mtls.rs` client certificates (`krishiv.auth.clientCertificates` off/optional/required, `clientCa`, `mtls.principalFrom` san-uri/san-dns/cn: exactly one value of that kind or refused, `mtls.userPrincipals`; roles from `krishiv.auth.roles.{viewer,operator,admin}` lists via `config.rs` `RoleLists`; a bearer token next to a certificate must name the same principal; `required` never falls back to a token); `der.rs` `CertNames::from_der`, the bounded no-recursion DER walk of the leaf's subject CN and DNS/URI SANs (fuzz target `fuzz/fuzz_targets/cert_names.rs` includes it by path); `tls_config.rs` `TlsConfig` (`krishiv.server.tls.{certFile,keyFile,minVersion}`) and `check_listener` (startup refusals: non-loopback without auth, non-loopback tokens without TLS, client certificates without TLS); `tls.rs` (feature `tls`) `ServerTls`, the rustls `ServerConfig` on aws-lc-rs with h2/http1.1 ALPN, WebPKI client verifier, a key file checked like a secret file and `reload()` swapping the certified key (a failed reload keeps the old one); `mtls_tests.rs` with in-memory handshakes against `tests/fixtures/tls/`; `oidc_config.rs` `OidcConfig` (`krishiv.auth.oidc.{issuer,audience,jwksUri,algorithms,principalClaim,roleClaim,clockSkew,jwksRefresh,allowInsecureIssuer}` and `krishiv.auth.oidc.roles.{viewer,operator,admin}`, the role-claim values by role, a namespace separate from the certificate names of `krishiv.auth.roles.*`; refused without feature `oidc`; locations https only unless the test flag, with a host, no userinfo, no fragment); `jose.rs` the pure JWS/JWK set/discovery parsers (16 KiB token cap, canonical unpadded base64url, duplicate JSON members refused, `alg` only RS256/384/512 and ES256/384, `crit` refused, header key members ignored; `check_claims`: `exp`/`nbf` with skew, exact `iss`, `aud`, principal and groups; fuzz target `fuzz/fuzz_targets/jwt_parse.rs` includes it by path); `oidc.rs` (feature `oidc`) signatures on aws-lc-rs (no `none`, no HMAC), `JwksCache` (an unknown `kid` asks for one fetch per minute, the first such request waits for it, others fail at once; a token without `kid` needs exactly one fitting key; a failed fetch or a set with no usable key keeps the keys; no keys at all is `Unavailable`) and the `krishiv-jwks` fetch thread (`HttpKeySource`: blocking reqwest, no redirects, 1 MiB cap, one 10 s deadline per fetch); the role of an OIDC principal is the highest any of its role-claim values has in the OIDC role lists; only an invalid token counts toward the throttle; `jose_tests.rs` (every build), `oidc_tests.rs` with keys generated per run, `oidc_http_tests.rs` against a loopback HTTP server; `roles.rs` `RolePolicy`, the ADR 0046 §3 matrix as an `AccessPolicy` (viewer: `Browse`, own `system.*` rows (`queries`, `sessions`), `/metrics`; operator: `Select`, `Usage`, `Monitor`, `Operate`; admin: `Modify`/`Create`/`Drop`/`Manage`; client code at `krishiv.auth.udf.minRole`; `Impersonate` never; no role denies all), `tests/roles_matrix.rs`. Metadata statements need `Browse` (`authorize` falls back to `Select` for providers that know only `Select`); `ADMIN`-class statements need `Manage` on `System("cluster")`; `SET` / `RESET key` of a registry key settable by `admin` (its Spark alias too) needs `Manage` on `System("cluster")` (`Session::authorize_set_key`, with the server-scoped store keys) - `src/partitions.rs` partitioned and bucketed tables: the layout `CREATE TABLE` stores (partition columns last), the partitions the catalog tracks (`ADD PARTITION`, `SET LOCATION` and static `INSERT` values cast to the column types, `DROP PARTITION` of a managed table: log first, then the directories; each partitioned table has a read/write lock (`InMemoryCatalogStore::partition_ddl` / `partition_write`, keyed by location, weak entries): `RENAME`/`DROP PARTITION`, `MSCK REPAIR` and `TRUNCATE` hold it exclusively (across their log commit retries) and re-read the entry under it, an `INSERT` holds it shared from its write to its partition registration; external tables refused, registered after `INSERT`/CTAS), `DESCRIBE ... PARTITION`, `SHOW PARTITIONS`, `ANALYZE ... PARTITION`; `tests/partitioned_tables.rs` - `src/column_stats.rs` `ANALYZE TABLE .. FOR COLUMNS` (`CommandUtils.statExprs`): one aggregate over the table, kept in `CatalogTable::column_stats` (the catalog is in memory, no on-disk format), printed by `DESCRIBE EXTENDED t col`; a write, `TRUNCATE`, `ADD PARTITION`, `DROP PARTITION` or `SET LOCATION` drops them (`updateTableStats`); published only to the entry the aggregate read (`CatalogTable::incarnation`, set by the store at create, kept by `replace_table`), so a table dropped and created again meanwhile gets none, nor one whose data changed after it was read (`CatalogTable::data_version`, moved by `data_changed`, which every stats-clearing change calls); `MSCK REPAIR` keeps them, as Spark 4.2.0 does; `tests/char_varchar_write.rs` - `src/table_commands/files.rs` a managed table's directory: `authorize_path` (the policy per privilege), `deletable_dir` (inside the root it was created under, no links, `MODIFY`), `delete_table_files`, `remove_dir` diff --git a/crates/krishiv-session/public-api.txt b/crates/krishiv-session/public-api.txt index 091cce119..dc3537350 100644 --- a/crates/krishiv-session/public-api.txt +++ b/crates/krishiv-session/public-api.txt @@ -373,6 +373,27 @@ pub fn krishiv_session::auth::Credentials<'_>::fmt(&self, &mut core::fmt::Format impl<'a> core::clone::Clone for krishiv_session::auth::Credentials<'a> pub fn krishiv_session::auth::Credentials<'a>::clone(&self) -> krishiv_session::auth::Credentials<'a> impl<'a> core::marker::Copy for krishiv_session::auth::Credentials<'a> +#[non_exhaustive] pub struct krishiv_session::auth::DeniedCount +pub krishiv_session::auth::DeniedCount::count: u64 +pub krishiv_session::auth::DeniedCount::privilege: krishiv_plan::analyzer::catalog::Privilege +pub krishiv_session::auth::DeniedCount::surface: krishiv_session::auth::Surface +impl core::clone::Clone for krishiv_session::auth::DeniedCount +pub fn krishiv_session::auth::DeniedCount::clone(&self) -> krishiv_session::auth::DeniedCount +impl core::cmp::Eq for krishiv_session::auth::DeniedCount +impl core::cmp::PartialEq for krishiv_session::auth::DeniedCount +pub fn krishiv_session::auth::DeniedCount::eq(&self, &krishiv_session::auth::DeniedCount) -> bool +impl core::fmt::Debug for krishiv_session::auth::DeniedCount +pub fn krishiv_session::auth::DeniedCount::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for krishiv_session::auth::DeniedCount +pub struct krishiv_session::auth::DeniedCounts +impl krishiv_session::auth::DeniedCounts +pub fn krishiv_session::auth::DeniedCounts::counts(&self) -> alloc::vec::Vec +pub fn krishiv_session::auth::DeniedCounts::new() -> Self +pub fn krishiv_session::auth::DeniedCounts::record(&self, krishiv_session::auth::Surface, krishiv_plan::analyzer::catalog::Privilege) +impl core::default::Default for krishiv_session::auth::DeniedCounts +pub fn krishiv_session::auth::DeniedCounts::default() -> Self +impl core::fmt::Debug for krishiv_session::auth::DeniedCounts +pub fn krishiv_session::auth::DeniedCounts::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result #[non_exhaustive] pub struct krishiv_session::auth::FailureCount pub krishiv_session::auth::FailureCount::count: u64 pub krishiv_session::auth::FailureCount::method: krishiv_session::auth::Method @@ -470,8 +491,10 @@ pub fn krishiv_session::auth::RolePolicy::version(&self) -> krishiv_plan::analyz pub struct krishiv_session::auth::ServerAuth impl krishiv_session::auth::ServerAuth pub fn krishiv_session::auth::ServerAuth::authenticate_at(&self, &krishiv_session::auth::Credentials<'_>, std::time::Instant) -> core::result::Result +pub fn krishiv_session::auth::ServerAuth::denials(&self) -> alloc::sync::Arc pub fn krishiv_session::auth::ServerAuth::failure_counts(&self) -> alloc::vec::Vec pub fn krishiv_session::auth::ServerAuth::is_enabled(&self) -> bool +pub fn krishiv_session::auth::ServerAuth::metrics(&self) -> krishiv_exec::metrics::registry::MetricsSnapshot pub fn krishiv_session::auth::ServerAuth::new(&krishiv_session::auth::AuthConfig) -> core::result::Result impl core::fmt::Debug for krishiv_session::auth::ServerAuth pub fn krishiv_session::auth::ServerAuth::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result @@ -1082,6 +1105,10 @@ impl core::fmt::Debug for krishiv_session::RuntimeConf pub fn krishiv_session::RuntimeConf::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result pub struct krishiv_session::Session impl krishiv_session::Session +pub async fn krishiv_session::Session::authorize_privilege(&self, &krishiv_plan::analyzer::catalog::Securable, krishiv_plan::analyzer::catalog::Privilege, krishiv_sql::classify::StatementClass) -> core::result::Result<(), krishiv_common::error::KrishivError> +pub async fn krishiv_session::Session::authorize_set_key(&self, &str) -> core::result::Result<(), krishiv_common::error::KrishivError> +pub fn krishiv_session::Session::count_denials(&self, alloc::sync::Arc, krishiv_session::auth::Surface) -> bool +impl krishiv_session::Session pub fn krishiv_session::Session::baggage(&self) -> alloc::vec::Vec<(alloc::string::String, alloc::string::String)> pub fn krishiv_session::Session::clear_trace_context(&self) pub fn krishiv_session::Session::remove_tag(&self, &str) @@ -1120,6 +1147,7 @@ pub fn krishiv_session::Session::interrupt_all(&self) -> alloc::vec::Vec alloc::vec::Vec pub fn krishiv_session::Session::is_closed(&self) -> bool pub fn krishiv_session::Session::new_session(&self) -> Self +pub fn krishiv_session::Session::new_session_as(&self, krishiv_common::principal::Principal, alloc::sync::Arc) -> Self pub fn krishiv_session::Session::running_queries(&self) -> usize pub fn krishiv_session::Session::runtime(&self) -> &alloc::sync::Arc pub fn krishiv_session::Session::set_client_info(&self, krishiv_session::Transport, core::option::Option<&str>, core::option::Option<&str>, core::option::Option<&str>) diff --git a/crates/krishiv-session/src/access.rs b/crates/krishiv-session/src/access.rs index 85c290f45..b47f638aa 100644 --- a/crates/krishiv-session/src/access.rs +++ b/crates/krishiv-session/src/access.rs @@ -167,9 +167,21 @@ pub(crate) fn audit( /// an access check fails closed because it cannot name what a statement /// touches (`KRISHIV_UNSUPPORTED_FEATURE`). fn log_denial(session: &Session, class: StatementClass, e: &KrishivError) { + if e.error_class() == ErrorClass::KrishivInsufficientPermissions + && let Some((counts, surface)) = session.state.denials.get() + && let Some((_, privilege)) = e + .message_parameters() + .iter() + .find(|(name, _)| *name == "privilege") + { + counts.record_named(*surface, privilege); + } + // ADR 0046 §5: the client's own `user_id` is recorded, marked unverified, never used. + let client_user_id = session.state.activity.client_user_id(); tracing::warn!( target: "krishiv::audit", principal = %session.state.principal.name, + client_user_id_unverified = client_user_id.as_deref(), statement_class = class.name(), error_class = e.error_class().name(), denial = %e, @@ -178,6 +190,17 @@ fn log_denial(session: &Session, class: StatementClass, e: &KrishivError) { } impl Session { + /// Counts every `KRISHIV_INSUFFICIENT_PERMISSIONS` denial of this session into `counts` + /// under `surface` from now on (a server's sessions: `krishiv_auth_denied_total`, ADR 0046 + /// §8). Set once; a second call changes nothing and returns `false`. + pub fn count_denials( + &self, + counts: Arc, + surface: crate::auth::Surface, + ) -> bool { + self.state.denials.set((counts, surface)).is_ok() + } + /// Checks what `statement` (of `class`) writes, creates or drops, and /// what it reads without a query, before it runs; an `ADMIN` statement /// needs `Manage` on the cluster (ADR 0046 §4). Names are located as analysis locates @@ -294,6 +317,29 @@ impl Session { Ok(views) } + /// Checks that this session's principal holds `privilege` on `securable` under the + /// session's policy, outside any statement: what a server checks for a request before it + /// runs one (the Connect request classes of ADR 0046 §3b). Fails closed as every check + /// does, and a denial writes one `krishiv::audit` record with `class`. + /// + /// # Errors + /// `KRISHIV_INSUFFICIENT_PERMISSIONS` (42501) when the policy does not allow it. + pub async fn authorize_privilege( + &self, + securable: &Securable, + privilege: Privilege, + class: StatementClass, + ) -> Result<(), KrishivError> { + let r = authorize( + &*self.state.policy, + &self.state.principal, + securable, + privilege, + ) + .await; + audit(self, class, r) + } + /// `privilege` on the permanent view `namespace.name`, besides the table-named check made /// before dispatch: the statements that show a view's definition, columns or properties /// need `Browse` on it (ADR 0046 §4), `SHOW TABLE EXTENDED` `Select`. diff --git a/crates/krishiv-session/src/auth/denials.rs b/crates/krishiv-session/src/auth/denials.rs new file mode 100644 index 000000000..afeb90724 --- /dev/null +++ b/crates/krishiv-session/src/auth/denials.rs @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: Apache-2.0 +//! The denial counter of a server (`krishiv_auth_denied_total{surface, privilege}`, ADR 0046 +//! §8): one count per access check refused with `KRISHIV_INSUFFICIENT_PERMISSIONS`, by the +//! listener the caller came in on and the privilege asked for. A server shares one +//! [`DeniedCounts`] between its listeners' guards and its sessions +//! ([`crate::Session::count_denials`]). + +use std::sync::atomic::{AtomicU64, Ordering}; + +use krishiv_plan::analyzer::Privilege; + +use super::Surface; + +/// Every privilege, in counter order (a closed set: the label stays bounded). +const PRIVILEGES: [Privilege; 10] = [ + Privilege::Select, + Privilege::Modify, + Privilege::Create, + Privilege::Drop, + Privilege::Monitor, + Privilege::Usage, + Privilege::Browse, + Privilege::Operate, + Privilege::Manage, + Privilege::Impersonate, +]; + +/// One denial counter. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct DeniedCount { + /// The listener. + pub surface: Surface, + /// The privilege asked for. + pub privilege: Privilege, + /// Denials so far. + pub count: u64, +} + +/// The denial counters of one server. +#[derive(Debug)] +pub struct DeniedCounts { + /// Indexed by `surface * PRIVILEGES.len() + privilege`. + counts: [AtomicU64; Surface::ALL.len() * PRIVILEGES.len()], +} + +impl Default for DeniedCounts { + fn default() -> Self { + Self { + counts: std::array::from_fn(|_| AtomicU64::new(0)), + } + } +} + +impl DeniedCounts { + /// Counters at zero. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Counts one denial of `privilege` on `surface`. + pub fn record(&self, surface: Surface, privilege: Privilege) { + if let Some(i) = index(surface, privilege) { + self.counts[i].fetch_add(1, Ordering::Relaxed); + } + } + + /// Counts one denial on `surface` of the privilege named as `Privilege`'s `Display` writes it + /// (the `privilege` parameter of `KRISHIV_INSUFFICIENT_PERMISSIONS`); an unknown name counts + /// nothing. + pub(crate) fn record_named(&self, surface: Surface, privilege: &str) { + if let Some(p) = PRIVILEGES.iter().find(|p| p.to_string() == privilege) { + self.record(surface, *p); + } + } + + /// Every counter, by surface then privilege. + #[must_use] + pub fn counts(&self) -> Vec { + let mut out = Vec::with_capacity(self.counts.len()); + for surface in Surface::ALL { + for privilege in PRIVILEGES { + if let Some(i) = index(surface, privilege) { + out.push(DeniedCount { + surface, + privilege, + count: self.counts[i].load(Ordering::Relaxed), + }); + } + } + } + out + } +} + +fn index(surface: Surface, privilege: Privilege) -> Option { + let s = Surface::ALL.iter().position(|x| *x == surface)?; + let p = PRIVILEGES.iter().position(|x| *x == privilege)?; + Some(s * PRIVILEGES.len() + p) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn each_surface_and_privilege_counts_on_its_own() { + let d = DeniedCounts::new(); + d.record(Surface::Connect, Privilege::Modify); + d.record(Surface::Connect, Privilege::Modify); + d.record_named(Surface::Ui, "OPERATE"); + d.record_named(Surface::Ui, "NOT_A_PRIVILEGE"); + let counts = d.counts(); + assert_eq!(counts.len(), Surface::ALL.len() * PRIVILEGES.len()); + let get = |s, p| { + counts + .iter() + .find(|c| c.surface == s && c.privilege == p) + .map(|c| c.count) + }; + assert_eq!(get(Surface::Connect, Privilege::Modify), Some(2)); + assert_eq!(get(Surface::Ui, Privilege::Operate), Some(1)); + assert_eq!(get(Surface::Admin, Privilege::Modify), Some(0)); + assert_eq!(counts.iter().map(|c| c.count).sum::(), 3); + } + + #[test] + fn every_privilege_has_a_counter() { + // A new `Privilege` variant must be added to `PRIVILEGES` (and the metric's label). + for p in PRIVILEGES { + assert!(index(Surface::Connect, p).is_some()); + } + let names: Vec = PRIVILEGES.iter().map(ToString::to_string).collect(); + assert_eq!( + names, + [ + "SELECT", + "MODIFY", + "CREATE", + "DROP", + "MONITOR", + "USAGE", + "BROWSE", + "OPERATE", + "MANAGE", + "IMPERSONATE" + ] + ); + } +} diff --git a/crates/krishiv-session/src/auth/mod.rs b/crates/krishiv-session/src/auth/mod.rs index a03d7510b..4276dd7c5 100644 --- a/crates/krishiv-session/src/auth/mod.rs +++ b/crates/krishiv-session/src/auth/mod.rs @@ -27,6 +27,7 @@ //! credential) and its failure counters. mod config; +mod denials; mod der; #[cfg_attr( all(not(feature = "oidc"), not(test)), @@ -69,6 +70,7 @@ use futures::future::BoxFuture; use krishiv_common::principal::Principal; pub use config::{AuthConfig, RoleLists, ThrottleConfig, TokenSpec, is_auth_key}; +pub use denials::{DeniedCount, DeniedCounts}; pub use der::{CertNames, DerError}; pub use jose::Alg; pub use mtls::{ClientCertificates, MtlsConfig, PrincipalFrom}; diff --git a/crates/krishiv-session/src/auth/roles.rs b/crates/krishiv-session/src/auth/roles.rs index 89b8968cf..09bd38e24 100644 --- a/crates/krishiv-session/src/auth/roles.rs +++ b/crates/krishiv-session/src/auth/roles.rs @@ -10,7 +10,7 @@ //! | Privilege | viewer | operator | admin | //! |---|---|---|---| //! | `Browse` (metadata) | yes | yes | yes | -//! | `Select` on `system.queries` (own rows; others' need `Monitor`), `capabilities`, `metrics` | yes | yes | yes | +//! | `Select` on `system.queries`, `system.sessions` (own rows; others' need `Monitor`), `capabilities`, `metrics` | yes | yes | yes | //! | `Select` on the other `system.*` tables (`memory`, `operators`, `caches`) | no | yes | yes | //! | `Select` on anything else (tables, views, functions, paths) | no (T062 grants) | yes | yes | //! | `Usage` on a secret or connection | no | yes | yes | @@ -37,10 +37,10 @@ pub(crate) const SYSTEM_UDF: &str = "udf"; pub(crate) const SYSTEM_METRICS: &str = "metrics"; /// The reserved `system` catalog. const SYSTEM_CATALOG: &str = "system"; -/// The `system` tables a viewer reads: `queries` shows only the principal's own rows without -/// `Monitor`, `capabilities` and `metrics` hold no principal's data (ADR 0046 §3, §3c). The -/// others (`memory`, `operators`, `caches`) name other principals' work: operator. -const VIEWER_SYSTEM_TABLES: [&str; 3] = ["queries", "capabilities", "metrics"]; +/// The `system` tables a viewer reads: `queries` and `sessions` show only the principal's own +/// rows without `Monitor`, `capabilities` and `metrics` hold no principal's data (ADR 0046 §3, +/// §3c). The others (`memory`, `operators`, `caches`) name other principals' work: operator. +const VIEWER_SYSTEM_TABLES: [&str; 4] = ["queries", "sessions", "capabilities", "metrics"]; /// The ADR 0046 role matrix for one session's principal. #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/crates/krishiv-session/src/auth/server.rs b/crates/krishiv-session/src/auth/server.rs index 9ebb60431..5e789b9ab 100644 --- a/crates/krishiv-session/src/auth/server.rs +++ b/crates/krishiv-session/src/auth/server.rs @@ -58,6 +58,8 @@ pub struct ServerAuth { oidc_roles: RoleLists, /// Indexed by `surface * Method::ALL.len() + method`. failures: [AtomicU64; Surface::ALL.len() * Method::ALL.len()], + /// The denials of this server's callers (its guards and sessions count into it). + denied: std::sync::Arc, } impl ServerAuth { @@ -122,6 +124,7 @@ impl ServerAuth { #[cfg(feature = "oidc")] oidc_roles: config.oidc.roles.clone(), failures: std::array::from_fn(|_| AtomicU64::new(0)), + denied: std::sync::Arc::new(super::DeniedCounts::new()), }) } @@ -268,6 +271,52 @@ impl ServerAuth { } } + /// The denial counters of this server's callers, shared with whatever counts into them + /// (`krishiv_auth_denied_total`, ADR 0046 §8). + #[must_use] + pub fn denials(&self) -> std::sync::Arc { + std::sync::Arc::clone(&self.denied) + } + + /// `krishiv_auth_failed_total{surface, method}` and `krishiv_auth_denied_total{surface, + /// privilege}` (ADR 0046 §8), every series including the zeros, for the server's `/metrics` + /// (merged into the engine's snapshot). + #[must_use] + pub fn metrics(&self) -> krishiv_exec::MetricsSnapshot { + let mut out = krishiv_exec::MetricsSnapshot::default(); + for f in self.failure_counts() { + out.counters.push(krishiv_exec::CounterSample { + name: "krishiv.auth.failed", + unit: "{request}", + labels: vec![ + ("surface".to_owned(), f.surface.as_str().to_owned()), + ("method".to_owned(), f.method.as_str().to_owned()), + ], + #[expect( + clippy::cast_precision_loss, + reason = "a counter past 2^53 loses only its low bits" + )] + value: f.count as f64, + }); + } + for d in self.denied.counts() { + out.counters.push(krishiv_exec::CounterSample { + name: "krishiv.auth.denied", + unit: "{request}", + labels: vec![ + ("surface".to_owned(), d.surface.as_str().to_owned()), + ("privilege".to_owned(), d.privilege.to_string()), + ], + #[expect( + clippy::cast_precision_loss, + reason = "a counter past 2^53 loses only its low bits" + )] + value: d.count as f64, + }); + } + out + } + /// The failure counters, every `(surface, method)` pair in order. #[must_use] pub fn failure_counts(&self) -> Vec { diff --git a/crates/krishiv-session/src/command.rs b/crates/krishiv-session/src/command.rs index adcd29ce0..2301dc1d6 100644 --- a/crates/krishiv-session/src/command.rs +++ b/crates/krishiv-session/src/command.rs @@ -145,7 +145,7 @@ impl Session { } .into()); } - self.authorize_set_store_key(key).await?; + self.authorize_set_key(key).await?; // The assignment sets through the session configuration, // so validation and error classes are its own; the row // shows the value as written, as in Spark. @@ -210,8 +210,8 @@ impl Session { /// Spark. pub(crate) async fn exec_reset(&self, r: &Reset) -> Result { match &r.key { - None => self.state.conf.reset(), - Some(key) => self.conf().unset(key)?, + None => self.state.conf.reset(), // every key it set passed `SET`'s check + Some(key) => self.unset_key(key).await?, } self.empty_frame().await } diff --git a/crates/krishiv-session/src/session.rs b/crates/krishiv-session/src/session.rs index 17965787d..5133ec590 100644 --- a/crates/krishiv-session/src/session.rs +++ b/crates/krishiv-session/src/session.rs @@ -189,6 +189,8 @@ pub(crate) struct SessionState { /// while expanding a view point into the view's text, not the outer /// statement's (`SourceId(0)` stays the outer statement's). sql_sources: AtomicU32, + /// Where this session counts its denials (a server's sessions, `Session::count_denials`). + pub(crate) denials: std::sync::OnceLock<(Arc, crate::auth::Surface)>, } impl Drop for SessionState { diff --git a/crates/krishiv-session/src/session/lifecycle.rs b/crates/krishiv-session/src/session/lifecycle.rs index 00b27ca24..f0613684e 100644 --- a/crates/krishiv-session/src/session/lifecycle.rs +++ b/crates/krishiv-session/src/session/lifecycle.rs @@ -146,6 +146,7 @@ impl Session { .unwrap_or_else(|| Arc::new(Unlimited)), statements: AtomicU64::new(0), sql_sources: AtomicU32::new(seed.sql_sources), + denials: std::sync::OnceLock::new(), }), }; session @@ -187,6 +188,29 @@ impl Session { // The runtime's starting configuration, validated when it was built // (for a private runtime, the builder's configuration). let config = Config::clone(&runtime.config); + inherit_denials(self, Self::on_runtime(runtime, config, parts)) + } + + /// [`Self::new_session`] acting for `principal` under `policy` instead of this session's + /// principal and policy: a server's session for one authenticated caller (P3-T002, ADR + /// 0046), built from a base session that holds the server's runtime and profile. The + /// principal and policy are the new session's for life. + #[must_use] + pub fn new_session_as( + &self, + principal: krishiv_common::principal::Principal, + policy: Arc, + ) -> Self { + let state = &self.state; + let runtime = Arc::clone(&state.runtime); + let parts = SessionParts { + profile: state.profile, + principal: Arc::new(principal), + policy, + functions: state.base_functions.clone(), + admission: Some(Arc::clone(&state.admission)), + }; + let config = Config::clone(&runtime.config); Self::on_runtime(runtime, config, parts) } @@ -269,7 +293,10 @@ impl Session { // A `close` that ran while the copy was made wins: no live clone of // a closed session. self.check_open()?; - Ok(Self::seeded(Arc::clone(&state.runtime), parts, seed)) + Ok(inherit_denials( + self, + Self::seeded(Arc::clone(&state.runtime), parts, seed), + )) } /// The runtime this session runs on. @@ -589,6 +616,15 @@ struct Seed { cwd: Option, } +/// `to`, counting its denials where `from` counts them (`new_session` and `clone_session` keep +/// the principal, so they keep the server's denial counter too). +fn inherit_denials(from: &Session, to: Session) -> Session { + if let Some((counts, surface)) = from.state.denials.get() { + to.count_denials(Arc::clone(counts), *surface); + } + to +} + #[cfg(test)] mod tests { use krishiv_exec::{ExecConfig, QueryContext, QueryId, WorkloadClass}; diff --git a/crates/krishiv-session/src/session/ops.rs b/crates/krishiv-session/src/session/ops.rs index 2fec77a2a..eba338dd2 100644 --- a/crates/krishiv-session/src/session/ops.rs +++ b/crates/krishiv-session/src/session/ops.rs @@ -105,9 +105,40 @@ impl Session { crate::access::audit(self, class, r) } - /// `SET key = ..` of a server-scoped store key is `Manage` on the cluster (P3-T067b); any - /// other key passes. Asked before the value is stored, so a refused `SET` changes nothing. - pub(crate) async fn authorize_set_store_key(&self, key: &str) -> Result<(), KrishivError> { + /// `RESET key`: the key's `SET` check ([`Self::authorize_set_key`]), then the unset. + pub(crate) async fn unset_key(&self, key: &str) -> Result<(), KrishivError> { + self.authorize_set_key(key).await?; + self.conf().unset(key) + } + + /// `SET key = ..` (or `RESET key`) of a key only an administrator may change is `Manage` + /// on the cluster (ADR 0046 §3): a registry key settable by `admin` (its Spark alias too), + /// or a server-scoped store key (P3-T067b). Any other key passes. Asked before the value + /// is stored, so a refused `SET` changes nothing. A `startup` key is refused by the + /// configuration itself, for every principal (`CANNOT_MODIFY_STATIC_CONFIG`). + /// + /// Public for server adapters that set configuration outside SQL (Spark Connect's `Config` + /// RPC), so they ask exactly what `SET` asks; not meant as user API. + /// + /// # Errors + /// `KRISHIV_INSUFFICIENT_PERMISSIONS` (42501) when the principal lacks `Manage`; the + /// denial is audited and counted like every other. + pub async fn authorize_set_key(&self, key: &str) -> Result<(), KrishivError> { + if krishiv_common::config::lookup(key) + .or_else(|| { + krishiv_common::config::lookup(krishiv_common::config::resolve_alternative(key)) + }) + .is_some_and(|e| e.settable_by == krishiv_common::config::SettableBy::Admin) + { + let r = authorize( + &*self.state.policy, + &self.state.principal, + &Securable::System("cluster".to_owned()), + Privilege::Manage, + ) + .await; + return crate::access::audit(self, StatementClass::Session, r); + } if !krishiv_io::is_server_scoped_option(key) { return Ok(()); } @@ -365,8 +396,11 @@ impl Session { } } // A store error after the anchor is open may be a link swapped into - // the path: the audited denial is what the caller sees then. - if written.is_err() + // the path: the audited denial is what the caller sees then. A denial + // is already audited (and counted): it is not asked about again. + if written + .as_ref() + .is_err_and(|e| e.error_class().name() != "KRISHIV_INSUFFICIENT_PERMISSIONS") && let Err(denied) = crate::access::still_physical(self, &at) && denied.error_class().name() == "KRISHIV_INSUFFICIENT_PERMISSIONS" { diff --git a/crates/krishiv-session/src/sessions.rs b/crates/krishiv-session/src/sessions.rs index b9c7f514b..86b970045 100644 --- a/crates/krishiv-session/src/sessions.rs +++ b/crates/krishiv-session/src/sessions.rs @@ -161,6 +161,15 @@ impl Activity { .store(ms.unwrap_or(NO_TIMEOUT), Ordering::Relaxed); } + /// The user id the client reported (client-asserted, never an identity). + pub(crate) fn client_user_id(&self) -> Option { + self.client + .lock() + .unwrap_or_else(PoisonError::into_inner) + .client_user_id + .clone() + } + /// Keeps the first non-empty client type and user id seen, and the latest server session id. pub(crate) fn set_client( &self, diff --git a/crates/krishiv-session/tests/access_audit.rs b/crates/krishiv-session/tests/access_audit.rs index f039ecfc3..a086f83ff 100644 --- a/crates/krishiv-session/tests/access_audit.rs +++ b/crates/krishiv-session/tests/access_audit.rs @@ -124,5 +124,27 @@ fn each_denial_leaves_one_audit_record() { !r.contains("deny-all"), "the policy's reason stays out: {r}" ); + assert!( + !r.contains("client_user_id_unverified"), + "no client id reported, none recorded: {r}" + ); } + + // ADR 0046 §5: a client's own `user_id` is recorded beside the principal, marked + // unverified (review N6). + s.set_client_info( + krishiv_session::Transport::Connect, + None, + Some("dana"), + None, + ); + block_on(s.sql("SELECT * FROM t")).unwrap_err(); + let records = take(); + assert_eq!(records.len(), 1, "{records:?}"); + assert!(records[0].contains("principal=ana "), "{}", records[0]); + assert!( + records[0].contains("client_user_id_unverified=dana "), + "{}", + records[0] + ); } diff --git a/crates/krishiv-session/tests/access_policy.rs b/crates/krishiv-session/tests/access_policy.rs index 7e987a8b4..733612325 100644 --- a/crates/krishiv-session/tests/access_policy.rs +++ b/crates/krishiv-session/tests/access_policy.rs @@ -767,6 +767,8 @@ fn a_directory_swapped_during_the_write_is_denied_and_cleaned_up() { }; let policy = PathPrefixPolicy::new([&root], Arc::new(AllowAll)).unwrap(); let s = session(Arc::new(policy)); + let counts = Arc::new(krishiv_session::auth::DeniedCounts::new()); + s.count_denials(Arc::clone(&counts), krishiv_session::auth::Surface::Connect); let df = block_on(s.sql("SELECT id FROM range(0, 20000, 1, 1)")).unwrap(); let result = block_on( df.write() @@ -778,6 +780,9 @@ fn a_directory_swapped_during_the_write_is_denied_and_cleaned_up() { "the swap happened during the write" ); assert_eq!(class(result), DENIED); + // One refusal, one denial counted: the error path does not ask about an audited denial again. + let denied: u64 = counts.counts().iter().map(|c| c.count).sum(); + assert_eq!(denied, 1); assert_eq!(tree(&outside), before, "nothing changed outside the root"); // Files written before the swap are in the renamed directory, inside // the root; the link now at `out` cannot be followed to remove them. diff --git a/crates/krishiv-session/tests/roles_matrix.rs b/crates/krishiv-session/tests/roles_matrix.rs index d867570fd..cece57e6e 100644 --- a/crates/krishiv-session/tests/roles_matrix.rs +++ b/crates/krishiv-session/tests/roles_matrix.rs @@ -60,6 +60,10 @@ const MATRIX: &[(&str, Role)] = &[ ("SELECT id FROM range(3)", Role::Viewer), // Session class. ("SET spark.sql.shuffle.partitions = 4", Role::Viewer), + // A key settable by `admin` only (ADR 0046 §3): `Manage` on the cluster, for `SET` and + // `RESET key` alike. + ("SET krishiv.exec.targetPartitionBytes = 64m", Role::Admin), + ("RESET krishiv.exec.targetPartitionBytes", Role::Admin), ("USE default", Role::Viewer), // Metadata (Browse). ("SHOW TABLES", Role::Viewer), @@ -72,6 +76,7 @@ const MATRIX: &[(&str, Role)] = &[ ("SELECT count(*) FROM system.queries", Role::Viewer), ("SELECT count(*) FROM SYSTEM.QUERIES", Role::Viewer), ("SELECT count(*) FROM system.capabilities", Role::Viewer), + ("SELECT count(*) FROM system.sessions", Role::Viewer), // A system table's schema is metadata, even where its rows are not (Browse, not Select). ("DESCRIBE system.memory", Role::Viewer), ("SHOW COLUMNS IN system.memory", Role::Viewer), diff --git a/crates/krishiv-session/tests/server_egress.rs b/crates/krishiv-session/tests/server_egress.rs index 799fee1a9..4f103b776 100644 --- a/crates/krishiv-session/tests/server_egress.rs +++ b/crates/krishiv-session/tests/server_egress.rs @@ -446,3 +446,32 @@ fn an_admins_stored_options_serve_other_writers() { Err(DENIED.to_owned()) ); } + +/// A server's sessions count their denials (`krishiv_auth_denied_total`, ADR 0046 §8) into the +/// counter they were given, under its surface and the privilege asked for; an allowed read +/// counts nothing. +#[test] +fn a_session_counts_its_denials() { + use krishiv_session::auth::{DeniedCounts, Surface}; + let tmp = data(); + let path = tmp.path().join("a.csv"); + let path = path.to_str().unwrap(); + let counts = Arc::new(DeniedCounts::new()); + let viewer = session(tmp.path(), Role::Viewer, &[]); + assert!(viewer.count_denials(Arc::clone(&counts), Surface::Connect)); + assert!( + !viewer.count_denials(Arc::new(DeniedCounts::new()), Surface::Ui), + "set once" + ); + assert_eq!(read_with(&viewer, path, &[]), Err(DENIED.to_owned())); + let operator = session(tmp.path(), Role::Operator, &[]); + operator.count_denials(Arc::clone(&counts), Surface::Connect); + assert_eq!(read_with(&operator, path, &[]), Ok(2)); + let nonzero: Vec<_> = counts + .counts() + .into_iter() + .filter(|c| c.count > 0) + .map(|c| (c.surface, c.privilege, c.count)) + .collect(); + assert_eq!(nonzero, [(Surface::Connect, Privilege::Select, 1)]); +} diff --git a/docs/guides/deploy.md b/docs/guides/deploy.md index 7a79f4682..0c2279a08 100644 --- a/docs/guides/deploy.md +++ b/docs/guides/deploy.md @@ -27,10 +27,37 @@ Nothing has been published to PyPI or crates.io yet. Until the first release, bu One process, started with `krishiv server` (a build with the `server` feature), runs the engine and serves Spark Connect; clients connect to it with a Spark Connect client such as PySpark. It listens on `127.0.0.1` port `15002` by default (`krishiv.connect.port`, see -[configuration](../reference/config/index.md)). Authentication is not available yet, so keep it on loopback and reach it through a tunnel or proxy; the server refuses a -non-loopback address until authentication exists. Where it differs from Spark see +[configuration](../reference/config/index.md)). Where it differs from Spark see [known differences](../concepts/known-differences.md#krishiv-server-spark-connect). +### Authentication, roles and TLS + +Without authentication settings the server binds loopback only, and the admin port (`/metrics`, the web UI) prints a +random token at start. To serve another address, configure at least one method with `--conf` (these settings are +read by the server itself and never become session configuration): + +- **Tokens:** `krishiv.auth.tokens..secret` (an `env:` or `file:` reference; a file readable by group or others + is refused), `.principal`, `.role` (`viewer`, `operator` or `admin`). Spark's `spark.connect.authenticate.token` is + one admin token. +- **OIDC:** `krishiv.auth.oidc.issuer`, `audience` and the role lists `krishiv.auth.oidc.roles.*`. +- **Client certificates:** `krishiv.auth.clientCertificates` (`optional` or `required`), `krishiv.auth.clientCa`, + `krishiv.auth.mtls.principalFrom`, and the role lists `krishiv.auth.roles.*`. + +A viewer sees metadata and its own queries and sessions, an operator also reads data and acts on others' running +work, an admin may also write, create, drop and change the cluster. A caller with no role is refused. + +TLS: `krishiv.server.tls.certFile` and `keyFile` (the key file must not be readable by group or others) turn on TLS for +both ports; clients then connect with `sc://host:15002/;use_ssl=true` and `https://`. The server refuses to start +when the settings do not fit: a non-loopback address without an authentication method, a token or OIDC on a +non-loopback address without TLS, or client certificates without TLS. On Unix, `SIGHUP` reloads the certificate and +key; new connections get the new ones, open connections keep theirs, and a file that fails to load leaves the old +pair serving. + +Every refused request is counted on `/metrics`: `krishiv_auth_failed_total` (unauthenticated or throttled, by port and +method) and `krishiv_auth_denied_total` (permission denied, by port and privilege). The `krishiv::auth` and +`krishiv::audit` log records describe each refusal, but logs are rate limited (`KRISHIV_LOG_RATE`) and best effort: +they are not an audit trail. Use the counters to see how many refusals happened. + ## Distributed A scheduler and workers run one query across machines. It is in development; the mode is not available yet. diff --git a/docs/reference/config/index.md b/docs/reference/config/index.md index 646fe656b..1c665ccd8 100644 --- a/docs/reference/config/index.md +++ b/docs/reference/config/index.md @@ -21,7 +21,7 @@ Keys under `krishiv.experimental.` are experimental and off by default. Spark al | `krishiv.connect.execute.retryBufferSize` | size | `10MB` | cluster | startup | 0.3.0 | `spark.connect.execute.reattachable.observerRetryBufferSize` | Bytes of already-sent `ExecutePlan` responses a reattachable operation keeps, newest first, so a client that lost its stream can reattach and resume; older ones are dropped. `0` keeps none. The bytes count against the memory pool. | | `krishiv.connect.execute.senderMaxStreamDuration` | duration (bare number: ms) | `2min` | cluster | startup | 0.3.0 | `spark.connect.execute.reattachable.senderMaxStreamDuration` | After this long a reattachable `ExecutePlan` or `ReattachExecute` stream ends without `ResultComplete` and the client reattaches to continue. `0` never ends it. | | `krishiv.connect.execute.senderMaxStreamSize` | size | `1GB` | cluster | startup | 0.3.0 | `spark.connect.execute.reattachable.senderMaxStreamSize` | After sending this many bytes (the response that crosses it is still sent) a reattachable stream ends without `ResultComplete` and the client reattaches to continue. `0` never ends it. | -| `krishiv.connect.handshakeTimeout` | duration | `10s` | cluster | startup | 0.3.0 | | A new Spark Connect connection that sends no first byte for this long is closed, so silent sockets cannot hold `krishiv.connect.maxConnections` for longer. `0s` means no deadline. | +| `krishiv.connect.handshakeTimeout` | duration | `10s` | cluster | startup | 0.3.0 | | A new Spark Connect connection that sends no first byte for this long is closed, so silent sockets cannot hold `krishiv.connect.maxConnections` for longer; with TLS the whole handshake must also finish within it. `0s` means no deadline for either, so a client that stalls mid-handshake holds its connection slot until it disconnects. | | `krishiv.connect.http2.maxConcurrentStreams` | positive int | `100` | cluster | startup | 0.3.0 | | Most concurrent RPCs (HTTP/2 streams) on one Spark Connect connection; more are refused. | | `krishiv.connect.http2.maxHeaderListBytes` | size | `16KB` | cluster | startup | 0.3.0 | | The largest header list of a Spark Connect request. | | `krishiv.connect.http2.maxPendingAcceptResets` | positive int | `20` | cluster | startup | 0.3.0 | | Streams a client may open and reset before the server accepts them, before the connection is closed (the HTTP/2 rapid-reset bound). | diff --git a/docs/reference/metrics-v1.md b/docs/reference/metrics-v1.md index aafbeccc1..a564513c5 100644 --- a/docs/reference/metrics-v1.md +++ b/docs/reference/metrics-v1.md @@ -43,3 +43,5 @@ The `denied` value of `status` is reserved: access-policy denials fail before ex | `krishiv.io.request.duration` | `krishiv_io_request_duration_seconds` | histogram | `s` | op; store_kind | no source yet | No source yet: the I/O layer keeps no per-request durations, so this entry emits nothing until they land. | | `krishiv.exec.driver.stalls` | `krishiv_exec_driver_stalls_total` | counter | `1` | — | query fold | Driver polls longer than the stall threshold, summed over the query's drivers. | | `krishiv.build.info` | `krishiv_build_info` | gauge | `1` | version; git_sha | scrape | The build: always 1, labelled with its version and commit. | +| `krishiv.auth.failed` | `krishiv_auth_failed_total` | counter | `{request}` | surface(connect, admin, metrics, ui); method(token, oidc, mtls, throttle, none) | scrape | Requests to `krishiv server` refused as unauthenticated (a missing, malformed, wrong or expired credential) or throttled, by listener and the method tried. Served only by `krishiv server` with authentication configured. | +| `krishiv.auth.denied` | `krishiv_auth_denied_total` | counter | `{request}` | surface(connect, admin, metrics, ui); privilege(SELECT, MODIFY, CREATE, DROP, MONITOR, USAGE, BROWSE, OPERATE, MANAGE, IMPERSONATE) | scrape | Access checks of `krishiv server` callers refused with `KRISHIV_INSUFFICIENT_PERMISSIONS`, by listener and the privilege asked for. Served only by `krishiv server` with authentication configured. |