From c2cc1d0226735565799ced7724f93f3a422138f0 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 28 Sep 2022 19:34:13 -0700 Subject: [PATCH 1/8] tmp - initial changes --- Cargo.lock | 4 ++ Cargo.toml | 4 +- nexus/src/external_api/console_api.rs | 68 +++++++++++++-------------- 3 files changed, 39 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b053a1df0e4..97e2ab9e00d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6712,3 +6712,7 @@ dependencies = [ "quote", "syn", ] + +[[patch.unused]] +name = "dropshot" +version = "0.8.1-dev" diff --git a/Cargo.toml b/Cargo.toml index a95b95c9c64..be0f17405ac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -95,8 +95,8 @@ panic = "abort" # crucible, 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" } #[patch."https://github.com/oxidecomputer/propolis"] diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index ee15844800d..1328c7623f7 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -25,7 +25,9 @@ use crate::{ }; use anyhow::Context; use dropshot::{ - endpoint, HttpError, HttpResponseOk, Path, Query, RequestContext, TypedBody, + endpoint, http_response_see_other, http_response_temporary_redirect, + HttpError, HttpResponseOk, HttpResponseSeeOther, + HttpResponseTemporaryRedirect, Path, Query, RequestContext, TypedBody, }; use http::{header, Response, StatusCode}; use hyper::Body; @@ -53,7 +55,7 @@ pub struct SpoofLoginBody { // console to use the generated client for this request tags = ["hidden"], }] -pub async fn spoof_login( +pub async fn login_spoof( rqctx: Arc>>, params: TypedBody, ) -> Result, HttpError> { @@ -67,10 +69,9 @@ 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? + Err(Error::Unauthenticated { + internal_message: format!("unknown user specified"), + })?; } let user_id = user_id.unwrap(); @@ -235,10 +236,10 @@ impl RelayState { path = "/login/{silo_name}/{provider_name}", tags = ["login"], }] -pub async fn login( +pub async fn login_begin( rqctx: Arc>>, path_params: Path, -) -> Result, HttpError> { +) -> Result { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -259,8 +260,8 @@ pub async fn login( match identity_provider { IdentityProviderType::Saml(saml_identity_provider) => { - // Relay state is sent to the IDP, to be sent back to the SP after a - // successful login. + // Relay state is sent to the IDP, to be sent back to the SP + // after a successful login. let relay_state: Option = if let Some(value) = request.headers().get(hyper::header::REFERER) { @@ -298,16 +299,12 @@ pub async fn login( |e| HttpError::for_internal_error(e.to_string()), )?; - Ok(Response::builder() - .status(StatusCode::FOUND) - .header(http::header::LOCATION, sign_in_url) - .body("".into())?) + Ok(http_response_temporary_redirect(sign_in_url)) } } }; - // TODO this doesn't work because the response is Response - //apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await - handler.await + + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } /// Authenticate a user @@ -319,11 +316,11 @@ pub async fn login( path = "/login/{silo_name}/{provider_name}", tags = ["login"], }] -pub async fn consume_credentials( +pub async fn login_saml( rqctx: Arc>>, path_params: Path, body_bytes: dropshot::UntypedBody, -) -> Result, HttpError> { +) -> Result { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -372,11 +369,11 @@ pub async fn consume_credentials( .await?; if user.is_none() { - info!(&apictx.log, "user 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? + Err(Error::Unauthenticated { + internal_message: format!( + "no matching user found or credentials were not valid" + ), + })?; } let user = user.unwrap(); @@ -396,28 +393,28 @@ pub async fn consume_credentials( debug!( &apictx.log, - "successful login to silo {} using provider {}: authenticated subject {} = user id {}", + "successful login to silo {} using provider {}: authenticated \ + subject {} = user id {}", path_params.silo_name, path_params.provider_name, authenticated_subject.external_id, user.id(), ); - Ok(Response::builder() - .status(StatusCode::FOUND) - .header(http::header::LOCATION, next_url) - .header( + let mut response_with_headers = http_response_see_other(next_url); + { + let mut headers = response_with_headers.headers_mut(); + headers.append( header::SET_COOKIE, session_cookie_header_value( &session.token, apictx.session_idle_timeout(), ), - ) - .body("".into())?) // TODO: what do we return from login? + ); + } + Ok(response_with_headers); }; - // TODO this doesn't work because the response is Response - //apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await - handler.await + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } // Log user out of web console by deleting session in both server and browser @@ -455,7 +452,8 @@ pub async fn logout( Ok(Response::builder() .status(StatusCode::NO_CONTENT) .header(header::SET_COOKIE, clear_session_cookie_header_value()) - .body("".into())?) + .body(Body::empty())?) + // TODO no metrics here } #[derive(Deserialize, JsonSchema)] From b0a7e4faa06df8fc96b947432700a8362be2c0d2 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 28 Sep 2022 20:37:37 -0700 Subject: [PATCH 2/8] clean up console_api.rs --- Cargo.lock | 18 +++---- nexus/src/external_api/console_api.rs | 47 ++++++++++-------- nexus/src/external_api/http_entrypoints.rs | 14 +++--- nexus/tests/integration_tests/console_api.rs | 2 +- nexus/tests/integration_tests/saml.rs | 4 +- nexus/tests/output/nexus_tags.txt | 6 +-- .../output/uncovered-authz-endpoints.txt | 6 +-- openapi/nexus.json | 48 ++++++++++++++----- 8 files changed, 84 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 97e2ab9e00d..8d558a14f70 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1226,8 +1226,7 @@ checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" [[package]] name = "dropshot" -version = "0.7.1-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c849d8684f39c5a785782e781fac88f88de49e4b" +version = "0.8.1-dev" dependencies = [ "async-stream", "async-trait", @@ -1266,8 +1265,7 @@ dependencies = [ [[package]] name = "dropshot_endpoint" -version = "0.7.1-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c849d8684f39c5a785782e781fac88f88de49e4b" +version = "0.8.1-dev" dependencies = [ "proc-macro2", "quote", @@ -3583,9 +3581,9 @@ dependencies = [ [[package]] name = "paste" -version = "1.0.8" +version = "1.0.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9423e2b32f7a043629287a536f21951e8c6a82482d0acb1eeebfc90bc2225b22" +checksum = "b1de2e551fb905ac83f73f7aedf2f0cb4a0da7e35efa24a202a936269f1f18e1" [[package]] name = "path-absolutize" @@ -4929,9 +4927,9 @@ dependencies = [ [[package]] name = "sha1" -version = "0.10.1" +version = "0.10.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c77f4e7f65455545c2153c1253d25056825e77ee2533f0e41deb65a93a34852f" +checksum = "f04293dc80c3993519f2d7f6f511707ee7094fe0c6d3406feb330cdb3540eba3" dependencies = [ "cfg-if 1.0.0", "cpufeatures", @@ -6712,7 +6710,3 @@ dependencies = [ "quote", "syn", ] - -[[patch.unused]] -name = "dropshot" -version = "0.8.1-dev" diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 1328c7623f7..fcb4c0817df 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -25,14 +25,15 @@ use crate::{ }; use anyhow::Context; use dropshot::{ - endpoint, http_response_see_other, http_response_temporary_redirect, - HttpError, HttpResponseOk, HttpResponseSeeOther, - HttpResponseTemporaryRedirect, Path, Query, RequestContext, TypedBody, + endpoint, http_response_found, http_response_see_other, HttpError, + HttpResponseFound, HttpResponseOk, HttpResponseSeeOther, Path, Query, + RequestContext, TypedBody, }; use http::{header, Response, StatusCode}; use hyper::Body; use lazy_static::lazy_static; use mime_guess; +use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -83,7 +84,7 @@ pub async fn login_spoof( let session = nexus.session_create(&authn_opctx, user_id).await?; Ok(Response::builder() - .status(StatusCode::OK) + .status(StatusCode::NO_CONTENT) .header( header::SET_COOKIE, session_cookie_header_value( @@ -91,7 +92,7 @@ pub async fn login_spoof( apictx.session_idle_timeout(), ), ) - .body("".into())?) // TODO: what do we return from login? + .body(Body::empty())?) } // Silos have one or more identity providers, and an unauthenticated user will @@ -236,10 +237,10 @@ impl RelayState { path = "/login/{silo_name}/{provider_name}", tags = ["login"], }] -pub async fn login_begin( +pub async fn login_saml_begin( rqctx: Arc>>, path_params: Path, -) -> Result { +) -> Result { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -299,7 +300,7 @@ pub async fn login_begin( |e| HttpError::for_internal_error(e.to_string()), )?; - Ok(http_response_temporary_redirect(sign_in_url)) + http_response_found(sign_in_url) } } }; @@ -401,18 +402,25 @@ pub async fn login_saml( user.id(), ); - let mut response_with_headers = http_response_see_other(next_url); + let mut response_with_headers = http_response_see_other(next_url)?; + { - let mut headers = response_with_headers.headers_mut(); + let headers = response_with_headers.headers_mut(); headers.append( header::SET_COOKIE, - session_cookie_header_value( + http::HeaderValue::from_str(&session_cookie_header_value( &session.token, apictx.session_idle_timeout(), - ), + )) + .map_err(|error| { + HttpError::for_internal_error(format!( + "unsupported cookie value: {:#}", + error + )) + })?, ); } - Ok(response_with_headers); + Ok(response_with_headers) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -471,7 +479,7 @@ pub struct RestPathParam { path = "/spoof_login", unpublished = true, }] -pub async fn spoof_login_form( +pub async fn login_spoof_begin( rqctx: Arc>>, ) -> Result, HttpError> { serve_console_index(rqctx.context()).await @@ -531,16 +539,15 @@ fn get_login_url(redirect_url: Option) -> String { path = "/login", unpublished = true, }] -pub async fn login_redirect( +pub async fn login_begin( _rqctx: Arc>>, query_params: Query, -) -> Result, HttpError> { +) -> Result { + // TODO metrics let query = query_params.into_inner(); let redirect_url = query.state.filter(|s| !s.trim().is_empty()); - Ok(Response::builder() - .status(StatusCode::FOUND) - .header(http::header::LOCATION, get_login_url(redirect_url)) - .body("".into())?) + let login_url = get_login_url(redirect_url); + http_response_found(login_url.clone()) } /// Fetch the user associated with the current session diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index a0a4c6e50b7..bdbf6ebd195 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -245,19 +245,19 @@ pub fn external_api() -> NexusApiDescription { api.register(user_list)?; // Console API operations - api.register(console_api::spoof_login)?; - api.register(console_api::spoof_login_form)?; - api.register(console_api::login_redirect)?; - api.register(console_api::session_me)?; + api.register(console_api::login_begin)?; + api.register(console_api::login_spoof_begin)?; + api.register(console_api::login_spoof)?; + api.register(console_api::login_saml_begin)?; + api.register(console_api::login_saml)?; api.register(console_api::logout)?; + + api.register(console_api::session_me)?; api.register(console_api::console_page)?; api.register(console_api::console_root)?; api.register(console_api::console_settings_page)?; api.register(console_api::asset)?; - api.register(console_api::login)?; - api.register(console_api::consume_credentials)?; - api.register(device_auth::device_auth_request)?; api.register(device_auth::device_auth_verify)?; api.register(device_auth::device_auth_success)?; diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 42b1e92c237..e3924f66fbb 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -397,7 +397,7 @@ fn get_header_value(resp: TestResponse, header_name: HeaderName) -> String { async fn log_in_and_extract_token(testctx: &ClientTestContext) -> String { let login = RequestBuilder::new(&testctx, Method::POST, "/login") .body(Some(&SpoofLoginBody { username: "unprivileged".to_string() })) - .expect_status(Some(StatusCode::OK)) + .expect_status(Some(StatusCode::NO_CONTENT)) .execute() .await .expect("failed to log in"); diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index 909743232a4..ecb9259104d 100644 --- a/nexus/tests/integration_tests/saml.rs +++ b/nexus/tests/integration_tests/saml.rs @@ -985,7 +985,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { }) .unwrap(), )) - .expect_status(Some(StatusCode::FOUND)), + .expect_status(Some(StatusCode::SEE_OTHER)), ) .execute() .await @@ -1084,7 +1084,7 @@ async fn test_post_saml_response_with_relay_state( }) .unwrap(), )) - .expect_status(Some(StatusCode::FOUND)), + .expect_status(Some(StatusCode::SEE_OTHER)), ) .execute() .await diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index a46f39818e3..3fa987fbadf 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -12,9 +12,9 @@ OPERATION ID URL PATH device_access_token /device/token device_auth_confirm /device/confirm device_auth_request /device/auth +login_spoof /login logout /logout session_me /session/me -spoof_login /login API operations found with tag "images" OPERATION ID URL PATH @@ -49,8 +49,8 @@ instance_view_by_id /by-id/instances/{id} API operations found with tag "login" OPERATION ID URL PATH -consume_credentials /login/{silo_name}/{provider_name} -login /login/{silo_name}/{provider_name} +login_saml /login/{silo_name}/{provider_name} +login_saml_begin /login/{silo_name}/{provider_name} API operations found with tag "metrics" OPERATION ID URL PATH diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index 6107c0f27fa..b085926cff0 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,8 +1,8 @@ API endpoints with no coverage in authz tests: -login (get "/login/{silo_name}/{provider_name}") +login_saml_begin (get "/login/{silo_name}/{provider_name}") device_auth_request (post "/device/auth") device_auth_confirm (post "/device/confirm") device_access_token (post "/device/token") -spoof_login (post "/login") -consume_credentials (post "/login/{silo_name}/{provider_name}") +login_spoof (post "/login") +login_saml (post "/login/{silo_name}/{provider_name}") logout (post "/logout") diff --git a/openapi/nexus.json b/openapi/nexus.json index a0c6da2df53..9f52e45d278 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -535,7 +535,7 @@ "tags": [ "hidden" ], - "operationId": "spoof_login", + "operationId": "login_spoof", "requestBody": { "content": { "application/json": { @@ -565,7 +565,7 @@ ], "summary": "Prompt user login", "description": "Either display a page asking a user for their credentials, or redirect them to their identity provider.", - "operationId": "login", + "operationId": "login_saml_begin", "parameters": [ { "in": "path", @@ -587,13 +587,24 @@ } ], "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} + "302": { + "description": "redirect (302)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } } } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } }, @@ -603,7 +614,7 @@ ], "summary": "Authenticate a user", "description": "Either receive a username and password, or some sort of identity provider data (like a SAMLResponse). Use these to set the user's session cookie.", - "operationId": "consume_credentials", + "operationId": "login_saml", "parameters": [ { "in": "path", @@ -636,13 +647,24 @@ "required": true }, "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} + "303": { + "description": "redirect (303)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } } } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } } From 76a9b00b823a4031366e71a06e240f915f9b26c8 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 28 Sep 2022 20:44:14 -0700 Subject: [PATCH 3/8] clippy --- nexus/src/external_api/console_api.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index fcb4c0817df..4e28c88cd58 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -71,7 +71,7 @@ pub async fn login_spoof( if user_id.is_none() { Err(Error::Unauthenticated { - internal_message: format!("unknown user specified"), + internal_message: String::from("unknown user specified"), })?; } @@ -371,7 +371,7 @@ pub async fn login_saml( if user.is_none() { Err(Error::Unauthenticated { - internal_message: format!( + internal_message: String::from( "no matching user found or credentials were not valid" ), })?; @@ -547,7 +547,7 @@ pub async fn login_begin( let query = query_params.into_inner(); let redirect_url = query.state.filter(|s| !s.trim().is_empty()); let login_url = get_login_url(redirect_url); - http_response_found(login_url.clone()) + http_response_found(login_url) } /// Fetch the user associated with the current session From 99b1c6e16b7d9b5c33b507608c3e8ffae79c6438 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 28 Sep 2022 21:06:49 -0700 Subject: [PATCH 4/8] update OpenAPI --- nexus/src/external_api/console_api.rs | 57 +++++++++++++++++++-------- openapi/nexus.json | 30 +++++++------- 2 files changed, 57 insertions(+), 30 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 4e28c88cd58..9458693fede 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -26,7 +26,8 @@ use crate::{ use anyhow::Context; use dropshot::{ endpoint, http_response_found, http_response_see_other, HttpError, - HttpResponseFound, HttpResponseOk, HttpResponseSeeOther, Path, Query, + HttpResponseFound, HttpResponseHeaders, HttpResponseOk, + HttpResponseSeeOther, HttpResponseUpdatedNoContent, Path, Query, RequestContext, TypedBody, }; use http::{header, Response, StatusCode}; @@ -59,7 +60,9 @@ pub struct SpoofLoginBody { pub async fn login_spoof( rqctx: Arc>>, params: TypedBody, -) -> Result, HttpError> { +) -> Result, HttpError> { + // TODO metrics + let apictx = rqctx.context(); let nexus = &apictx.nexus; let params = params.into_inner(); @@ -83,16 +86,25 @@ pub async fn login_spoof( let authn_opctx = nexus.opctx_external_authn(); let session = nexus.session_create(&authn_opctx, user_id).await?; - Ok(Response::builder() - .status(StatusCode::NO_CONTENT) - .header( + let mut response = + HttpResponseHeaders::new_unnamed(HttpResponseUpdatedNoContent()); + { + let headers = response.headers_mut(); + headers.append( header::SET_COOKIE, - session_cookie_header_value( + http::HeaderValue::from_str(&session_cookie_header_value( &session.token, apictx.session_idle_timeout(), - ), - ) - .body(Body::empty())?) + )) + .map_err(|error| { + HttpError::for_internal_error(format!( + "unsupported cookie value: {:#}", + error + )) + })?, + ); + }; + Ok(response) } // Silos have one or more identity providers, and an unauthenticated user will @@ -372,7 +384,7 @@ pub async fn login_saml( if user.is_none() { Err(Error::Unauthenticated { internal_message: String::from( - "no matching user found or credentials were not valid" + "no matching user found or credentials were not valid", ), })?; } @@ -437,7 +449,8 @@ pub async fn login_saml( pub async fn logout( rqctx: Arc>>, cookies: Cookies, -) -> Result, HttpError> { +) -> Result, HttpError> { + // TODO metrics let nexus = &rqctx.context().nexus; let opctx = OpContext::for_external_api(&rqctx).await; let token = cookies.get(SESSION_COOKIE_COOKIE_NAME); @@ -457,11 +470,23 @@ pub async fn logout( // 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(StatusCode::NO_CONTENT) - .header(header::SET_COOKIE, clear_session_cookie_header_value()) - .body(Body::empty())?) - // TODO no metrics here + let mut response = + HttpResponseHeaders::new_unnamed(HttpResponseUpdatedNoContent()); + { + let headers = response.headers_mut(); + headers.append( + header::SET_COOKIE, + http::HeaderValue::from_str(&clear_session_cookie_header_value()) + .map_err(|error| { + HttpError::for_internal_error(format!( + "unsupported cookie value: {:#}", + error + )) + })?, + ); + }; + + Ok(response) } #[derive(Deserialize, JsonSchema)] diff --git a/openapi/nexus.json b/openapi/nexus.json index 9f52e45d278..8c475c9f26b 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -547,13 +547,14 @@ "required": true }, "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} - } - } + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } } @@ -676,13 +677,14 @@ ], "operationId": "logout", "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} - } - } + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } } From 5fc8d15307bcf661b8faa0e9b153de86cfa33f66 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 28 Sep 2022 21:21:05 -0700 Subject: [PATCH 5/8] add metrics, make spoof_login behave like saml_login, fix dep patch to work in CI --- Cargo.lock | 2 + Cargo.toml | 3 +- nexus/src/external_api/console_api.rs | 163 ++++++++++--------- nexus/tests/integration_tests/console_api.rs | 2 +- 4 files changed, 91 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8d558a14f70..ecc43a57dac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1227,6 +1227,7 @@ checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" [[package]] name = "dropshot" version = "0.8.1-dev" +source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#9a45f74a118d07ca71b96a9e2dfb212554ac7e87" dependencies = [ "async-stream", "async-trait", @@ -1266,6 +1267,7 @@ dependencies = [ [[package]] name = "dropshot_endpoint" version = "0.8.1-dev" +source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#9a45f74a118d07ca71b96a9e2dfb212554ac7e87" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index be0f17405ac..ffca52fc9bf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,7 +96,8 @@ panic = "abort" # uncomment one of these blocks. # [patch."https://github.com/oxidecomputer/dropshot"] -dropshot = { path = "../dropshot/dropshot" } +dropshot = { git = "https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87", branch = "redirects" } + #[patch."https://github.com/oxidecomputer/steno"] #steno = { path = "../steno" } #[patch."https://github.com/oxidecomputer/propolis"] diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 9458693fede..48c28a2d9ed 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -60,51 +60,51 @@ pub struct SpoofLoginBody { pub async fn login_spoof( rqctx: Arc>>, params: TypedBody, -) -> Result, HttpError> { - // TODO metrics - +) -> Result { let apictx = rqctx.context(); - let nexus = &apictx.nexus; - let params = params.into_inner(); - let user_id: Option = match params.username.as_str() { - "privileged" => Some(USER_TEST_PRIVILEGED.id()), - "unprivileged" => Some(USER_TEST_UNPRIVILEGED.id()), - _ => None, - }; + let handler = async { + let nexus = &apictx.nexus; + let params = params.into_inner(); + let user_id: Option = match params.username.as_str() { + "privileged" => Some(USER_TEST_PRIVILEGED.id()), + "unprivileged" => Some(USER_TEST_UNPRIVILEGED.id()), + _ => None, + }; - if user_id.is_none() { - Err(Error::Unauthenticated { - internal_message: String::from("unknown user specified"), - })?; - } + if user_id.is_none() { + Err(Error::Unauthenticated { + internal_message: String::from("unknown user specified"), + })?; + } - let user_id = user_id.unwrap(); - - // For now, we use the external authn context to create the session. - // Once we have real SAML login, maybe we can cons up a real OpContext for - // this user and use their own privileges to create the session. - let authn_opctx = nexus.opctx_external_authn(); - let session = nexus.session_create(&authn_opctx, user_id).await?; - - let mut response = - HttpResponseHeaders::new_unnamed(HttpResponseUpdatedNoContent()); - { - let headers = response.headers_mut(); - headers.append( - header::SET_COOKIE, - http::HeaderValue::from_str(&session_cookie_header_value( - &session.token, - apictx.session_idle_timeout(), - )) - .map_err(|error| { - HttpError::for_internal_error(format!( - "unsupported cookie value: {:#}", - error + let user_id = user_id.unwrap(); + + // For now, we use the external authn context to create the session. + // Once we have real SAML login, maybe we can cons up a real OpContext + // for this user and use their own privileges to create the session. + let authn_opctx = nexus.opctx_external_authn(); + let session = nexus.session_create(&authn_opctx, user_id).await?; + + let mut response = http_response_see_other(String::from("/"))?; + { + let headers = response.headers_mut(); + headers.append( + header::SET_COOKIE, + http::HeaderValue::from_str(&session_cookie_header_value( + &session.token, + apictx.session_idle_timeout(), )) - })?, - ); + .map_err(|error| { + HttpError::for_internal_error(format!( + "unsupported cookie value: {:#}", + error + )) + })?, + ); + }; + Ok(response) }; - Ok(response) + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } // Silos have one or more identity providers, and an unauthenticated user will @@ -450,43 +450,49 @@ pub async fn logout( rqctx: Arc>>, cookies: Cookies, ) -> Result, HttpError> { - // TODO metrics - let nexus = &rqctx.context().nexus; - let opctx = OpContext::for_external_api(&rqctx).await; - let token = cookies.get(SESSION_COOKIE_COOKIE_NAME); + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + let opctx = OpContext::for_external_api(&rqctx).await; + let token = cookies.get(SESSION_COOKIE_COOKIE_NAME); - if let Ok(opctx) = opctx { - if let Some(token) = token { - nexus.session_hard_delete(&opctx, token.value()).await?; + if let Ok(opctx) = opctx { + if let Some(token) = token { + nexus.session_hard_delete(&opctx, token.value()).await?; + } } - } - // 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. - - // 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. - - let mut response = - HttpResponseHeaders::new_unnamed(HttpResponseUpdatedNoContent()); - { - let headers = response.headers_mut(); - headers.append( - header::SET_COOKIE, - http::HeaderValue::from_str(&clear_session_cookie_header_value()) + // 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. + + // 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. + + let mut response = + HttpResponseHeaders::new_unnamed(HttpResponseUpdatedNoContent()); + { + let headers = response.headers_mut(); + headers.append( + header::SET_COOKIE, + http::HeaderValue::from_str( + &clear_session_cookie_header_value(), + ) .map_err(|error| { - HttpError::for_internal_error(format!( - "unsupported cookie value: {:#}", - error - )) - })?, - ); + HttpError::for_internal_error(format!( + "unsupported cookie value: {:#}", + error + )) + })?, + ); + }; + + Ok(response) }; - Ok(response) + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } #[derive(Deserialize, JsonSchema)] @@ -565,14 +571,17 @@ fn get_login_url(redirect_url: Option) -> String { unpublished = true, }] pub async fn login_begin( - _rqctx: Arc>>, + rqctx: Arc>>, query_params: Query, ) -> Result { - // TODO metrics - let query = query_params.into_inner(); - let redirect_url = query.state.filter(|s| !s.trim().is_empty()); - let login_url = get_login_url(redirect_url); - http_response_found(login_url) + let apictx = rqctx.context(); + let handler = async { + let query = query_params.into_inner(); + let redirect_url = query.state.filter(|s| !s.trim().is_empty()); + let login_url = get_login_url(redirect_url); + http_response_found(login_url) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } /// Fetch the user associated with the current session diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index e3924f66fbb..b95ba590013 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -397,7 +397,7 @@ fn get_header_value(resp: TestResponse, header_name: HeaderName) -> String { async fn log_in_and_extract_token(testctx: &ClientTestContext) -> String { let login = RequestBuilder::new(&testctx, Method::POST, "/login") .body(Some(&SpoofLoginBody { username: "unprivileged".to_string() })) - .expect_status(Some(StatusCode::NO_CONTENT)) + .expect_status(Some(StatusCode::SEE_OTHER)) .execute() .await .expect("failed to log in"); From 2564b68f9488f907d42920fa74ec2a35ad98b860 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 28 Sep 2022 21:38:48 -0700 Subject: [PATCH 6/8] fix openapi spec --- openapi/nexus.json | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/openapi/nexus.json b/openapi/nexus.json index 8c475c9f26b..b51b3626444 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -547,8 +547,18 @@ "required": true }, "responses": { - "204": { - "description": "resource updated" + "303": { + "description": "redirect (303)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + } + } }, "4XX": { "$ref": "#/components/responses/Error" From e58dd3425137f943467916cef9c2ad0c3f41d0cb Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 30 Sep 2022 14:16:22 -0700 Subject: [PATCH 7/8] update for dropshot review edits --- Cargo.lock | 4 ++-- openapi/nexus.json | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca4b0a9e3b2..601d6ee2600 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1163,7 +1163,7 @@ checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" [[package]] name = "dropshot" version = "0.8.1-dev" -source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#9a45f74a118d07ca71b96a9e2dfb212554ac7e87" +source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#74d5e18144135f632fbf9b6ac067985807f68014" dependencies = [ "async-stream", "async-trait", @@ -1203,7 +1203,7 @@ dependencies = [ [[package]] name = "dropshot_endpoint" version = "0.8.1-dev" -source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#9a45f74a118d07ca71b96a9e2dfb212554ac7e87" +source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#74d5e18144135f632fbf9b6ac067985807f68014" dependencies = [ "proc-macro2", "quote", diff --git a/openapi/nexus.json b/openapi/nexus.json index b51b3626444..8bad5a1b690 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -548,7 +548,7 @@ }, "responses": { "303": { - "description": "redirect (303)", + "description": "redirect (see other)", "headers": { "location": { "description": "HTTP \"Location\" header", @@ -599,7 +599,7 @@ ], "responses": { "302": { - "description": "redirect (302)", + "description": "redirect (found)", "headers": { "location": { "description": "HTTP \"Location\" header", @@ -659,7 +659,7 @@ }, "responses": { "303": { - "description": "redirect (303)", + "description": "redirect (see other)", "headers": { "location": { "description": "HTTP \"Location\" header", From 92629926bf5ff1f99f39aae559485fe5cc73a6a7 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 3 Oct 2022 10:25:44 -0700 Subject: [PATCH 8/8] update deps for dropshot#451 integration --- Cargo.lock | 4 ++-- Cargo.toml | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 601d6ee2600..6ad3e3b6b67 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1163,7 +1163,7 @@ checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" [[package]] name = "dropshot" version = "0.8.1-dev" -source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#74d5e18144135f632fbf9b6ac067985807f68014" +source = "git+https://github.com/oxidecomputer/dropshot?branch=main#0ae4dd3601b56e5cf0d706889ba1d46470e27326" dependencies = [ "async-stream", "async-trait", @@ -1203,7 +1203,7 @@ dependencies = [ [[package]] name = "dropshot_endpoint" version = "0.8.1-dev" -source = "git+https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87?branch=redirects#74d5e18144135f632fbf9b6ac067985807f68014" +source = "git+https://github.com/oxidecomputer/dropshot?branch=main#0ae4dd3601b56e5cf0d706889ba1d46470e27326" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index ffca52fc9bf..a95b95c9c64 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -95,9 +95,8 @@ panic = "abort" # crucible, or steno in the parent directory. If you want to use those, # uncomment one of these blocks. # -[patch."https://github.com/oxidecomputer/dropshot"] -dropshot = { git = "https://github.com/oxidecomputer/dropshot?rev=9a45f74a118d07ca71b96a9e2dfb212554ac7e87", branch = "redirects" } - +#[patch."https://github.com/oxidecomputer/dropshot"] +#dropshot = { path = "../dropshot/dropshot" } #[patch."https://github.com/oxidecomputer/steno"] #steno = { path = "../steno" } #[patch."https://github.com/oxidecomputer/propolis"]