From 785adc58fb65681a7e7786830bea81049ed19ef1 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 11 Nov 2021 18:00:41 -0600 Subject: [PATCH 01/35] draft console server --- nexus/examples/config.toml | 4 ++ nexus/src/config.rs | 17 +++++++ nexus/src/http_entrypoints_console.rs | 66 +++++++++++++++++++++++++++ nexus/src/lib.rs | 60 ++++++++++++++++-------- nexus/tests/config.test.toml | 8 ++-- smf/nexus/config.toml | 4 ++ 6 files changed, 136 insertions(+), 23 deletions(-) create mode 100644 nexus/src/http_entrypoints_console.rs diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 96c86fcced7..20aae8aba19 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -28,6 +28,10 @@ bind_address = "127.0.0.1:12220" # IP address and TCP port on which to listen for the internal API bind_address = "127.0.0.1:12221" +[dropshot_console] +# IP address and TCP port on which to listen for the console API +bind_address = "127.0.0.1:12222" + [log] # Show log messages of this level and more severe level = "info" diff --git a/nexus/src/config.rs b/nexus/src/config.rs index 9faccaebd01..e60b3932b99 100644 --- a/nexus/src/config.rs +++ b/nexus/src/config.rs @@ -38,6 +38,8 @@ pub struct Config { pub dropshot_external: ConfigDropshot, /** Dropshot configuration for internal API server */ pub dropshot_internal: ConfigDropshot, + /** Dropshot configuration for console server */ + pub dropshot_console: ConfigDropshot, /** Identifier for this instance of Nexus */ pub id: uuid::Uuid, /** Server-wide logging configuration. */ @@ -267,6 +269,9 @@ mod test { [dropshot_internal] bind_address = "10.1.2.3:4568" request_body_max_bytes = 1024 + [dropshot_console] + bind_address = "10.1.2.3:4569" + request_body_max_bytes = 1024 [database] url = "postgresql://127.0.0.1?sslmode=disable" [log] @@ -299,6 +304,12 @@ mod test { .unwrap(), ..Default::default() }, + dropshot_console: ConfigDropshot { + bind_address: "10.1.2.3:4569" + .parse::() + .unwrap(), + ..Default::default() + }, log: ConfigLogging::File { level: ConfigLoggingLevel::Debug, if_exists: ConfigLoggingIfExists::Fail, @@ -326,6 +337,9 @@ mod test { [dropshot_internal] bind_address = "10.1.2.3:4568" request_body_max_bytes = 1024 + [dropshot_console] + bind_address = "10.1.2.3:4569" + request_body_max_bytes = 1024 [database] url = "postgresql://127.0.0.1?sslmode=disable" [log] @@ -361,6 +375,9 @@ mod test { [dropshot_internal] bind_address = "10.1.2.3:4568" request_body_max_bytes = 1024 + [dropshot_console] + bind_address = "10.1.2.3:4569" + request_body_max_bytes = 1024 [database] url = "postgresql://127.0.0.1?sslmode=disable" [log] diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs new file mode 100644 index 00000000000..9ff666fee06 --- /dev/null +++ b/nexus/src/http_entrypoints_console.rs @@ -0,0 +1,66 @@ +use super::ServerContext; + +use crate::context::OpContext; +use dropshot::{ + endpoint, ApiDescription, HttpError, HttpResponseUpdatedNoContent, + RequestContext, +}; +use std::sync::Arc; + +type NexusApiDescription = ApiDescription>; + +/** + * Returns a description of the part of the nexus API dedicated to the web console + */ +pub fn api() -> NexusApiDescription { + fn register_endpoints(api: &mut NexusApiDescription) -> Result<(), String> { + api.register(logout)?; + Ok(()) + } + + let mut api = NexusApiDescription::new(); + if let Err(err) = register_endpoints(&mut api) { + panic!("failed to register entrypoints: {}", err); + } + api +} + +/** + * Log user out of web console by deleting session. + */ +#[endpoint { + method = POST, + path = "/logout", + }] +async fn logout( + rqctx: Arc>>, +) -> Result { + let opctx = OpContext::for_external_api(&rqctx).await; + if let Ok(_opctx) = opctx { + // if they have a session, look it up by token and delete it + + // TODO: how do we get the token in order to look up the session and + // delete it? inside the cookie auth scheme, we pull it from the cookie, + // but we don't return it from the scheme. The result of that scheme + // only tells us the Actor, i.e., the user ID. We can't just delete all + // sessions for that user ID because a user could have sessions going in + // multiple browsers and only want to log out one. + } + + // If user's session was already expired, they fail auth and their session + // gets automatically deleted by the auth scheme. If they have no session + // (e.g., they cleared their cookies while sitting on the page) they will + // also fail auth. However, even though they failed auth, we probably don't + // want to send them back a 401 like we would for a normal request. They are + // in fact logged out like they intended, and we want to send them the + // response that will clear their cookie in the browser. + + // TODO: Set-Cookie with empty value and expiration date in the past so it + // gets deleted by the browser + Ok(HttpResponseUpdatedNoContent()) +} + +// TODO: +// - POST login +// - /* route for serving bundle (redirect to configured IdP if unauthed) +// - /assets/* for images and fonts diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 4884a8bc8e7..27950a0f618 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -17,6 +17,7 @@ mod authz; mod config; mod context; pub mod db; // Public only for some documentation examples +mod http_entrypoints_console; mod http_entrypoints_external; mod http_entrypoints_internal; mod nexus; @@ -25,6 +26,7 @@ mod sagas; pub use config::Config; pub use context::ServerContext; +use http_entrypoints_console::api as console_api; use http_entrypoints_external::external_api; use http_entrypoints_internal::internal_api; pub use nexus::Nexus; @@ -75,6 +77,8 @@ pub struct Server { pub http_server_external: dropshot::HttpServer>, /** dropshot server for internal API */ pub http_server_internal: dropshot::HttpServer>, + /** dropshot server for console API */ + pub http_server_console: dropshot::HttpServer>, } impl Server { @@ -111,10 +115,25 @@ impl Server { ) .map_err(|error| format!("initializing internal server: {}", error))?; + let c2 = Arc::clone(&apictx); + let http_server_starter_console = dropshot::HttpServerStarter::new( + &config.dropshot_console, + console_api(), + c2, + &log.new(o!("component" => "dropshot_console")), + ) + .map_err(|error| format!("initializing console server: {}", error))?; + let http_server_external = http_server_starter_external.start(); let http_server_internal = http_server_starter_internal.start(); - - Ok(Server { apictx, http_server_external, http_server_internal }) + let http_server_console = http_server_starter_console.start(); + + Ok(Server { + apictx, + http_server_external, + http_server_internal, + http_server_console, + }) } /** @@ -125,22 +144,27 @@ impl Server { * or until something else initiates a graceful shutdown. */ pub async fn wait_for_finish(self) -> Result<(), String> { - let result_external = self.http_server_external.await; - let result_internal = self.http_server_internal.await; - - match (result_external, result_internal) { - (Ok(()), Ok(())) => Ok(()), - (Err(error_external), Err(error_internal)) => Err(format!( - "errors from both external and internal HTTP \ - servers(external: \"{}\", internal: \"{}\"", - error_external, error_internal - )), - (Err(error_external), Ok(())) => { - Err(format!("external server: {}", error_external)) - } - (Ok(()), Err(error_internal)) => { - Err(format!("internal server: {}", error_internal)) - } + let errors = vec![ + self.http_server_external + .await + .map_err(|e| format!("external: {}", e)), + self.http_server_internal + .await + .map_err(|e| format!("internal: {}", e)), + self.http_server_console + .await + .map_err(|e| format!("console: {}", e)), + ] + .into_iter() + .filter(Result::is_err) + .map(|r| r.unwrap_err()) + .collect::>(); + + if errors.len() > 0 { + let msg = format!("errors shutting down: ({})", errors.join(", ")); + Err(msg) + } else { + Ok(()) } } diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index d1ad5b008df..cf4c2eb08c0 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -27,14 +27,12 @@ url = "postgresql://root@127.0.0.1:0/omicron?sslmode=disable" [dropshot_external] bind_address = "127.0.0.1:0" -# -# NOTE: for the test suite, the port MUST be 0 (in order to bind to any -# available port) because the test suite will be running many servers -# concurrently. -# [dropshot_internal] bind_address = "127.0.0.1:0" +[dropshot_console] +bind_address = "127.0.0.1:0" + # # NOTE: for the test suite, if mode = "file", the file path MUST be the sentinel # string "UNUSED". The actual path will be generated by the test suite for each diff --git a/smf/nexus/config.toml b/smf/nexus/config.toml index 5b61e1c2b8d..1c46eea4f54 100644 --- a/smf/nexus/config.toml +++ b/smf/nexus/config.toml @@ -23,6 +23,10 @@ bind_address = "127.0.0.1:12220" # IP address and TCP port on which to listen for the internal API bind_address = "127.0.0.1:12221" +[dropshot_console] +# IP address and TCP port on which to listen for the console API +bind_address = "127.0.0.1:12222" + [log] # Show log messages of this level and more severe level = "info" From e83e39cfb8e3ce34b51e179ddeac173557a35914 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 11 Nov 2021 20:55:33 -0600 Subject: [PATCH 02/35] stub out asset and console_page routes --- Cargo.lock | 20 +++++ nexus/Cargo.toml | 1 + nexus/src/http_entrypoints_console.rs | 116 ++++++++++++++++++++++++-- 3 files changed, 131 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 75a0d953c31..08e9d6b0f53 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1243,6 +1243,16 @@ version = "0.3.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2a60c7ce501c71e03a9c9c0d35b861413ae925bd979cc7a4e30d060069aaac8d" +[[package]] +name = "mime_guess" +version = "2.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2684d4c2e97d99848d30b324b00c8fcc7e5c897b7cbb5819b09e7c90e8baf212" +dependencies = [ + "mime", + "unicase", +] + [[package]] name = "mio" version = "0.7.14" @@ -1462,6 +1472,7 @@ dependencies = [ "lazy_static", "libc 0.2.107", "macaddr", + "mime_guess", "newtype_derive", "omicron-common", "omicron-rpaths", @@ -3501,6 +3512,15 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "56dee185309b50d1f11bfedef0fe6d036842e3fb77413abef29f8f8d1c5d4c1c" +[[package]] +name = "unicase" +version = "2.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50f37be617794602aabbeee0be4f259dc1778fabe05e2d67ee8f79326d5cb4f6" +dependencies = [ + "version_check", +] + [[package]] name = "unicode-bidi" version = "0.3.7" diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index bcd8e74f448..a9b2ce2358a 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -23,6 +23,7 @@ ipnetwork = "0.18" lazy_static = "1.4.0" libc = "0.2.107" macaddr = { version = "1.0.1", features = [ "serde_std" ]} +mime_guess = "2.0.3" newtype_derive = "0.1.6" oso = "0.22" # See omicron-rpaths for more about the "pq-sys" dependency. diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index 9ff666fee06..4681812afc0 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -1,11 +1,19 @@ +/*! + * Handler functions (entrypoints) for console-related routes. + */ use super::ServerContext; use crate::context::OpContext; use dropshot::{ - endpoint, ApiDescription, HttpError, HttpResponseUpdatedNoContent, + endpoint, ApiDescription, HttpError, HttpResponseUpdatedNoContent, Path, RequestContext, }; -use std::sync::Arc; +use http::{Response, StatusCode}; +use hyper::Body; +use mime_guess; +use schemars::JsonSchema; +use serde::Deserialize; +use std::{path::PathBuf, sync::Arc}; type NexusApiDescription = ApiDescription>; @@ -15,6 +23,8 @@ type NexusApiDescription = ApiDescription>; pub fn api() -> NexusApiDescription { fn register_endpoints(api: &mut NexusApiDescription) -> Result<(), String> { api.register(logout)?; + api.register(console_page)?; + api.register(asset)?; Ok(()) } @@ -60,7 +70,101 @@ async fn logout( Ok(HttpResponseUpdatedNoContent()) } -// TODO: -// - POST login -// - /* route for serving bundle (redirect to configured IdP if unauthed) -// - /assets/* for images and fonts +#[derive(Deserialize, JsonSchema)] +struct RestPathParam { + path: Vec, +} + +// TODO: /c/ prefix is def not what we want long-term but it makes things easy for now +#[endpoint { + method = GET, + path = "/c/{path:.*}", + }] +async fn console_page( + rqctx: Arc>>, + _path_params: Path, +) -> Result, HttpError> { + let _opctx = OpContext::for_external_api(&rqctx).await; + // redirect to IdP if not logged in + + // otherwise serve HTML page with bundle in script tag + + // HTML doesn't need to be static -- we'll probably find a reason to do some minimal + // templating, e.g., putting a CSRF token in the page + + // amusingly, at least to start out, I don't think we care about the path + // because the real routing is all client-side. we serve the same HTML + // regardless, the app starts on the client and renders the right page and + // makes the right API requests. + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") + .body("".into())?) +} + +#[endpoint { + method = GET, + path = "/assets/{path:.*}", + // don't want this to show up in the OpenAPI spec (which we don't generate anyway) + unpublished = true, + }] +async fn asset( + _rqctx: Arc>>, + path_params: Path, +) -> Result, HttpError> { + // we *could* auth gate the static assets but it would be kind of weird. prob not necessary + let path = path_params.into_inner().path; + let file = find_file(path, PathBuf::new()); + + if file.is_none() { + // 404 + } + + let file = file.unwrap(); + + // Derive the MIME type from the file name + let content_type = mime_guess::from_path(&file) // should be the actual file + .first() + .map_or("text/plain".to_string(), |m| m.to_string()); + + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, &content_type) + .body("".into())?) +} + +// This is a bit stripped down from the dropshot example. For example, we don't +// want to tell the user why the request failed, we just 404 on everything. + +// TODO: we should probably return a Result so we can at least log the problems +// internally. it would also clean up the control flow +fn find_file(path: Vec, root_dir: PathBuf) -> Option { + // start from `root_dir` + let mut current = root_dir; + for component in &path { + // The previous iteration needs to have resulted in a directory. + if !current.is_dir() { + return None; + } + // Dropshot won't ever give us dot-components. + assert_ne!(component, "."); + assert_ne!(component, ".."); + current.push(component); + + // block symlinks + match current.symlink_metadata() { + Ok(m) => { + if m.file_type().is_symlink() { + return None; + } + } + Err(_) => return None, + } + } + // if we've landed on a directory, we can't serve that so it's a 404 again + if current.is_dir() { + return None; + } + + Some(current) +} From 490a1e5ba97bfbd6b73dcf78614c34e3203d74d9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 11 Nov 2021 23:02:11 -0600 Subject: [PATCH 03/35] Cookies extractor --- Cargo.lock | 2 -- Cargo.toml | 4 ++-- nexus/src/authn/external/cookies.rs | 25 +++++++++++++++++++++++++ nexus/src/authn/external/mod.rs | 2 +- nexus/src/http_entrypoints_console.rs | 2 ++ 5 files changed, 30 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 08e9d6b0f53..e7f2c611e2b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -526,7 +526,6 @@ checksum = "4bb454f0228b18c7f4c3b0ebbee346ed9c52e7443b0999cd543ff3571205701d" [[package]] name = "dropshot" version = "0.5.2-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c193292b40e271a31b302c4b38bfddd40ba255a4" dependencies = [ "async-trait", "base64", @@ -560,7 +559,6 @@ dependencies = [ [[package]] name = "dropshot_endpoint" version = "0.5.2-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c193292b40e271a31b302c4b38bfddd40ba255a4" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index e1427ee0306..26ae904950f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,8 +40,8 @@ panic = "abort" # It's common during development to use a local copy of dropshot or steno in the # parent directory. If you want to use those, uncomment one of these blocks. # -#[patch."https://github.com/oxidecomputer/dropshot"] -#dropshot = { path = "../dropshot/dropshot" } +[patch."https://github.com/oxidecomputer/dropshot"] +dropshot = { path = "../dropshot/dropshot" } #[patch."https://github.com/oxidecomputer/steno"] #steno = { path = "../steno" } diff --git a/nexus/src/authn/external/cookies.rs b/nexus/src/authn/external/cookies.rs index e6dd02b1fef..c2edd24aaaf 100644 --- a/nexus/src/authn/external/cookies.rs +++ b/nexus/src/authn/external/cookies.rs @@ -1,5 +1,10 @@ use anyhow::Context; +use async_trait::async_trait; use cookie::{Cookie, CookieJar, ParseError}; +use dropshot::{ + Extractor, ExtractorMetadata, HttpError, RequestContext, ServerContext, +}; +use std::sync::Arc; pub fn parse_cookies( headers: &http::HeaderMap, @@ -19,6 +24,26 @@ pub fn parse_cookies( } Ok(cookies) } +pub struct Cookies(pub CookieJar); + +NewtypeFrom! { () pub struct Cookies(pub CookieJar); } +NewtypeDeref! { () pub struct Cookies(pub CookieJar); } + +#[async_trait] +impl Extractor for Cookies { + async fn from_request( + rqctx: Arc>, + ) -> Result { + let request = &rqctx.request.lock().await; + let cookies = parse_cookies(request.headers()) + .unwrap_or_else(|_| CookieJar::new()); + Ok(cookies.into()) + } + + fn metadata() -> ExtractorMetadata { + ExtractorMetadata { paginated: false, parameters: vec![] } + } +} #[cfg(test)] mod test { diff --git a/nexus/src/authn/external/mod.rs b/nexus/src/authn/external/mod.rs index cc0d66d2f1a..c8f09c1831f 100644 --- a/nexus/src/authn/external/mod.rs +++ b/nexus/src/authn/external/mod.rs @@ -4,7 +4,7 @@ use crate::authn; use async_trait::async_trait; use authn::Reason; -mod cookies; +pub mod cookies; pub mod session_cookie; pub mod spoof; diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index 4681812afc0..de8649f8016 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -3,6 +3,7 @@ */ use super::ServerContext; +use crate::authn::external::cookies::Cookies; use crate::context::OpContext; use dropshot::{ endpoint, ApiDescription, HttpError, HttpResponseUpdatedNoContent, Path, @@ -44,6 +45,7 @@ pub fn api() -> NexusApiDescription { }] async fn logout( rqctx: Arc>>, + cookies: Cookies, ) -> Result { let opctx = OpContext::for_external_api(&rqctx).await; if let Ok(_opctx) = opctx { From 9476cfd0528f7f7d4d592816904622b5dca24f5c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 17 Nov 2021 14:46:52 -0600 Subject: [PATCH 04/35] logout is closer to correct, add fake login, session cookie value helper --- nexus/src/http_entrypoints_console.rs | 92 ++++++++++++++++++++------- 1 file changed, 68 insertions(+), 24 deletions(-) diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index de8649f8016..a5cabbe0b72 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -3,13 +3,17 @@ */ use super::ServerContext; -use crate::authn::external::cookies::Cookies; +use crate::authn::external::{ + cookies::Cookies, + session_cookie::{SessionStore, SESSION_COOKIE_COOKIE_NAME}, +}; +use crate::authn::TEST_USER_UUID_PRIVILEGED; use crate::context::OpContext; +use chrono::Duration; use dropshot::{ - endpoint, ApiDescription, HttpError, HttpResponseUpdatedNoContent, Path, - RequestContext, + endpoint, ApiDescription, HttpError, Path, RequestContext, TypedBody, }; -use http::{Response, StatusCode}; +use http::{header, Response, StatusCode}; use hyper::Body; use mime_guess; use schemars::JsonSchema; @@ -23,6 +27,7 @@ type NexusApiDescription = ApiDescription>; */ pub fn api() -> NexusApiDescription { fn register_endpoints(api: &mut NexusApiDescription) -> Result<(), String> { + api.register(login)?; api.register(logout)?; api.register(console_page)?; api.register(asset)?; @@ -36,6 +41,47 @@ pub fn api() -> NexusApiDescription { api } +#[derive(Clone, Debug, Deserialize, JsonSchema)] +#[serde(rename_all = "camelCase")] +pub struct LoginParams { + username: String, + password: String, +} + +// TODO: this should live alongside the session cookie stuff +fn session_cookie_value(token: &str, max_age: Duration) -> String { + format!( + "{}=\"{}\"; Secure; HttpOnly; SameSite=Lax; Max-Age={}", + SESSION_COOKIE_COOKIE_NAME, + token, + max_age.num_seconds() + ) +} + +#[endpoint { + method = POST, + path = "/login", + }] +async fn login( + rqctx: Arc>>, + _params: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let session = nexus + // TODO: obviously + .session_create(TEST_USER_UUID_PRIVILEGED.parse().unwrap()) + .await?; + Ok(Response::builder() + .status(200) + .header( + header::SET_COOKIE, + session_cookie_value(&session.token, apictx.session_idle_timeout()), + ) + .body("ok".into()) + .unwrap()) +} + /** * Log user out of web console by deleting session. */ @@ -46,30 +92,28 @@ pub fn api() -> NexusApiDescription { async fn logout( rqctx: Arc>>, cookies: Cookies, -) -> Result { +) -> Result, HttpError> { + let nexus = &rqctx.context().nexus; let opctx = OpContext::for_external_api(&rqctx).await; - if let Ok(_opctx) = opctx { - // if they have a session, look it up by token and delete it - - // TODO: how do we get the token in order to look up the session and - // delete it? inside the cookie auth scheme, we pull it from the cookie, - // but we don't return it from the scheme. The result of that scheme - // only tells us the Actor, i.e., the user ID. We can't just delete all - // sessions for that user ID because a user could have sessions going in - // multiple browsers and only want to log out one. + let token = cookies.get(SESSION_COOKIE_COOKIE_NAME); + + if opctx.is_ok() && token.is_some() { + nexus.session_hard_delete(token.unwrap().value().to_string()).await?; } - // If user's session was already expired, they fail auth and their session - // gets automatically deleted by the auth scheme. If they have no session + // If user's session was already expired, they failed auth and their session + // was automatically deleted by the auth scheme. If they have no session // (e.g., they cleared their cookies while sitting on the page) they will - // also fail auth. However, even though they failed auth, we probably don't - // want to send them back a 401 like we would for a normal request. They are - // in fact logged out like they intended, and we want to send them the - // response that will clear their cookie in the browser. - - // TODO: Set-Cookie with empty value and expiration date in the past so it - // gets deleted by the browser - Ok(HttpResponseUpdatedNoContent()) + // also fail auth. + + // Even if the user failed auth, we don't want to send them back a 401 like + // we would for a normal request. They are in fact logged out like they + // intended, and we should send the standard success response. + + Ok(Response::builder() + .status(200) + .header(header::SET_COOKIE, session_cookie_value("", Duration::zero())) + .body("".into())?) } #[derive(Deserialize, JsonSchema)] From e020e467d388570233441280d7a253c9b67c7ca9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 17 Nov 2021 16:53:51 -0600 Subject: [PATCH 05/35] working tests for session create and session auth, that's cool --- nexus/src/http_entrypoints_console.rs | 3 + nexus/tests/common/mod.rs | 7 +++ nexus/tests/config.test.toml | 2 +- nexus/tests/test_console_api.rs | 86 +++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 nexus/tests/test_console_api.rs diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index a5cabbe0b72..d10948c34de 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -58,6 +58,9 @@ fn session_cookie_value(token: &str, max_age: Duration) -> String { ) } +// for now this is just for testing purposes, and the username and password are +// ignored. we will probably end up with a real username/password login +// endpoint, but I think it will only be for use while setting up the rack #[endpoint { method = POST, path = "/login", diff --git a/nexus/tests/common/mod.rs b/nexus/tests/common/mod.rs index a61e6d232d4..761a2749311 100644 --- a/nexus/tests/common/mod.rs +++ b/nexus/tests/common/mod.rs @@ -29,6 +29,7 @@ pub const PRODUCER_UUID: &str = "a6458b7d-87c3-4483-be96-854d814c20de"; pub struct ControlPlaneTestContext { pub external_client: ClientTestContext, pub internal_client: ClientTestContext, + pub console_client: ClientTestContext, pub server: omicron_nexus::Server, pub database: dev::db::CockroachInstance, pub clickhouse: dev::clickhouse::ClickHouseInstance, @@ -42,6 +43,7 @@ impl ControlPlaneTestContext { pub async fn teardown(mut self) { self.server.http_server_external.close().await.unwrap(); self.server.http_server_internal.close().await.unwrap(); + self.server.http_server_console.close().await.unwrap(); self.database.cleanup().await.unwrap(); self.clickhouse.cleanup().await.unwrap(); self.sled_agent.http_server.close().await.unwrap(); @@ -107,6 +109,10 @@ pub async fn test_setup_with_config( server.http_server_internal.local_addr(), logctx.log.new(o!("component" => "internal client test context")), ); + let testctx_console = ClientTestContext::new( + server.http_server_console.local_addr(), + logctx.log.new(o!("component" => "console api client test context")), + ); /* Set up a single sled agent. */ let sa_id = Uuid::parse_str(SLED_AGENT_UUID).unwrap(); @@ -144,6 +150,7 @@ pub async fn test_setup_with_config( server, external_client: testctx_external, internal_client: testctx_internal, + console_client: testctx_console, database, clickhouse, sled_agent: sa, diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index cf4c2eb08c0..c487c1f0f39 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -8,7 +8,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" # List of authentication schemes to support. [authn] -schemes_external = [ "spoof" ] +schemes_external = [ "spoof", "session_cookie" ] session_idle_timeout_minutes = 60 session_absolute_timeout_minutes = 480 diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs new file mode 100644 index 00000000000..496ab95c32d --- /dev/null +++ b/nexus/tests/test_console_api.rs @@ -0,0 +1,86 @@ +pub mod common; +use common::test_setup; +use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::OrganizationCreateParams; + +use http::{header, method::Method, StatusCode}; +use hyper; + +extern crate slog; + +#[tokio::test] +async fn test_sessions() { + let cptestctx = test_setup("test_sessions").await; + let client = &cptestctx.console_client; + let external_client = &cptestctx.external_client; + + // Set-Cookie responses only work if you make dropshot's test utils not + // panic! when it sees the Set-Cookie header + + let resp = client + .make_request_with_body( + Method::POST, + "/logout", + "".into(), + StatusCode::OK, + ) + .await + .unwrap(); + + let session_cookie = + resp.headers().get("set-cookie").unwrap().to_str().unwrap(); + assert_eq!( + session_cookie, + "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" + ); + + let resp = client + .make_request_with_body( + Method::POST, + "/login", + "{ \"username\": \"\", \"password\": \"\"}".into(), + StatusCode::OK, + ) + .await + .unwrap(); + + let session_cookie = + resp.headers().get("set-cookie").unwrap().to_str().unwrap(); + let (session_token, rest) = session_cookie.split_once("; ").unwrap(); + + assert!(session_token.starts_with("session=")); + assert_eq!(rest, "Secure; HttpOnly; SameSite=Lax; Max-Age=3600"); + + let org_params = OrganizationCreateParams { + identity: IdentityMetadataCreateParams { + name: "my-org".parse().unwrap(), + description: "an org".to_string(), + }, + }; + + // hitting auth-gated endpoint without session cookie 401s + + let _ = external_client + .make_request_with_body( + Method::POST, + "/organizations", + serde_json::to_string(&org_params).unwrap().into(), + StatusCode::UNAUTHORIZED, + ) + .await; + + // now make same request with cookie + + let request = hyper::Request::builder() + .header(header::COOKIE, session_token) + .method(Method::POST) + .uri(external_client.url("/organizations")) + .body(serde_json::to_string(&org_params).unwrap().into()) + .expect("attempted to construct invalid test request"); + external_client + .make_request_with_request(request, StatusCode::CREATED) + .await + .expect("failed to make request"); + + cptestctx.teardown().await; +} From 3bc6880d9129ec57035d46a9fd0851f3a23d52f9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 17 Nov 2021 22:42:26 -0600 Subject: [PATCH 06/35] fix config files --- nexus/examples/config-file.toml | 4 ++++ nexus/tests/config.test.toml | 2 ++ 2 files changed, 6 insertions(+) diff --git a/nexus/examples/config-file.toml b/nexus/examples/config-file.toml index 7de19abbac8..b4f57c57757 100644 --- a/nexus/examples/config-file.toml +++ b/nexus/examples/config-file.toml @@ -27,6 +27,10 @@ bind_address = "127.0.0.1:12220" # IP address and TCP port on which to listen for the internal API bind_address = "127.0.0.1:12221" +[dropshot_internal] +# IP address and TCP port on which to listen for the console API +bind_address = "127.0.0.1:12222" + [log] # Show log messages of this level and more severe level = "info" diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index c487c1f0f39..367038abbca 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -27,9 +27,11 @@ url = "postgresql://root@127.0.0.1:0/omicron?sslmode=disable" [dropshot_external] bind_address = "127.0.0.1:0" +# port must be 0. see above [dropshot_internal] bind_address = "127.0.0.1:0" +# port must be 0. see above [dropshot_console] bind_address = "127.0.0.1:0" From ae1a19da750592483b9add4f0e3af4f98d3097d2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 17 Nov 2021 23:28:30 -0600 Subject: [PATCH 07/35] logout makes session token stop working --- nexus/tests/test_console_api.rs | 47 ++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 496ab95c32d..5ee37ee1541 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -11,13 +11,13 @@ extern crate slog; #[tokio::test] async fn test_sessions() { let cptestctx = test_setup("test_sessions").await; - let client = &cptestctx.console_client; + let console_client = &cptestctx.console_client; let external_client = &cptestctx.external_client; // Set-Cookie responses only work if you make dropshot's test utils not // panic! when it sees the Set-Cookie header - let resp = client + let resp = console_client .make_request_with_body( Method::POST, "/logout", @@ -27,14 +27,15 @@ async fn test_sessions() { .await .unwrap(); - let session_cookie = + // logout always gives the same response whether you have a session or not + let set_cookie_header = resp.headers().get("set-cookie").unwrap().to_str().unwrap(); assert_eq!( - session_cookie, + set_cookie_header, "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" ); - let resp = client + let resp = console_client .make_request_with_body( Method::POST, "/login", @@ -71,16 +72,48 @@ async fn test_sessions() { // now make same request with cookie - let request = hyper::Request::builder() + let get_orgs = hyper::Request::builder() .header(header::COOKIE, session_token) .method(Method::POST) .uri(external_client.url("/organizations")) .body(serde_json::to_string(&org_params).unwrap().into()) .expect("attempted to construct invalid test request"); external_client - .make_request_with_request(request, StatusCode::CREATED) + .make_request_with_request(get_orgs, StatusCode::CREATED) .await .expect("failed to make request"); + // logout with an actual session should delete the session in the db + let logout_request = hyper::Request::builder() + .header(header::COOKIE, session_token) + .method(Method::POST) + .uri(console_client.url("/logout")) + .body("".into()) + .expect("attempted to construct invalid test request"); + let logout_resp = console_client + .make_request_with_request(logout_request, StatusCode::OK) + .await + .unwrap(); + + // logout clears the cookie client-side + let set_cookie_header = + logout_resp.headers().get("set-cookie").unwrap().to_str().unwrap(); + assert_eq!( + set_cookie_header, + "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" + ); + + // now the same request with the same session cookie should 401 because + // logout also deletes the session server-side + let request = hyper::Request::builder() + .header(header::COOKIE, session_token) + .method(Method::POST) + .uri(external_client.url("/organizations")) + .body(serde_json::to_string(&org_params).unwrap().into()) + .expect("attempted to construct invalid test request"); + let _ = external_client + .make_request_with_request(request, StatusCode::UNAUTHORIZED) + .await; + cptestctx.teardown().await; } From 5fae933166716b7112ac73db7460100c758bd40d Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 10:37:07 -0600 Subject: [PATCH 08/35] file server and login redirect pretty much work --- nexus/src/http_entrypoints_console.rs | 52 +++++++++++++++----- nexus/tests/fixtures/hello.txt | 1 + nexus/tests/test_console_api.rs | 70 +++++++++++++++++++++++++-- 3 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 nexus/tests/fixtures/hello.txt diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index d10948c34de..f793e209708 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -18,7 +18,7 @@ use hyper::Body; use mime_guess; use schemars::JsonSchema; use serde::Deserialize; -use std::{path::PathBuf, sync::Arc}; +use std::{env::current_dir, path::PathBuf, sync::Arc}; type NexusApiDescription = ApiDescription>; @@ -133,10 +133,9 @@ async fn console_page( rqctx: Arc>>, _path_params: Path, ) -> Result, HttpError> { - let _opctx = OpContext::for_external_api(&rqctx).await; - // redirect to IdP if not logged in + let opctx = OpContext::for_external_api(&rqctx).await; - // otherwise serve HTML page with bundle in script tag + // if authed, serve HTML page with bundle in script tag // HTML doesn't need to be static -- we'll probably find a reason to do some minimal // templating, e.g., putting a CSRF token in the page @@ -145,9 +144,19 @@ async fn console_page( // because the real routing is all client-side. we serve the same HTML // regardless, the app starts on the client and renders the right page and // makes the right API requests. + if let Ok(opctx) = opctx { + if opctx.authn.actor().is_some() { + return Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") + .body("".into())?); + } + } + + // otherwise redirect to idp Ok(Response::builder() - .status(StatusCode::OK) - .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") + .status(StatusCode::FOUND) + .header(http::header::LOCATION, "idp.com/login") .body("".into())?) } @@ -161,16 +170,35 @@ async fn asset( _rqctx: Arc>>, path_params: Path, ) -> Result, HttpError> { - // we *could* auth gate the static assets but it would be kind of weird. prob not necessary + // we *could* auth gate the static assets but it would be kind of weird. + // prob not necessary. one way would be to have two directories, one that + // requires auth and one that doesn't. I'd rather not let path = path_params.into_inner().path; - let file = find_file(path, PathBuf::new()); + // TODO: make path configurable, confirm existence at startup (though it can + // always be deleted later, so maybe confirming existence isn't that + // important) + let mut assets_dir = current_dir().map_err(|_| { + HttpError::for_internal_error( + "couldn't pull current execution directory".to_string(), + ) + })?; + assets_dir.push("tests"); + assets_dir.push("fixtures"); + let file = find_file(path, assets_dir); if file.is_none() { - // 404 + return Err(HttpError::for_not_found( + None, + "file not found".to_string(), + )); } let file = file.unwrap(); + let body = tokio::fs::read(&file) + .await + .map_err(|_| HttpError::for_bad_request(None, "EBADF".to_string()))?; + // Derive the MIME type from the file name let content_type = mime_guess::from_path(&file) // should be the actual file .first() @@ -179,14 +207,14 @@ async fn asset( Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, &content_type) - .body("".into())?) + .body(body.into())?) } // This is a bit stripped down from the dropshot example. For example, we don't // want to tell the user why the request failed, we just 404 on everything. -// TODO: we should probably return a Result so we can at least log the problems -// internally. it would also clean up the control flow +// TODO: return a Result so we can at least log the problems internally. it +// would also clean up the control flow fn find_file(path: Vec, root_dir: PathBuf) -> Option { // start from `root_dir` let mut current = root_dir; diff --git a/nexus/tests/fixtures/hello.txt b/nexus/tests/fixtures/hello.txt new file mode 100644 index 00000000000..d2aeeec3fa6 --- /dev/null +++ b/nexus/tests/fixtures/hello.txt @@ -0,0 +1 @@ +hello there \ No newline at end of file diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 5ee37ee1541..4d7f6fe31f2 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -1,13 +1,17 @@ +use dropshot::test_util::read_string; +use http::{header, method::Method, StatusCode}; +use hyper; + pub mod common; use common::test_setup; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::OrganizationCreateParams; -use http::{header, method::Method, StatusCode}; -use hyper; - extern crate slog; +// TODO: test authed/not-authed with requests to console pages instead of +// /organizations + #[tokio::test] async fn test_sessions() { let cptestctx = test_setup("test_sessions").await; @@ -117,3 +121,63 @@ async fn test_sessions() { cptestctx.teardown().await; } + +#[tokio::test] +async fn test_console_pages() { + let cptestctx = test_setup("test_console_pages").await; + let client = &cptestctx.console_client; + + // request to console page route without auth should redirect to IdP + let unauthed_response = client + .make_request_with_body( + Method::GET, + // 404s will be handled client-side unless we want to pull in the + // entire route tree from the client (which we may well want to do) + "/c/irrelevant-path", + "".into(), + StatusCode::FOUND, + ) + .await + .unwrap(); + + let location_header = + unauthed_response.headers().get("location").unwrap().to_str().unwrap(); + assert_eq!(location_header, "idp.com/login"); + + // get session + + // hit console page with session, should get back HTML response + + cptestctx.teardown().await; +} + +#[tokio::test] +async fn test_assets() { + let cptestctx = test_setup("test_assets").await; + let client = &cptestctx.console_client; + + // nonexistent file 404s + let _ = client + .make_request_with_body( + Method::GET, + "/assets/nonexistent.svg", + "".into(), + StatusCode::NOT_FOUND, + ) + .await; + + // existing file is returned + let mut response = client + .make_request_with_body( + Method::GET, + "/assets/hello.txt", + "".into(), + StatusCode::OK, + ) + .await + .unwrap(); + let file_contents = read_string(&mut response).await; + assert_eq!(file_contents, "hello there".to_string()); + + cptestctx.teardown().await; +} From fdc0d31ad1df12b46b69889f97a4ba3e99485df5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 10:44:57 -0600 Subject: [PATCH 09/35] fix confusing variable names --- nexus/src/lib.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 27950a0f618..9dd2bc6bafe 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -97,29 +97,26 @@ impl Server { let pool = db::Pool::new(&config.database); let apictx = ServerContext::new(rack_id, ctxlog, pool, &config); - let c1 = Arc::clone(&apictx); let http_server_starter_external = dropshot::HttpServerStarter::new( &config.dropshot_external, external_api(), - c1, + Arc::clone(&apictx), &log.new(o!("component" => "dropshot_external")), ) .map_err(|error| format!("initializing external server: {}", error))?; - let c2 = Arc::clone(&apictx); let http_server_starter_internal = dropshot::HttpServerStarter::new( &config.dropshot_internal, internal_api(), - c2, + Arc::clone(&apictx), &log.new(o!("component" => "dropshot_internal")), ) .map_err(|error| format!("initializing internal server: {}", error))?; - let c2 = Arc::clone(&apictx); let http_server_starter_console = dropshot::HttpServerStarter::new( &config.dropshot_console, console_api(), - c2, + Arc::clone(&apictx), &log.new(o!("component" => "dropshot_console")), ) .map_err(|error| format!("initializing console server: {}", error))?; From b765a4da0c1863a2ae75e21a7a854c526a899035 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 12:39:19 -0600 Subject: [PATCH 10/35] move session cookie header generator into session cookie auth scheme --- nexus/src/authn/external/session_cookie.rs | 38 ++++++++++++++++++++-- nexus/src/http_entrypoints_console.rs | 23 +++++-------- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/nexus/src/authn/external/session_cookie.rs b/nexus/src/authn/external/session_cookie.rs index 8a361734f68..31901a7d5ef 100644 --- a/nexus/src/authn/external/session_cookie.rs +++ b/nexus/src/authn/external/session_cookie.rs @@ -49,6 +49,21 @@ pub const SESSION_COOKIE_COOKIE_NAME: &str = "session"; pub const SESSION_COOKIE_SCHEME_NAME: authn::SchemeName = authn::SchemeName("session_cookie"); +/// Generate session cookie header +pub fn session_cookie_header_value(token: &str, max_age: Duration) -> String { + format!( + "{}=\"{}\"; Secure; HttpOnly; SameSite=Lax; Max-Age={}", + SESSION_COOKIE_COOKIE_NAME, + token, + max_age.num_seconds() + ) +} + +/// Generate session cookie with empty token and max-age=0 so browser deletes it +pub fn clear_session_cookie_header_value() -> String { + session_cookie_header_value("", Duration::zero()) +} + /// Implements an authentication scheme where we check the DB to see if we have /// a session matching the token in a cookie ([`SESSION_COOKIE_COOKIE_NAME`]) on /// the request. This is meant to be used by the web console. @@ -148,8 +163,9 @@ fn get_token_from_cookie( #[cfg(test)] mod test { use super::{ - get_token_from_cookie, Details, HttpAuthnScheme, - HttpAuthnSessionCookie, Reason, SchemeResult, Session, SessionStore, + get_token_from_cookie, session_cookie_header_value, Details, + HttpAuthnScheme, HttpAuthnSessionCookie, Reason, SchemeResult, Session, + SessionStore, }; use async_trait::async_trait; use chrono::{DateTime, Duration, Utc}; @@ -355,4 +371,22 @@ mod test { let token = get_token_from_cookie(&headers); assert_eq!(token, None); } + + #[test] + fn test_session_cookie_value() { + assert_eq!( + session_cookie_header_value("abc", Duration::seconds(5)), + "session=\"abc\"; Secure; HttpOnly; SameSite=Lax; Max-Age=5" + ); + + assert_eq!( + session_cookie_header_value("abc", Duration::seconds(-5)), + "session=\"abc\"; Secure; HttpOnly; SameSite=Lax; Max-Age=-5" + ); + + assert_eq!( + session_cookie_header_value("", Duration::zero()), + "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" + ); + } } diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index f793e209708..4a7400cdd00 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -5,11 +5,13 @@ use super::ServerContext; use crate::authn::external::{ cookies::Cookies, - session_cookie::{SessionStore, SESSION_COOKIE_COOKIE_NAME}, + session_cookie::{ + clear_session_cookie_header_value, session_cookie_header_value, + SessionStore, SESSION_COOKIE_COOKIE_NAME, + }, }; use crate::authn::TEST_USER_UUID_PRIVILEGED; use crate::context::OpContext; -use chrono::Duration; use dropshot::{ endpoint, ApiDescription, HttpError, Path, RequestContext, TypedBody, }; @@ -48,16 +50,6 @@ pub struct LoginParams { password: String, } -// TODO: this should live alongside the session cookie stuff -fn session_cookie_value(token: &str, max_age: Duration) -> String { - format!( - "{}=\"{}\"; Secure; HttpOnly; SameSite=Lax; Max-Age={}", - SESSION_COOKIE_COOKIE_NAME, - token, - max_age.num_seconds() - ) -} - // for now this is just for testing purposes, and the username and password are // ignored. we will probably end up with a real username/password login // endpoint, but I think it will only be for use while setting up the rack @@ -79,7 +71,10 @@ async fn login( .status(200) .header( header::SET_COOKIE, - session_cookie_value(&session.token, apictx.session_idle_timeout()), + session_cookie_header_value( + &session.token, + apictx.session_idle_timeout(), + ), ) .body("ok".into()) .unwrap()) @@ -115,7 +110,7 @@ async fn logout( Ok(Response::builder() .status(200) - .header(header::SET_COOKIE, session_cookie_value("", Duration::zero())) + .header(header::SET_COOKIE, clear_session_cookie_header_value()) .body("".into())?) } From 18802efe8c6d90459380b1c20bfcf7822d3354c0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 15:07:38 -0600 Subject: [PATCH 11/35] clean up file serving --- nexus/src/http_entrypoints_console.rs | 70 +++++++++++++-------------- nexus/tests/test_console_api.rs | 45 +++++++++++++---- 2 files changed, 70 insertions(+), 45 deletions(-) diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/http_entrypoints_console.rs index 4a7400cdd00..6c93be9e6b3 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/http_entrypoints_console.rs @@ -155,6 +155,8 @@ async fn console_page( .body("".into())?) } +/// Return a static asset from the configured assets directory. 404 on virtually +/// any error for now. #[endpoint { method = GET, path = "/assets/{path:.*}", @@ -168,7 +170,9 @@ async fn asset( // we *could* auth gate the static assets but it would be kind of weird. // prob not necessary. one way would be to have two directories, one that // requires auth and one that doesn't. I'd rather not + let path = path_params.into_inner().path; + // TODO: make path configurable, confirm existence at startup (though it can // always be deleted later, so maybe confirming existence isn't that // important) @@ -179,64 +183,58 @@ async fn asset( })?; assets_dir.push("tests"); assets_dir.push("fixtures"); - let file = find_file(path, assets_dir); - if file.is_none() { - return Err(HttpError::for_not_found( - None, - "file not found".to_string(), - )); + fn not_found() -> HttpError { + HttpError::for_not_found(None, "File not found".to_string()) } - let file = file.unwrap(); - - let body = tokio::fs::read(&file) - .await - .map_err(|_| HttpError::for_bad_request(None, "EBADF".to_string()))?; + let file = find_file(path, assets_dir).ok_or_else(not_found)?; + let file_contents = + tokio::fs::read(&file).await.map_err(|_| not_found())?; // Derive the MIME type from the file name - let content_type = mime_guess::from_path(&file) // should be the actual file + let content_type = mime_guess::from_path(&file) .first() .map_or("text/plain".to_string(), |m| m.to_string()); Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, &content_type) - .body(body.into())?) + .body(file_contents.into())?) } -// This is a bit stripped down from the dropshot example. For example, we don't -// want to tell the user why the request failed, we just 404 on everything. - -// TODO: return a Result so we can at least log the problems internally. it -// would also clean up the control flow +/// Starting from `root_dir`, follow the segments of `path` down the file tree +/// until we find a file (or not). Do not follow symlinks. fn find_file(path: Vec, root_dir: PathBuf) -> Option { - // start from `root_dir` - let mut current = root_dir; - for component in &path { - // The previous iteration needs to have resulted in a directory. + let mut current = root_dir; // start from `root_dir` + for segment in &path { + // If we hit a non-directory thing already and we still have segments + // left in the path, bail. We have nowhere to go. if !current.is_dir() { return None; } - // Dropshot won't ever give us dot-components. - assert_ne!(component, "."); - assert_ne!(component, ".."); - current.push(component); - - // block symlinks - match current.symlink_metadata() { - Ok(m) => { - if m.file_type().is_symlink() { - return None; - } - } - Err(_) => return None, + + current.push(segment); + + // don't follow symlinks + if is_symlink_or_metadata_error(¤t) { + return None; } } - // if we've landed on a directory, we can't serve that so it's a 404 again + + // can't serve a directory if current.is_dir() { return None; } Some(current) } + +fn is_symlink_or_metadata_error(path: &PathBuf) -> bool { + match path.symlink_metadata() { + Ok(m) => m.file_type().is_symlink(), + // Error means either the user doesn't have permission to pull metadata + // or the path doesn't exist. + Err(_) => true, + } +} diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 4d7f6fe31f2..094b1133742 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -9,9 +9,6 @@ use omicron_common::api::external::OrganizationCreateParams; extern crate slog; -// TODO: test authed/not-authed with requests to console pages instead of -// /organizations - #[tokio::test] async fn test_sessions() { let cptestctx = test_setup("test_sessions").await; @@ -63,8 +60,7 @@ async fn test_sessions() { }, }; - // hitting auth-gated endpoint without session cookie 401s - + // hitting auth-gated API endpoint without session cookie 401s let _ = external_client .make_request_with_body( Method::POST, @@ -74,16 +70,36 @@ async fn test_sessions() { ) .await; - // now make same request with cookie + // console pages don't 401, they 302 + let _ = console_client + .make_request_with_body( + Method::GET, + "/c/whatever", + "".into(), + StatusCode::FOUND, + ) + .await; - let get_orgs = hyper::Request::builder() + // now make same requests with cookie + let create_org = hyper::Request::builder() .header(header::COOKIE, session_token) .method(Method::POST) .uri(external_client.url("/organizations")) .body(serde_json::to_string(&org_params).unwrap().into()) .expect("attempted to construct invalid test request"); external_client - .make_request_with_request(get_orgs, StatusCode::CREATED) + .make_request_with_request(create_org, StatusCode::CREATED) + .await + .expect("failed to make request"); + + let get_console_page = hyper::Request::builder() + .header(header::COOKIE, session_token) + .method(Method::GET) + .uri(console_client.url("/c/whatever")) + .body("".into()) + .expect("attempted to construct invalid test request"); + console_client + .make_request_with_request(get_console_page, StatusCode::OK) .await .expect("failed to make request"); @@ -107,7 +123,7 @@ async fn test_sessions() { "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" ); - // now the same request with the same session cookie should 401 because + // now the same requests with the same session cookie should 401 because // logout also deletes the session server-side let request = hyper::Request::builder() .header(header::COOKIE, session_token) @@ -119,6 +135,17 @@ async fn test_sessions() { .make_request_with_request(request, StatusCode::UNAUTHORIZED) .await; + let get_console_page = hyper::Request::builder() + .header(header::COOKIE, session_token) + .method(Method::GET) + .uri(console_client.url("/c/whatever")) + .body("".into()) + .expect("attempted to construct invalid test request"); + console_client + .make_request_with_request(get_console_page, StatusCode::FOUND) + .await + .expect("failed to make request"); + cptestctx.teardown().await; } From af72045e3c82b6d35fac496f4a66a1ea72ee9781 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 15:11:44 -0600 Subject: [PATCH 12/35] stop using local dropshot --- Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 26ae904950f..e1427ee0306 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,8 +40,8 @@ panic = "abort" # It's common during development to use a local copy of dropshot or steno in the # parent directory. If you want to use those, uncomment one of these blocks. # -[patch."https://github.com/oxidecomputer/dropshot"] -dropshot = { path = "../dropshot/dropshot" } +#[patch."https://github.com/oxidecomputer/dropshot"] +#dropshot = { path = "../dropshot/dropshot" } #[patch."https://github.com/oxidecomputer/steno"] #steno = { path = "../steno" } From 98d5f898df23d712e34e9258b771d8a0f83aab51 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 15:15:26 -0600 Subject: [PATCH 13/35] make console_api match the new dir structure --- .../http_entrypoints.rs} | 5 ++--- nexus/src/console_api/mod.rs | 1 + nexus/src/lib.rs | 4 ++-- nexus/tests/test_console_api.rs | 9 +++++---- 4 files changed, 10 insertions(+), 9 deletions(-) rename nexus/src/{http_entrypoints_console.rs => console_api/http_entrypoints.rs} (99%) create mode 100644 nexus/src/console_api/mod.rs diff --git a/nexus/src/http_entrypoints_console.rs b/nexus/src/console_api/http_entrypoints.rs similarity index 99% rename from nexus/src/http_entrypoints_console.rs rename to nexus/src/console_api/http_entrypoints.rs index 6c93be9e6b3..72b03580f75 100644 --- a/nexus/src/http_entrypoints_console.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -1,8 +1,6 @@ /*! * Handler functions (entrypoints) for console-related routes. */ -use super::ServerContext; - use crate::authn::external::{ cookies::Cookies, session_cookie::{ @@ -12,6 +10,7 @@ use crate::authn::external::{ }; use crate::authn::TEST_USER_UUID_PRIVILEGED; use crate::context::OpContext; +use crate::ServerContext; use dropshot::{ endpoint, ApiDescription, HttpError, Path, RequestContext, TypedBody, }; @@ -27,7 +26,7 @@ type NexusApiDescription = ApiDescription>; /** * Returns a description of the part of the nexus API dedicated to the web console */ -pub fn api() -> NexusApiDescription { +pub fn console_api() -> NexusApiDescription { fn register_endpoints(api: &mut NexusApiDescription) -> Result<(), String> { api.register(login)?; api.register(logout)?; diff --git a/nexus/src/console_api/mod.rs b/nexus/src/console_api/mod.rs new file mode 100644 index 00000000000..7f08fd4ac54 --- /dev/null +++ b/nexus/src/console_api/mod.rs @@ -0,0 +1 @@ +pub mod http_entrypoints; diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index cc4ef8d0037..a744094f8eb 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -15,19 +15,19 @@ pub mod authn; // Public only for testing mod authz; mod config; +mod console_api; mod context; pub mod db; // Public only for some documentation examples pub mod external_api; // public for testing -mod http_entrypoints_console; pub mod internal_api; // public for testing mod nexus; mod saga_interface; mod sagas; pub use config::Config; +use console_api::http_entrypoints::console_api; pub use context::ServerContext; use external_api::http_entrypoints::external_api; -use http_entrypoints_console::api as console_api; use internal_api::http_entrypoints::internal_api; pub use nexus::Nexus; pub use nexus::TestInterfaces; diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 094b1133742..a888ba84e3a 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -5,7 +5,7 @@ use hyper; pub mod common; use common::test_setup; use omicron_common::api::external::IdentityMetadataCreateParams; -use omicron_common::api::external::OrganizationCreateParams; +use omicron_nexus::external_api::params::OrganizationCreate; extern crate slog; @@ -15,8 +15,9 @@ async fn test_sessions() { let console_client = &cptestctx.console_client; let external_client = &cptestctx.external_client; - // Set-Cookie responses only work if you make dropshot's test utils not - // panic! when it sees the Set-Cookie header + // TODO: responses with set-cookie in them won't work until this uses the + // new test helpers that don't lock you into a particular set of allowed + // headers. See https://github.com/oxidecomputer/omicron/pull/403 let resp = console_client .make_request_with_body( @@ -53,7 +54,7 @@ async fn test_sessions() { assert!(session_token.starts_with("session=")); assert_eq!(rest, "Secure; HttpOnly; SameSite=Lax; Max-Age=3600"); - let org_params = OrganizationCreateParams { + let org_params = OrganizationCreate { identity: IdentityMetadataCreateParams { name: "my-org".parse().unwrap(), description: "an org".to_string(), From efe80ebf5c35c010248fc12bf51b2680ebdd3998 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 15:45:29 -0600 Subject: [PATCH 14/35] real internal error message for file not found errors --- nexus/src/console_api/http_entrypoints.rs | 41 +++++++++++------------ 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/console_api/http_entrypoints.rs index 72b03580f75..285926d6b78 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -183,13 +183,9 @@ async fn asset( assets_dir.push("tests"); assets_dir.push("fixtures"); - fn not_found() -> HttpError { - HttpError::for_not_found(None, "File not found".to_string()) - } - - let file = find_file(path, assets_dir).ok_or_else(not_found)?; + let file = find_file(path, assets_dir)?; let file_contents = - tokio::fs::read(&file).await.map_err(|_| not_found())?; + tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; // Derive the MIME type from the file name let content_type = mime_guess::from_path(&file) @@ -202,38 +198,39 @@ async fn asset( .body(file_contents.into())?) } +fn not_found(internal_msg: &str) -> HttpError { + HttpError::for_not_found(None, internal_msg.to_string()) +} + /// Starting from `root_dir`, follow the segments of `path` down the file tree /// until we find a file (or not). Do not follow symlinks. -fn find_file(path: Vec, root_dir: PathBuf) -> Option { +fn find_file( + path: Vec, + root_dir: PathBuf, +) -> Result { let mut current = root_dir; // start from `root_dir` for segment in &path { // If we hit a non-directory thing already and we still have segments // left in the path, bail. We have nowhere to go. if !current.is_dir() { - return None; + return Err(not_found("ENOTDIR")); } current.push(segment); - // don't follow symlinks - if is_symlink_or_metadata_error(¤t) { - return None; + // Don't follow symlinks. + // Error means either the user doesn't have permission to pull + // metadata or the path doesn't exist. + let m = current.symlink_metadata().map_err(|_| not_found("ENOENT"))?; + if m.file_type().is_symlink() { + return Err(not_found("EMLINK")); } } // can't serve a directory if current.is_dir() { - return None; + return Err(not_found("EISDIR")); } - Some(current) -} - -fn is_symlink_or_metadata_error(path: &PathBuf) -> bool { - match path.symlink_metadata() { - Ok(m) => m.file_type().is_symlink(), - // Error means either the user doesn't have permission to pull metadata - // or the path doesn't exist. - Err(_) => true, - } + Ok(current) } From 356cf7c89eb424659470701bcf068a3c4998ba3f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 16:17:38 -0600 Subject: [PATCH 15/35] make assets directory configurable --- nexus/examples/config-file.toml | 4 ++++ nexus/examples/config.toml | 4 ++++ nexus/src/config.rs | 6 ++++++ nexus/src/console_api/http_entrypoints.rs | 24 +++++++---------------- nexus/src/context.rs | 17 +++++++++++++++- nexus/tests/config.test.toml | 4 ++++ smf/nexus/config.toml | 4 ++++ 7 files changed, 45 insertions(+), 18 deletions(-) diff --git a/nexus/examples/config-file.toml b/nexus/examples/config-file.toml index b4f57c57757..ecaf97b4c0b 100644 --- a/nexus/examples/config-file.toml +++ b/nexus/examples/config-file.toml @@ -5,6 +5,10 @@ # Identifier for this instance of Nexus id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +# Directory of static assets for the console +# NOTE: relative to /nexus +assets_directory = "tests/fixtures" # TODO: figure out value + # List of authentication schemes to support. # # This is not fleshed out yet and the only reason to change it now is for diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 20aae8aba19..0902f6a2be1 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -5,6 +5,10 @@ # Identifier for this instance of Nexus id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +# Directory of static assets for the console +# NOTE: relative to /nexus +assets_directory = "tests/fixtures" # TODO: figure out value + # List of authentication schemes to support. # # This is not fleshed out yet and the only reason to change it now is for diff --git a/nexus/src/config.rs b/nexus/src/config.rs index e60b3932b99..ac97668a5be 100644 --- a/nexus/src/config.rs +++ b/nexus/src/config.rs @@ -42,6 +42,8 @@ pub struct Config { pub dropshot_console: ConfigDropshot, /** Identifier for this instance of Nexus */ pub id: uuid::Uuid, + /** */ + pub assets_directory: PathBuf, /** Server-wide logging configuration. */ pub log: ConfigLogging, /** Database parameters */ @@ -259,6 +261,7 @@ mod test { "valid", r##" id = "28b90dc4-c22a-65ba-f49a-f051fe01208f" + assets_directory = "tests/fixtures" [authn] schemes_external = [] session_idle_timeout_minutes = 60 @@ -287,6 +290,7 @@ mod test { config, Config { id: "28b90dc4-c22a-65ba-f49a-f051fe01208f".parse().unwrap(), + assets_directory: "tests/fixtures".parse().unwrap(), authn: AuthnConfig { schemes_external: Vec::new(), session_idle_timeout_minutes: 60, @@ -327,6 +331,7 @@ mod test { "valid", r##" id = "28b90dc4-c22a-65ba-f49a-f051fe01208f" + assets_directory = "tests/fixtures" [authn] schemes_external = [ "spoof", "session_cookie" ] session_idle_timeout_minutes = 60 @@ -365,6 +370,7 @@ mod test { "bad authn.schemes_external", r##" id = "28b90dc4-c22a-65ba-f49a-f051fe01208f" + assets_directory = "tests/fixtures" [authn] schemes_external = ["trust-me"] session_idle_timeout_minutes = 60 diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/console_api/http_entrypoints.rs index 285926d6b78..c96628098d1 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -19,7 +19,7 @@ use hyper::Body; use mime_guess; use schemars::JsonSchema; use serde::Deserialize; -use std::{env::current_dir, path::PathBuf, sync::Arc}; +use std::{path::PathBuf, sync::Arc}; type NexusApiDescription = ApiDescription>; @@ -143,7 +143,7 @@ async fn console_page( return Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") - .body("".into())?); + .body("".into())?); // TODO: actual HTML } } @@ -163,27 +163,17 @@ async fn console_page( unpublished = true, }] async fn asset( - _rqctx: Arc>>, + rqctx: Arc>>, path_params: Path, ) -> Result, HttpError> { // we *could* auth gate the static assets but it would be kind of weird. // prob not necessary. one way would be to have two directories, one that // requires auth and one that doesn't. I'd rather not + let apictx = rqctx.context(); let path = path_params.into_inner().path; - // TODO: make path configurable, confirm existence at startup (though it can - // always be deleted later, so maybe confirming existence isn't that - // important) - let mut assets_dir = current_dir().map_err(|_| { - HttpError::for_internal_error( - "couldn't pull current execution directory".to_string(), - ) - })?; - assets_dir.push("tests"); - assets_dir.push("fixtures"); - - let file = find_file(path, assets_dir)?; + let file = find_file(path, &apictx.assets_directory)?; let file_contents = tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; @@ -206,9 +196,9 @@ fn not_found(internal_msg: &str) -> HttpError { /// until we find a file (or not). Do not follow symlinks. fn find_file( path: Vec, - root_dir: PathBuf, + root_dir: &PathBuf, ) -> Result { - let mut current = root_dir; // start from `root_dir` + let mut current = root_dir.to_owned(); // start from `root_dir` for segment in &path { // If we hit a non-directory thing already and we still have segments // left in the path, bail. We have nowhere to go. diff --git a/nexus/src/context.rs b/nexus/src/context.rs index 41f60f1e4ac..d34709ea29a 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -19,7 +19,9 @@ use oximeter::types::ProducerRegistry; use oximeter_instruments::http::{HttpService, LatencyTracker}; use slog::Logger; use std::collections::BTreeMap; +use std::env::current_dir; use std::fmt::Debug; +use std::path::PathBuf; use std::sync::Arc; use std::time::Instant; use std::time::SystemTime; @@ -43,8 +45,10 @@ pub struct ServerContext { pub external_latencies: LatencyTracker, /** registry of metric producers */ pub producer_registry: ProducerRegistry, - /** the whole config */ + /** tunable settings needed at runtime */ pub tunables: Tunables, + /** directory containing static assets for the console */ + pub assets_directory: PathBuf, } pub struct Tunables { @@ -101,6 +105,16 @@ impl ServerContext { .register_producer(external_latencies.clone()) .unwrap(); + // TODO: currently relative to the execution dir, should this be absolute? + let assets_directory = current_dir() + .expect("could not access current directory") + .join(config.assets_directory.to_owned()); + // TODO: do we want to assert this? there are no other asserts in here + assert!( + assets_directory.exists(), + "assets directory must exist at start time" + ); + Arc::new(ServerContext { nexus: Nexus::new_with_id( rack_id, @@ -123,6 +137,7 @@ impl ServerContext { config.authn.session_absolute_timeout_minutes.into(), ), }, + assets_directory, }) } } diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index 367038abbca..e9feb0dd437 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -6,6 +6,10 @@ # NOTE: The test suite always overrides this. id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +# Directory of static assets for the console +# NOTE: relative to /nexus +assets_directory = "tests/fixtures" + # List of authentication schemes to support. [authn] schemes_external = [ "spoof", "session_cookie" ] diff --git a/smf/nexus/config.toml b/smf/nexus/config.toml index 1c46eea4f54..0d74db83a8b 100644 --- a/smf/nexus/config.toml +++ b/smf/nexus/config.toml @@ -5,6 +5,10 @@ # Identifier for this instance of Nexus id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +# Directory of static assets for the console +# NOTE: relative to /nexus +assets_directory = "tests/fixtures" # TODO: figure out value + [authn] # TODO(https://github.com/oxidecomputer/omicron/issues/372): Remove "spoof". schemes_external = ["spoof"] From 87a880038b111fea3f16c10cfdee3585a6f8f964 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 20:00:34 -0600 Subject: [PATCH 16/35] lovely unit tests for find_file, integration test for symlink --- nexus/src/console_api/http_entrypoints.rs | 82 ++++++++++++++++++- .../fixtures/a_directory/another_file.txt | 1 + nexus/tests/fixtures/a_symlink | 1 + nexus/tests/test_console_api.rs | 10 +++ 4 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 nexus/tests/fixtures/a_directory/another_file.txt create mode 120000 nexus/tests/fixtures/a_symlink diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/console_api/http_entrypoints.rs index c96628098d1..1f4d0a66a27 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -203,7 +203,7 @@ fn find_file( // If we hit a non-directory thing already and we still have segments // left in the path, bail. We have nowhere to go. if !current.is_dir() { - return Err(not_found("ENOTDIR")); + return Err(not_found("ENOENT")); } current.push(segment); @@ -224,3 +224,83 @@ fn find_file( Ok(current) } + +#[cfg(test)] +mod test { + use super::find_file; + use http::StatusCode; + use std::{env::current_dir, path::PathBuf}; + + fn get_path(path_str: &str) -> Vec { + path_str.split("/").map(|s| s.to_string()).collect() + } + + #[test] + fn test_find_file_finds_file() { + let root = current_dir().unwrap(); + let file = find_file(get_path("tests/fixtures/hello.txt"), &root); + assert!(file.is_ok()); + } + + #[test] + fn test_find_file_404_on_nonexistent() { + let root = current_dir().unwrap(); + let error = + find_file(get_path("tests/fixtures/nonexistent.svg"), &root) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + assert_eq!(error.internal_message, "ENOENT".to_string()); + } + + #[test] + fn test_find_file_404_on_nonexistent_nested() { + let root = current_dir().unwrap(); + let error = + find_file(get_path("tests/fixtures/a/b/c/nonexistent.svg"), &root) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + assert_eq!(error.internal_message, "ENOENT".to_string()); + } + + #[test] + fn test_find_file_404_on_directory() { + let root = current_dir().unwrap(); + let error = find_file(get_path("tests/fixtures/a_directory"), &root) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + assert_eq!(error.internal_message, "EISDIR".to_string()); + } + + #[test] + fn test_find_file_404_on_symlink() { + let root = current_dir().unwrap(); + let path_str = "tests/fixtures/a_symlink"; + + // the file in question does exist and is a symlink + assert!(root + .join(PathBuf::from(path_str)) + .symlink_metadata() + .unwrap() + .file_type() + .is_symlink()); + + // so we 404 + let error = find_file(get_path(path_str), &root).unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + assert_eq!(error.internal_message, "EMLINK".to_string()); + } + + #[test] + fn test_find_file_wont_follow_symlink() { + let root = current_dir().unwrap(); + let path_str = "tests/fixtures/a_symlink/another_file.txt"; + + // the file in question does exist + assert!(root.join(PathBuf::from(path_str)).exists()); + + // but it 404s because the path goes through a symlink + let error = find_file(get_path(path_str), &root).unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + assert_eq!(error.internal_message, "EMLINK".to_string()); + } +} diff --git a/nexus/tests/fixtures/a_directory/another_file.txt b/nexus/tests/fixtures/a_directory/another_file.txt new file mode 100644 index 00000000000..d9ea2697614 --- /dev/null +++ b/nexus/tests/fixtures/a_directory/another_file.txt @@ -0,0 +1 @@ +some words \ No newline at end of file diff --git a/nexus/tests/fixtures/a_symlink b/nexus/tests/fixtures/a_symlink new file mode 120000 index 00000000000..dcd68e42424 --- /dev/null +++ b/nexus/tests/fixtures/a_symlink @@ -0,0 +1 @@ +./a_directory \ No newline at end of file diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index a888ba84e3a..1272ae74053 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -194,6 +194,16 @@ async fn test_assets() { ) .await; + // symlink 404s + let _ = client + .make_request_with_body( + Method::GET, + "/assets/a_symlink", + "".into(), + StatusCode::NOT_FOUND, + ) + .await; + // existing file is returned let mut response = client .make_request_with_body( From 7de6ffc686f490de1f3e083b3f709d1a308ebbd6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Nov 2021 20:08:49 -0600 Subject: [PATCH 17/35] take out comment wondering whether to auth static files --- nexus/src/console_api/http_entrypoints.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/console_api/http_entrypoints.rs index 1f4d0a66a27..55de084e825 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -154,8 +154,8 @@ async fn console_page( .body("".into())?) } -/// Return a static asset from the configured assets directory. 404 on virtually -/// any error for now. +/// Fetch a static asset from the configured assets directory. 404 on virtually +/// all errors. No auth. NO SENSITIVE FILES. #[endpoint { method = GET, path = "/assets/{path:.*}", @@ -166,10 +166,6 @@ async fn asset( rqctx: Arc>>, path_params: Path, ) -> Result, HttpError> { - // we *could* auth gate the static assets but it would be kind of weird. - // prob not necessary. one way would be to have two directories, one that - // requires auth and one that doesn't. I'd rather not - let apictx = rqctx.context(); let path = path_params.into_inner().path; From 5b6b028bd04536a08f7826ac982b672abbf61739 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 19 Nov 2021 15:39:53 -0600 Subject: [PATCH 18/35] use new helpers in tests, response_body() -> parsed_body() --- nexus/src/console_api/http_entrypoints.rs | 14 +- nexus/src/lib.rs | 2 +- nexus/tests/common/http_testing.rs | 13 +- nexus/tests/common/resource_helpers.rs | 2 +- nexus/tests/test_authz.rs | 2 +- nexus/tests/test_console_api.rs | 214 +++++++++------------- 6 files changed, 99 insertions(+), 148 deletions(-) diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/console_api/http_entrypoints.rs index 55de084e825..c2c1b373e79 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -18,7 +18,7 @@ use http::{header, Response, StatusCode}; use hyper::Body; use mime_guess; use schemars::JsonSchema; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use std::{path::PathBuf, sync::Arc}; type NexusApiDescription = ApiDescription>; @@ -42,11 +42,11 @@ pub fn console_api() -> NexusApiDescription { api } -#[derive(Clone, Debug, Deserialize, JsonSchema)] +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct LoginParams { - username: String, - password: String, + pub username: String, + pub password: String, } // for now this is just for testing purposes, and the username and password are @@ -67,7 +67,7 @@ async fn login( .session_create(TEST_USER_UUID_PRIVILEGED.parse().unwrap()) .await?; Ok(Response::builder() - .status(200) + .status(StatusCode::OK) .header( header::SET_COOKIE, session_cookie_header_value( @@ -75,7 +75,7 @@ async fn login( apictx.session_idle_timeout(), ), ) - .body("ok".into()) + .body("ok".into()) // TODO: what do we return from login? .unwrap()) } @@ -108,7 +108,7 @@ async fn logout( // intended, and we should send the standard success response. Ok(Response::builder() - .status(200) + .status(StatusCode::NO_CONTENT) .header(header::SET_COOKIE, clear_session_cookie_header_value()) .body("".into())?) } diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index a744094f8eb..756e1f92cc1 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -15,7 +15,7 @@ pub mod authn; // Public only for testing mod authz; mod config; -mod console_api; +pub mod console_api; // public for testing mod context; pub mod db; // Public only for some documentation examples pub mod external_api; // public for testing diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 7770d9c0da9..74b9ce0607c 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -77,6 +77,7 @@ impl<'a> RequestBuilder<'a> { http::header::CONTENT_TYPE, http::header::DATE, http::header::SET_COOKIE, + http::header::LOCATION, http::header::HeaderName::from_static("x-request-id"), ]), error: None, @@ -298,7 +299,7 @@ impl<'a> RequestBuilder<'a> { }; if status.is_client_error() || status.is_server_error() { let error_body = test_response - .response_body::() + .parsed_body::() .context("parsing error body")?; ensure!( error_body.request_id == request_id_header, @@ -317,14 +318,14 @@ impl<'a> RequestBuilder<'a> { pub struct TestResponse { pub status: http::StatusCode, pub headers: http::HeaderMap, - body: bytes::Bytes, + pub body: bytes::Bytes, } impl TestResponse { /// Parse the response body as an instance of `R` and returns it /// /// Fails if the body could not be parsed as an `R`. - pub fn response_body( + pub fn parsed_body( &self, ) -> Result { serde_json::from_slice(self.body.as_ref()) @@ -434,7 +435,7 @@ pub mod dropshot_compat { .execute() .await .unwrap() - .response_body::() + .parsed_body::() .unwrap() } @@ -450,7 +451,7 @@ pub mod dropshot_compat { .execute() .await .unwrap() - .response_body::>() + .parsed_body::>() .unwrap() } @@ -468,7 +469,7 @@ pub mod dropshot_compat { .execute() .await .unwrap() - .response_body::() + .parsed_body::() .unwrap() } } diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index 8464920fbf7..7d883757d5e 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -28,7 +28,7 @@ pub async fn create_organization( .execute() .await .expect("failed to make request") - .response_body() + .parsed_body() .unwrap() } diff --git a/nexus/tests/test_authz.rs b/nexus/tests/test_authz.rs index fe071b5b80e..7cc73c4c16f 100644 --- a/nexus/tests/test_authz.rs +++ b/nexus/tests/test_authz.rs @@ -94,6 +94,6 @@ async fn try_create_organization( .execute() .await .expect("failed to make request") - .response_body() + .parsed_body() .unwrap() } diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 1272ae74053..5282dc6fa21 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -1,10 +1,11 @@ -use dropshot::test_util::read_string; +use http::header::HeaderName; use http::{header, method::Method, StatusCode}; -use hyper; pub mod common; +use common::http_testing::{RequestBuilder, TestResponse}; use common::test_setup; use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_nexus::console_api::http_entrypoints::LoginParams; use omicron_nexus::external_api::params::OrganizationCreate; extern crate slog; @@ -12,43 +13,32 @@ extern crate slog; #[tokio::test] async fn test_sessions() { let cptestctx = test_setup("test_sessions").await; - let console_client = &cptestctx.console_client; - let external_client = &cptestctx.external_client; - - // TODO: responses with set-cookie in them won't work until this uses the - // new test helpers that don't lock you into a particular set of allowed - // headers. See https://github.com/oxidecomputer/omicron/pull/403 - - let resp = console_client - .make_request_with_body( - Method::POST, - "/logout", - "".into(), - StatusCode::OK, - ) + let console_testctx = &cptestctx.console_client; + let external_testctx = &cptestctx.external_client; + + let logout = RequestBuilder::new(&console_testctx, Method::POST, "/logout") + .expect_status(Some(StatusCode::NO_CONTENT)) + .execute() .await .unwrap(); // logout always gives the same response whether you have a session or not - let set_cookie_header = - resp.headers().get("set-cookie").unwrap().to_str().unwrap(); assert_eq!( - set_cookie_header, + get_header_value(logout, header::SET_COOKIE), "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" ); - let resp = console_client - .make_request_with_body( - Method::POST, - "/login", - "{ \"username\": \"\", \"password\": \"\"}".into(), - StatusCode::OK, - ) + let login = RequestBuilder::new(&console_testctx, Method::POST, "/login") + .body(Some(LoginParams { + username: "".parse().unwrap(), + password: "".parse().unwrap(), + })) + .expect_status(Some(StatusCode::OK)) + .execute() .await .unwrap(); - let session_cookie = - resp.headers().get("set-cookie").unwrap().to_str().unwrap(); + let session_cookie = get_header_value(login, header::SET_COOKIE); let (session_token, rest) = session_cookie.split_once("; ").unwrap(); assert!(session_token.starts_with("session=")); @@ -62,114 +52,82 @@ async fn test_sessions() { }; // hitting auth-gated API endpoint without session cookie 401s - let _ = external_client - .make_request_with_body( - Method::POST, - "/organizations", - serde_json::to_string(&org_params).unwrap().into(), - StatusCode::UNAUTHORIZED, - ) - .await; + let _ = + RequestBuilder::new(&external_testctx, Method::POST, "/organizations") + .body(Some(org_params.clone())) + .expect_status(Some(StatusCode::UNAUTHORIZED)) + .execute() + .await; // console pages don't 401, they 302 - let _ = console_client - .make_request_with_body( - Method::GET, - "/c/whatever", - "".into(), - StatusCode::FOUND, - ) + let _ = RequestBuilder::new(&external_testctx, Method::POST, "/c/whatever") + .expect_status(Some(StatusCode::FOUND)) + .execute() .await; // now make same requests with cookie - let create_org = hyper::Request::builder() - .header(header::COOKIE, session_token) - .method(Method::POST) - .uri(external_client.url("/organizations")) - .body(serde_json::to_string(&org_params).unwrap().into()) - .expect("attempted to construct invalid test request"); - external_client - .make_request_with_request(create_org, StatusCode::CREATED) - .await - .expect("failed to make request"); - - let get_console_page = hyper::Request::builder() + let _ = + RequestBuilder::new(&external_testctx, Method::POST, "/organizations") + .header(header::COOKIE, session_token) + .body(Some(org_params.clone())) + // TODO: explicit expect_status not needed. decide whether to keep it anyway + .expect_status(Some(StatusCode::CREATED)) + .execute() + .await; + + let _ = RequestBuilder::new(&console_testctx, Method::GET, "/c/whatever") .header(header::COOKIE, session_token) - .method(Method::GET) - .uri(console_client.url("/c/whatever")) - .body("".into()) - .expect("attempted to construct invalid test request"); - console_client - .make_request_with_request(get_console_page, StatusCode::OK) - .await - .expect("failed to make request"); + .expect_status(Some(StatusCode::OK)) + .execute() + .await; // logout with an actual session should delete the session in the db - let logout_request = hyper::Request::builder() + let logout = RequestBuilder::new(&console_testctx, Method::POST, "/logout") .header(header::COOKIE, session_token) - .method(Method::POST) - .uri(console_client.url("/logout")) - .body("".into()) - .expect("attempted to construct invalid test request"); - let logout_resp = console_client - .make_request_with_request(logout_request, StatusCode::OK) + .expect_status(Some(StatusCode::NO_CONTENT)) + .execute() .await .unwrap(); // logout clears the cookie client-side - let set_cookie_header = - logout_resp.headers().get("set-cookie").unwrap().to_str().unwrap(); assert_eq!( - set_cookie_header, + get_header_value(logout, header::SET_COOKIE), "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" ); - // now the same requests with the same session cookie should 401 because + // now the same requests with the same session cookie should 401/302 because // logout also deletes the session server-side - let request = hyper::Request::builder() + let _ = + RequestBuilder::new(&external_testctx, Method::POST, "/organizations") + .header(header::COOKIE, session_token) + .body(Some(org_params)) + .expect_status(Some(StatusCode::UNAUTHORIZED)) + .execute() + .await; + + let _ = RequestBuilder::new(&console_testctx, Method::GET, "/c/whatever") .header(header::COOKIE, session_token) - .method(Method::POST) - .uri(external_client.url("/organizations")) - .body(serde_json::to_string(&org_params).unwrap().into()) - .expect("attempted to construct invalid test request"); - let _ = external_client - .make_request_with_request(request, StatusCode::UNAUTHORIZED) + .expect_status(Some(StatusCode::FOUND)) + .execute() .await; - let get_console_page = hyper::Request::builder() - .header(header::COOKIE, session_token) - .method(Method::GET) - .uri(console_client.url("/c/whatever")) - .body("".into()) - .expect("attempted to construct invalid test request"); - console_client - .make_request_with_request(get_console_page, StatusCode::FOUND) - .await - .expect("failed to make request"); - cptestctx.teardown().await; } #[tokio::test] async fn test_console_pages() { let cptestctx = test_setup("test_console_pages").await; - let client = &cptestctx.console_client; + let testctx = &cptestctx.console_client; // request to console page route without auth should redirect to IdP - let unauthed_response = client - .make_request_with_body( - Method::GET, - // 404s will be handled client-side unless we want to pull in the - // entire route tree from the client (which we may well want to do) - "/c/irrelevant-path", - "".into(), - StatusCode::FOUND, - ) - .await - .unwrap(); - - let location_header = - unauthed_response.headers().get("location").unwrap().to_str().unwrap(); + let unauthed_response = + RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") + .expect_status(Some(StatusCode::FOUND)) + .execute() + .await + .unwrap(); + + let location_header = get_header_value(unauthed_response, header::LOCATION); assert_eq!(location_header, "idp.com/login"); // get session @@ -182,40 +140,32 @@ async fn test_console_pages() { #[tokio::test] async fn test_assets() { let cptestctx = test_setup("test_assets").await; - let client = &cptestctx.console_client; + let testctx = &cptestctx.console_client; // nonexistent file 404s - let _ = client - .make_request_with_body( - Method::GET, - "/assets/nonexistent.svg", - "".into(), - StatusCode::NOT_FOUND, - ) - .await; + let _ = + RequestBuilder::new(&testctx, Method::GET, "/assets/nonexistent.svg") + .expect_status(Some(StatusCode::NOT_FOUND)) + .execute() + .await; // symlink 404s - let _ = client - .make_request_with_body( - Method::GET, - "/assets/a_symlink", - "".into(), - StatusCode::NOT_FOUND, - ) + let _ = RequestBuilder::new(&testctx, Method::GET, "/assets/a_symlink") + .expect_status(Some(StatusCode::NOT_FOUND)) + .execute() .await; // existing file is returned - let mut response = client - .make_request_with_body( - Method::GET, - "/assets/hello.txt", - "".into(), - StatusCode::OK, - ) + let resp = RequestBuilder::new(&testctx, Method::GET, "/assets/hello.txt") + .execute() .await .unwrap(); - let file_contents = read_string(&mut response).await; - assert_eq!(file_contents, "hello there".to_string()); + + assert_eq!(resp.body, "hello there".as_bytes()); cptestctx.teardown().await; } + +fn get_header_value(resp: TestResponse, header_name: HeaderName) -> String { + resp.headers.get(header_name).unwrap().to_str().unwrap().to_string() +} From a43dd4491f021459bf474cdb05ccd01ab6b95490 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 19 Nov 2021 16:29:11 -0600 Subject: [PATCH 19/35] add expect_response_header and use it --- nexus/tests/common/http_testing.rs | 64 ++++++++++++++++++++++++++++++ nexus/tests/test_console_api.rs | 42 +++++++++----------- 2 files changed, 83 insertions(+), 23 deletions(-) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 74b9ce0607c..086b42578d9 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -55,6 +55,8 @@ pub struct RequestBuilder<'a> { expected_status: Option, allowed_headers: Option>, + // doesn't need Option<> because if it's empty, we don't check anything + expected_response_headers: http::HeaderMap, } impl<'a> RequestBuilder<'a> { @@ -80,6 +82,7 @@ impl<'a> RequestBuilder<'a> { http::header::LOCATION, http::header::HeaderName::from_static("x-request-id"), ]), + expected_response_headers: http::HeaderMap::new(), error: None, } } @@ -166,6 +169,48 @@ impl<'a> RequestBuilder<'a> { self } + /// Add header and value to check for at execution time + /// + /// Behaves like header() rather than expect_allowed_headers() in that it + /// takes one header at a time rather than a whole set. + pub fn expect_response_header( + mut self, + name: K, + value: V, + ) -> Self + where + K: TryInto + Debug, + V: TryInto + Debug, + KE: std::error::Error + Send + Sync + 'static, + VE: std::error::Error + Send + Sync + 'static, + { + let header_name_dbg = format!("{:?}", name); + let header_value_dbg = format!("{:?}", value); + let header_name: Result = + name.try_into().with_context(|| { + format!("converting header name {}", header_name_dbg) + }); + let header_value: Result = + value.try_into().with_context(|| { + format!( + "converting value for header {}: {}", + header_name_dbg, header_value_dbg + ) + }); + match (header_name, header_value) { + (Ok(name), Ok(value)) => { + self.expected_response_headers.append(name, value); + } + (Err(error), _) => { + self.error = Some(error); + } + (_, Err(error)) => { + self.error = Some(error); + } + }; + self + } + /// Make the HTTP request using the given `client`, verify the returned /// response, and make the response available to the caller /// @@ -228,6 +273,25 @@ impl<'a> RequestBuilder<'a> { } } + // Check that we do have all expected headers + for (header_name, expected_value) in + self.expected_response_headers.iter() + { + ensure!( + headers.contains_key(header_name), + "response did not contain expected header {:?}", + header_name + ); + let actual_value = headers.get(header_name).unwrap(); + ensure!( + actual_value == expected_value, + "response contained expected header {:?}, but with value {:?} instead of expected {:?}", + header_name, + actual_value, + expected_value, + ); + } + // Sanity check the Date header in the response. This check assumes // that the server is running on the same system as the client, which is // currently true because this is running in a test suite. We may need diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 5282dc6fa21..d7999a7ab10 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -16,18 +16,18 @@ async fn test_sessions() { let console_testctx = &cptestctx.console_client; let external_testctx = &cptestctx.external_client; - let logout = RequestBuilder::new(&console_testctx, Method::POST, "/logout") + // logout always gives the same response whether you have a session or not + let _ = RequestBuilder::new(&console_testctx, Method::POST, "/logout") .expect_status(Some(StatusCode::NO_CONTENT)) + .expect_response_header( + header::SET_COOKIE, + "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0", + ) .execute() .await .unwrap(); - // logout always gives the same response whether you have a session or not - assert_eq!( - get_header_value(logout, header::SET_COOKIE), - "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" - ); - + // log in and pull the token out of the header so we can use it for authed requests let login = RequestBuilder::new(&console_testctx, Method::POST, "/login") .body(Some(LoginParams { username: "".parse().unwrap(), @@ -82,19 +82,18 @@ async fn test_sessions() { .await; // logout with an actual session should delete the session in the db - let logout = RequestBuilder::new(&console_testctx, Method::POST, "/logout") + let _ = RequestBuilder::new(&console_testctx, Method::POST, "/logout") .header(header::COOKIE, session_token) .expect_status(Some(StatusCode::NO_CONTENT)) + // logout also clears the cookie client-side + .expect_response_header( + header::SET_COOKIE, + "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0", + ) .execute() .await .unwrap(); - // logout clears the cookie client-side - assert_eq!( - get_header_value(logout, header::SET_COOKIE), - "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" - ); - // now the same requests with the same session cookie should 401/302 because // logout also deletes the session server-side let _ = @@ -120,15 +119,12 @@ async fn test_console_pages() { let testctx = &cptestctx.console_client; // request to console page route without auth should redirect to IdP - let unauthed_response = - RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") - .expect_status(Some(StatusCode::FOUND)) - .execute() - .await - .unwrap(); - - let location_header = get_header_value(unauthed_response, header::LOCATION); - assert_eq!(location_header, "idp.com/login"); + let _ = RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") + .expect_status(Some(StatusCode::FOUND)) + .expect_response_header(header::LOCATION, "idp.com/login") + .execute() + .await + .unwrap(); // get session From 98e95bda0d9d08226afaa29607cf37f8f2e6702c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 22 Nov 2021 11:43:29 -0600 Subject: [PATCH 20/35] render html from console pages route --- nexus/src/console_api/http_entrypoints.rs | 9 +++++++-- nexus/tests/test_console_api.rs | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/console_api/http_entrypoints.rs index c2c1b373e79..68625794cef 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/console_api/http_entrypoints.rs @@ -140,17 +140,22 @@ async fn console_page( // makes the right API requests. if let Ok(opctx) = opctx { if opctx.authn.actor().is_some() { + let apictx = rqctx.context(); + let file = + &apictx.assets_directory.join(PathBuf::from("index.html")); + let file_contents = + tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; return Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") - .body("".into())?); // TODO: actual HTML + .body(file_contents.into())?); } } // otherwise redirect to idp Ok(Response::builder() .status(StatusCode::FOUND) - .header(http::header::LOCATION, "idp.com/login") + .header(http::header::LOCATION, "https://idp.com/login") .body("".into())?) } diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index d7999a7ab10..eeb29df4ff7 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -121,7 +121,7 @@ async fn test_console_pages() { // request to console page route without auth should redirect to IdP let _ = RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") .expect_status(Some(StatusCode::FOUND)) - .expect_response_header(header::LOCATION, "idp.com/login") + .expect_response_header(header::LOCATION, "https://idp.com/login") .execute() .await .unwrap(); From 7c58beadd59361986b6adf1c2e3f6b6b441020c9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 22 Nov 2021 12:26:26 -0600 Subject: [PATCH 21/35] serve console routes from external_api dropshots server to avoid cors --- nexus/examples/config.toml | 4 - nexus/src/config.rs | 17 ---- nexus/src/console_api/mod.rs | 1 - .../console_api.rs} | 81 ++++++++----------- nexus/src/external_api/http_entrypoints.rs | 11 ++- nexus/src/external_api/mod.rs | 1 + nexus/src/lib.rs | 25 +----- nexus/tests/common/mod.rs | 7 -- nexus/tests/config.test.toml | 4 - nexus/tests/test_console_api.rs | 61 +++++++------- smf/nexus/config.toml | 4 - 11 files changed, 76 insertions(+), 140 deletions(-) delete mode 100644 nexus/src/console_api/mod.rs rename nexus/src/{console_api/http_entrypoints.rs => external_api/console_api.rs} (87%) diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 0902f6a2be1..eb4af042805 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -32,10 +32,6 @@ bind_address = "127.0.0.1:12220" # IP address and TCP port on which to listen for the internal API bind_address = "127.0.0.1:12221" -[dropshot_console] -# IP address and TCP port on which to listen for the console API -bind_address = "127.0.0.1:12222" - [log] # Show log messages of this level and more severe level = "info" diff --git a/nexus/src/config.rs b/nexus/src/config.rs index ac97668a5be..f63bba9512d 100644 --- a/nexus/src/config.rs +++ b/nexus/src/config.rs @@ -38,8 +38,6 @@ pub struct Config { pub dropshot_external: ConfigDropshot, /** Dropshot configuration for internal API server */ pub dropshot_internal: ConfigDropshot, - /** Dropshot configuration for console server */ - pub dropshot_console: ConfigDropshot, /** Identifier for this instance of Nexus */ pub id: uuid::Uuid, /** */ @@ -272,9 +270,6 @@ mod test { [dropshot_internal] bind_address = "10.1.2.3:4568" request_body_max_bytes = 1024 - [dropshot_console] - bind_address = "10.1.2.3:4569" - request_body_max_bytes = 1024 [database] url = "postgresql://127.0.0.1?sslmode=disable" [log] @@ -308,12 +303,6 @@ mod test { .unwrap(), ..Default::default() }, - dropshot_console: ConfigDropshot { - bind_address: "10.1.2.3:4569" - .parse::() - .unwrap(), - ..Default::default() - }, log: ConfigLogging::File { level: ConfigLoggingLevel::Debug, if_exists: ConfigLoggingIfExists::Fail, @@ -342,9 +331,6 @@ mod test { [dropshot_internal] bind_address = "10.1.2.3:4568" request_body_max_bytes = 1024 - [dropshot_console] - bind_address = "10.1.2.3:4569" - request_body_max_bytes = 1024 [database] url = "postgresql://127.0.0.1?sslmode=disable" [log] @@ -381,9 +367,6 @@ mod test { [dropshot_internal] bind_address = "10.1.2.3:4568" request_body_max_bytes = 1024 - [dropshot_console] - bind_address = "10.1.2.3:4569" - request_body_max_bytes = 1024 [database] url = "postgresql://127.0.0.1?sslmode=disable" [log] diff --git a/nexus/src/console_api/mod.rs b/nexus/src/console_api/mod.rs deleted file mode 100644 index 7f08fd4ac54..00000000000 --- a/nexus/src/console_api/mod.rs +++ /dev/null @@ -1 +0,0 @@ -pub mod http_entrypoints; diff --git a/nexus/src/console_api/http_entrypoints.rs b/nexus/src/external_api/console_api.rs similarity index 87% rename from nexus/src/console_api/http_entrypoints.rs rename to nexus/src/external_api/console_api.rs index 68625794cef..c3bb3e8273f 100644 --- a/nexus/src/console_api/http_entrypoints.rs +++ b/nexus/src/external_api/console_api.rs @@ -1,5 +1,9 @@ /*! * Handler functions (entrypoints) for console-related routes. + * + * This was originally conceived as a separate dropshot server from the external API, + * but in order to avoid CORS issues for now, we are serving these routes directly + * from the external API. */ use crate::authn::external::{ cookies::Cookies, @@ -11,9 +15,7 @@ use crate::authn::external::{ use crate::authn::TEST_USER_UUID_PRIVILEGED; use crate::context::OpContext; use crate::ServerContext; -use dropshot::{ - endpoint, ApiDescription, HttpError, Path, RequestContext, TypedBody, -}; +use dropshot::{endpoint, HttpError, Path, RequestContext, TypedBody}; use http::{header, Response, StatusCode}; use hyper::Body; use mime_guess; @@ -21,27 +23,6 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::{path::PathBuf, sync::Arc}; -type NexusApiDescription = ApiDescription>; - -/** - * Returns a description of the part of the nexus API dedicated to the web console - */ -pub fn console_api() -> NexusApiDescription { - fn register_endpoints(api: &mut NexusApiDescription) -> Result<(), String> { - api.register(login)?; - api.register(logout)?; - api.register(console_page)?; - api.register(asset)?; - Ok(()) - } - - let mut api = NexusApiDescription::new(); - if let Err(err) = register_endpoints(&mut api) { - panic!("failed to register entrypoints: {}", err); - } - api -} - #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct LoginParams { @@ -53,10 +34,11 @@ pub struct LoginParams { // ignored. we will probably end up with a real username/password login // endpoint, but I think it will only be for use while setting up the rack #[endpoint { - method = POST, - path = "/login", - }] -async fn login( + method = POST, + path = "/login", + unpublished = true, +}] +pub async fn login( rqctx: Arc>>, _params: TypedBody, ) -> Result, HttpError> { @@ -80,13 +62,14 @@ async fn login( } /** - * Log user out of web console by deleting session. - */ +* Log user out of web console by deleting session. +*/ #[endpoint { - method = POST, - path = "/logout", - }] -async fn logout( + method = POST, + path = "/logout", + unpublished = true, +}] +pub async fn logout( rqctx: Arc>>, cookies: Cookies, ) -> Result, HttpError> { @@ -114,16 +97,23 @@ async fn logout( } #[derive(Deserialize, JsonSchema)] -struct RestPathParam { +pub struct RestPathParam { path: Vec, } -// TODO: /c/ prefix is def not what we want long-term but it makes things easy for now +// Dropshot does not have route match ranking and does not allow overlapping +// route definitions, so we cannot have a catchall `/*` route for console pages +// and then also define, e.g., `/api/blah/blah` and give the latter priority +// because it's a more specific match. So for now we simply give the console +// catchall route a prefix to avoid overlap. Long-term, if a route prefix is +// part of the solution, we would probably prefer it to be on the API endpoints, +// not on the console pages. #[endpoint { - method = GET, - path = "/c/{path:.*}", - }] -async fn console_page( + method = GET, + path = "/c/{path:.*}", + unpublished = true, +}] +pub async fn console_page( rqctx: Arc>>, _path_params: Path, ) -> Result, HttpError> { @@ -162,12 +152,11 @@ async fn console_page( /// Fetch a static asset from the configured assets directory. 404 on virtually /// all errors. No auth. NO SENSITIVE FILES. #[endpoint { - method = GET, - path = "/assets/{path:.*}", - // don't want this to show up in the OpenAPI spec (which we don't generate anyway) - unpublished = true, - }] -async fn asset( + method = GET, + path = "/assets/{path:.*}", + unpublished = true, +}] +pub async fn asset( rqctx: Arc>>, path_params: Path, ) -> Result, HttpError> { diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 0fa10a6b31c..a4966457133 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -6,8 +6,10 @@ use crate::db; use crate::db::model::Name; use crate::ServerContext; -use super::params; -use super::views::{Organization, Project}; +use super::{ + console_api, params, + views::{Organization, Project}, +}; use crate::context::OpContext; use dropshot::endpoint; use dropshot::ApiDescription; @@ -134,6 +136,11 @@ pub fn external_api() -> NexusApiDescription { api.register(sagas_get)?; api.register(sagas_get_saga)?; + api.register(console_api::login)?; + api.register(console_api::logout)?; + api.register(console_api::console_page)?; + api.register(console_api::asset)?; + Ok(()) } diff --git a/nexus/src/external_api/mod.rs b/nexus/src/external_api/mod.rs index df2e9092b8a..be47fbda546 100644 --- a/nexus/src/external_api/mod.rs +++ b/nexus/src/external_api/mod.rs @@ -1,3 +1,4 @@ +pub mod console_api; pub mod http_entrypoints; pub mod params; pub mod views; diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 756e1f92cc1..1dcaa5e97bf 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -15,7 +15,6 @@ pub mod authn; // Public only for testing mod authz; mod config; -pub mod console_api; // public for testing mod context; pub mod db; // Public only for some documentation examples pub mod external_api; // public for testing @@ -25,7 +24,6 @@ mod saga_interface; mod sagas; pub use config::Config; -use console_api::http_entrypoints::console_api; pub use context::ServerContext; use external_api::http_entrypoints::external_api; use internal_api::http_entrypoints::internal_api; @@ -77,8 +75,6 @@ pub struct Server { pub http_server_external: dropshot::HttpServer>, /** dropshot server for internal API */ pub http_server_internal: dropshot::HttpServer>, - /** dropshot server for console API */ - pub http_server_console: dropshot::HttpServer>, } impl Server { @@ -113,24 +109,10 @@ impl Server { ) .map_err(|error| format!("initializing internal server: {}", error))?; - let http_server_starter_console = dropshot::HttpServerStarter::new( - &config.dropshot_console, - console_api(), - Arc::clone(&apictx), - &log.new(o!("component" => "dropshot_console")), - ) - .map_err(|error| format!("initializing console server: {}", error))?; - let http_server_external = http_server_starter_external.start(); let http_server_internal = http_server_starter_internal.start(); - let http_server_console = http_server_starter_console.start(); - - Ok(Server { - apictx, - http_server_external, - http_server_internal, - http_server_console, - }) + + Ok(Server { apictx, http_server_external, http_server_internal }) } /** @@ -148,9 +130,6 @@ impl Server { self.http_server_internal .await .map_err(|e| format!("internal: {}", e)), - self.http_server_console - .await - .map_err(|e| format!("console: {}", e)), ] .into_iter() .filter(Result::is_err) diff --git a/nexus/tests/common/mod.rs b/nexus/tests/common/mod.rs index 5f05c2d60dd..a8f529561eb 100644 --- a/nexus/tests/common/mod.rs +++ b/nexus/tests/common/mod.rs @@ -30,7 +30,6 @@ pub const PRODUCER_UUID: &str = "a6458b7d-87c3-4483-be96-854d814c20de"; pub struct ControlPlaneTestContext { pub external_client: ClientTestContext, pub internal_client: ClientTestContext, - pub console_client: ClientTestContext, pub server: omicron_nexus::Server, pub database: dev::db::CockroachInstance, pub clickhouse: dev::clickhouse::ClickHouseInstance, @@ -44,7 +43,6 @@ impl ControlPlaneTestContext { pub async fn teardown(mut self) { self.server.http_server_external.close().await.unwrap(); self.server.http_server_internal.close().await.unwrap(); - self.server.http_server_console.close().await.unwrap(); self.database.cleanup().await.unwrap(); self.clickhouse.cleanup().await.unwrap(); self.sled_agent.http_server.close().await.unwrap(); @@ -110,10 +108,6 @@ pub async fn test_setup_with_config( server.http_server_internal.local_addr(), logctx.log.new(o!("component" => "internal client test context")), ); - let testctx_console = ClientTestContext::new( - server.http_server_console.local_addr(), - logctx.log.new(o!("component" => "console api client test context")), - ); /* Set up a single sled agent. */ let sa_id = Uuid::parse_str(SLED_AGENT_UUID).unwrap(); @@ -151,7 +145,6 @@ pub async fn test_setup_with_config( server, external_client: testctx_external, internal_client: testctx_internal, - console_client: testctx_console, database, clickhouse, sled_agent: sa, diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index e9feb0dd437..b8d7904d2fd 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -35,10 +35,6 @@ bind_address = "127.0.0.1:0" [dropshot_internal] bind_address = "127.0.0.1:0" -# port must be 0. see above -[dropshot_console] -bind_address = "127.0.0.1:0" - # # NOTE: for the test suite, if mode = "file", the file path MUST be the sentinel # string "UNUSED". The actual path will be generated by the test suite for each diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index eeb29df4ff7..7f818702955 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -5,7 +5,7 @@ pub mod common; use common::http_testing::{RequestBuilder, TestResponse}; use common::test_setup; use omicron_common::api::external::IdentityMetadataCreateParams; -use omicron_nexus::console_api::http_entrypoints::LoginParams; +use omicron_nexus::external_api::console_api::LoginParams; use omicron_nexus::external_api::params::OrganizationCreate; extern crate slog; @@ -13,11 +13,10 @@ extern crate slog; #[tokio::test] async fn test_sessions() { let cptestctx = test_setup("test_sessions").await; - let console_testctx = &cptestctx.console_client; - let external_testctx = &cptestctx.external_client; + let testctx = &cptestctx.external_client; // logout always gives the same response whether you have a session or not - let _ = RequestBuilder::new(&console_testctx, Method::POST, "/logout") + let _ = RequestBuilder::new(&testctx, Method::POST, "/logout") .expect_status(Some(StatusCode::NO_CONTENT)) .expect_response_header( header::SET_COOKIE, @@ -28,7 +27,7 @@ async fn test_sessions() { .unwrap(); // log in and pull the token out of the header so we can use it for authed requests - let login = RequestBuilder::new(&console_testctx, Method::POST, "/login") + let login = RequestBuilder::new(&testctx, Method::POST, "/login") .body(Some(LoginParams { username: "".parse().unwrap(), password: "".parse().unwrap(), @@ -52,37 +51,36 @@ async fn test_sessions() { }; // hitting auth-gated API endpoint without session cookie 401s - let _ = - RequestBuilder::new(&external_testctx, Method::POST, "/organizations") - .body(Some(org_params.clone())) - .expect_status(Some(StatusCode::UNAUTHORIZED)) - .execute() - .await; + let _ = RequestBuilder::new(&testctx, Method::POST, "/organizations") + .body(Some(org_params.clone())) + .expect_status(Some(StatusCode::UNAUTHORIZED)) + .execute() + .await; // console pages don't 401, they 302 - let _ = RequestBuilder::new(&external_testctx, Method::POST, "/c/whatever") + let _ = RequestBuilder::new(&testctx, Method::POST, "/c/whatever") .expect_status(Some(StatusCode::FOUND)) .execute() .await; // now make same requests with cookie - let _ = - RequestBuilder::new(&external_testctx, Method::POST, "/organizations") - .header(header::COOKIE, session_token) - .body(Some(org_params.clone())) - // TODO: explicit expect_status not needed. decide whether to keep it anyway - .expect_status(Some(StatusCode::CREATED)) - .execute() - .await; + let _ = RequestBuilder::new(&testctx, Method::POST, "/organizations") + .header(header::COOKIE, session_token) + .body(Some(org_params.clone())) + // TODO: explicit expect_status not needed. decide whether to keep it anyway + .expect_status(Some(StatusCode::CREATED)) + .execute() + .await; - let _ = RequestBuilder::new(&console_testctx, Method::GET, "/c/whatever") + let _ = RequestBuilder::new(&testctx, Method::GET, "/c/whatever") .header(header::COOKIE, session_token) .expect_status(Some(StatusCode::OK)) .execute() .await; + // TODO: expect error here // logout with an actual session should delete the session in the db - let _ = RequestBuilder::new(&console_testctx, Method::POST, "/logout") + let _ = RequestBuilder::new(&testctx, Method::POST, "/logout") .header(header::COOKIE, session_token) .expect_status(Some(StatusCode::NO_CONTENT)) // logout also clears the cookie client-side @@ -96,15 +94,14 @@ async fn test_sessions() { // now the same requests with the same session cookie should 401/302 because // logout also deletes the session server-side - let _ = - RequestBuilder::new(&external_testctx, Method::POST, "/organizations") - .header(header::COOKIE, session_token) - .body(Some(org_params)) - .expect_status(Some(StatusCode::UNAUTHORIZED)) - .execute() - .await; + let _ = RequestBuilder::new(&testctx, Method::POST, "/organizations") + .header(header::COOKIE, session_token) + .body(Some(org_params)) + .expect_status(Some(StatusCode::UNAUTHORIZED)) + .execute() + .await; - let _ = RequestBuilder::new(&console_testctx, Method::GET, "/c/whatever") + let _ = RequestBuilder::new(&testctx, Method::GET, "/c/whatever") .header(header::COOKIE, session_token) .expect_status(Some(StatusCode::FOUND)) .execute() @@ -116,7 +113,7 @@ async fn test_sessions() { #[tokio::test] async fn test_console_pages() { let cptestctx = test_setup("test_console_pages").await; - let testctx = &cptestctx.console_client; + let testctx = &cptestctx.external_client; // request to console page route without auth should redirect to IdP let _ = RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") @@ -136,7 +133,7 @@ async fn test_console_pages() { #[tokio::test] async fn test_assets() { let cptestctx = test_setup("test_assets").await; - let testctx = &cptestctx.console_client; + let testctx = &cptestctx.external_client; // nonexistent file 404s let _ = diff --git a/smf/nexus/config.toml b/smf/nexus/config.toml index 0d74db83a8b..1817a44cffa 100644 --- a/smf/nexus/config.toml +++ b/smf/nexus/config.toml @@ -27,10 +27,6 @@ bind_address = "127.0.0.1:12220" # IP address and TCP port on which to listen for the internal API bind_address = "127.0.0.1:12221" -[dropshot_console] -# IP address and TCP port on which to listen for the console API -bind_address = "127.0.0.1:12222" - [log] # Show log messages of this level and more severe level = "info" From 227ddf78c7e024b9bbc040d7bfdea97d7ff56325 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 22 Nov 2021 14:43:45 -0600 Subject: [PATCH 22/35] put expects on all the tests --- nexus/tests/test_console_api.rs | 55 +++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 7f818702955..4b84f6a6f6f 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -16,7 +16,7 @@ async fn test_sessions() { let testctx = &cptestctx.external_client; // logout always gives the same response whether you have a session or not - let _ = RequestBuilder::new(&testctx, Method::POST, "/logout") + RequestBuilder::new(&testctx, Method::POST, "/logout") .expect_status(Some(StatusCode::NO_CONTENT)) .expect_response_header( header::SET_COOKIE, @@ -24,7 +24,7 @@ async fn test_sessions() { ) .execute() .await - .unwrap(); + .expect("failed to clear cookie and 204 on logout"); // log in and pull the token out of the header so we can use it for authed requests let login = RequestBuilder::new(&testctx, Method::POST, "/login") @@ -35,7 +35,7 @@ async fn test_sessions() { .expect_status(Some(StatusCode::OK)) .execute() .await - .unwrap(); + .expect("failed to log in"); let session_cookie = get_header_value(login, header::SET_COOKIE); let (session_token, rest) = session_cookie.split_once("; ").unwrap(); @@ -51,36 +51,41 @@ async fn test_sessions() { }; // hitting auth-gated API endpoint without session cookie 401s - let _ = RequestBuilder::new(&testctx, Method::POST, "/organizations") + RequestBuilder::new(&testctx, Method::POST, "/organizations") .body(Some(org_params.clone())) .expect_status(Some(StatusCode::UNAUTHORIZED)) .execute() - .await; + .await + .expect("failed to 401 on unauthed API request"); // console pages don't 401, they 302 - let _ = RequestBuilder::new(&testctx, Method::POST, "/c/whatever") + RequestBuilder::new(&testctx, Method::GET, "/c/whatever") .expect_status(Some(StatusCode::FOUND)) .execute() - .await; + .await + .expect("failed to 302 on unauthed console page request"); // now make same requests with cookie - let _ = RequestBuilder::new(&testctx, Method::POST, "/organizations") + RequestBuilder::new(&testctx, Method::POST, "/organizations") .header(header::COOKIE, session_token) .body(Some(org_params.clone())) // TODO: explicit expect_status not needed. decide whether to keep it anyway .expect_status(Some(StatusCode::CREATED)) .execute() - .await; + .await + .expect("failed to create org with session cookie"); - let _ = RequestBuilder::new(&testctx, Method::GET, "/c/whatever") + RequestBuilder::new(&testctx, Method::GET, "/c/whatever") .header(header::COOKIE, session_token) - .expect_status(Some(StatusCode::OK)) + .expect_status(Some(StatusCode::NOT_FOUND)) + // TODO: this will stop 404ing once we handle rendering the template better + // .expect_status(Some(StatusCode::OK)) .execute() - .await; - // TODO: expect error here + .await + .expect("failed to get console page with session cookie"); // logout with an actual session should delete the session in the db - let _ = RequestBuilder::new(&testctx, Method::POST, "/logout") + RequestBuilder::new(&testctx, Method::POST, "/logout") .header(header::COOKIE, session_token) .expect_status(Some(StatusCode::NO_CONTENT)) // logout also clears the cookie client-side @@ -90,22 +95,24 @@ async fn test_sessions() { ) .execute() .await - .unwrap(); + .expect("failed to log out"); // now the same requests with the same session cookie should 401/302 because // logout also deletes the session server-side - let _ = RequestBuilder::new(&testctx, Method::POST, "/organizations") + RequestBuilder::new(&testctx, Method::POST, "/organizations") .header(header::COOKIE, session_token) .body(Some(org_params)) .expect_status(Some(StatusCode::UNAUTHORIZED)) .execute() - .await; + .await + .expect("failed to get 401 for unauthed API request"); - let _ = RequestBuilder::new(&testctx, Method::GET, "/c/whatever") + RequestBuilder::new(&testctx, Method::GET, "/c/whatever") .header(header::COOKIE, session_token) .expect_status(Some(StatusCode::FOUND)) .execute() - .await; + .await + .expect("failed to get 302 for unauthed console request"); cptestctx.teardown().await; } @@ -121,7 +128,7 @@ async fn test_console_pages() { .expect_response_header(header::LOCATION, "https://idp.com/login") .execute() .await - .unwrap(); + .expect("failed to redirect to IdP on auth failure"); // get session @@ -140,19 +147,21 @@ async fn test_assets() { RequestBuilder::new(&testctx, Method::GET, "/assets/nonexistent.svg") .expect_status(Some(StatusCode::NOT_FOUND)) .execute() - .await; + .await + .expect("failed to 404 on nonexistent asset"); // symlink 404s let _ = RequestBuilder::new(&testctx, Method::GET, "/assets/a_symlink") .expect_status(Some(StatusCode::NOT_FOUND)) .execute() - .await; + .await + .expect("failed to 404 on symlink"); // existing file is returned let resp = RequestBuilder::new(&testctx, Method::GET, "/assets/hello.txt") .execute() .await - .unwrap(); + .expect("failed to get existing file"); assert_eq!(resp.body, "hello there".as_bytes()); From 7a533e2da0991cad0ed24039df3b09f03ab5c1c4 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 22 Nov 2021 17:11:44 -0600 Subject: [PATCH 23/35] whoops --- nexus/examples/config-file.toml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/nexus/examples/config-file.toml b/nexus/examples/config-file.toml index ecaf97b4c0b..58b59bc4d25 100644 --- a/nexus/examples/config-file.toml +++ b/nexus/examples/config-file.toml @@ -31,10 +31,6 @@ bind_address = "127.0.0.1:12220" # IP address and TCP port on which to listen for the internal API bind_address = "127.0.0.1:12221" -[dropshot_internal] -# IP address and TCP port on which to listen for the console API -bind_address = "127.0.0.1:12222" - [log] # Show log messages of this level and more severe level = "info" From c3bff0cb1aee1e91521620f0f934a8ecaa164f3b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 22 Nov 2021 17:14:51 -0600 Subject: [PATCH 24/35] premature optimization --- nexus/src/external_api/console_api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index c3bb3e8273f..f37fb07d7f6 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -170,7 +170,7 @@ pub async fn asset( // Derive the MIME type from the file name let content_type = mime_guess::from_path(&file) .first() - .map_or("text/plain".to_string(), |m| m.to_string()); + .map_or_else(|| "text/plain".to_string(), |m| m.to_string()); Ok(Response::builder() .status(StatusCode::OK) From 89fdc9a000529f8db71772dd309aa1ea183669ad Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 00:21:36 -0600 Subject: [PATCH 25/35] extract common header parsing code in test helpers --- nexus/tests/common/http_testing.rs | 78 ++++++++++++++---------------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 086b42578d9..7cba5ed7d44 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -95,30 +95,14 @@ impl<'a> RequestBuilder<'a> { KE: std::error::Error + Send + Sync + 'static, VE: std::error::Error + Send + Sync + 'static, { - let header_name_dbg = format!("{:?}", name); - let header_value_dbg = format!("{:?}", value); - let header_name: Result = - name.try_into().with_context(|| { - format!("converting header name {}", header_name_dbg) - }); - let header_value: Result = - value.try_into().with_context(|| { - format!( - "converting value for header {}: {}", - header_name_dbg, header_value_dbg - ) - }); - match (header_name, header_value) { - (Ok(name), Ok(value)) => { - self.headers.append(name, value); - } - (Err(error), _) => { + match parse_header_pair(name, value) { + Err(error) => { self.error = Some(error); } - (_, Err(error)) => { - self.error = Some(error); + Ok((name, value)) => { + self.headers.append(name, value); } - }; + } self } @@ -184,30 +168,14 @@ impl<'a> RequestBuilder<'a> { KE: std::error::Error + Send + Sync + 'static, VE: std::error::Error + Send + Sync + 'static, { - let header_name_dbg = format!("{:?}", name); - let header_value_dbg = format!("{:?}", value); - let header_name: Result = - name.try_into().with_context(|| { - format!("converting header name {}", header_name_dbg) - }); - let header_value: Result = - value.try_into().with_context(|| { - format!( - "converting value for header {}: {}", - header_name_dbg, header_value_dbg - ) - }); - match (header_name, header_value) { - (Ok(name), Ok(value)) => { - self.expected_response_headers.append(name, value); - } - (Err(error), _) => { + match parse_header_pair(name, value) { + Err(error) => { self.error = Some(error); } - (_, Err(error)) => { - self.error = Some(error); + Ok((name, value)) => { + self.expected_response_headers.append(name, value); } - }; + } self } @@ -378,6 +346,32 @@ impl<'a> RequestBuilder<'a> { } } +fn parse_header_pair( + name: K, + value: V, +) -> Result<(http::header::HeaderName, http::header::HeaderValue), anyhow::Error> +where + K: TryInto + Debug, + V: TryInto + Debug, + KE: std::error::Error + Send + Sync + 'static, + VE: std::error::Error + Send + Sync + 'static, +{ + let header_name_dbg = format!("{:?}", name); + let header_value_dbg = format!("{:?}", value); + + let header_name = name.try_into().with_context(|| { + format!("converting header name {}", header_name_dbg) + })?; + let header_value = value.try_into().with_context(|| { + format!( + "converting value for header {}: {}", + header_name_dbg, header_value_dbg + ) + })?; + + Ok((header_name, header_value)) +} + /// Represents a response from an HTTP server pub struct TestResponse { pub status: http::StatusCode, From 33ec2519558168a5a96f78f93cae7135d6a2250e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 11:06:28 -0600 Subject: [PATCH 26/35] handle nonexistent assets dir at startup with error instead of panic --- nexus/examples/config.toml | 4 ++-- nexus/src/context.rs | 17 ++++++++--------- nexus/src/lib.rs | 2 +- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 68feb672f1c..bb1d7ba0055 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -6,8 +6,8 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" # Directory of static assets for the console -# NOTE: relative to /nexus -assets_directory = "tests/fixtures" # TODO: figure out value +# NOTE: relative to repo root +assets_directory = "nexus/tests/fixtures" # TODO: figure out value # List of authentication schemes to support. # diff --git a/nexus/src/context.rs b/nexus/src/context.rs index d34709ea29a..fec0d5f7b19 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -68,7 +68,7 @@ impl ServerContext { log: Logger, pool: db::Pool, config: &config::Config, - ) -> Arc { + ) -> Result, String> { let nexus_schemes = config .authn .schemes_external @@ -107,15 +107,14 @@ impl ServerContext { // TODO: currently relative to the execution dir, should this be absolute? let assets_directory = current_dir() - .expect("could not access current directory") + .map_err(|e| e.to_string())? .join(config.assets_directory.to_owned()); - // TODO: do we want to assert this? there are no other asserts in here - assert!( - assets_directory.exists(), - "assets directory must exist at start time" - ); - Arc::new(ServerContext { + if !assets_directory.exists() { + return Err("assets_directory must exist at start time".to_string()); + } + + Ok(Arc::new(ServerContext { nexus: Nexus::new_with_id( rack_id, log.new(o!("component" => "nexus")), @@ -138,7 +137,7 @@ impl ServerContext { ), }, assets_directory, - }) + })) } } diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 1dcaa5e97bf..2eb7850dae5 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -91,7 +91,7 @@ impl Server { let ctxlog = log.new(o!("component" => "ServerContext")); let pool = db::Pool::new(&config.database); - let apictx = ServerContext::new(rack_id, ctxlog, pool, &config); + let apictx = ServerContext::new(rack_id, ctxlog, pool, &config)?; let http_server_starter_external = dropshot::HttpServerStarter::new( &config.dropshot_external, From 9332861dd81c6a4fa93beb3a3b4c7aa070a712dd Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 15:53:28 -0600 Subject: [PATCH 27/35] login form, shenanigans necessary to serve console --- .gitignore | 1 + nexus/examples/config.toml | 4 +- nexus/src/authn/external/session_cookie.rs | 8 +-- nexus/src/external_api/console_api.rs | 65 ++++++++++++++++------ nexus/src/external_api/http_entrypoints.rs | 3 +- nexus/tests/test_console_api.rs | 5 +- 6 files changed, 59 insertions(+), 27 deletions(-) diff --git a/.gitignore b/.gitignore index 8557fc69bc1..2c5818e5200 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,4 @@ tools/clickhouse* tools/cockroach* clickhouse/ cockroachdb/ +nexus/assets/ diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index bb1d7ba0055..75f2b253c5e 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -7,7 +7,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" # Directory of static assets for the console # NOTE: relative to repo root -assets_directory = "nexus/tests/fixtures" # TODO: figure out value +assets_directory = "nexus/assets" # TODO: figure out value # List of authentication schemes to support. # @@ -16,7 +16,7 @@ assets_directory = "nexus/tests/fixtures" # TODO: figure out value # yet. [authn] # TODO(https://github.com/oxidecomputer/omicron/issues/372): Remove "spoof". -schemes_external = ["spoof"] +schemes_external = ["spoof", "session_cookie"] session_idle_timeout_minutes = 60 session_absolute_timeout_minutes = 480 diff --git a/nexus/src/authn/external/session_cookie.rs b/nexus/src/authn/external/session_cookie.rs index 31901a7d5ef..fb2d235c883 100644 --- a/nexus/src/authn/external/session_cookie.rs +++ b/nexus/src/authn/external/session_cookie.rs @@ -52,7 +52,7 @@ pub const SESSION_COOKIE_SCHEME_NAME: authn::SchemeName = /// Generate session cookie header pub fn session_cookie_header_value(token: &str, max_age: Duration) -> String { format!( - "{}=\"{}\"; Secure; HttpOnly; SameSite=Lax; Max-Age={}", + "{}={}; Secure; HttpOnly; SameSite=Lax; Max-Age={}", SESSION_COOKIE_COOKIE_NAME, token, max_age.num_seconds() @@ -376,17 +376,17 @@ mod test { fn test_session_cookie_value() { assert_eq!( session_cookie_header_value("abc", Duration::seconds(5)), - "session=\"abc\"; Secure; HttpOnly; SameSite=Lax; Max-Age=5" + "session=abc; Secure; HttpOnly; SameSite=Lax; Max-Age=5" ); assert_eq!( session_cookie_header_value("abc", Duration::seconds(-5)), - "session=\"abc\"; Secure; HttpOnly; SameSite=Lax; Max-Age=-5" + "session=abc; Secure; HttpOnly; SameSite=Lax; Max-Age=-5" ); assert_eq!( session_cookie_header_value("", Duration::zero()), - "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0" + "session=; Secure; HttpOnly; SameSite=Lax; Max-Age=0" ); } } diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index f37fb07d7f6..19ceaee75e2 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -12,7 +12,7 @@ use crate::authn::external::{ SessionStore, SESSION_COOKIE_COOKIE_NAME, }, }; -use crate::authn::TEST_USER_UUID_PRIVILEGED; +use crate::authn::{TEST_USER_UUID_PRIVILEGED, TEST_USER_UUID_UNPRIVILEGED}; use crate::context::OpContext; use crate::ServerContext; use dropshot::{endpoint, HttpError, Path, RequestContext, TypedBody}; @@ -22,12 +22,12 @@ use mime_guess; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::{path::PathBuf, sync::Arc}; +use uuid::Uuid; #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(rename_all = "camelCase")] pub struct LoginParams { pub username: String, - pub password: String, } // for now this is just for testing purposes, and the username and password are @@ -38,15 +38,28 @@ pub struct LoginParams { path = "/login", unpublished = true, }] -pub async fn login( +pub async fn spoof_login( rqctx: Arc>>, - _params: TypedBody, + params: TypedBody, ) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; + let params = params.into_inner(); + let user_id: Option = match params.username.as_str() { + "privileged" => Some(TEST_USER_UUID_PRIVILEGED.parse().unwrap()), + "unprivileged" => Some(TEST_USER_UUID_UNPRIVILEGED.parse().unwrap()), + _ => None, + }; + + if user_id.is_none() { + return Ok(Response::builder() + .status(StatusCode::UNAUTHORIZED) + .body("".into())?); // TODO: failed login response body? + } + let session = nexus // TODO: obviously - .session_create(TEST_USER_UUID_PRIVILEGED.parse().unwrap()) + .session_create(user_id.unwrap()) .await?; Ok(Response::builder() .status(StatusCode::OK) @@ -57,8 +70,7 @@ pub async fn login( apictx.session_idle_timeout(), ), ) - .body("ok".into()) // TODO: what do we return from login? - .unwrap()) + .body("ok".into())?) // TODO: what do we return from login? } /** @@ -101,6 +113,34 @@ pub struct RestPathParam { path: Vec, } +// Serve the console bundle without an auth gate just for the login form. This +// is meant to stand in for the customers identity provider. Since this is a +// placeholder, it's easiest to build the form into the console bundle. If we +// really wanted a login form, we would probably make it a standalone page. +// Otherwise the user is downloading a bunch of JS for nothing. +#[endpoint { + method = GET, + path = "/login", + unpublished = true, +}] +pub async fn spoof_login_form( + rqctx: Arc>>, +) -> Result, HttpError> { + serve_console_index(rqctx.context().assets_directory.clone()).await +} + +async fn serve_console_index( + assets_directory: PathBuf, +) -> Result, HttpError> { + let file = assets_directory.join(PathBuf::from("index.html")); + let file_contents = + tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") + .body(file_contents.into())?) +} + // Dropshot does not have route match ranking and does not allow overlapping // route definitions, so we cannot have a catchall `/*` route for console pages // and then also define, e.g., `/api/blah/blah` and give the latter priority @@ -131,21 +171,14 @@ pub async fn console_page( if let Ok(opctx) = opctx { if opctx.authn.actor().is_some() { let apictx = rqctx.context(); - let file = - &apictx.assets_directory.join(PathBuf::from("index.html")); - let file_contents = - tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; - return Ok(Response::builder() - .status(StatusCode::OK) - .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") - .body(file_contents.into())?); + return serve_console_index(apictx.assets_directory.clone()).await; } } // otherwise redirect to idp Ok(Response::builder() .status(StatusCode::FOUND) - .header(http::header::LOCATION, "https://idp.com/login") + .header(http::header::LOCATION, "/login") .body("".into())?) } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d6b31a8df8a..15e44fc6e27 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -142,7 +142,8 @@ pub fn external_api() -> NexusApiDescription { api.register(sagas_get)?; api.register(sagas_get_saga)?; - api.register(console_api::login)?; + api.register(console_api::spoof_login)?; + api.register(console_api::spoof_login_form)?; api.register(console_api::logout)?; api.register(console_api::console_page)?; api.register(console_api::asset)?; diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index 4b84f6a6f6f..f3b69d9fb69 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -28,10 +28,7 @@ async fn test_sessions() { // log in and pull the token out of the header so we can use it for authed requests let login = RequestBuilder::new(&testctx, Method::POST, "/login") - .body(Some(LoginParams { - username: "".parse().unwrap(), - password: "".parse().unwrap(), - })) + .body(Some(LoginParams { username: "privileged".to_string() })) .expect_status(Some(StatusCode::OK)) .execute() .await From ddf642dc665457ed1cd86cac5acd14850d6d9bb8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 17:23:22 -0600 Subject: [PATCH 28/35] response_body -> parsed_body merge issue --- nexus/tests/common/resource_helpers.rs | 2 +- nexus/tests/test_disks.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index 9de1a114062..6d1a1e2ccaf 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -25,7 +25,7 @@ where .execute() .await .expect("failed to make request") - .response_body() + .parsed_body() .unwrap() } diff --git a/nexus/tests/test_disks.rs b/nexus/tests/test_disks.rs index bbf893ed802..8c6ecabc923 100644 --- a/nexus/tests/test_disks.rs +++ b/nexus/tests/test_disks.rs @@ -73,7 +73,7 @@ async fn test_disks() { .execute() .await .expect("unexpected success") - .response_body::() + .parsed_body::() .unwrap(); assert_eq!(error.message, "not found: disk with name \"just-rainsticks\""); From 92a19bef05dff1b4b06945c8438707e295b39af6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 17:04:18 -0600 Subject: [PATCH 29/35] include login and logout post endpoint in openapi spec --- nexus/src/external_api/console_api.rs | 11 +++++---- nexus/tests/test_console_api.rs | 4 ++-- openapi/nexus.json | 34 +++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 19ceaee75e2..6dd3245c520 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -36,7 +36,8 @@ pub struct LoginParams { #[endpoint { method = POST, path = "/login", - unpublished = true, + // this would be unpublished, but it's convenient for the console to be able + // to use the generated client to make this post }] pub async fn spoof_login( rqctx: Arc>>, @@ -74,12 +75,14 @@ pub async fn spoof_login( } /** -* Log user out of web console by deleting session. -*/ + * Log user out of web console by deleting session. + */ #[endpoint { + // important for security that this be a POST despite the empty req body method = POST, path = "/logout", - unpublished = true, + // this would be unpublished, but it's convenient for the console to be able + // to use the generated client to make this post }] pub async fn logout( rqctx: Arc>>, diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index f3b69d9fb69..ebf42fe7a24 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -20,7 +20,7 @@ async fn test_sessions() { .expect_status(Some(StatusCode::NO_CONTENT)) .expect_response_header( header::SET_COOKIE, - "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0", + "session=; Secure; HttpOnly; SameSite=Lax; Max-Age=0", ) .execute() .await @@ -122,7 +122,7 @@ async fn test_console_pages() { // request to console page route without auth should redirect to IdP let _ = RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") .expect_status(Some(StatusCode::FOUND)) - .expect_response_header(header::LOCATION, "https://idp.com/login") + .expect_response_header(header::LOCATION, "/login") .execute() .await .expect("failed to redirect to IdP on auth failure"); diff --git a/openapi/nexus.json b/openapi/nexus.json index dcad0530566..a2643af9beb 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -174,6 +174,29 @@ } } }, + "/login": { + "post": { + "operationId": "spoof_login", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/LoginParams" + } + } + }, + "required": true + }, + "responses": {} + } + }, + "/logout": { + "post": { + "description": "Log user out of web console by deleting session.", + "operationId": "logout", + "responses": {} + } + }, "/organizations": { "get": { "description": "List all organizations.", @@ -3166,6 +3189,17 @@ "minLength": 1, "maxLength": 11 }, + "LoginParams": { + "type": "object", + "properties": { + "username": { + "type": "string" + } + }, + "required": [ + "username" + ] + }, "Name": { "title": "A name used in the API", "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'.", From eeaefc4056702237ab24bbab8fc091c37a43463a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 18:17:25 -0600 Subject: [PATCH 30/35] update tests --- nexus/tests/fixtures/index.html | 1 + nexus/tests/test_console_api.rs | 83 ++++++++++++++++++++++++--------- 2 files changed, 62 insertions(+), 22 deletions(-) create mode 100644 nexus/tests/fixtures/index.html diff --git a/nexus/tests/fixtures/index.html b/nexus/tests/fixtures/index.html new file mode 100644 index 00000000000..6c70bcfe4d4 --- /dev/null +++ b/nexus/tests/fixtures/index.html @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index ebf42fe7a24..ceba24d2e9b 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -1,3 +1,4 @@ +use dropshot::test_util::ClientTestContext; use http::header::HeaderName; use http::{header, method::Method, StatusCode}; @@ -27,18 +28,7 @@ async fn test_sessions() { .expect("failed to clear cookie and 204 on logout"); // log in and pull the token out of the header so we can use it for authed requests - let login = RequestBuilder::new(&testctx, Method::POST, "/login") - .body(Some(LoginParams { username: "privileged".to_string() })) - .expect_status(Some(StatusCode::OK)) - .execute() - .await - .expect("failed to log in"); - - let session_cookie = get_header_value(login, header::SET_COOKIE); - let (session_token, rest) = session_cookie.split_once("; ").unwrap(); - - assert!(session_token.starts_with("session=")); - assert_eq!(rest, "Secure; HttpOnly; SameSite=Lax; Max-Age=3600"); + let session_token = log_in_and_extract_token(&testctx).await; let org_params = OrganizationCreate { identity: IdentityMetadataCreateParams { @@ -64,7 +54,7 @@ async fn test_sessions() { // now make same requests with cookie RequestBuilder::new(&testctx, Method::POST, "/organizations") - .header(header::COOKIE, session_token) + .header(header::COOKIE, &session_token) .body(Some(org_params.clone())) // TODO: explicit expect_status not needed. decide whether to keep it anyway .expect_status(Some(StatusCode::CREATED)) @@ -73,22 +63,20 @@ async fn test_sessions() { .expect("failed to create org with session cookie"); RequestBuilder::new(&testctx, Method::GET, "/c/whatever") - .header(header::COOKIE, session_token) - .expect_status(Some(StatusCode::NOT_FOUND)) - // TODO: this will stop 404ing once we handle rendering the template better - // .expect_status(Some(StatusCode::OK)) + .header(header::COOKIE, &session_token) + .expect_status(Some(StatusCode::OK)) .execute() .await .expect("failed to get console page with session cookie"); // logout with an actual session should delete the session in the db RequestBuilder::new(&testctx, Method::POST, "/logout") - .header(header::COOKIE, session_token) + .header(header::COOKIE, &session_token) .expect_status(Some(StatusCode::NO_CONTENT)) // logout also clears the cookie client-side .expect_response_header( header::SET_COOKIE, - "session=\"\"; Secure; HttpOnly; SameSite=Lax; Max-Age=0", + "session=; Secure; HttpOnly; SameSite=Lax; Max-Age=0", ) .execute() .await @@ -97,7 +85,7 @@ async fn test_sessions() { // now the same requests with the same session cookie should 401/302 because // logout also deletes the session server-side RequestBuilder::new(&testctx, Method::POST, "/organizations") - .header(header::COOKIE, session_token) + .header(header::COOKIE, &session_token) .body(Some(org_params)) .expect_status(Some(StatusCode::UNAUTHORIZED)) .execute() @@ -105,7 +93,7 @@ async fn test_sessions() { .expect("failed to get 401 for unauthed API request"); RequestBuilder::new(&testctx, Method::GET, "/c/whatever") - .header(header::COOKIE, session_token) + .header(header::COOKIE, &session_token) .expect_status(Some(StatusCode::FOUND)) .execute() .await @@ -127,9 +115,43 @@ async fn test_console_pages() { .await .expect("failed to redirect to IdP on auth failure"); - // get session + let session_token = log_in_and_extract_token(&testctx).await; // hit console page with session, should get back HTML response + let console_page = + RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") + .header(http::header::COOKIE, session_token) + .expect_status(Some(StatusCode::OK)) + .expect_response_header( + http::header::CONTENT_TYPE, + "text/html; charset=UTF-8", + ) + .execute() + .await + .expect("failed to get console index"); + + assert_eq!(console_page.body, "".as_bytes()); + + cptestctx.teardown().await; +} + +#[tokio::test] +async fn text_login_form() { + let cptestctx = test_setup("test_login_form").await; + let testctx = &cptestctx.external_client; + + // login route returns bundle too, but is not auth gated + let console_page = RequestBuilder::new(&testctx, Method::GET, "/login") + .expect_status(Some(StatusCode::OK)) + .expect_response_header( + http::header::CONTENT_TYPE, + "text/html; charset=UTF-8", + ) + .execute() + .await + .expect("failed to get login form"); + + assert_eq!(console_page.body, "".as_bytes()); cptestctx.teardown().await; } @@ -168,3 +190,20 @@ async fn test_assets() { fn get_header_value(resp: TestResponse, header_name: HeaderName) -> String { resp.headers.get(header_name).unwrap().to_str().unwrap().to_string() } + +async fn log_in_and_extract_token(testctx: &ClientTestContext) -> String { + let login = RequestBuilder::new(&testctx, Method::POST, "/login") + .body(Some(LoginParams { username: "privileged".to_string() })) + .expect_status(Some(StatusCode::OK)) + .execute() + .await + .expect("failed to log in"); + + let session_cookie = get_header_value(login, header::SET_COOKIE); + let (session_token, rest) = session_cookie.split_once("; ").unwrap(); + + assert!(session_token.starts_with("session=")); + assert_eq!(rest, "Secure; HttpOnly; SameSite=Lax; Max-Age=3600"); + + session_token.to_string() +} From 6500f8cf60265c7641ba5f34d0dd64fb091a590f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 21:12:18 -0600 Subject: [PATCH 31/35] we don't need the /c/ prefix! all the console routes start with /orgs ! --- nexus/src/external_api/console_api.rs | 2 +- nexus/tests/test_console_api.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 6dd3245c520..9099e2e0332 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -153,7 +153,7 @@ async fn serve_console_index( // not on the console pages. #[endpoint { method = GET, - path = "/c/{path:.*}", + path = "/orgs/{path:.*}", unpublished = true, }] pub async fn console_page( diff --git a/nexus/tests/test_console_api.rs b/nexus/tests/test_console_api.rs index ceba24d2e9b..e591b6a386f 100644 --- a/nexus/tests/test_console_api.rs +++ b/nexus/tests/test_console_api.rs @@ -46,7 +46,7 @@ async fn test_sessions() { .expect("failed to 401 on unauthed API request"); // console pages don't 401, they 302 - RequestBuilder::new(&testctx, Method::GET, "/c/whatever") + RequestBuilder::new(&testctx, Method::GET, "/orgs/whatever") .expect_status(Some(StatusCode::FOUND)) .execute() .await @@ -62,7 +62,7 @@ async fn test_sessions() { .await .expect("failed to create org with session cookie"); - RequestBuilder::new(&testctx, Method::GET, "/c/whatever") + RequestBuilder::new(&testctx, Method::GET, "/orgs/whatever") .header(header::COOKIE, &session_token) .expect_status(Some(StatusCode::OK)) .execute() @@ -92,7 +92,7 @@ async fn test_sessions() { .await .expect("failed to get 401 for unauthed API request"); - RequestBuilder::new(&testctx, Method::GET, "/c/whatever") + RequestBuilder::new(&testctx, Method::GET, "/orgs/whatever") .header(header::COOKIE, &session_token) .expect_status(Some(StatusCode::FOUND)) .execute() @@ -108,7 +108,7 @@ async fn test_console_pages() { let testctx = &cptestctx.external_client; // request to console page route without auth should redirect to IdP - let _ = RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") + let _ = RequestBuilder::new(&testctx, Method::GET, "/orgs/irrelevant-path") .expect_status(Some(StatusCode::FOUND)) .expect_response_header(header::LOCATION, "/login") .execute() @@ -119,7 +119,7 @@ async fn test_console_pages() { // hit console page with session, should get back HTML response let console_page = - RequestBuilder::new(&testctx, Method::GET, "/c/irrelevant-path") + RequestBuilder::new(&testctx, Method::GET, "/orgs/irrelevant-path") .header(http::header::COOKIE, session_token) .expect_status(Some(StatusCode::OK)) .expect_response_header( From c50476a85cd4ef9ddc1a2e646ff13209fa69caa2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 23 Nov 2021 21:44:33 -0600 Subject: [PATCH 32/35] cache-control headers, comment why we're not doing asset checks on start --- nexus/examples/config-file.toml | 2 +- nexus/src/context.rs | 3 +++ nexus/src/external_api/console_api.rs | 5 ++++- nexus/tests/common/http_testing.rs | 3 ++- smf/nexus/config.toml | 2 +- 5 files changed, 11 insertions(+), 4 deletions(-) diff --git a/nexus/examples/config-file.toml b/nexus/examples/config-file.toml index 58b59bc4d25..274a5e2d842 100644 --- a/nexus/examples/config-file.toml +++ b/nexus/examples/config-file.toml @@ -6,7 +6,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" # Directory of static assets for the console -# NOTE: relative to /nexus +# NOTE: relative to repo root assets_directory = "tests/fixtures" # TODO: figure out value # List of authentication schemes to support. diff --git a/nexus/src/context.rs b/nexus/src/context.rs index fec0d5f7b19..a9b3a8edca0 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -114,6 +114,9 @@ impl ServerContext { return Err("assets_directory must exist at start time".to_string()); } + // TODO: check for particular assets, like console index.html + // leaving that out for now so we don't break nexus in dev for everyone + Ok(Arc::new(ServerContext { nexus: Nexus::new_with_id( rack_id, diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 9099e2e0332..296b607c423 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -141,6 +141,7 @@ async fn serve_console_index( Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") + .header(http::header::CACHE_CONTROL, "max-age=600") .body(file_contents.into())?) } @@ -150,7 +151,8 @@ async fn serve_console_index( // because it's a more specific match. So for now we simply give the console // catchall route a prefix to avoid overlap. Long-term, if a route prefix is // part of the solution, we would probably prefer it to be on the API endpoints, -// not on the console pages. +// not on the console pages. Conveniently, all the console page routes start +// with /orgs already. #[endpoint { method = GET, path = "/orgs/{path:.*}", @@ -211,6 +213,7 @@ pub async fn asset( Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, &content_type) + .header(http::header::CACHE_CONTROL, "max-age=600") .body(file_contents.into())?) } diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index d059c18ff94..235f79630a3 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -75,11 +75,12 @@ impl<'a> RequestBuilder<'a> { body: hyper::Body::empty(), expected_status: None, allowed_headers: Some(vec![ + http::header::CACHE_CONTROL, http::header::CONTENT_LENGTH, http::header::CONTENT_TYPE, http::header::DATE, - http::header::SET_COOKIE, http::header::LOCATION, + http::header::SET_COOKIE, http::header::HeaderName::from_static("x-request-id"), ]), expected_response_headers: http::HeaderMap::new(), diff --git a/smf/nexus/config.toml b/smf/nexus/config.toml index 1817a44cffa..71b0f51d610 100644 --- a/smf/nexus/config.toml +++ b/smf/nexus/config.toml @@ -6,7 +6,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" # Directory of static assets for the console -# NOTE: relative to /nexus +# NOTE: relative to repo root assets_directory = "tests/fixtures" # TODO: figure out value [authn] From 1a5eefe7fcc1e2e49b61e66e7e32b83bbdd7b24c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 24 Nov 2021 11:08:03 -0600 Subject: [PATCH 33/35] update comments --- nexus/src/external_api/console_api.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 296b607c423..05bbd4c57a2 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -30,14 +30,13 @@ pub struct LoginParams { pub username: String, } -// for now this is just for testing purposes, and the username and password are -// ignored. we will probably end up with a real username/password login +/// This is just for demo purposes. we will probably end up with a real username/password login // endpoint, but I think it will only be for use while setting up the rack #[endpoint { method = POST, path = "/login", - // this would be unpublished, but it's convenient for the console to be able - // to use the generated client to make this post + // TODO: this should be unpublished, but for now it's convenient for the + // console to use the generated client for this request }] pub async fn spoof_login( rqctx: Arc>>, @@ -55,6 +54,7 @@ pub async fn spoof_login( if user_id.is_none() { return Ok(Response::builder() .status(StatusCode::UNAUTHORIZED) + .header(header::SET_COOKIE, clear_session_cookie_header_value()) .body("".into())?); // TODO: failed login response body? } @@ -75,14 +75,14 @@ pub async fn spoof_login( } /** - * Log user out of web console by deleting session. + * Log user out of web console by deleting session in both server and browser. */ #[endpoint { // important for security that this be a POST despite the empty req body method = POST, path = "/logout", - // this would be unpublished, but it's convenient for the console to be able - // to use the generated client to make this post + // TODO: this should be unpublished, but for now it's convenient for the + // console to use the generated client for this request }] pub async fn logout( rqctx: Arc>>, @@ -119,8 +119,8 @@ pub struct RestPathParam { // Serve the console bundle without an auth gate just for the login form. This // is meant to stand in for the customers identity provider. Since this is a // placeholder, it's easiest to build the form into the console bundle. If we -// really wanted a login form, we would probably make it a standalone page. -// Otherwise the user is downloading a bunch of JS for nothing. +// really wanted a login form, we would probably make it a standalone page, +// otherwise the user is downloading a bunch of JS for nothing. #[endpoint { method = GET, path = "/login", From 24604a19cda9194a8c1266a48e9bfeed3512e1e0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 24 Nov 2021 12:00:21 -0600 Subject: [PATCH 34/35] make cache-control header tunable, rearrange config a bit --- nexus/examples/config-file.toml | 6 ++-- nexus/examples/config.toml | 6 ++-- nexus/src/config.rs | 41 +++++++++++++++++------- nexus/src/context.rs | 29 ++++++++++------- nexus/src/external_api/console_api.rs | 45 ++++++++++++++++----------- nexus/tests/config.test.toml | 6 ++-- smf/nexus/config.toml | 6 ++-- 7 files changed, 89 insertions(+), 50 deletions(-) diff --git a/nexus/examples/config-file.toml b/nexus/examples/config-file.toml index 274a5e2d842..82fc24ed508 100644 --- a/nexus/examples/config-file.toml +++ b/nexus/examples/config-file.toml @@ -5,9 +5,13 @@ # Identifier for this instance of Nexus id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +[console] # Directory of static assets for the console # NOTE: relative to repo root assets_directory = "tests/fixtures" # TODO: figure out value +cache_control_max_age_minutes = 10 +session_idle_timeout_minutes = 60 +session_absolute_timeout_minutes = 480 # List of authentication schemes to support. # @@ -16,8 +20,6 @@ assets_directory = "tests/fixtures" # TODO: figure out value # yet. [authn] schemes_external = [] -session_idle_timeout_minutes = 60 -session_absolute_timeout_minutes = 480 [database] # URL for connecting to the database diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 75f2b253c5e..b9d6ccc8f6c 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -5,9 +5,13 @@ # Identifier for this instance of Nexus id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +[console] # Directory of static assets for the console # NOTE: relative to repo root assets_directory = "nexus/assets" # TODO: figure out value +cache_control_max_age_minutes = 10 +session_idle_timeout_minutes = 60 +session_absolute_timeout_minutes = 480 # List of authentication schemes to support. # @@ -17,8 +21,6 @@ assets_directory = "nexus/assets" # TODO: figure out value [authn] # TODO(https://github.com/oxidecomputer/omicron/issues/372): Remove "spoof". schemes_external = ["spoof", "session_cookie"] -session_idle_timeout_minutes = 60 -session_absolute_timeout_minutes = 480 [database] # URL for connecting to the database diff --git a/nexus/src/config.rs b/nexus/src/config.rs index f63bba9512d..104b4b04e10 100644 --- a/nexus/src/config.rs +++ b/nexus/src/config.rs @@ -23,6 +23,13 @@ use std::path::{Path, PathBuf}; pub struct AuthnConfig { /** allowed authentication schemes for external HTTP server */ pub schemes_external: Vec, +} + +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub struct ConsoleConfig { + pub assets_directory: PathBuf, + /** how long the browser can cache static assets */ + pub cache_control_max_age_minutes: u32, /** how long a session can be idle before expiring */ pub session_idle_timeout_minutes: u32, /** how long a session can exist before expiring */ @@ -40,8 +47,8 @@ pub struct Config { pub dropshot_internal: ConfigDropshot, /** Identifier for this instance of Nexus */ pub id: uuid::Uuid, - /** */ - pub assets_directory: PathBuf, + /** Console-related tunables */ + pub console: ConsoleConfig, /** Server-wide logging configuration. */ pub log: ConfigLogging, /** Database parameters */ @@ -151,7 +158,10 @@ impl Config { #[cfg(test)] mod test { - use super::{AuthnConfig, Config, LoadError, LoadErrorKind, SchemeName}; + use super::{ + AuthnConfig, Config, ConsoleConfig, LoadError, LoadErrorKind, + SchemeName, + }; use crate::db; use dropshot::ConfigDropshot; use dropshot::ConfigLogging; @@ -259,11 +269,13 @@ mod test { "valid", r##" id = "28b90dc4-c22a-65ba-f49a-f051fe01208f" + [console] assets_directory = "tests/fixtures" - [authn] - schemes_external = [] + cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60 session_absolute_timeout_minutes = 480 + [authn] + schemes_external = [] [dropshot_external] bind_address = "10.1.2.3:4567" request_body_max_bytes = 1024 @@ -285,12 +297,13 @@ mod test { config, Config { id: "28b90dc4-c22a-65ba-f49a-f051fe01208f".parse().unwrap(), - assets_directory: "tests/fixtures".parse().unwrap(), - authn: AuthnConfig { - schemes_external: Vec::new(), + console: ConsoleConfig { + assets_directory: "tests/fixtures".parse().unwrap(), + cache_control_max_age_minutes: 10, session_idle_timeout_minutes: 60, session_absolute_timeout_minutes: 480 }, + authn: AuthnConfig { schemes_external: Vec::new() }, dropshot_external: ConfigDropshot { bind_address: "10.1.2.3:4567" .parse::() @@ -320,11 +333,13 @@ mod test { "valid", r##" id = "28b90dc4-c22a-65ba-f49a-f051fe01208f" + [console] assets_directory = "tests/fixtures" - [authn] - schemes_external = [ "spoof", "session_cookie" ] + cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60 session_absolute_timeout_minutes = 480 + [authn] + schemes_external = [ "spoof", "session_cookie" ] [dropshot_external] bind_address = "10.1.2.3:4567" request_body_max_bytes = 1024 @@ -356,11 +371,13 @@ mod test { "bad authn.schemes_external", r##" id = "28b90dc4-c22a-65ba-f49a-f051fe01208f" + [console] assets_directory = "tests/fixtures" - [authn] - schemes_external = ["trust-me"] + cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60 session_absolute_timeout_minutes = 480 + [authn] + schemes_external = ["trust-me"] [dropshot_external] bind_address = "10.1.2.3:4567" request_body_max_bytes = 1024 diff --git a/nexus/src/context.rs b/nexus/src/context.rs index a9b3a8edca0..2f4d033a356 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -45,17 +45,19 @@ pub struct ServerContext { pub external_latencies: LatencyTracker, /** registry of metric producers */ pub producer_registry: ProducerRegistry, - /** tunable settings needed at runtime */ - pub tunables: Tunables, - /** directory containing static assets for the console */ - pub assets_directory: PathBuf, + /** tunable settings needed for the console at runtime */ + pub console_config: ConsoleConfig, } -pub struct Tunables { +pub struct ConsoleConfig { /** how long a session can be idle before expiring */ pub session_idle_timeout: Duration, /** how long a session can exist before expiring */ pub session_absolute_timeout: Duration, + /** how long browsers can cache static assets */ + pub cache_control_max_age: Duration, + /** directory containing static assets */ + pub assets_directory: PathBuf, } impl ServerContext { @@ -108,7 +110,7 @@ impl ServerContext { // TODO: currently relative to the execution dir, should this be absolute? let assets_directory = current_dir() .map_err(|e| e.to_string())? - .join(config.assets_directory.to_owned()); + .join(config.console.assets_directory.to_owned()); if !assets_directory.exists() { return Err("assets_directory must exist at start time".to_string()); @@ -131,15 +133,18 @@ impl ServerContext { internal_latencies, external_latencies, producer_registry, - tunables: Tunables { + console_config: ConsoleConfig { session_idle_timeout: Duration::minutes( - config.authn.session_idle_timeout_minutes.into(), + config.console.session_idle_timeout_minutes.into(), ), session_absolute_timeout: Duration::minutes( - config.authn.session_absolute_timeout_minutes.into(), + config.console.session_absolute_timeout_minutes.into(), + ), + assets_directory, + cache_control_max_age: Duration::minutes( + config.console.cache_control_max_age_minutes.into(), ), }, - assets_directory, })) } } @@ -379,11 +384,11 @@ impl SessionStore for Arc { } fn session_idle_timeout(&self) -> Duration { - self.tunables.session_idle_timeout + self.console_config.session_idle_timeout } fn session_absolute_timeout(&self) -> Duration { - self.tunables.session_absolute_timeout + self.console_config.session_absolute_timeout } } diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 05bbd4c57a2..a6fa0c7afaf 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -129,20 +129,7 @@ pub struct RestPathParam { pub async fn spoof_login_form( rqctx: Arc>>, ) -> Result, HttpError> { - serve_console_index(rqctx.context().assets_directory.clone()).await -} - -async fn serve_console_index( - assets_directory: PathBuf, -) -> Result, HttpError> { - let file = assets_directory.join(PathBuf::from("index.html")); - let file_contents = - tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; - Ok(Response::builder() - .status(StatusCode::OK) - .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") - .header(http::header::CACHE_CONTROL, "max-age=600") - .body(file_contents.into())?) + serve_console_index(rqctx.context()).await } // Dropshot does not have route match ranking and does not allow overlapping @@ -175,8 +162,7 @@ pub async fn console_page( // makes the right API requests. if let Ok(opctx) = opctx { if opctx.authn.actor().is_some() { - let apictx = rqctx.context(); - return serve_console_index(apictx.assets_directory.clone()).await; + return serve_console_index(rqctx.context()).await; } } @@ -201,7 +187,7 @@ pub async fn asset( let apictx = rqctx.context(); let path = path_params.into_inner().path; - let file = find_file(path, &apictx.assets_directory)?; + let file = find_file(path, &apictx.console_config.assets_directory)?; let file_contents = tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; @@ -213,7 +199,30 @@ pub async fn asset( Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, &content_type) - .header(http::header::CACHE_CONTROL, "max-age=600") + .header(http::header::CACHE_CONTROL, cache_control_header_value(apictx)) + .body(file_contents.into())?) +} + +fn cache_control_header_value(apictx: &Arc) -> String { + format!( + "max-age={}", + apictx.console_config.cache_control_max_age.num_seconds() + ) +} + +async fn serve_console_index( + apictx: &Arc, +) -> Result, HttpError> { + let file = apictx + .console_config + .assets_directory + .join(PathBuf::from("index.html")); + let file_contents = + tokio::fs::read(&file).await.map_err(|_| not_found("EBADF"))?; + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, "text/html; charset=UTF-8") + .header(http::header::CACHE_CONTROL, cache_control_header_value(apictx)) .body(file_contents.into())?) } diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index b8d7904d2fd..b3c653c03fa 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -6,15 +6,17 @@ # NOTE: The test suite always overrides this. id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +[console] # Directory of static assets for the console # NOTE: relative to /nexus assets_directory = "tests/fixtures" +cache_control_max_age_minutes = 10 +session_idle_timeout_minutes = 60 +session_absolute_timeout_minutes = 480 # List of authentication schemes to support. [authn] schemes_external = [ "spoof", "session_cookie" ] -session_idle_timeout_minutes = 60 -session_absolute_timeout_minutes = 480 # # NOTE: for the test suite, the database URL will be replaced with one diff --git a/smf/nexus/config.toml b/smf/nexus/config.toml index 71b0f51d610..37c78b5b686 100644 --- a/smf/nexus/config.toml +++ b/smf/nexus/config.toml @@ -5,15 +5,17 @@ # Identifier for this instance of Nexus id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" +[console] # Directory of static assets for the console # NOTE: relative to repo root assets_directory = "tests/fixtures" # TODO: figure out value +cache_control_max_age_minutes = 10 +session_idle_timeout_minutes = 60 +session_absolute_timeout_minutes = 480 [authn] # TODO(https://github.com/oxidecomputer/omicron/issues/372): Remove "spoof". schemes_external = ["spoof"] -session_idle_timeout_minutes = 60 -session_absolute_timeout_minutes = 480 [database] # URL for connecting to the database From 159c4d0b68cdf366f4bd41645e5a43d6206fcf68 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 24 Nov 2021 13:05:37 -0600 Subject: [PATCH 35/35] assets dir relative to CARGO_MANIFEST_DIR for more stable results --- .gitignore | 1 - nexus/examples/config-file.toml | 3 +-- nexus/examples/config.toml | 5 ++--- nexus/src/context.rs | 11 ++++++----- nexus/src/external_api/console_api.rs | 6 ++---- nexus/tests/config.test.toml | 3 +-- openapi/nexus.json | 1 - smf/nexus/config.toml | 3 +-- 8 files changed, 13 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 2c5818e5200..8557fc69bc1 100644 --- a/.gitignore +++ b/.gitignore @@ -8,4 +8,3 @@ tools/clickhouse* tools/cockroach* clickhouse/ cockroachdb/ -nexus/assets/ diff --git a/nexus/examples/config-file.toml b/nexus/examples/config-file.toml index 82fc24ed508..9ca2ae19bb3 100644 --- a/nexus/examples/config-file.toml +++ b/nexus/examples/config-file.toml @@ -6,8 +6,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" [console] -# Directory of static assets for the console -# NOTE: relative to repo root +# Directory of static assets for the console. Relative to nexus/. assets_directory = "tests/fixtures" # TODO: figure out value cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60 diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index b9d6ccc8f6c..255b8ad7a82 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -6,9 +6,8 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" [console] -# Directory of static assets for the console -# NOTE: relative to repo root -assets_directory = "nexus/assets" # TODO: figure out value +# Directory of static assets for the console. Relative to nexus/. +assets_directory = "tests/fixtures" # TODO: figure out value cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60 session_absolute_timeout_minutes = 480 diff --git a/nexus/src/context.rs b/nexus/src/context.rs index 2f4d033a356..5db0a8af3a3 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -19,7 +19,7 @@ use oximeter::types::ProducerRegistry; use oximeter_instruments::http::{HttpService, LatencyTracker}; use slog::Logger; use std::collections::BTreeMap; -use std::env::current_dir; +use std::env; use std::fmt::Debug; use std::path::PathBuf; use std::sync::Arc; @@ -107,10 +107,11 @@ impl ServerContext { .register_producer(external_latencies.clone()) .unwrap(); - // TODO: currently relative to the execution dir, should this be absolute? - let assets_directory = current_dir() - .map_err(|e| e.to_string())? - .join(config.console.assets_directory.to_owned()); + let assets_directory = PathBuf::from( + env::var("CARGO_MANIFEST_DIR") + .map_err(|_e| "env var CARGO_MANIFEST_DIR must be defined because the path for static assets is relative to it")?, + ) + .join(config.console.assets_directory.to_owned()); if !assets_directory.exists() { return Err("assets_directory must exist at start time".to_string()); diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index a6fa0c7afaf..5d5835b2355 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -30,7 +30,7 @@ pub struct LoginParams { pub username: String, } -/// This is just for demo purposes. we will probably end up with a real username/password login +// This is just for demo purposes. we will probably end up with a real username/password login // endpoint, but I think it will only be for use while setting up the rack #[endpoint { method = POST, @@ -74,9 +74,7 @@ pub async fn spoof_login( .body("ok".into())?) // TODO: what do we return from login? } -/** - * Log user out of web console by deleting session in both server and browser. - */ +// Log user out of web console by deleting session in both server and browser #[endpoint { // important for security that this be a POST despite the empty req body method = POST, diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index b3c653c03fa..a2333531689 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -7,8 +7,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" [console] -# Directory of static assets for the console -# NOTE: relative to /nexus +# Directory of static assets for the console. Relative to nexus/. assets_directory = "tests/fixtures" cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60 diff --git a/openapi/nexus.json b/openapi/nexus.json index a2643af9beb..7a86eafa1cf 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -192,7 +192,6 @@ }, "/logout": { "post": { - "description": "Log user out of web console by deleting session.", "operationId": "logout", "responses": {} } diff --git a/smf/nexus/config.toml b/smf/nexus/config.toml index 37c78b5b686..e5d47fbc674 100644 --- a/smf/nexus/config.toml +++ b/smf/nexus/config.toml @@ -6,8 +6,7 @@ id = "e6bff1ff-24fb-49dc-a54e-c6a350cd4d6c" [console] -# Directory of static assets for the console -# NOTE: relative to repo root +# Directory of static assets for the console. Relative to nexus/. assets_directory = "tests/fixtures" # TODO: figure out value cache_control_max_age_minutes = 10 session_idle_timeout_minutes = 60