diff --git a/Cargo.lock b/Cargo.lock index f86c2158769..fd2bbead354 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1162,8 +1162,8 @@ 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" +source = "git+https://github.com/oxidecomputer/dropshot?branch=main#0ae4dd3601b56e5cf0d706889ba1d46470e27326" dependencies = [ "async-stream", "async-trait", @@ -1202,8 +1202,8 @@ 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" +source = "git+https://github.com/oxidecomputer/dropshot?branch=main#0ae4dd3601b56e5cf0d706889ba1d46470e27326" dependencies = [ "proc-macro2", "quote", @@ -3536,9 +3536,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" @@ -4876,9 +4876,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", diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 9c7b4d0f8b2..33c9b115b4c 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -25,12 +25,16 @@ use crate::{ }; use anyhow::Context; use dropshot::{ - endpoint, HttpError, HttpResponseOk, Path, Query, RequestContext, TypedBody, + endpoint, http_response_found, http_response_see_other, HttpError, + HttpResponseFound, HttpResponseHeaders, HttpResponseOk, + HttpResponseSeeOther, HttpResponseUpdatedNoContent, 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}; @@ -53,44 +57,54 @@ 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> { +) -> 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() { - return Ok(Response::builder() - .status(StatusCode::UNAUTHORIZED) - .header(header::SET_COOKIE, clear_session_cookie_header_value()) - .body("".into())?); // TODO: failed login response body? - } + if user_id.is_none() { + Err(Error::Unauthenticated { + internal_message: String::from("unknown user specified"), + })?; + } - let user_id = user_id.unwrap(); + 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?; + // 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?; - Ok(Response::builder() - .status(StatusCode::OK) - .header( - header::SET_COOKIE, - session_cookie_header_value( - &session.token, - apictx.session_idle_timeout(), - ), - ) - .body("".into())?) // TODO: what do we return from login? + 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) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } // Silos have one or more identity providers, and an unauthenticated user will @@ -235,10 +249,10 @@ impl RelayState { path = "/login/{silo_name}/{provider_name}", tags = ["login"], }] -pub async fn login( +pub async fn login_saml_begin( rqctx: Arc>>, path_params: Path, -) -> Result, HttpError> { +) -> Result { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -259,8 +273,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 +312,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())?) + http_response_found(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 +329,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 +382,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: String::from( + "no matching user found or credentials were not valid", + ), + })?; } let user = user.unwrap(); @@ -396,28 +406,35 @@ 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 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(), - ), - ) - .body("".into())?) // TODO: what do we return from login? + )) + .map_err(|error| { + HttpError::for_internal_error(format!( + "unsupported cookie value: {:#}", + error + )) + })?, + ); + } + 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 @@ -432,30 +449,50 @@ pub async fn consume_credentials( pub async fn logout( rqctx: Arc>>, cookies: Cookies, -) -> Result, HttpError> { - let nexus = &rqctx.context().nexus; - let opctx = OpContext::for_external_api(&rqctx).await; - let token = cookies.get(SESSION_COOKIE_COOKIE_NAME); +) -> Result, HttpError> { + 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. + // 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. + // 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(StatusCode::NO_CONTENT) - .header(header::SET_COOKIE, clear_session_cookie_header_value()) - .body("".into())?) + 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) + }; + + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } #[derive(Deserialize, JsonSchema)] @@ -473,7 +510,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 @@ -533,16 +570,18 @@ fn get_login_url(redirect_url: Option) -> String { path = "/login", unpublished = true, }] -pub async fn login_redirect( - _rqctx: Arc>>, +pub async fn login_begin( + rqctx: Arc>>, query_params: Query, -) -> Result, HttpError> { - 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())?) +) -> Result { + 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/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index bcc2ffc8bd2..85837f11d5c 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -245,20 +245,20 @@ 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::console_system_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 7e0bfd0f448..2094a105c55 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -398,7 +398,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::SEE_OTHER)) .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..8bad5a1b690 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": { @@ -547,13 +547,24 @@ "required": true }, "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} + "303": { + "description": "redirect (see other)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } } } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } } @@ -565,7 +576,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 +598,24 @@ } ], "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} + "302": { + "description": "redirect (found)", + "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 +625,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 +658,24 @@ "required": true }, "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} + "303": { + "description": "redirect (see other)", + "headers": { + "location": { + "description": "HTTP \"Location\" header", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } } } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } } @@ -654,13 +687,14 @@ ], "operationId": "logout", "responses": { - "default": { - "description": "", - "content": { - "*/*": { - "schema": {} - } - } + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" } } }