From 4086d584fb8e823a76a41e040af1f465db72a5ac Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 27 Jan 2022 16:12:37 -0800 Subject: [PATCH 01/17] authz: make it easier to test authn/authz protection for new endpoints --- nexus/tests/integration_tests/authz.rs | 102 ----- nexus/tests/integration_tests/mod.rs | 2 +- .../tests/integration_tests/roles_builtin.rs | 40 -- nexus/tests/integration_tests/unauthorized.rs | 365 ++++++++++++++++++ .../tests/integration_tests/users_builtin.rs | 24 +- 5 files changed, 370 insertions(+), 163 deletions(-) delete mode 100644 nexus/tests/integration_tests/authz.rs create mode 100644 nexus/tests/integration_tests/unauthorized.rs diff --git a/nexus/tests/integration_tests/authz.rs b/nexus/tests/integration_tests/authz.rs deleted file mode 100644 index 8a3eceef1eb..00000000000 --- a/nexus/tests/integration_tests/authz.rs +++ /dev/null @@ -1,102 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -//! Basic end-to-end tests for authorization -use dropshot::HttpErrorResponseBody; -use nexus_test_utils::http_testing::RequestBuilder; - -use http::method::Method; -use http::StatusCode; -use nexus_test_utils::ControlPlaneTestContext; -use nexus_test_utils_macros::nexus_test; -use omicron_common::api::external::IdentityMetadataCreateParams; -use omicron_nexus::authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF; -use omicron_nexus::external_api::params; - -// TODO-coverage It would be nice to have tests that attempt to hit every -// OpenAPI endpoint with valid arguments and: -// (a) missing credentials (should all fail with 401, 403, or 404) -// (b) bogus credentials (should all fail with a 400-level error) -// (c) valid credentials for a user with no access to anything (should fail with -// 403 or 404) -// For now, we manually test one endpoint that we know should be protected with -// authz so that we're at least testing the mechanism itself. Testing this for -// all endpoints would ensure that we've applied the mechanism consistently and -// correctly for all endpoints. -#[nexus_test] -async fn test_authz_basic(cptestctx: &ControlPlaneTestContext) { - let client = &cptestctx.external_client; - - // With no credentials, we should get back a 401 "Unauthorized" response. - let error = - try_create_organization(client, None, StatusCode::UNAUTHORIZED).await; - assert_eq!(error.error_code, Some(String::from("Unauthorized"))); - assert_eq!(error.message.as_str(), "credentials missing or invalid"); - - // If we provide the valid credentials of an unprivileged user, we should - // get back a 403 "Forbidden" response. - let error = try_create_organization( - client, - Some(omicron_nexus::authn::USER_TEST_UNPRIVILEGED.id.to_string()), - StatusCode::FORBIDDEN, - ) - .await; - assert_eq!(error.error_code, Some(String::from("Forbidden"))); - assert_eq!(error.message.as_str(), "Forbidden"); - - // If we provide invalid credentials altogether, we should get an error. - // This is sort of duplicated by a test in test_authn_http() (which tests - // the authentication system in general, outside the context of Nexus). - // This one verifies that we've correctly integrated authn with Nexus. - let error = try_create_organization( - client, - Some(String::from( - omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_ACTOR, - )), - StatusCode::UNAUTHORIZED, - ) - .await; - assert_eq!(error.error_code, Some(String::from("Unauthorized"))); - assert_eq!(error.message.as_str(), "credentials missing or invalid"); - - let error = try_create_organization( - client, - Some(String::from( - omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_CREDS, - )), - StatusCode::UNAUTHORIZED, - ) - .await; - assert_eq!(error.error_code, Some(String::from("Unauthorized"))); - assert_eq!(error.message.as_str(), "credentials missing or invalid"); -} - -async fn try_create_organization( - client: &dropshot::test_util::ClientTestContext, - maybe_user_id: Option, - expected_status: http::StatusCode, -) -> HttpErrorResponseBody { - let input = params::OrganizationCreate { - identity: IdentityMetadataCreateParams { - name: "a-crime-family".parse().unwrap(), - description: "an org".to_string(), - }, - }; - - let mut builder = - RequestBuilder::new(client, Method::POST, "/organizations") - .body(Some(&input)) - .expect_status(Some(expected_status)); - if let Some(user_id) = maybe_user_id { - let authn_header = http::HeaderValue::from_str(&user_id).unwrap(); - builder = builder.header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header); - } - - builder - .execute() - .await - .expect("failed to make request") - .parsed_body() - .unwrap() -} diff --git a/nexus/tests/integration_tests/mod.rs b/nexus/tests/integration_tests/mod.rs index 7889565a5a4..3c172a759f6 100644 --- a/nexus/tests/integration_tests/mod.rs +++ b/nexus/tests/integration_tests/mod.rs @@ -4,7 +4,6 @@ //! the way it is. mod authn_http; -mod authz; mod basic; mod commands; mod console_api; @@ -18,6 +17,7 @@ mod roles_builtin; mod router_routes; mod subnet_allocation; mod timeseries; +mod unauthorized; mod users_builtin; mod vpc_firewall; mod vpc_routers; diff --git a/nexus/tests/integration_tests/roles_builtin.rs b/nexus/tests/integration_tests/roles_builtin.rs index 36cd9584594..084f447eb7e 100644 --- a/nexus/tests/integration_tests/roles_builtin.rs +++ b/nexus/tests/integration_tests/roles_builtin.rs @@ -17,28 +17,6 @@ use omicron_nexus::external_api::views::Role; async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) { let testctx = &cptestctx.external_client; - // Standard authn / authz checks - NexusRequest::expect_failure( - testctx, - StatusCode::UNAUTHORIZED, - Method::GET, - "/roles", - ) - .execute() - .await - .unwrap(); - - NexusRequest::expect_failure( - testctx, - StatusCode::FORBIDDEN, - Method::GET, - "/roles", - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .unwrap(); - // Success cases let roles = NexusRequest::object_get(&testctx, "/roles") .authn_as(AuthnMode::PrivilegedUser) @@ -92,24 +70,6 @@ async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) { assert_eq!(one_role, *r); } - // Standard authnn/authz checks - RequestBuilder::new(testctx, Method::GET, "/roles/fleet.admin") - .expect_status(Some(StatusCode::NOT_FOUND)) - .execute() - .await - .unwrap(); - - NexusRequest::expect_failure( - testctx, - StatusCode::NOT_FOUND, - Method::GET, - "/roles/fleet.admin", - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .unwrap(); - // Invalid name: missing "." NexusRequest::new( RequestBuilder::new(testctx, Method::GET, "/roles/fleet_admin") diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs new file mode 100644 index 00000000000..67473216c24 --- /dev/null +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -0,0 +1,365 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! This test hits authorization-protected endpoints with unauthenticated and +//! unauthorized users to make sure we get the expected behavior (generally: +//! 401, 403, or 404). + +// TODO-coverage +// * It would be good to add a built-in test user that can read everything in +// the world and use that to exercise 404 vs. 401/403 behavior. +// * It'd be nice to verify that all the endpoints listed here are within the +// OpenAPI spec. +// * It'd be nice to produce a list of endpoints from the OpenAPI spec that are +// not checked here. We could put this into an expectorate file and make sure +// that we don't add new unchecked endpoints. +// * When we finish authz, maybe the hardcoded information here can come instead +// from the OpenAPI spec? + +use dropshot::test_util::ClientTestContext; +use dropshot::HttpErrorResponseBody; +use http::method::Method; +use http::StatusCode; +use lazy_static::lazy_static; +use nexus_test_utils::http_testing::AuthnMode; +use nexus_test_utils::http_testing::NexusRequest; +use nexus_test_utils::http_testing::RequestBuilder; +use nexus_test_utils::http_testing::TestResponse; +use nexus_test_utils::ControlPlaneTestContext; +use nexus_test_utils_macros::nexus_test; +use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::IdentityMetadataUpdateParams; +use omicron_nexus::authn; +use omicron_nexus::authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF; +use omicron_nexus::external_api::params; + +#[nexus_test] +async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let log = &cptestctx.logctx.log; + + // Create test data. + for request in &*SETUP_REQUESTS { + NexusRequest::objects_post(client, request.url, &request.body) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); + } + + for endpoint in &*VERIFY_ENDPOINTS { + verify_endpoint(&log, client, endpoint).await; + } +} + +struct SetupReq { + url: &'static str, + body: serde_json::Value, +} + +lazy_static! { + static ref DEMO_ORG_CREATE: params::OrganizationCreate = + params::OrganizationCreate { + identity: IdentityMetadataCreateParams { + name: "demo-org".parse().unwrap(), + description: "".parse().unwrap(), + }, + }; + static ref DEMO_PROJECT_CREATE: params::ProjectCreate = + params::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "demo-project".parse().unwrap(), + description: "".parse().unwrap(), + }, + }; + static ref SETUP_REQUESTS: Vec = vec![ + SetupReq { + url: "/organizations", + body: serde_json::to_value(&*DEMO_ORG_CREATE).unwrap() + }, + SetupReq { + url: "/organizations/demo-org/projects", + body: serde_json::to_value(&*DEMO_PROJECT_CREATE).unwrap(), + }, + ]; +} + +enum Visibility { + Everyone, + Protected, +} + +enum AllowedMethod { + Delete, + Get, + Post(serde_json::Value), + Put(serde_json::Value), +} + +impl AllowedMethod { + fn http_method(&self) -> &'static http::Method { + match self { + AllowedMethod::Delete => &Method::DELETE, + AllowedMethod::Get => &Method::GET, + AllowedMethod::Post(_) => &Method::POST, + AllowedMethod::Put(_) => &Method::PUT, + } + } + + fn body(&self) -> Option<&serde_json::Value> { + match self { + AllowedMethod::Delete | AllowedMethod::Get => None, + AllowedMethod::Post(body) => Some(&body), + AllowedMethod::Put(body) => Some(&body), + } + } +} + +struct VerifyEndpoint { + url: &'static str, + visibility: Visibility, + allowed_methods: Vec, +} + +lazy_static! { + static ref URL_USERS_DB_INIT: String = + format!("/users/{}", authn::USER_DB_INIT.name); + static ref VERIFY_ENDPOINTS: Vec = vec![ + VerifyEndpoint { + url: "/organizations", + visibility: Visibility::Everyone, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_ORG_CREATE).unwrap() + ) + ], + }, + VerifyEndpoint { + url: "/organizations/demo-org", + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + AllowedMethod::Put( + serde_json::to_value(¶ms::OrganizationUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + } + }).unwrap() + ), + ], + }, + VerifyEndpoint { + url: "/organizations/demo-org/projects", + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_PROJECT_CREATE).unwrap() + ), + ], + }, + VerifyEndpoint { + url: "/roles", + visibility: Visibility::Everyone, + allowed_methods: vec![AllowedMethod::Get], + }, + VerifyEndpoint { + url: "/roles/fleet.admin", + visibility: Visibility::Protected, + allowed_methods: vec![AllowedMethod::Get], + }, + VerifyEndpoint { + url: "/users", + visibility: Visibility::Everyone, + allowed_methods: vec![AllowedMethod::Get], + }, + VerifyEndpoint { + url: &*URL_USERS_DB_INIT, + visibility: Visibility::Protected, + allowed_methods: vec![AllowedMethod::Get], + }, + + // XXX what other endpoints? + // XXX remove manual tests in random other tests + // XXX clean up and document this test + ]; +} + +async fn verify_endpoint( + log: &slog::Logger, + client: &ClientTestContext, + endpoint: &VerifyEndpoint, +) { + info!(log, "test endpoint"; "url" => endpoint.url); + let methods = + [Method::GET, Method::PUT, Method::POST, Method::DELETE, Method::TRACE]; + + // XXX Tests that we want to run + // privileged GET /url => always a 200 + // unauthenticated GET, DELETE /url + // if public, then 401; otherwise 404 + // unauthenticated PUT /url + // if has body + // if public, then 401; otherwise 404 + // else + // if public, then 405; otherwise 404 (how can this possibly work + // XXX it'll probably be 405 and that's probably okay) + // unauthenticated POST /url: same as unauthenticated PUT + // unauthenticated TRACE: always 405 + // + // unauthorized GET, DELETE /url: + // if public, then 403; otherwise 404 + // unauthorized PUT /url: + // if has body + // if public, then 403; otherwise 404 + // else + // always 405 + // + + let unauthn_status = match endpoint.visibility { + Visibility::Everyone => StatusCode::UNAUTHORIZED, + Visibility::Protected => StatusCode::NOT_FOUND, + }; + + let unauthz_status = match endpoint.visibility { + Visibility::Everyone => StatusCode::FORBIDDEN, + Visibility::Protected => StatusCode::NOT_FOUND, + }; + + // Make one GET request as an authorized user to make sure we get a + // "200 OK" response. Otherwise, the test might later succeed by + // coincidence. We might find a 404 because of something that actually + // doesn't exist rather than something that's just hidden from + // unauthorized users. + NexusRequest::object_get(client, endpoint.url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); + + for method in methods { + let allowed = endpoint + .allowed_methods + .iter() + .find(|allowed| method == *allowed.http_method()); + + let body = match method { + // Always supply some body for POST and PUT. If this is an allowed + // method, then it will have a body in the structure. Otherwise, + // make one up. + Method::POST | Method::PUT => allowed + .and_then(|a| a.body()) + .cloned() + .or_else(|| Some(serde_json::Value::String(String::new()))), + _ => allowed.and_then(|a| a.body()).cloned(), + }; + + // First, make an authenticated, unauthorized request. + let expected_status = if allowed.is_none() { + StatusCode::METHOD_NOT_ALLOWED + } else { + unauthz_status + }; + let response = NexusRequest::new( + RequestBuilder::new(client, method.clone(), endpoint.url) + .body(body.as_ref()) + .expect_status(Some(expected_status)), + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .unwrap(); + verify_response(&response); + + // First, let's make an unauthenticated request. + let expected_status = if allowed.is_none() { + StatusCode::METHOD_NOT_ALLOWED + } else { + unauthn_status + }; + let response = + RequestBuilder::new(client, method.clone(), endpoint.url) + .body(body.as_ref()) + .expect_status(Some(expected_status)) + .execute() + .await + .unwrap(); + verify_response(&response); + + // If we provide invalid credentials altogether, we should get the same + // error as if we were unauthenticated. This is sort of duplicated by a + // test in test_authn_http() (which tests the authentication system in + // general, outside the context of Nexus). These two tests verify that + // we've correctly integrated authn with Nexus. + // + // First, try a syntactically valid authn header for a non-existent + // actor. + + let expected_status = if allowed.is_none() { + StatusCode::METHOD_NOT_ALLOWED + } else { + // The 401 that you get for authentication failure overrides a 404 + // that you might get if you were authenticated but couldn't see the + // resource in question. That is, you should always see a 401 if + // you fail to authenticate, whether or not the resource exists. + StatusCode::UNAUTHORIZED + }; + let bad_actor_authn_header = http::HeaderValue::from_str( + omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_ACTOR, + ) + .unwrap(); + let response = + RequestBuilder::new(client, method.clone(), endpoint.url) + .body(body.as_ref()) + .expect_status(Some(expected_status)) + .header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, bad_actor_authn_header) + .execute() + .await + .unwrap(); + verify_response(&response); + + // Now try a syntactically valid authn header. + let bad_creds_authn_header = http::HeaderValue::from_str( + omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_CREDS, + ) + .unwrap(); + let response = + RequestBuilder::new(client, method.clone(), endpoint.url) + .body(body.as_ref()) + .expect_status(Some(expected_status)) + .header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, bad_creds_authn_header) + .execute() + .await + .unwrap(); + verify_response(&response); + } +} + +fn verify_response(response: &TestResponse) { + let error: HttpErrorResponseBody = response.parsed_body().unwrap(); + match response.status { + StatusCode::UNAUTHORIZED => { + assert_eq!(error.error_code.unwrap(), "Unauthorized"); + assert_eq!(error.message, "credentials missing or invalid"); + } + StatusCode::FORBIDDEN => { + assert_eq!(error.error_code.unwrap(), "Forbidden"); + assert_eq!(error.message, "Forbidden"); + } + StatusCode::NOT_FOUND => { + assert_eq!(error.error_code.unwrap(), "ObjectNotFound"); + assert!(error.message.starts_with("not found: ")); + assert!(error.message.contains(" with name \"")); + assert!(error.message.ends_with("\"")); + } + StatusCode::METHOD_NOT_ALLOWED => { + assert!(error.error_code.is_none()); + assert_eq!(error.message, "Method Not Allowed"); + } + _ => unimplemented!(), + } +} diff --git a/nexus/tests/integration_tests/users_builtin.rs b/nexus/tests/integration_tests/users_builtin.rs index 210c29b8d3f..50f737f4dfa 100644 --- a/nexus/tests/integration_tests/users_builtin.rs +++ b/nexus/tests/integration_tests/users_builtin.rs @@ -1,37 +1,18 @@ //! Sanity-tests for built-in users use dropshot::ResultsPage; -use http::Method; -use http::StatusCode; -use std::collections::BTreeMap; - use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; -use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; use omicron_nexus::authn; use omicron_nexus::external_api::views::User; +use std::collections::BTreeMap; #[nexus_test] async fn test_users_builtin(cptestctx: &ControlPlaneTestContext) { let testctx = &cptestctx.external_client; - RequestBuilder::new(testctx, Method::GET, "/users") - .expect_status(Some(StatusCode::UNAUTHORIZED)) - .execute() - .await - .unwrap(); - - NexusRequest::new( - RequestBuilder::new(testctx, Method::GET, "/users") - .expect_status(Some(StatusCode::FORBIDDEN)), - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .unwrap(); - let mut users = NexusRequest::object_get(&testctx, "/users") .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -55,4 +36,7 @@ async fn test_users_builtin(cptestctx: &ControlPlaneTestContext) { users.remove(&authn::USER_TEST_UNPRIVILEGED.name.to_string()).unwrap(); assert_eq!(u.identity.id, authn::USER_TEST_UNPRIVILEGED.id); assert!(users.is_empty(), "found unexpected built-in users"); + + // TODO-coverage add test for fetching individual users, including invalid + // names? See roles_builtin.rs. } From 2c28623dd0b64194aba3e05ce94aaba7b3349c79 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 28 Jan 2022 19:43:33 -0800 Subject: [PATCH 02/17] rip out now-redundant tests --- nexus/tests/integration_tests/basic.rs | 109 +----------------- .../tests/integration_tests/organizations.rs | 46 -------- nexus/tests/integration_tests/projects.rs | 80 +------------ nexus/tests/integration_tests/unauthorized.rs | 27 ++++- 4 files changed, 28 insertions(+), 234 deletions(-) diff --git a/nexus/tests/integration_tests/basic.rs b/nexus/tests/integration_tests/basic.rs index 55648c53f59..52ffb4edf68 100644 --- a/nexus/tests/integration_tests/basic.rs +++ b/nexus/tests/integration_tests/basic.rs @@ -177,27 +177,6 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { create_organization(&client, &org_name).await; let projects_url = "/organizations/test-org/projects"; - /* Unauthenticated and unauthorized users cannot list projects. */ - NexusRequest::expect_failure( - client, - http::StatusCode::NOT_FOUND, - http::Method::GET, - projects_url, - ) - .execute() - .await - .expect("failed to make request"); - NexusRequest::expect_failure( - client, - http::StatusCode::NOT_FOUND, - http::Method::GET, - projects_url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - /* * Verify that there are no projects to begin with. */ @@ -240,30 +219,6 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { project_ids }; - /* - * Unauthenticated and unauthorized users cannot fetch the Project. - */ - let simproject1_url = "/organizations/test-org/projects/simproject1"; - NexusRequest::expect_failure( - client, - http::StatusCode::NOT_FOUND, - http::Method::GET, - simproject1_url, - ) - .execute() - .await - .expect("failed to make request"); - NexusRequest::expect_failure( - client, - http::StatusCode::NOT_FOUND, - http::Method::GET, - simproject1_url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - /* * Error case: GET /organizations/test-org/projects/simproject1/nonexistent * (a path that does not exist beneath a resource that does exist) @@ -384,41 +339,6 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { expected_projects[1].identity.description ); - /* - * Unprivileged users should not be able to update a Project. - */ - let project_update = params::ProjectUpdate { - identity: IdentityMetadataUpdateParams { - name: None, - description: None, - }, - }; - NexusRequest::new( - RequestBuilder::new( - client, - Method::PUT, - "/organizations/test-org/projects/simproject3", - ) - .body(Some(&project_update)) - .expect_status(Some(StatusCode::NOT_FOUND)), - ) - .execute() - .await - .expect("failed to make request"); - NexusRequest::new( - RequestBuilder::new( - client, - Method::PUT, - "/organizations/test-org/projects/simproject3", - ) - .body(Some(&project_update)) - .expect_status(Some(StatusCode::NOT_FOUND)), - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - /* * Update "simproject3". We'll make sure that's reflected in the other * requests. @@ -511,6 +431,8 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!("already exists: project \"simproject1\"", error.message); + // TODO-coverage try to rename it to a name that conflicts + /* * Try to create a project with an unsupported name. * TODO-polish why doesn't serde include the field name in this error? @@ -559,33 +481,6 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { assert_eq!(project.identity.name, "honor-roller"); assert_eq!(project.identity.description, "a soapbox racer"); - /* - * Attempt to create a project without authenticating or without privileges. - * TODO-security TODO-correctness One thing that's a little strange here: we - * currently return a 404 if you attempt to create a Project inside an - * Organization and you're not authorized to do that. In an ideal world, - * we'd return a 403 if you can _see_ the Organization and a 404 if not. - * But we don't really know if you should be able to see the Organization. - * Right now, the only real way to tell that is if you have permissions on - * anything _inside_ the Organization, which is incredibly expensive to - * determine in general. - */ - RequestBuilder::new(client, Method::POST, &projects_url) - .expect_status(Some(StatusCode::NOT_FOUND)) - .body(Some(&project_create)) - .execute() - .await - .expect("expected request to fail"); - NexusRequest::new( - RequestBuilder::new(client, Method::POST, &projects_url) - .body(Some(&project_create)) - .expect_status(Some(StatusCode::NOT_FOUND)), - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("expected request to fail"); - /* * List projects again and verify all of our changes. We should have: * diff --git a/nexus/tests/integration_tests/organizations.rs b/nexus/tests/integration_tests/organizations.rs index aab6f3187b3..a28407e8002 100644 --- a/nexus/tests/integration_tests/organizations.rs +++ b/nexus/tests/integration_tests/organizations.rs @@ -34,29 +34,6 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!(organization.identity.name, o1_name); - // You should get a 404 if not authenticated. - NexusRequest::expect_failure( - &client, - StatusCode::NOT_FOUND, - Method::GET, - &o1_url, - ) - .execute() - .await - .expect("failed to make request"); - - // Same if you're authenticated but not authorized to see it. - NexusRequest::expect_failure( - &client, - StatusCode::NOT_FOUND, - Method::GET, - &o1_url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - let o2_url = format!("/organizations/{}", o2_name); let organization: Organization = NexusRequest::object_get(&client, &o2_url) .authn_as(AuthnMode::PrivilegedUser) @@ -86,29 +63,6 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { assert_eq!(organizations[0].identity.name, o2_name); assert_eq!(organizations[1].identity.name, o1_name); - // You should get a 404 if you attempt to delete an organization if you are - // unauthenticated or unauthorized. - NexusRequest::expect_failure( - &client, - StatusCode::NOT_FOUND, - Method::DELETE, - &o1_url, - ) - .execute() - .await - .expect("failed to make request"); - - NexusRequest::expect_failure( - &client, - StatusCode::NOT_FOUND, - Method::DELETE, - &o1_url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - // Verify DELETE /organization/{org} works let o1_old_id = organizations[1].identity.id; NexusRequest::object_delete(&client, &o1_url) diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index fc715980393..3a463ffbb3c 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -2,12 +2,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; -use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::resource_helpers::project_get; -use omicron_common::api::external::IdentityMetadataUpdateParams; -use omicron_nexus::external_api::params; use omicron_nexus::external_api::views::Project; use nexus_test_utils::resource_helpers::{create_organization, create_project}; @@ -37,32 +33,8 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { let project: Project = project_get(&client, &p2_url).await; assert_eq!(project.identity.name, p2_name); - /* - * Unauthenticated and unauthorized users should not be able to list - * Projects. - */ - let projects_url = format!("/organizations/{}/projects", org_name); - NexusRequest::expect_failure( - &client, - http::StatusCode::NOT_FOUND, - http::Method::GET, - &projects_url, - ) - .execute() - .await - .expect("failed to make request"); - NexusRequest::expect_failure( - &client, - http::StatusCode::NOT_FOUND, - http::Method::GET, - &projects_url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - /* Verify the list of Projects. */ + let projects_url = format!("/organizations/{}/projects", org_name); let projects = NexusRequest::iter_collection_authn::( &client, &projects_url, @@ -97,54 +69,4 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { .all_items; assert_eq!(projects.len(), 1); assert_eq!(projects[0].identity.name, p1_name); - - // Unauthenticated and unauthorized users should not be able to delete or - // modify a Project. - NexusRequest::expect_failure( - &client, - http::StatusCode::NOT_FOUND, - http::Method::DELETE, - &p1_url, - ) - .execute() - .await - .expect("failed to make request"); - NexusRequest::expect_failure( - &client, - http::StatusCode::NOT_FOUND, - http::Method::DELETE, - &p1_url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); - - NexusRequest::new( - RequestBuilder::new(&client, http::Method::PUT, &p1_url) - .body(Some(¶ms::ProjectUpdate { - identity: IdentityMetadataUpdateParams { - name: None, - description: None, - }, - })) - .expect_status(Some(http::StatusCode::NOT_FOUND)), - ) - .execute() - .await - .expect("failed to make request"); - NexusRequest::new( - RequestBuilder::new(&client, http::Method::PUT, &p1_url) - .body(Some(¶ms::ProjectUpdate { - identity: IdentityMetadataUpdateParams { - name: None, - description: None, - }, - })) - .expect_status(Some(http::StatusCode::NOT_FOUND)), - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("failed to make request"); } diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 67473216c24..2778e80a249 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -152,6 +152,15 @@ lazy_static! { ), ], }, + + // TODO-security TODO-correctness One thing that's a little strange + // here: we currently return a 404 if you attempt to create a Project + // inside an Organization and you're not authorized to do that. In an + // ideal world, we'd return a 403 if you can _see_ the Organization and + // a 404 if not. But we don't really know if you should be able to see + // the Organization. Right now, the only real way to tell that is if + // you have permissions on anything _inside_ the Organization, which is + // incredibly expensive to determine in general. VerifyEndpoint { url: "/organizations/demo-org/projects", visibility: Visibility::Protected, @@ -162,6 +171,22 @@ lazy_static! { ), ], }, + VerifyEndpoint { + url: "/organizations/demo-org/projects/demo-project", + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + AllowedMethod::Put( + serde_json::to_value(params::ProjectUpdate{ + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + }, + }).unwrap() + ), + ], + }, VerifyEndpoint { url: "/roles", visibility: Visibility::Everyone, @@ -183,8 +208,6 @@ lazy_static! { allowed_methods: vec![AllowedMethod::Get], }, - // XXX what other endpoints? - // XXX remove manual tests in random other tests // XXX clean up and document this test ]; } From 3eec4e3d5b67a806d6abca995f2a9162babc48d9 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 28 Jan 2022 20:55:44 -0800 Subject: [PATCH 03/17] clean up and document new test --- nexus/tests/integration_tests/unauthorized.rs | 316 ++++++++++++------ 1 file changed, 221 insertions(+), 95 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 2778e80a249..d71e50d1b88 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -2,20 +2,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! This test hits authorization-protected endpoints with unauthenticated and -//! unauthorized users to make sure we get the expected behavior (generally: -//! 401, 403, or 404). - -// TODO-coverage -// * It would be good to add a built-in test user that can read everything in -// the world and use that to exercise 404 vs. 401/403 behavior. -// * It'd be nice to verify that all the endpoints listed here are within the -// OpenAPI spec. -// * It'd be nice to produce a list of endpoints from the OpenAPI spec that are -// not checked here. We could put this into an expectorate file and make sure -// that we don't add new unchecked endpoints. -// * When we finish authz, maybe the hardcoded information here can come instead -// from the OpenAPI spec? +//! Verify the behavior of API endpoints when hit by unauthenticated and +//! unauthorized users use dropshot::test_util::ClientTestContext; use dropshot::HttpErrorResponseBody; @@ -30,10 +18,39 @@ use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; +use omicron_common::api::external::Name; use omicron_nexus::authn; use omicron_nexus::authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF; use omicron_nexus::external_api::params; +// This test hits a list Nexus API endpoints using both unauthenticated and +// unauthorized requests to make sure we get the expected behavior (generally: +// 401, 403, or 404). This is trickier than it sounds because the appropriate +// error code depends on what the user was trying to do and what other +// permissions they have on the resource. Notably, if you try to do anything +// with a resource that you're not even supposed to be able to see, you should +// get a 404 "Not Found", not a 403 "Forbidden". It's critical to get this +// right because the alternative can leak information to a potential attacker. +// +// Fortunately, most endpoints behave the same way when it comes to +// unauthenticated or unauthorized requests so it's possible to exhaustively +// test much of the API. +// +// This test works in two phases. First, we execute a sequence of setup +// requests that create all the resources that we're going to test with. Then +// we run through the list of endpoints we're going to test and verify each one. +// See `verify_endpoint()` for exactly what we do for each one. +// +// TODO-coverage: +// * It would be good to add a built-in test user that can read everything in +// the world and use that to exercise 404 vs. 401/403 behavior. +// * It'd be nice to verify that all the endpoints listed here are within the +// OpenAPI spec. +// * It'd be nice to produce a list of endpoints from the OpenAPI spec that are +// not checked here. We could put this into an expectorate file and make sure +// that we don't add new unchecked endpoints. +// * When we finish authz, maybe the hardcoded information here can come instead +// from the OpenAPI spec? #[nexus_test] async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; @@ -48,56 +65,137 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { .unwrap(); } + // Verify the hardcoded endpoints. for endpoint in &*VERIFY_ENDPOINTS { verify_endpoint(&log, client, endpoint).await; } } +// +// SETUP PHASE +// + +/// Describes a request made during the setup phase to create a resource that +/// we'll use later in the verification phase +/// +/// The setup phase takes a list of `SetupReq` structs and issues `POST` +/// requests to each one's `url` with the specific `body`. struct SetupReq { + /// url to send the `POST` to url: &'static str, + /// body of the `POST` request body: serde_json::Value, } lazy_static! { + /// List of requests to execute at setup time + static ref SETUP_REQUESTS: Vec = vec![ + // Create an Organization + SetupReq { + url: "/organizations", + body: serde_json::to_value(&*DEMO_ORG_CREATE).unwrap() + }, + // Create a Project in the Organization + SetupReq { + url: "/organizations/demo-org/projects", + body: serde_json::to_value(&*DEMO_PROJECT_CREATE).unwrap(), + }, + ]; + + // Organization used for testing + static ref DEMO_ORG_NAME: Name = "demo-org".parse().unwrap(); + static ref DEMO_ORG_URL: String = + format!("/organizations/{}", *DEMO_ORG_NAME); + static ref DEMO_ORG_PROJECTS_URL: String = + format!("{}/projects", *DEMO_ORG_URL); static ref DEMO_ORG_CREATE: params::OrganizationCreate = params::OrganizationCreate { identity: IdentityMetadataCreateParams { - name: "demo-org".parse().unwrap(), + name: DEMO_ORG_NAME.clone(), description: "".parse().unwrap(), }, }; + + // Project used for testing + static ref DEMO_PROJECT_NAME: Name = "demo-project".parse().unwrap(); + static ref DEMO_PROJECT_URL: String = + format!("{}/{}", *DEMO_ORG_PROJECTS_URL, *DEMO_PROJECT_NAME); static ref DEMO_PROJECT_CREATE: params::ProjectCreate = params::ProjectCreate { identity: IdentityMetadataCreateParams { - name: "demo-project".parse().unwrap(), + name: DEMO_PROJECT_NAME.clone(), description: "".parse().unwrap(), }, }; - static ref SETUP_REQUESTS: Vec = vec![ - SetupReq { - url: "/organizations", - body: serde_json::to_value(&*DEMO_ORG_CREATE).unwrap() - }, - SetupReq { - url: "/organizations/demo-org/projects", - body: serde_json::to_value(&*DEMO_PROJECT_CREATE).unwrap(), - }, - ]; } +// +// VERIFY PHASE +// + +/// Describes an API endpoint to be verified +/// +/// `url` is an HTTP path for the endpoint. (Note that we might talk about the +/// "get organization" endpoint, and might write that "GET +/// /organizations/{organization_name}". But the URL here is for a specific +/// HTTP resource, so it would look like "/organizations/demo-org" rather than +/// "/organizations/{organization_name}".) +/// +/// `visibility` specifies whether an HTTP resource handled by this endpoint is +/// visible to unauthenticated or unauthorized users. If it's +/// [`Visibility::Public`] (like "/organizations"), unauthorized users can +/// expect to get back a 401 or 403 when they attempt to access it. If it's +/// [`Visibility::Protected`] (like a specific Organization), unauthorized users +/// will get a 404. +/// +/// `allowed_methods` specifies what HTTP methods are supported for this +/// HTTP resource. The test runner tests a variety of HTTP methods. For each +/// method, if it's not in this list, we expect a 405 "Method Not Allowed" +/// response. For `PUT` and `POST`, the item in `allowed_methods` also contains +/// the contents of the body to send with the `PUT` or `POST` request. This +/// should be valid input for the endpoint. Otherwise, Nexus could choose to +/// fail with a 400-level validation error, which would obscure the authn/authz +/// error we're looking for. +struct VerifyEndpoint { + /// URL path for the HTTP resource to test + url: &'static str, + + /// Visibility of the HTTP resource + visibility: Visibility, + + /// Methods that are supposed for the underlying API endpoint + allowed_methods: Vec, +} + +/// Describes the visibility of an HTTP resource enum Visibility { - Everyone, + /// All users can see the resource (including unauthenticated or + /// unauthorized users). + /// + /// "/organizations" is Public, for example. + Public, + /// Only users with certain privileges can see this endpoint. + /// + /// "/organizations/demo-org" is not public, for example. Protected, } +/// Describes an HTTP method supported by a particular API endpoint enum AllowedMethod { + /// HTTP "DELETE" method Delete, + /// HTTP "GET" method Get, + /// HTTP "POST" method, with sample input (which should be valid input for + /// this endpoint) Post(serde_json::Value), + /// HTTP "PUT" method, with sample input (which should be valid input for + /// this endpoint) Put(serde_json::Value), } impl AllowedMethod { + /// Returns the [`http::Method`] used to make a request for this HTTP method fn http_method(&self) -> &'static http::Method { match self { AllowedMethod::Delete => &Method::DELETE, @@ -107,6 +205,10 @@ impl AllowedMethod { } } + /// Returns a JSON value suitable for use as the request body when making a + /// request to a specific endpoint using this HTTP method + /// + /// If this returns `None`, the request body should be empty. fn body(&self) -> Option<&serde_json::Value> { match self { AllowedMethod::Delete | AllowedMethod::Get => None, @@ -116,19 +218,13 @@ impl AllowedMethod { } } -struct VerifyEndpoint { - url: &'static str, - visibility: Visibility, - allowed_methods: Vec, -} - lazy_static! { static ref URL_USERS_DB_INIT: String = format!("/users/{}", authn::USER_DB_INIT.name); static ref VERIFY_ENDPOINTS: Vec = vec![ VerifyEndpoint { url: "/organizations", - visibility: Visibility::Everyone, + visibility: Visibility::Public, allowed_methods: vec![ AllowedMethod::Get, AllowedMethod::Post( @@ -137,7 +233,7 @@ lazy_static! { ], }, VerifyEndpoint { - url: "/organizations/demo-org", + url: &*DEMO_ORG_URL, visibility: Visibility::Protected, allowed_methods: vec![ AllowedMethod::Get, @@ -162,7 +258,7 @@ lazy_static! { // you have permissions on anything _inside_ the Organization, which is // incredibly expensive to determine in general. VerifyEndpoint { - url: "/organizations/demo-org/projects", + url: &*DEMO_ORG_PROJECTS_URL, visibility: Visibility::Protected, allowed_methods: vec![ AllowedMethod::Get, @@ -172,7 +268,7 @@ lazy_static! { ], }, VerifyEndpoint { - url: "/organizations/demo-org/projects/demo-project", + url: &*DEMO_PROJECT_URL, visibility: Visibility::Protected, allowed_methods: vec![ AllowedMethod::Get, @@ -187,9 +283,10 @@ lazy_static! { ), ], }, + VerifyEndpoint { url: "/roles", - visibility: Visibility::Everyone, + visibility: Visibility::Public, allowed_methods: vec![AllowedMethod::Get], }, VerifyEndpoint { @@ -197,9 +294,10 @@ lazy_static! { visibility: Visibility::Protected, allowed_methods: vec![AllowedMethod::Get], }, + VerifyEndpoint { url: "/users", - visibility: Visibility::Everyone, + visibility: Visibility::Public, allowed_methods: vec![AllowedMethod::Get], }, VerifyEndpoint { @@ -207,63 +305,94 @@ lazy_static! { visibility: Visibility::Protected, allowed_methods: vec![AllowedMethod::Get], }, - - // XXX clean up and document this test ]; } +/// Verifies a single API endpoint, described with `endpoint` +/// +/// (Technically, a single `VerifyEndpoint` struct describes an HTTP resource, +/// like "/organizations". There are several API endpoints there, like "GET +/// /organizations" and "POST /organizations". We're a little loose with the +/// terminology here.) +/// +/// This test makes requests using a bunch of different HTTP methods: GET, PUT, +/// POST, and DELETE because the API makes heavy use of those; plus TRACE as a +/// sort of control to make sure we get back 405 "Method Not Allowed" for some +/// other method. (This is not really related to authorization per se, but +/// getting 405 back for TRACE lets us know that the server correctly handles +/// unsupported methods, which _is_ a security issue.) +/// +/// Endpoints usually only support a few of these methods. +/// `endpoint.allowed_methods` tells us which ones and provides request bodies +/// to use for PUT and POST requests. We always make requets for all of these +/// HTTP methods, even the unsupported ones. We expect to get back a 405 for +/// the unsupported ones. (This helps verify that we don't accidentally support +/// DELETE on a resource, for example!) +/// +/// The expected result for each resource is a little tricky: +/// - If the requested method is not allowed, we always expect 405 "Method Not +/// Allowed". +/// - If the resource is not publicly visible, then we expect a 404 for both +/// unauthenticated and unauthorized users. +/// - If the resource is publicly visible (based on `endpoint.visibility`), then +/// we expect a 401 for unauthenticated users and a 403 for unauthenticated, +/// unauthorized users. Note that "visible" here doesn't mean "accessible". +/// We assume that everybody is allowed to know that "/organizations" exists. +/// But they're not necessarily allowed to _use_ it. That's why it's correct +/// to get 401/403 on "GET /organizations", even though it's a GET and you +/// might think all GETs to things you can't access should be 404s. +/// +/// We also make requests to each resource with bogus credentials of various +/// forms to make sure they're all correctingly using the authentication +/// subsystem. +/// +/// We also make one request to GET the endpoint using a privileged user to +/// ensure that we get a 200. (If that returned 404, then there's probably some +/// other bug causing the endpoint to return a 404, and it would be wrong for us +/// to believe we correctly got a 404 for an unauthorized user because they were +/// unauthorized.) +/// +/// There are some weird cases here. For example, if you try to "POST +/// /organizations/demo-org", then you'll get back a 405, even if you can't see +/// "demo-org" (which you would normally think would result in a 404). This is +/// a little weird in that you can "learn" about what API endpoints exist. But +/// you already know that because we publish the API spec. And you can't learn +/// what _resources_ actually exist this way. async fn verify_endpoint( log: &slog::Logger, client: &ClientTestContext, endpoint: &VerifyEndpoint, ) { info!(log, "test endpoint"; "url" => endpoint.url); - let methods = - [Method::GET, Method::PUT, Method::POST, Method::DELETE, Method::TRACE]; - - // XXX Tests that we want to run - // privileged GET /url => always a 200 - // unauthenticated GET, DELETE /url - // if public, then 401; otherwise 404 - // unauthenticated PUT /url - // if has body - // if public, then 401; otherwise 404 - // else - // if public, then 405; otherwise 404 (how can this possibly work - // XXX it'll probably be 405 and that's probably okay) - // unauthenticated POST /url: same as unauthenticated PUT - // unauthenticated TRACE: always 405 - // - // unauthorized GET, DELETE /url: - // if public, then 403; otherwise 404 - // unauthorized PUT /url: - // if has body - // if public, then 403; otherwise 404 - // else - // always 405 - // + // Determine the expected status code for unauthenticated requests, based on + // the endpoint's visibility. let unauthn_status = match endpoint.visibility { - Visibility::Everyone => StatusCode::UNAUTHORIZED, + Visibility::Public => StatusCode::UNAUTHORIZED, Visibility::Protected => StatusCode::NOT_FOUND, }; + // Determine the expected status code for authenticated, unauthorized + // requests, based on the endpoint's visibility. let unauthz_status = match endpoint.visibility { - Visibility::Everyone => StatusCode::FORBIDDEN, + Visibility::Public => StatusCode::FORBIDDEN, Visibility::Protected => StatusCode::NOT_FOUND, }; - // Make one GET request as an authorized user to make sure we get a - // "200 OK" response. Otherwise, the test might later succeed by - // coincidence. We might find a 404 because of something that actually - // doesn't exist rather than something that's just hidden from - // unauthorized users. + // Make one GET request as an authorized user to make sure we get a "200 OK" + // response. Otherwise, the test might later succeed by coincidence. We + // might find a 404 because of something that actually doesn't exist rather + // than something that's just hidden from unauthorized users. NexusRequest::object_get(client, endpoint.url) .authn_as(AuthnMode::PrivilegedUser) .execute() .await .unwrap(); + // For each of the HTTP methods we use plus TRACE, we'll make several + // requests to this URL and verify the results. + let methods = + [Method::GET, Method::PUT, Method::POST, Method::DELETE, Method::TRACE]; for method in methods { let allowed = endpoint .allowed_methods @@ -282,10 +411,9 @@ async fn verify_endpoint( }; // First, make an authenticated, unauthorized request. - let expected_status = if allowed.is_none() { - StatusCode::METHOD_NOT_ALLOWED - } else { - unauthz_status + let expected_status = match allowed { + Some(_) => unauthz_status, + None => StatusCode::METHOD_NOT_ALLOWED, }; let response = NexusRequest::new( RequestBuilder::new(client, method.clone(), endpoint.url) @@ -298,11 +426,10 @@ async fn verify_endpoint( .unwrap(); verify_response(&response); - // First, let's make an unauthenticated request. - let expected_status = if allowed.is_none() { - StatusCode::METHOD_NOT_ALLOWED - } else { - unauthn_status + // Next, make an unauthenticated request. + let expected_status = match allowed { + Some(_) => unauthn_status, + None => StatusCode::METHOD_NOT_ALLOWED, }; let response = RequestBuilder::new(client, method.clone(), endpoint.url) @@ -313,24 +440,22 @@ async fn verify_endpoint( .unwrap(); verify_response(&response); - // If we provide invalid credentials altogether, we should get the same - // error as if we were unauthenticated. This is sort of duplicated by a - // test in test_authn_http() (which tests the authentication system in - // general, outside the context of Nexus). These two tests verify that - // we've correctly integrated authn with Nexus. - // - // First, try a syntactically valid authn header for a non-existent - // actor. - - let expected_status = if allowed.is_none() { - StatusCode::METHOD_NOT_ALLOWED - } else { + // Now try a few requests with bogus credentials. We should get the + // same error as if we were unauthenticated. This is sort of duplicated + // by a test in test_authn_http() (which tests the authentication system + // in general, outside the context of Nexus). This version is an + // end-to-end test. + let expected_status = match allowed { // The 401 that you get for authentication failure overrides a 404 // that you might get if you were authenticated but couldn't see the // resource in question. That is, you should always see a 401 if // you fail to authenticate, whether or not the resource exists. - StatusCode::UNAUTHORIZED + Some(_) => StatusCode::UNAUTHORIZED, + None => StatusCode::METHOD_NOT_ALLOWED, }; + + // First, try a syntactically valid authn header for a non-existent + // actor. let bad_actor_authn_header = http::HeaderValue::from_str( omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_ACTOR, ) @@ -345,7 +470,7 @@ async fn verify_endpoint( .unwrap(); verify_response(&response); - // Now try a syntactically valid authn header. + // Now try a syntactically invalid authn header. let bad_creds_authn_header = http::HeaderValue::from_str( omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_CREDS, ) @@ -362,6 +487,7 @@ async fn verify_endpoint( } } +/// Verifies the body of an HTTP response for status codes 401, 403, 404, or 405 fn verify_response(response: &TestResponse) { let error: HttpErrorResponseBody = response.parsed_body().unwrap(); match response.status { From af6b8d70bdac1342fac1a05e3f7b04b1df4bc3ba Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 31 Jan 2022 10:42:46 -0800 Subject: [PATCH 04/17] self-review: clean up --- nexus/tests/integration_tests/unauthorized.rs | 82 +++++++++---------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index d71e50d1b88..bba9645a836 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -57,6 +57,7 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { let log = &cptestctx.logctx.log; // Create test data. + info!(log, "setting up resource hierarchy"); for request in &*SETUP_REQUESTS { NexusRequest::objects_post(client, request.url, &request.body) .authn_as(AuthnMode::PrivilegedUser) @@ -66,6 +67,7 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { } // Verify the hardcoded endpoints. + info!(log, "verifying endpoints"); for endpoint in &*VERIFY_ENDPOINTS { verify_endpoint(&log, client, endpoint).await; } @@ -97,7 +99,7 @@ lazy_static! { }, // Create a Project in the Organization SetupReq { - url: "/organizations/demo-org/projects", + url: &*DEMO_ORG_PROJECTS_URL, body: serde_json::to_value(&*DEMO_PROJECT_CREATE).unwrap(), }, ]; @@ -134,47 +136,46 @@ lazy_static! { // /// Describes an API endpoint to be verified -/// -/// `url` is an HTTP path for the endpoint. (Note that we might talk about the -/// "get organization" endpoint, and might write that "GET -/// /organizations/{organization_name}". But the URL here is for a specific -/// HTTP resource, so it would look like "/organizations/demo-org" rather than -/// "/organizations/{organization_name}".) -/// -/// `visibility` specifies whether an HTTP resource handled by this endpoint is -/// visible to unauthenticated or unauthorized users. If it's -/// [`Visibility::Public`] (like "/organizations"), unauthorized users can -/// expect to get back a 401 or 403 when they attempt to access it. If it's -/// [`Visibility::Protected`] (like a specific Organization), unauthorized users -/// will get a 404. -/// -/// `allowed_methods` specifies what HTTP methods are supported for this -/// HTTP resource. The test runner tests a variety of HTTP methods. For each -/// method, if it's not in this list, we expect a 405 "Method Not Allowed" -/// response. For `PUT` and `POST`, the item in `allowed_methods` also contains -/// the contents of the body to send with the `PUT` or `POST` request. This -/// should be valid input for the endpoint. Otherwise, Nexus could choose to -/// fail with a 400-level validation error, which would obscure the authn/authz -/// error we're looking for. struct VerifyEndpoint { /// URL path for the HTTP resource to test + /// + /// Note that we might talk about the "GET organization" endpoint, and might + /// write that "GET /organizations/{organization_name}". But the URL here + /// is for a specific HTTP resource, so it would look like + /// "/organizations/demo-org" rather than + /// "/organizations/{organization_name}". url: &'static str, - /// Visibility of the HTTP resource + /// Specifies whether an HTTP resource handled by this endpoint is visible + /// to unauthenticated or unauthorized users + /// + /// If it's [`Visibility::Public`] (like "/organizations"), unauthorized + /// users can expect to get back a 401 or 403 when they attempt to access + /// it. If it's [`Visibility::Protected`] (like a specific Organization), + /// unauthorized users will get a 404. visibility: Visibility, - /// Methods that are supposed for the underlying API endpoint + /// Specifies what HTTP methods are supported for this HTTP resource + /// + /// The test runner tests a variety of HTTP methods. For each method, if + /// it's not in this list, we expect a 405 "Method Not Allowed" response. + /// For `PUT` and `POST`, the item in `allowed_methods` also contains the + /// contents of the body to send with the `PUT` or `POST` request. This + /// should be valid input for the endpoint. Otherwise, Nexus could choose + /// to fail with a 400-level validation error, which would obscure the + /// authn/authz error we're looking for. allowed_methods: Vec, } /// Describes the visibility of an HTTP resource enum Visibility { /// All users can see the resource (including unauthenticated or - /// unauthorized users). + /// unauthorized users) /// /// "/organizations" is Public, for example. Public, - /// Only users with certain privileges can see this endpoint. + + /// Only users with certain privileges can see this endpoint /// /// "/organizations/demo-org" is not public, for example. Protected, @@ -221,6 +222,8 @@ impl AllowedMethod { lazy_static! { static ref URL_USERS_DB_INIT: String = format!("/users/{}", authn::USER_DB_INIT.name); + + /// List of endpoints to be verified static ref VERIFY_ENDPOINTS: Vec = vec![ VerifyEndpoint { url: "/organizations", @@ -324,7 +327,7 @@ lazy_static! { /// /// Endpoints usually only support a few of these methods. /// `endpoint.allowed_methods` tells us which ones and provides request bodies -/// to use for PUT and POST requests. We always make requets for all of these +/// to use for PUT and POST requests. We always make requests for all of these /// HTTP methods, even the unsupported ones. We expect to get back a 405 for /// the unsupported ones. (This helps verify that we don't accidentally support /// DELETE on a resource, for example!) @@ -363,7 +366,8 @@ async fn verify_endpoint( client: &ClientTestContext, endpoint: &VerifyEndpoint, ) { - info!(log, "test endpoint"; "url" => endpoint.url); + let log = log.new(o!("url" => endpoint.url)); + info!(log, "test: begin endpoint"); // Determine the expected status code for unauthenticated requests, based on // the endpoint's visibility. @@ -383,14 +387,15 @@ async fn verify_endpoint( // response. Otherwise, the test might later succeed by coincidence. We // might find a 404 because of something that actually doesn't exist rather // than something that's just hidden from unauthorized users. + info!(log, "test: privileged GET"); NexusRequest::object_get(client, endpoint.url) .authn_as(AuthnMode::PrivilegedUser) .execute() .await .unwrap(); - // For each of the HTTP methods we use plus TRACE, we'll make several - // requests to this URL and verify the results. + // For each of the HTTP methods we use in the API as well as TRACE, we'll + // make several requests to this URL and verify the results. let methods = [Method::GET, Method::PUT, Method::POST, Method::DELETE, Method::TRACE]; for method in methods { @@ -399,18 +404,10 @@ async fn verify_endpoint( .iter() .find(|allowed| method == *allowed.http_method()); - let body = match method { - // Always supply some body for POST and PUT. If this is an allowed - // method, then it will have a body in the structure. Otherwise, - // make one up. - Method::POST | Method::PUT => allowed - .and_then(|a| a.body()) - .cloned() - .or_else(|| Some(serde_json::Value::String(String::new()))), - _ => allowed.and_then(|a| a.body()).cloned(), - }; + let body = allowed.and_then(|a| a.body()).cloned(); // First, make an authenticated, unauthorized request. + info!(log, "test: authenticated, unauthorized"; "method" => ?method); let expected_status = match allowed { Some(_) => unauthz_status, None => StatusCode::METHOD_NOT_ALLOWED, @@ -427,6 +424,7 @@ async fn verify_endpoint( verify_response(&response); // Next, make an unauthenticated request. + info!(log, "test: unauthenticated"; "method" => ?method); let expected_status = match allowed { Some(_) => unauthn_status, None => StatusCode::METHOD_NOT_ALLOWED, @@ -456,6 +454,7 @@ async fn verify_endpoint( // First, try a syntactically valid authn header for a non-existent // actor. + info!(log, "test: bogus creds: bad actor"; "method" => ?method); let bad_actor_authn_header = http::HeaderValue::from_str( omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_ACTOR, ) @@ -471,6 +470,7 @@ async fn verify_endpoint( verify_response(&response); // Now try a syntactically invalid authn header. + info!(log, "test: bogus creds: bad cred syntax"; "method" => ?method); let bad_creds_authn_header = http::HeaderValue::from_str( omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_CREDS, ) From 80b00138d27222c3bb648fe5554cf902bcf0da62 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 27 Jan 2022 13:14:40 -0800 Subject: [PATCH 05/17] authz: protect disk list endpoint --- nexus/src/db/datastore.rs | 10 ++++++---- nexus/src/external_api/http_entrypoints.rs | 2 ++ nexus/src/nexus.rs | 10 ++++++---- nexus/tests/integration_tests/disks.rs | 5 ++++- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index dbf4b154d65..92f89bc7e55 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1192,16 +1192,18 @@ impl DataStore { pub async fn project_list_disks( &self, - project_id: &Uuid, + opctx: &OpContext, + authz_project: &authz::Project, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - use db::schema::disk::dsl; + opctx.authorize(authz::Action::ListChildren, authz_project).await?; + use db::schema::disk::dsl; paginated(dsl::disk, dsl::name, &pagparams) .filter(dsl::time_deleted.is_null()) - .filter(dsl::project_id.eq(*project_id)) + .filter(dsl::project_id.eq(authz_project.id())) .select(Disk::as_select()) - .load_async::(self.pool()) + .load_async::(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 46c5915cfea..4fc7d4454e8 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -577,8 +577,10 @@ async fn project_disks_get( let organization_name = &path.organization_name; let project_name = &path.project_name; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let disks = nexus .project_list_disks( + &opctx, organization_name, project_name, &data_page_params_for(&rqctx, &query)? diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 0de77a45af8..74eb689dd93 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -665,16 +665,18 @@ impl Nexus { pub async fn project_list_disks( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let project_id = self + let authz_project = self .db_datastore .project_lookup_path(organization_name, project_name) - .await? - .id(); - self.db_datastore.project_list_disks(&project_id, pagparams).await + .await?; + self.db_datastore + .project_list_disks(opctx, &authz_project, pagparams) + .await } pub async fn project_create_disk( diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 05935ac1953..7f5baa0ce78 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -860,7 +860,10 @@ async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { } async fn disks_list(client: &ClientTestContext, list_url: &str) -> Vec { - objects_list_page::(client, list_url).await.items + NexusRequest::iter_collection_authn(client, list_url, "", 10) + .await + .expect("failed to list disks") + .all_items } fn disks_eq(disk1: &Disk, disk2: &Disk) { From 01e768198685b9bbc64b7d210d7a5b9ca526d0ff Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 31 Jan 2022 12:29:54 -0800 Subject: [PATCH 06/17] protect create too and update tests --- nexus/src/external_api/http_entrypoints.rs | 2 + nexus/src/nexus.rs | 6 + nexus/test-utils/src/resource_helpers.rs | 75 +++++++--- nexus/tests/integration_tests/disks.rs | 136 ++++-------------- nexus/tests/integration_tests/unauthorized.rs | 31 ++++ 5 files changed, 123 insertions(+), 127 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 4fc7d4454e8..54ac30f3733 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -618,8 +618,10 @@ async fn project_disks_post( let project_name = &path.project_name; let new_disk_params = &new_disk.into_inner(); let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let disk = nexus .project_create_disk( + &opctx, &organization_name, &project_name, &new_disk_params, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 74eb689dd93..bbb739d06f6 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -681,6 +681,7 @@ impl Nexus { pub async fn project_create_disk( self: &Arc, + opctx: &OpContext, organization_name: &Name, project_name: &Name, params: ¶ms::DiskCreate, @@ -690,6 +691,11 @@ impl Nexus { .project_lookup_path(organization_name, project_name) .await?; + // TODO-security This may need to be revisited once we implement authz + // checks for saga actions. Even then, though, it still will be correct + // (if possibly redundant) to check this here. + opctx.authorize(authz::Action::CreateChild, &authz_project).await?; + /* * Until we implement snapshots, do not allow disks to be created with a * snapshot id. diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 9cf091b31f4..8f1ba7df714 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -9,6 +9,8 @@ use dropshot::test_util::ClientTestContext; use dropshot::HttpErrorResponseBody; use dropshot::Method; use http::StatusCode; +use omicron_common::api::external::ByteCount; +use omicron_common::api::external::Disk; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::VpcRouter; use omicron_nexus::external_api::params; @@ -30,33 +32,50 @@ where .unwrap() } -pub async fn create_organization( +pub async fn object_create( client: &ClientTestContext, - organization_name: &str, -) -> Organization { - let input = params::OrganizationCreate { - identity: IdentityMetadataCreateParams { - name: organization_name.parse().unwrap(), - description: "an org".to_string(), - }, - }; - NexusRequest::objects_post(client, "/organizations", &input) + path: &str, + input: &InputType, +) -> OutputType +where + InputType: serde::Serialize, + OutputType: serde::de::DeserializeOwned, +{ + NexusRequest::objects_post(client, path, input) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .expect("failed to make request") + .expect("failed to make \"create\" request") .parsed_body() .unwrap() } +pub async fn create_organization( + client: &ClientTestContext, + organization_name: &str, +) -> Organization { + object_create( + client, + "/organizations", + ¶ms::OrganizationCreate { + identity: IdentityMetadataCreateParams { + name: organization_name.parse().unwrap(), + description: "an org".to_string(), + }, + }, + ) + .await +} + pub async fn create_project( client: &ClientTestContext, organization_name: &str, project_name: &str, ) -> Project { - NexusRequest::objects_post( + let url = format!("/organizations/{}/projects", &organization_name); + object_create( client, - &format!("/organizations/{}/projects", &organization_name), + &url, ¶ms::ProjectCreate { identity: IdentityMetadataCreateParams { name: project_name.parse().unwrap(), @@ -64,12 +83,32 @@ pub async fn create_project( }, }, ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() .await - .expect("failed to make request") - .parsed_body() - .unwrap() +} + +pub async fn create_disk( + client: &ClientTestContext, + organization_name: &str, + project_name: &str, + disk_name: &str, +) -> Disk { + let url = format!( + "/organizations/{}/projects/{}/disks", + organization_name, project_name + ); + object_create( + client, + &url, + ¶ms::DiskCreate { + identity: IdentityMetadataCreateParams { + name: disk_name.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + snapshot_id: None, + size: ByteCount::from_gibibytes_u32(1), + }, + ) + .await } pub async fn create_vpc( diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 7f5baa0ce78..95377c151cd 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -5,6 +5,7 @@ //! Tests basic disk support in the API use crucible_agent_client::types::State as RegionState; +use dropshot::HttpErrorResponseBody; use http::method::Method; use http::StatusCode; use omicron_common::api::external::ByteCount; @@ -30,6 +31,7 @@ use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::identity_eq; +use nexus_test_utils::resource_helpers::create_disk; use nexus_test_utils::resource_helpers::create_organization; use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils::ControlPlaneTestContext; @@ -159,15 +161,7 @@ async fn test_disk_create_attach_detach_delete( // Create a disk. let disk_url = format!("{}/{}", disks_url, DISK_NAME); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let disk: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let disk = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; assert_eq!(disk.identity.name, DISK_NAME); assert_eq!(disk.identity.description, "sells rainsticks"); assert_eq!(disk.project_id, test.project_id); @@ -324,19 +318,22 @@ async fn test_disk_create_disk_that_already_exists_fails( snapshot_id: None, size: ByteCount::from_gibibytes_u32(1), }; - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let _ = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; let disk_url = format!("{}/{}", disks_url, DISK_NAME); let disk = disk_get(&client, &disk_url).await; // Attempt to create a second disk with a conflicting name. - let error = client - .make_request_error_body( - Method::POST, - &disks_url, - new_disk, - StatusCode::BAD_REQUEST, - ) - .await; + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!("already exists: disk \"{}\"", DISK_NAME) @@ -357,15 +354,7 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { // Create a disk. let disk_url = format!("{}/{}", disks_url, DISK_NAME); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let disk: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let disk = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; // Create an instance to attach the disk. let url_instances = get_instances_url(); @@ -676,76 +665,12 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { ); } -#[nexus_test] -async fn test_disk_deletion_requires_authentication( - cptestctx: &ControlPlaneTestContext, -) { - let client = &cptestctx.external_client; - DiskTest::new(&cptestctx).await; - let disks_url = get_disks_url(); - - // Create a disk. - let disk_url = format!("{}/{}", disks_url, DISK_NAME); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; - - const BAD_DISK_NAME: &str = "wonderful-knife"; - let bad_disk_url = format!("{}/{}", disks_url, BAD_DISK_NAME); - - // If we are not authenticated, we should not be able to delete the disk. - // - // We should see the same error regardless of the existence of the disk. - let urls = [&disk_url, &bad_disk_url]; - for url in &urls { - NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - Method::DELETE, - &url, - ) - .execute() - .await - .expect("expected request to fail"); - } - - // If we are unprivileged, we should not be able to delete the disk. - // - // We should see the same error regardless of the existence of the disk. - for url in &urls { - NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - Method::DELETE, - &url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("expected request to fail"); - } - - // Privileged users can delete disks. - NexusRequest::object_delete(client, &disk_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to delete disk"); -} - #[nexus_test] async fn test_disk_creation_region_requested_then_started( cptestctx: &ControlPlaneTestContext, ) { let client = &cptestctx.external_client; let test = DiskTest::new(&cptestctx).await; - let disks_url = get_disks_url(); // Before we create a disk, set the response from the Crucible Agent: // no matter what regions get requested, they'll always *start* as @@ -768,15 +693,7 @@ async fn test_disk_creation_region_requested_then_started( // The disk is created successfully, even when this "requested" -> "started" // transition occurs. - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } // Tests that region allocation failure causes disk allocation to fail. @@ -822,14 +739,15 @@ async fn test_disk_region_creation_failure( // // TODO: Maybe consider making this a more informative error? // How should we propagate this to the client? - client - .make_request_error_body( - Method::POST, - &disks_url, - new_disk.clone(), - StatusCode::INTERNAL_SERVER_ERROR, - ) - .await; + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::INTERNAL_SERVER_ERROR)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); // After the failed allocation, the disk should not exist. let disks = disks_list(&client, &disks_url).await; @@ -852,7 +770,7 @@ async fn test_disk_region_creation_failure( test.sled_agent.get_crucible_dataset(test.zpool_id, *id).await; crucible.set_create_callback(Box::new(|_| RegionState::Created)).await; } - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let _ = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index bba9645a836..7abbdbea957 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -16,6 +16,7 @@ use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::http_testing::TestResponse; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; +use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Name; @@ -122,6 +123,8 @@ lazy_static! { static ref DEMO_PROJECT_NAME: Name = "demo-project".parse().unwrap(); static ref DEMO_PROJECT_URL: String = format!("{}/{}", *DEMO_ORG_PROJECTS_URL, *DEMO_PROJECT_NAME); + static ref DEMO_PROJECT_URL_DISKS: String = + format!("{}/disks", *DEMO_PROJECT_URL); static ref DEMO_PROJECT_CREATE: params::ProjectCreate = params::ProjectCreate { identity: IdentityMetadataCreateParams { @@ -129,6 +132,20 @@ lazy_static! { description: "".parse().unwrap(), }, }; + + // Disk used for testing + static ref DEMO_DISK_NAME: Name = "demo-disk".parse().unwrap(); + static ref DEMO_DISK_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_DISKS, *DEMO_DISK_NAME); + static ref DEMO_DISK_CREATE: params::DiskCreate = + params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_PROJECT_NAME.clone(), + description: "".parse().unwrap(), + }, + snapshot_id: None, + size: ByteCount::from_gibibytes_u32(16), + }; } // @@ -287,6 +304,20 @@ lazy_static! { ], }, + VerifyEndpoint { + url: &*DEMO_PROJECT_URL_DISKS, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_DISK_CREATE).unwrap() + ), + ], + }, + + // TODO-coverage The single disk endpoint belongs here, but we've only + // implemented authz for DELETE, not GET yet. + VerifyEndpoint { url: "/roles", visibility: Visibility::Public, From ce5057cfa1115184fd19d3172f11f77e87ddcd5c Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 31 Jan 2022 12:40:25 -0800 Subject: [PATCH 07/17] fix oxapi_demo --- tools/oxapi_demo | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/oxapi_demo b/tools/oxapi_demo index fdd1395ff5a..1851f0683c2 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -249,7 +249,7 @@ function cmd_project_list_instances function cmd_project_list_disks { [[ $# != 2 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME" - do_curl "/organizations/$1/projects/$2/disks" + do_curl_authn "/organizations/$1/projects/$2/disks" } function cmd_project_list_vpcs @@ -328,7 +328,7 @@ function cmd_disk_create_demo { [[ $# != 3 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME DISK_NAME" mkjson name="$3" description="a disk called $3" size=1024 | - do_curl "/organizations/$1/projects/$2/disks" -X POST -T - + do_curl_authn "/organizations/$1/projects/$2/disks" -X POST -T - } function cmd_disk_get From b2c731586c007023b04b0950dbfc2a76012f8a31 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 31 Jan 2022 15:36:21 -0800 Subject: [PATCH 08/17] lookup_path -> lookup_by_path --- nexus/src/db/datastore.rs | 8 ++++---- nexus/src/nexus.rs | 36 ++++++++++++++++++------------------ 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index bae4e4bfcae..e2a6e3abbcb 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -567,7 +567,7 @@ impl DataStore { /// /// This function does no authz checks because it is not possible to know /// just by looking up an Organization's id what privileges are required. - pub async fn organization_lookup_path( + pub async fn organization_lookup_by_path( &self, name: &Name, ) -> LookupResult { @@ -701,7 +701,7 @@ impl DataStore { ) -> UpdateResult { use db::schema::organization::dsl; - let authz_org = self.organization_lookup_path(name).await?; + let authz_org = self.organization_lookup_by_path(name).await?; opctx.authorize(authz::Action::Modify, &authz_org).await?; diesel::update(dsl::organization) @@ -795,13 +795,13 @@ impl DataStore { /// /// This function does no authz checks because it is not possible to know /// just by looking up an Project's id what privileges are required. - pub async fn project_lookup_path( + pub async fn project_lookup_by_path( &self, organization_name: &Name, project_name: &Name, ) -> LookupResult { let authz_org = - self.organization_lookup_path(organization_name).await?; + self.organization_lookup_by_path(organization_name).await?; self.project_lookup_noauthz(&authz_org, project_name) .await .map(|(p, _)| p) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index bbb739d06f6..963184c21d2 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -549,7 +549,7 @@ impl Nexus { ) -> CreateResult { let org = self .db_datastore - .organization_lookup_path(organization_name) + .organization_lookup_by_path(organization_name) .await?; // Create a project. @@ -591,7 +591,7 @@ impl Nexus { ) -> LookupResult { let authz_org = self .db_datastore - .organization_lookup_path(organization_name) + .organization_lookup_by_path(organization_name) .await?; Ok(self .db_datastore @@ -608,7 +608,7 @@ impl Nexus { ) -> ListResultVec { let authz_org = self .db_datastore - .organization_lookup_path(organization_name) + .organization_lookup_by_path(organization_name) .await?; self.db_datastore .projects_list_by_name(opctx, &authz_org, pagparams) @@ -623,7 +623,7 @@ impl Nexus { ) -> ListResultVec { let authz_org = self .db_datastore - .organization_lookup_path(organization_name) + .organization_lookup_by_path(organization_name) .await?; self.db_datastore .projects_list_by_id(opctx, &authz_org, pagparams) @@ -638,7 +638,7 @@ impl Nexus { ) -> DeleteResult { let authz_project = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await?; self.db_datastore.project_delete(opctx, &authz_project).await } @@ -652,7 +652,7 @@ impl Nexus { ) -> UpdateResult { let authz_project = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await?; self.db_datastore .project_update(opctx, &authz_project, new_params.clone().into()) @@ -672,7 +672,7 @@ impl Nexus { ) -> ListResultVec { let authz_project = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await?; self.db_datastore .project_list_disks(opctx, &authz_project, pagparams) @@ -688,7 +688,7 @@ impl Nexus { ) -> CreateResult { let authz_project = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await?; // TODO-security This may need to be revisited once we implement authz @@ -734,7 +734,7 @@ impl Nexus { ) -> LookupResult<(db::model::Disk, authz::ProjectChild)> { let authz_project = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await?; let disk = self .db_datastore @@ -819,7 +819,7 @@ impl Nexus { ) -> ListResultVec { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); self.db_datastore.project_list_instances(&project_id, pagparams).await @@ -833,7 +833,7 @@ impl Nexus { ) -> CreateResult { let authz_project = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await?; let saga_params = Arc::new(sagas::ParamsInstanceCreate { @@ -917,7 +917,7 @@ impl Nexus { */ let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); let instance = self @@ -936,7 +936,7 @@ impl Nexus { ) -> UpdateResult { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); let instance = self @@ -971,7 +971,7 @@ impl Nexus { ) -> LookupResult { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); self.db_datastore @@ -1573,7 +1573,7 @@ impl Nexus { ) -> ListResultVec { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); let vpcs = @@ -1589,7 +1589,7 @@ impl Nexus { ) -> CreateResult { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); let vpc_id = Uuid::new_v4(); @@ -1698,7 +1698,7 @@ impl Nexus { ) -> LookupResult { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); Ok(self.db_datastore.vpc_fetch_by_name(&project_id, vpc_name).await?) @@ -1713,7 +1713,7 @@ impl Nexus { ) -> UpdateResult<()> { let project_id = self .db_datastore - .project_lookup_path(organization_name, project_name) + .project_lookup_by_path(organization_name, project_name) .await? .id(); let vpc = From c6d1d2b2fc301de85e08b6bcd53dab98ee433cbf Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 1 Feb 2022 14:10:21 -0800 Subject: [PATCH 09/17] first (rough) cut: authz for disk lookup in the datastore --- nexus/src/authn/mod.rs | 13 +- nexus/src/authz/api_resources.rs | 7 +- nexus/src/authz/mod.rs | 3 +- nexus/src/context.rs | 77 +++-- nexus/src/db/datastore.rs | 263 +++++++++++++++--- nexus/src/db/fixed_data/user_builtin.rs | 10 + nexus/src/external_api/http_entrypoints.rs | 9 +- nexus/src/internal_api/http_entrypoints.rs | 4 +- nexus/src/nexus.rs | 160 ++++++----- nexus/src/sagas.rs | 13 +- .../tests/integration_tests/users_builtin.rs | 2 + tools/oxapi_demo | 4 +- 12 files changed, 413 insertions(+), 152 deletions(-) diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index af42013f5db..fc5661866e7 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -27,6 +27,7 @@ pub mod external; pub use crate::db::fixed_data::user_builtin::USER_DB_INIT; +pub use crate::db::fixed_data::user_builtin::USER_INTERNAL_API; pub use crate::db::fixed_data::user_builtin::USER_SAGA_RECOVERY; pub use crate::db::fixed_data::user_builtin::USER_TEST_PRIVILEGED; pub use crate::db::fixed_data::user_builtin::USER_TEST_UNPRIVILEGED; @@ -82,6 +83,11 @@ impl Context { Context { kind: Kind::Unauthenticated, schemes_tried: vec![] } } + /// Returns an authenticated context for handling internal API contexts + pub fn internal_api() -> Context { + Context::context_for_actor(USER_INTERNAL_API.id) + } + /// Returns an authenticated context for saga recovery pub fn internal_saga_recovery() -> Context { Context::context_for_actor(USER_SAGA_RECOVERY.id) @@ -101,7 +107,6 @@ impl Context { } /// Returns an authenticated context for a special testing user - #[cfg(test)] pub fn internal_test_user() -> Context { Context::test_context_for_actor(USER_TEST_PRIVILEGED.id) } @@ -109,7 +114,6 @@ impl Context { /// Returns an authenticated context for a specific user /// /// This is used for unit testing the authorization rules. - #[cfg(test)] pub fn test_context_for_actor(actor_id: Uuid) -> Context { Context::context_for_actor(actor_id) } @@ -119,6 +123,7 @@ impl Context { mod test { use super::Context; use super::USER_DB_INIT; + use super::USER_INTERNAL_API; use super::USER_SAGA_RECOVERY; use super::USER_TEST_PRIVILEGED; @@ -142,6 +147,10 @@ mod test { let authn = Context::internal_saga_recovery(); let actor = authn.actor().unwrap(); assert_eq!(actor.0, USER_SAGA_RECOVERY.id); + + let authn = Context::internal_api(); + let actor = authn.actor().unwrap(); + assert_eq!(actor.0, USER_INTERNAL_API.id); } } diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index 93196437a61..bafeb11c1bd 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -432,8 +432,8 @@ pub struct ProjectChild { } impl ProjectChild { - pub fn id(&self) -> &Uuid { - &self.resource_id + pub fn id(&self) -> Uuid { + self.resource_id } } @@ -472,3 +472,6 @@ impl ApiResourceError for ProjectChild { self.lookup_type.clone().into_not_found(self.resource_type) } } + +pub type Disk = ProjectChild; +pub type Instance = ProjectChild; diff --git a/nexus/src/authz/mod.rs b/nexus/src/authz/mod.rs index a77205dbf92..27d25396df6 100644 --- a/nexus/src/authz/mod.rs +++ b/nexus/src/authz/mod.rs @@ -164,11 +164,12 @@ mod actor; mod api_resources; pub use api_resources::ApiResourceError; +pub use api_resources::Disk; pub use api_resources::Fleet; pub use api_resources::FleetChild; +pub use api_resources::Instance; pub use api_resources::Organization; pub use api_resources::Project; -pub use api_resources::ProjectChild; pub use api_resources::FLEET; mod context; diff --git a/nexus/src/context.rs b/nexus/src/context.rs index db2e92066d8..d94957192d6 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -43,6 +43,8 @@ pub struct ServerContext { pub log: Logger, /** authenticator for external HTTP requests */ pub external_authn: authn::external::Authenticator>, + /** authentication context used for internal HTTP requests */ + pub internal_authn: Arc, /** authorizer */ pub authz: Arc, /** internal API request latency tracker */ @@ -91,6 +93,7 @@ impl ServerContext { }) .collect(); let external_authn = authn::external::Authenticator::new(nexus_schemes); + let internal_authn = Arc::new(authn::Context::internal_api()); let authz = Arc::new(authz::Authz::new()); let create_tracker = |name: &str| { let target = HttpService { name: name.to_string(), id: config.id }; @@ -145,6 +148,7 @@ impl ServerContext { ), log, external_authn, + internal_authn, authz, internal_latencies, external_latencies, @@ -196,9 +200,10 @@ pub struct OpContext { enum OpKind { /// Handling an external API request ExternalApiRequest, + /// Handling an internal API request + InternalApiRequest, /// Background operations in Nexus Background, - #[cfg(test)] /// Unit tests UnitTest, } @@ -220,12 +225,55 @@ impl OpContext { datastore, ); - let request = rqctx.request.lock().await; + let (log, metadata) = + OpContext::log_and_metadata_for_request(rqctx, &authn).await; + + Ok(OpContext { + log, + authz, + authn, + created_instant, + created_walltime, + metadata, + kind: OpKind::ExternalApiRequest, + }) + } + + /// Returns a context suitable for use in handling internal API operations + // TODO-security this should eventually do some kind of authentication + pub async fn for_internal_api( + rqctx: &dropshot::RequestContext>, + ) -> OpContext { + let created_instant = Instant::now(); + let created_walltime = SystemTime::now(); + let apictx = rqctx.context(); + let authn = Arc::clone(&apictx.internal_authn); + let datastore = Arc::clone(apictx.nexus.datastore()); + let authz = authz::Context::new( + Arc::clone(&authn), + Arc::clone(&apictx.authz), + datastore, + ); + + let (log, metadata) = + OpContext::log_and_metadata_for_request(rqctx, &authn).await; + + OpContext { + log, + authz, + authn, + created_instant, + created_walltime, + metadata, + kind: OpKind::InternalApiRequest, + } + } + + async fn log_and_metadata_for_request( + rqctx: &dropshot::RequestContext, + authn: &authn::Context, + ) -> (slog::Logger, BTreeMap) { let mut metadata = BTreeMap::new(); - metadata.insert(String::from("request_id"), rqctx.request_id.clone()); - metadata - .insert(String::from("http_method"), request.method().to_string()); - metadata.insert(String::from("http_uri"), request.uri().to_string()); let log = if let Some(Actor(actor_id)) = authn.actor() { metadata @@ -240,15 +288,13 @@ impl OpContext { rqctx.log.new(o!("authenticated" => false)) }; - Ok(OpContext { - log, - authz, - authn, - created_instant, - created_walltime, - metadata, - kind: OpKind::ExternalApiRequest, - }) + let request = rqctx.request.lock().await; + metadata.insert(String::from("request_id"), rqctx.request_id.clone()); + metadata + .insert(String::from("http_method"), request.method().to_string()); + metadata.insert(String::from("http_uri"), request.uri().to_string()); + + (log, metadata) } /// Returns a context suitable for use in background operations in Nexus @@ -279,7 +325,6 @@ impl OpContext { /// Returns a context suitable for automated unit tests where an OpContext /// is needed outside of a Dropshot context - #[cfg(test)] pub fn for_unit_tests( log: slog::Logger, datastore: Arc, diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index e2a6e3abbcb..1f3046da9c0 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2,6 +2,15 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +// XXX TODO in this change +// - update "unauthorized" test +// - make sure we have coverage for the case where you try to attach or detach +// an Instance when not authorized. Make sure we actually check if a change +// was made!! (Should the rest of the "unauthorized.rs" tests also try to +// assess this? Maybe do a GET after doing all the other things and make sure +// nothing changed? Maybe do that and also have the PUT bodies actually make +// a change?) + /*! * Primary control plane interface for database read and write operations */ @@ -561,6 +570,33 @@ impl DataStore { }) } + /// Fetch an [`authz::Organization`] based on its id + pub async fn organization_lookup_by_id( + &self, + organization_id: Uuid, + ) -> LookupResult { + use db::schema::organization::dsl; + // We only do this database lookup to verify that the Organization with + // this id exists and hasn't been deleted. + let _: Uuid = dsl::organization + .filter(dsl::time_deleted.is_null()) + .filter(dsl::id.eq(organization_id)) + .select(dsl::id) + .first_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Organization, + LookupType::ById(organization_id), + ), + ) + })?; + Ok(authz::FLEET + .organization(organization_id, LookupType::ById(organization_id))) + } + /// Look up the id for an organization based on its name /// /// Returns an [`authz::Organization`] (which makes the id available). @@ -789,6 +825,32 @@ impl DataStore { }) } + /// Fetch an [`authz::Project`] based on its id + pub async fn project_lookup_by_id( + &self, + project_id: Uuid, + ) -> LookupResult { + use db::schema::project::dsl; + let organization_id = dsl::project + .filter(dsl::time_deleted.is_null()) + .filter(dsl::id.eq(project_id)) + .select(dsl::organization_id) + .first_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Project, + LookupType::ById(project_id), + ), + ) + })?; + let authz_organization = + self.organization_lookup_by_id(organization_id).await?; + Ok(authz_organization.project(project_id, LookupType::ById(project_id))) + } + /// Look up the id for a Project based on its name /// /// Returns an [`authz::Project`] (which makes the id available). @@ -814,10 +876,10 @@ impl DataStore { authz_org: &authz::Organization, name: &Name, ) -> LookupResult<(authz::Project, Project)> { - let (authz_org, db_org) = + let (authz_project, db_project) = self.project_lookup_noauthz(authz_org, name).await?; - opctx.authorize(authz::Action::Read, &authz_org).await?; - Ok((authz_org, db_org)) + opctx.authorize(authz::Action::Read, &authz_project).await?; + Ok((authz_project, db_project)) } /// Delete a project @@ -1143,6 +1205,114 @@ impl DataStore { * Disks */ + /// Fetches a Disk from the database and returns both the database row + /// and an [`authz::Disk`] for doing authz checks + /// + /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases + /// and caveats. + // TODO-security See the note on organization_lookup_noauthz(). + async fn disk_lookup_noauthz( + &self, + authz_project: &authz::Project, + disk_name: &Name, + ) -> LookupResult<(authz::Disk, Disk)> { + use db::schema::disk::dsl; + dsl::disk + .filter(dsl::time_deleted.is_null()) + .filter(dsl::project_id.eq(authz_project.id())) + .filter(dsl::name.eq(disk_name.clone())) + .select(Disk::as_select()) + .first_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Disk, + LookupType::ByName(disk_name.as_str().to_owned()), + ), + ) + }) + .map(|d| { + ( + authz_project.child_generic( + ResourceType::Disk, + d.id(), + LookupType::from(&disk_name.0), + ), + d, + ) + }) + } + + /// Fetch an [`authz::Disk`] based on its id + pub async fn disk_lookup_by_id( + &self, + disk_id: Uuid, + ) -> LookupResult { + use db::schema::disk::dsl; + let project_id = dsl::disk + .filter(dsl::time_deleted.is_null()) + .filter(dsl::id.eq(disk_id)) + .select(dsl::project_id) + .first_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Disk, + LookupType::ById(disk_id), + ), + ) + })?; + let authz_project = self.project_lookup_by_id(project_id).await?; + Ok(authz_project.child_generic( + ResourceType::Disk, + disk_id, + LookupType::ById(disk_id), + )) + } + + /// Look up the id for a Disk based on its name + /// + /// Returns an [`authz::Disk`] (which makes the id available). + /// + /// Like the other "lookup_by_path()" functions, this function does no authz + /// checks. + // TODO-security For Containers in the hierarchy (like Organizations and + // Projects), we don't do an authz check in the "lookup_by_path" functions + // because we don't know if the caller has access to do the lookup. For + // leaf resources (like Instances and Disks), though, we do. We could do + // the authz check here, and in disk_fetch_by_id() too. Should we? + pub async fn disk_lookup_by_path( + &self, + organization_name: &Name, + project_name: &Name, + disk_name: &Name, + ) -> LookupResult { + let authz_project = self + .project_lookup_by_path(organization_name, project_name) + .await?; + self.disk_lookup_noauthz(&authz_project, disk_name) + .await + .map(|(d, _)| d) + } + + /// Lookup a Disk by name and return the full database record, along with + /// an [`authz::Disk`] for subsequent authorization checks + pub async fn disk_fetch( + &self, + opctx: &OpContext, + authz_project: &authz::Project, + name: &Name, + ) -> LookupResult<(authz::Disk, Disk)> { + let (authz_disk, db_disk) = + self.disk_lookup_noauthz(authz_project, name).await?; + opctx.authorize(authz::Action::Read, &authz_disk).await?; + Ok((authz_disk, db_disk)) + } + /** * List disks associated with a given instance. */ @@ -1213,19 +1383,23 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - pub async fn disk_update_runtime( + /// See `disk_update_runtime()`. This version should only be used from + /// sagas, which do not currently have authn contexts. + // TODO-security Like project_delete_disk_no_auth(), this should be removed + // once we have support for authz checks from sagas. + pub async fn disk_update_runtime_no_auth( &self, - disk_id: &Uuid, + authz_disk: &authz::Disk, new_runtime: &DiskRuntimeState, ) -> Result { + let disk_id = authz_disk.id(); use db::schema::disk::dsl; - let updated = diesel::update(dsl::disk) .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(*disk_id)) + .filter(dsl::id.eq(disk_id)) .filter(dsl::state_generation.lt(new_runtime.gen)) .set(new_runtime.clone()) - .check_if_exists::(*disk_id) + .check_if_exists::(disk_id) .execute_and_check(self.pool()) .await .map(|r| match r.status { @@ -1235,57 +1409,61 @@ impl DataStore { .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::Disk, - LookupType::ById(*disk_id), - ), + ErrorHandler::NotFoundByResource(authz_disk), ) })?; Ok(updated) } - pub async fn disk_fetch(&self, disk_id: &Uuid) -> LookupResult { - use db::schema::disk::dsl; - - dsl::disk - .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(*disk_id)) - .select(Disk::as_select()) - .get_result_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Disk, - LookupType::ById(*disk_id), - ), - ) - }) - } - - pub async fn disk_fetch_by_name( + pub async fn disk_update_runtime( &self, - project_id: &Uuid, - disk_name: &Name, + opctx: &OpContext, + authz_disk: &authz::Disk, + new_runtime: &DiskRuntimeState, + ) -> Result { + // TODO-security This permission might be overloaded here. The way disk + // runtime updates work is that the caller in Nexus first updates the + // Sled Agent to make a change, then updates to the database to reflect + // that change. So by the time we get here, we better have already done + // an authz check, or we will have already made some unauthorized change + // to the system! At the same time, we don't want just anybody to be + // able to modify the database state. So we _do_ still want an authz + // check here. Arguably it's for a different kind of action, but it + // doesn't seem that useful to split it out right now. + opctx.authorize(authz::Action::Modify, authz_disk).await?; + self.disk_update_runtime_no_auth(authz_disk, new_runtime).await + } + + /// Fetches information about a Disk that the caller has previously fetched + /// + /// The principal difference from `disk_fetch` is that this function takes + /// an `authz_disk` and does a lookup by id rather than the full path of + /// names (organization name, project name, and disk name). This could be + /// called `disk_lookup_by_id`, except that you might expect to get back an + /// `authz::Disk` as well. We cannot return you a new `authz::Disk` because + /// we don't know how you looked up the Disk in the first place. However, + /// you must have previously looked it up, which is why we call this + /// `refetch`. + pub async fn disk_refetch( + &self, + opctx: &OpContext, + authz_disk: &authz::Disk, ) -> LookupResult { use db::schema::disk::dsl; + opctx.authorize(authz::Action::Read, authz_disk).await?; + dsl::disk .filter(dsl::time_deleted.is_null()) - .filter(dsl::project_id.eq(*project_id)) - .filter(dsl::name.eq(disk_name.clone())) + .filter(dsl::id.eq(authz_disk.id())) .select(Disk::as_select()) - .get_result_async(self.pool()) + .get_result_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::Disk, - LookupType::ByName(disk_name.as_str().to_owned()), - ), + ErrorHandler::NotFoundByResource(authz_disk), ) }) } @@ -2482,6 +2660,7 @@ impl DataStore { let builtin_users = [ // Note: "db_init" is also a builtin user, but that one by necessity // is created with the database. + &*authn::USER_INTERNAL_API, &*authn::USER_SAGA_RECOVERY, &*authn::USER_TEST_PRIVILEGED, &*authn::USER_TEST_UNPRIVILEGED, diff --git a/nexus/src/db/fixed_data/user_builtin.rs b/nexus/src/db/fixed_data/user_builtin.rs index 4bdb23c2a06..ebe65a295ab 100644 --- a/nexus/src/db/fixed_data/user_builtin.rs +++ b/nexus/src/db/fixed_data/user_builtin.rs @@ -39,6 +39,14 @@ lazy_static! { "used for seeding initial database data", ); + /// Internal user used by Nexus when handling internal API requests + pub static ref USER_INTERNAL_API: UserBuiltinConfig = + UserBuiltinConfig::new_static( + "001de000-05e4-4000-8000-000000000002", + "internal-api", + "used by Nexus when handling internal API requests", + ); + /// Internal user used by Nexus when recovering sagas pub static ref USER_SAGA_RECOVERY: UserBuiltinConfig = UserBuiltinConfig::new_static( @@ -74,6 +82,7 @@ lazy_static! { mod test { use super::super::assert_valid_uuid; use super::USER_DB_INIT; + use super::USER_INTERNAL_API; use super::USER_SAGA_RECOVERY; use super::USER_TEST_PRIVILEGED; use super::USER_TEST_UNPRIVILEGED; @@ -81,6 +90,7 @@ mod test { #[test] fn test_builtin_user_ids_are_valid() { assert_valid_uuid(&USER_DB_INIT.id); + assert_valid_uuid(&USER_INTERNAL_API.id); assert_valid_uuid(&USER_SAGA_RECOVERY.id); assert_valid_uuid(&USER_TEST_PRIVILEGED.id); assert_valid_uuid(&USER_TEST_UNPRIVILEGED.id); diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 54ac30f3733..d1a4455d2ad 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -661,8 +661,9 @@ async fn project_disks_get_disk( let project_name = &path.project_name; let disk_name = &path.disk_name; let handler = async { - let (disk, _) = nexus - .project_lookup_disk(&organization_name, &project_name, &disk_name) + let opctx = OpContext::for_external_api(&rqctx).await?; + let disk = nexus + .disk_fetch(&opctx, &organization_name, &project_name, &disk_name) .await?; Ok(HttpResponseOk(disk.into())) }; @@ -1027,8 +1028,10 @@ async fn instance_disks_attach( let project_name = &path.project_name; let instance_name = &path.instance_name; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let disk = nexus .instance_attach_disk( + &opctx, &organization_name, &project_name, &instance_name, @@ -1057,8 +1060,10 @@ async fn instance_disks_detach( let project_name = &path.project_name; let instance_name = &path.instance_name; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let disk = nexus .instance_detach_disk( + &opctx, &organization_name, &project_name, &instance_name, diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index 92706dc279f..edf97b34273 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -2,6 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use crate::context::OpContext; /** * Handler functions (entrypoints) for HTTP APIs internal to the control plane */ @@ -206,7 +207,8 @@ async fn cpapi_disks_put( let path = path_params.into_inner(); let new_state = new_runtime_state.into_inner(); let handler = async { - nexus.notify_disk_updated(&path.disk_id, &new_state).await?; + let opctx = OpContext::for_internal_api(&rqctx).await; + nexus.notify_disk_updated(&opctx, path.disk_id, &new_state).await?; Ok(HttpResponseUpdatedNoContent()) }; apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 963184c21d2..6532a1eecaa 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -33,7 +33,6 @@ use omicron_common::api::external; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; -use omicron_common::api::external::Disk; use omicron_common::api::external::DiskState; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; @@ -43,7 +42,6 @@ use omicron_common::api::external::Ipv6Net; use omicron_common::api::external::ListResult; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; -use omicron_common::api::external::LookupType; use omicron_common::api::external::PaginationOrder; use omicron_common::api::external::ResourceType; use omicron_common::api::external::RouteDestination; @@ -726,29 +724,22 @@ impl Nexus { Ok(disk_created) } - pub async fn project_lookup_disk( + pub async fn disk_fetch( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, disk_name: &Name, - ) -> LookupResult<(db::model::Disk, authz::ProjectChild)> { + ) -> LookupResult { let authz_project = self .db_datastore .project_lookup_by_path(organization_name, project_name) .await?; - let disk = self + Ok(self .db_datastore - .disk_fetch_by_name(&authz_project.id(), disk_name) - .await?; - let disk_id = disk.id(); - Ok(( - disk, - authz_project.child_generic( - ResourceType::Disk, - disk_id, - LookupType::from(&disk_name.0), - ), - )) + .disk_fetch(opctx, &authz_project, disk_name) + .await? + .1) } pub async fn project_delete_disk( @@ -758,8 +749,9 @@ impl Nexus { project_name: &Name, disk_name: &Name, ) -> DeleteResult { - let (disk, authz_disk) = self - .project_lookup_disk(organization_name, project_name, disk_name) + let authz_disk = self + .db_datastore + .disk_lookup_by_path(organization_name, project_name, disk_name) .await?; // TODO: We need to sort out the authorization checks. @@ -771,7 +763,7 @@ impl Nexus { opctx.authorize(authz::Action::Delete, &authz_disk).await?; let saga_params = - Arc::new(sagas::ParamsDiskDelete { disk_id: disk.id() }); + Arc::new(sagas::ParamsDiskDelete { disk_id: authz_disk.id() }); self.execute_saga( Arc::clone(&sagas::SAGA_DISK_DELETE_TEMPLATE), sagas::SAGA_DISK_DELETE_NAME, @@ -1271,42 +1263,12 @@ impl Nexus { self.db_datastore.instance_list_disks(&instance.id(), pagparams).await } - /** - * Fetch information about whether this disk is attached to this instance. - */ - pub async fn instance_get_disk( - &self, - organization_name: &Name, - project_name: &Name, - instance_name: &Name, - disk_name: &Name, - ) -> LookupResult { - let instance = self - .project_lookup_instance( - organization_name, - project_name, - instance_name, - ) - .await?; - // TODO: This shouldn't be looking up multiple database entries by name, - // it should resolve names to IDs first. - let (disk, _) = self - .project_lookup_disk(organization_name, project_name, disk_name) - .await?; - if let Some(instance_id) = disk.runtime_state.attach_instance_id { - if instance_id == instance.id() { - return Ok(disk.into()); - } - } - - Err(Error::not_found_by_name(ResourceType::Disk, disk_name)) - } - /** * Attach a disk to an instance. */ pub async fn instance_attach_disk( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, instance_name: &Name, @@ -1321,8 +1283,13 @@ impl Nexus { .await?; // TODO: This shouldn't be looking up multiple database entries by name, // it should resolve names to IDs first. - let (disk, _) = self - .project_lookup_disk(organization_name, project_name, disk_name) + let authz_project = self + .db_datastore + .project_lookup_by_path(organization_name, project_name) + .await?; + let (authz_disk, db_disk) = self + .db_datastore + .disk_fetch(opctx, &authz_project, disk_name) .await?; let instance_id = &instance.id(); @@ -1359,12 +1326,14 @@ impl Nexus { Err(Error::InvalidRequest { message }) } - match &disk.state().into() { + match &db_disk.state().into() { /* * If we're already attaching or attached to the requested instance, * there's nothing else to do. + * TODO-security should it be an error if you're not authorized to + * do this and we did not actually have to do anything? */ - DiskState::Attached(id) if id == instance_id => return Ok(disk), + DiskState::Attached(id) if id == instance_id => return Ok(db_disk), /* * If the disk is currently attaching or attached to another @@ -1377,19 +1346,19 @@ impl Nexus { */ DiskState::Attached(id) => { assert_ne!(id, instance_id); - return disk_attachment_error(&disk); + return disk_attachment_error(&db_disk); } DiskState::Detaching(_) => { - return disk_attachment_error(&disk); + return disk_attachment_error(&db_disk); } DiskState::Attaching(id) if id != instance_id => { - return disk_attachment_error(&disk); + return disk_attachment_error(&db_disk); } DiskState::Destroyed => { - return disk_attachment_error(&disk); + return disk_attachment_error(&db_disk); } DiskState::Faulted => { - return disk_attachment_error(&disk); + return disk_attachment_error(&db_disk); } DiskState::Creating => (), @@ -1400,14 +1369,16 @@ impl Nexus { } self.disk_set_runtime( - &disk, + opctx, + &authz_disk, + &db_disk, self.instance_sled(&instance).await?, sled_agent_client::types::DiskStateRequested::Attached( *instance_id, ), ) .await?; - self.db_datastore.disk_fetch(&disk.id()).await + self.db_datastore.disk_refetch(opctx, &authz_disk).await } /** @@ -1415,6 +1386,7 @@ impl Nexus { */ pub async fn instance_detach_disk( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, instance_name: &Name, @@ -1429,21 +1401,30 @@ impl Nexus { .await?; // TODO: This shouldn't be looking up multiple database entries by name, // it should resolve names to IDs first. - let (disk, _) = self - .project_lookup_disk(organization_name, project_name, disk_name) + let authz_project = self + .db_datastore + .project_lookup_by_path(organization_name, project_name) + .await?; + let (authz_disk, db_disk) = self + .db_datastore + .disk_fetch(opctx, &authz_project, disk_name) .await?; let instance_id = &instance.id(); - match &disk.state().into() { + match &db_disk.state().into() { /* * This operation is a noop if the disk is not attached or already * detaching from the same instance. + * TODO-security should it be an error if you're not authorized to + * do this and we did not actually have to do anything? */ - DiskState::Creating => return Ok(disk), - DiskState::Detached => return Ok(disk), - DiskState::Destroyed => return Ok(disk), - DiskState::Faulted => return Ok(disk), - DiskState::Detaching(id) if id == instance_id => return Ok(disk), + DiskState::Creating => return Ok(db_disk), + DiskState::Detached => return Ok(db_disk), + DiskState::Destroyed => return Ok(db_disk), + DiskState::Faulted => return Ok(db_disk), + DiskState::Detaching(id) if id == instance_id => { + return Ok(db_disk) + } /* * This operation is not allowed if the disk is attached to some @@ -1471,12 +1452,17 @@ impl Nexus { } self.disk_set_runtime( - &disk, + opctx, + &authz_disk, + &db_disk, self.instance_sled(&instance).await?, sled_agent_client::types::DiskStateRequested::Detached, ) .await?; - Ok(disk) + // XXX it seems like there was a bug here where we would report the old + // state ("attached") instead of the new one ("detaching"). If not, I'm + // missing something about how this used to work. + self.db_datastore.disk_refetch(opctx, &authz_disk).await } /** @@ -1485,19 +1471,23 @@ impl Nexus { */ async fn disk_set_runtime( &self, - disk: &db::model::Disk, + opctx: &OpContext, + authz_disk: &authz::Disk, + db_disk: &db::model::Disk, sa: Arc, requested: sled_agent_client::types::DiskStateRequested, ) -> Result<(), Error> { - let runtime: DiskRuntimeState = disk.runtime().into(); + let runtime: DiskRuntimeState = db_disk.runtime().into(); + + opctx.authorize(authz::Action::Modify, authz_disk).await?; /* - * Ask the SA to begin the state change. Then update the database to - * reflect the new intermediate state. + * Ask the Sled Agent to begin the state change. Then update the + * database to reflect the new intermediate state. */ let new_runtime = sa .disk_put( - &disk.id(), + &authz_disk.id(), &sled_agent_client::types::DiskEnsureBody { initial_runtime: sled_agent_client::types::DiskRuntimeState::from( @@ -1512,7 +1502,7 @@ impl Nexus { let new_runtime: DiskRuntimeState = new_runtime.into(); self.db_datastore - .disk_update_runtime(&disk.id(), &new_runtime.into()) + .disk_update_runtime(opctx, authz_disk, &new_runtime.into()) .await .map(|_| ()) } @@ -2324,14 +2314,16 @@ impl Nexus { pub async fn notify_disk_updated( &self, - id: &Uuid, + opctx: &OpContext, + id: Uuid, new_state: &DiskRuntimeState, ) -> Result<(), Error> { let log = &self.log; + let authz_disk = self.db_datastore.disk_lookup_by_id(id).await?; let result = self .db_datastore - .disk_update_runtime(id, &new_state.clone().into()) + .disk_update_runtime(opctx, &authz_disk, &new_state.clone().into()) .await; /* TODO-cleanup commonize with notify_instance_updated() */ @@ -2504,8 +2496,14 @@ impl TestInterfaces for Nexus { &self, id: &Uuid, ) -> Result, Error> { - let disk = self.db_datastore.disk_fetch(id).await?; - let instance_id = disk.runtime().attach_instance_id.unwrap(); + let opctx = OpContext::for_unit_tests( + self.log.new(o!()), + Arc::clone(&self.db_datastore), + ); + let authz_disk = self.db_datastore.disk_lookup_by_id(*id).await?; + let db_disk = + self.db_datastore.disk_refetch(&opctx, &authz_disk).await?; + let instance_id = db_disk.runtime().attach_instance_id.unwrap(); let instance = self.db_datastore.instance_fetch(&instance_id).await?; self.instance_sled(&instance).await } diff --git a/nexus/src/sagas.rs b/nexus/src/sagas.rs index 7a30f34cfdb..db5110d4a08 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -740,12 +740,19 @@ async fn sdc_finalize_disk_record( ) -> Result<(), ActionError> { let osagactx = sagactx.user_data(); let _params = sagactx.saga_params(); + let datastore = osagactx.datastore(); let disk_id = sagactx.lookup::("disk_id")?; let disk_created = sagactx.lookup::("created_disk")?; - osagactx - .datastore() - .disk_update_runtime(&disk_id, &disk_created.runtime().detach()) + let authz_disk = datastore + .disk_lookup_by_id(disk_id) + .await + .map_err(ActionError::action_failed)?; + datastore + .disk_update_runtime_no_auth( + &authz_disk, + &disk_created.runtime().detach(), + ) .await .map_err(ActionError::action_failed)?; Ok(()) diff --git a/nexus/tests/integration_tests/users_builtin.rs b/nexus/tests/integration_tests/users_builtin.rs index 50f737f4dfa..7796d60ad33 100644 --- a/nexus/tests/integration_tests/users_builtin.rs +++ b/nexus/tests/integration_tests/users_builtin.rs @@ -27,6 +27,8 @@ async fn test_users_builtin(cptestctx: &ControlPlaneTestContext) { let u = users.remove(&authn::USER_DB_INIT.name.to_string()).unwrap(); assert_eq!(u.identity.id, authn::USER_DB_INIT.id); + let u = users.remove(&authn::USER_INTERNAL_API.name.to_string()).unwrap(); + assert_eq!(u.identity.id, authn::USER_INTERNAL_API.id); let u = users.remove(&authn::USER_SAGA_RECOVERY.name.to_string()).unwrap(); assert_eq!(u.identity.id, authn::USER_SAGA_RECOVERY.id); let u = diff --git a/tools/oxapi_demo b/tools/oxapi_demo index 1851f0683c2..a1e1e28fd0d 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -308,14 +308,14 @@ function cmd_instance_attach_disk { [[ $# != 4 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME DISK_NAME" mkjson disk="$4" | - do_curl "/organizations/$1/projects/$2/instances/$3/disks/attach" -X POST -T - + do_curl_authn "/organizations/$1/projects/$2/instances/$3/disks/attach" -X POST -T - } function cmd_instance_detach_disk { [[ $# != 4 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME DISK_NAME" mkjson disk="$4" | - do_curl "/organizations/$1/projects/$2/instances/$3/disks/detach" -X POST -T - + do_curl_authn "/organizations/$1/projects/$2/instances/$3/disks/detach" -X POST -T - } function cmd_instance_list_disks From b48b175e68b1af2e21a39f0e12d6c85e711d08b4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 1 Feb 2022 16:10:50 -0800 Subject: [PATCH 10/17] fix some tests --- nexus/src/db/datastore.rs | 10 +- .../db/fixed_data/role_assignment_builtin.rs | 29 +- nexus/tests/integration_tests/disks.rs | 347 +++++++++--------- nexus/tests/integration_tests/unauthorized.rs | 14 + 4 files changed, 212 insertions(+), 188 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 1f3046da9c0..86ea753dce8 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4,10 +4,12 @@ // XXX TODO in this change // - update "unauthorized" test +// - currently failing because there are no datasets to allocate the disk. +// what does the disk integration test do? // - make sure we have coverage for the case where you try to attach or detach -// an Instance when not authorized. Make sure we actually check if a change -// was made!! (Should the rest of the "unauthorized.rs" tests also try to -// assess this? Maybe do a GET after doing all the other things and make sure +// a Disk when not authorized. Make sure we actually check if a change was +// made!! (Should the rest of the "unauthorized.rs" tests also try to assess +// this? Maybe do a GET after doing all the other things and make sure // nothing changed? Maybe do that and also have the PUT bodies actually make // a change?) @@ -1284,7 +1286,7 @@ impl DataStore { // Projects), we don't do an authz check in the "lookup_by_path" functions // because we don't know if the caller has access to do the lookup. For // leaf resources (like Instances and Disks), though, we do. We could do - // the authz check here, and in disk_fetch_by_id() too. Should we? + // the authz check here, and in disk_lookup_by_id() too. Should we? pub async fn disk_lookup_by_path( &self, organization_name: &Name, diff --git a/nexus/src/db/fixed_data/role_assignment_builtin.rs b/nexus/src/db/fixed_data/role_assignment_builtin.rs index 07a579b3f18..3f2d4d9281b 100644 --- a/nexus/src/db/fixed_data/role_assignment_builtin.rs +++ b/nexus/src/db/fixed_data/role_assignment_builtin.rs @@ -10,13 +10,26 @@ use crate::db::model::RoleAssignmentBuiltin; use lazy_static::lazy_static; lazy_static! { - // The "test-privileged" user gets the "admin" role on the sole Fleet. - // This will grant them all permissions on all resources. pub static ref BUILTIN_ROLE_ASSIGNMENTS: Vec = - vec![RoleAssignmentBuiltin::new( - user_builtin::USER_TEST_PRIVILEGED.id, - role_builtin::FLEET_ADMIN.resource_type, - *FLEET_ID, - role_builtin::FLEET_ADMIN.role_name, - )]; + vec![ + // The "test-privileged" user gets the "admin" role on the sole + // Fleet. This will grant them all permissions on all resources. + RoleAssignmentBuiltin::new( + user_builtin::USER_TEST_PRIVILEGED.id, + role_builtin::FLEET_ADMIN.resource_type, + *FLEET_ID, + role_builtin::FLEET_ADMIN.role_name, + ), + + // The "internal-api" user gets the "admin" role on the sole Fleet. + // This will grant them all permissions on all resources. + // TODO-security We should scope this down (or, really, figure out a + // better internal authn/authz story). + RoleAssignmentBuiltin::new( + user_builtin::USER_INTERNAL_API.id, + role_builtin::FLEET_ADMIN.resource_type, + *FLEET_ID, + role_builtin::FLEET_ADMIN.role_name, + ), + ]; } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 95377c151cd..ceb3aa6e987 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -14,6 +14,7 @@ use omicron_common::api::external::DiskState; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Instance; use omicron_common::api::external::InstanceCpuCount; +use omicron_common::api::external::Name; use omicron_nexus::TestInterfaces as _; use omicron_nexus::{external_api::params, Nexus}; use omicron_sled_agent::sim::SledAgent; @@ -21,10 +22,8 @@ use sled_agent_client::TestInterfaces as _; use std::sync::Arc; use uuid::Uuid; -use dropshot::test_util::object_get; use dropshot::test_util::objects_list_page; use dropshot::test_util::objects_post; -use dropshot::test_util::read_json; use dropshot::test_util::ClientTestContext; use nexus_test_utils::http_testing::AuthnMode; @@ -215,16 +214,12 @@ async fn test_disk_create_attach_detach_delete( get_disk_detach_url(instance.identity.name.as_str()); // Start attaching the disk to the instance. - let mut response = client - .make_request( - Method::POST, - &url_instance_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); - let attached_disk: Disk = read_json(&mut response).await; + let attached_disk = disk_post( + client, + &url_instance_attach_disk, + disk.identity.name.clone(), + ) + .await; let instance_id = &instance.identity.id; assert_eq!(attached_disk.identity.name, disk.identity.name); assert_eq!(attached_disk.identity.id, disk.identity.id); @@ -240,28 +235,20 @@ async fn test_disk_create_attach_detach_delete( // Attach the disk to the same instance. This should complete immediately // with no state change. - client - .make_request( - Method::POST, - &url_instance_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = + disk_post(client, &url_instance_attach_disk, disk.identity.name).await; + assert_eq!(disk.state, DiskState::Attached(instance_id.clone())); let disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Attached(instance_id.clone())); // Begin detaching the disk. - client - .make_request( - Method::POST, - &url_instance_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = disk_post( + client, + &url_instance_detach_disk, + disk.identity.name.clone(), + ) + .await; + assert_eq!(disk.state, DiskState::Detaching(instance_id.clone())); let disk: Disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Detaching(instance_id.clone())); @@ -271,17 +258,15 @@ async fn test_disk_create_attach_detach_delete( assert_eq!(disk.state, DiskState::Detached); // Since detach is idempotent, we can detach it again. - client - .make_request( - Method::POST, - &url_instance_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = disk_post( + client, + &url_instance_detach_disk, + disk.identity.name.clone(), + ) + .await; + assert_eq!(disk.state, DiskState::Detached); - // A priveleged user should be able to delete the disk. + // Delete the disk. NexusRequest::object_delete(client, &disk_url) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -386,16 +371,12 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { get_disk_detach_url(instance.identity.name.as_str()); // Start attaching the disk to the instance. - let mut response = client - .make_request( - Method::POST, - &url_instance_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); - let attached_disk: Disk = read_json(&mut response).await; + let attached_disk = disk_post( + client, + &url_instance_attach_disk, + disk.identity.name.clone(), + ) + .await; let instance_id = &instance.identity.id; assert_eq!(attached_disk.identity.name, disk.identity.name); assert_eq!(attached_disk.identity.id, disk.identity.id); @@ -411,16 +392,8 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { // Attach the disk to the same instance. This should complete immediately // with no state change. - client - .make_request( - Method::POST, - &url_instance_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); - let disk = disk_get(&client, &disk_url).await; + let disk = + disk_post(client, &url_instance_attach_disk, disk.identity.name).await; assert_eq!(disk.state, DiskState::Attached(instance_id.clone())); // Create a second instance and try to attach the disk to that. This should @@ -443,19 +416,24 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { get_disk_attach_url(instance2.identity.name.as_str()); let url_instance2_detach_disk = get_disk_detach_url(instance2.identity.name.as_str()); - let error = client - .make_request_error_body( - Method::POST, - &url_instance2_attach_disk, - params::DiskIdentifier { disk: disk.identity.name.clone() }, - StatusCode::BAD_REQUEST, - ) - .await; + + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &url_instance2_attach_disk) + .body(Some(¶ms::DiskIdentifier { + disk: disk.identity.name.clone(), + })) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!( - "cannot attach disk \"{}\": disk is attached to another \ - instance", + "cannot attach disk \"{}\": disk is attached to another instance", DISK_NAME ) ); @@ -464,65 +442,60 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { assert_eq!(attached_disk.state, DiskState::Attached(instance_id.clone())); // Begin detaching the disk. - client - .make_request( - Method::POST, - &url_instance_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); - let disk: Disk = disk_get(&client, &disk_url).await; + let disk = + disk_post(client, &url_instance_detach_disk, disk.identity.name).await; + assert_eq!(disk.state, DiskState::Detaching(instance_id.clone())); + let disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Detaching(instance_id.clone())); // It's still illegal to attach this disk elsewhere. - let error = client - .make_request_error_body( - Method::POST, - &url_instance2_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::BAD_REQUEST, - ) - .await; + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &url_instance2_attach_disk) + .body(Some(¶ms::DiskIdentifier { + disk: disk.identity.name.clone(), + })) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!( - "cannot attach disk \"{}\": disk is attached to another \ - instance", + "cannot attach disk \"{}\": disk is attached to another instance", DISK_NAME ) ); // It's even illegal to attach this disk back to the same instance. - let error = client - .make_request_error_body( - Method::POST, - &url_instance_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::BAD_REQUEST, - ) - .await; - // TODO-debug the error message here is misleading. + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &url_instance_attach_disk) + .body(Some(¶ms::DiskIdentifier { + disk: disk.identity.name.clone(), + })) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!( - "cannot attach disk \"{}\": disk is attached to another \ - instance", + "cannot attach disk \"{}\": disk is attached to another instance", DISK_NAME ) ); // However, there's no problem attempting to detach it again. - client - .make_request( - Method::POST, - &url_instance_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = + disk_post(client, &url_instance_detach_disk, disk.identity.name).await; + assert_eq!(disk.state, DiskState::Detaching(instance_id.clone())); let disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Detaching(instance_id.clone())); @@ -532,36 +505,20 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { assert_eq!(disk.state, DiskState::Detached); // Since delete is idempotent, we can detach it again -- from either one. - client - .make_request( - Method::POST, - &url_instance_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); - client - .make_request( - Method::POST, - &url_instance2_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = + disk_post(client, &url_instance_detach_disk, disk.identity.name).await; + assert_eq!(disk.state, DiskState::Detached); + let disk = + disk_post(client, &url_instance2_detach_disk, disk.identity.name).await; + assert_eq!(disk.state, DiskState::Detached); // Now, start attaching it again to the second instance. - let mut response = client - .make_request( - Method::POST, - &url_instance2_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); - let attached_disk: Disk = read_json(&mut response).await; + let attached_disk = disk_post( + client, + &url_instance2_attach_disk, + disk.identity.name.clone(), + ) + .await; let instance2_id = &instance2.identity.id; assert_eq!(attached_disk.identity.name, disk.identity.name); assert_eq!(attached_disk.identity.id, disk.identity.id); @@ -572,40 +529,44 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { // At this point, it's not legal to attempt to attach it to a different // instance (the first one). - let error = client - .make_request_error_body( - Method::POST, - &url_instance_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::BAD_REQUEST, - ) - .await; + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &url_instance_attach_disk) + .body(Some(¶ms::DiskIdentifier { + disk: disk.identity.name.clone(), + })) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!( - "cannot attach disk \"{}\": disk is attached to another \ - instance", + "cannot attach disk \"{}\": disk is attached to another instance", DISK_NAME ) ); // It's fine to attempt another attachment to the same instance. - client - .make_request( - Method::POST, - &url_instance2_attach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = disk_post( + client, + &url_instance2_attach_disk, + disk.identity.name.clone(), + ) + .await; + assert_eq!(disk.state, DiskState::Attaching(instance2_id.clone())); let disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Attaching(instance2_id.clone())); // It's not allowed to delete a disk that's attaching. - let error = NexusRequest::new( - RequestBuilder::new(client, Method::DELETE, &disk_url) - .expect_status(Some(StatusCode::BAD_REQUEST)), + let error = NexusRequest::expect_failure( + client, + StatusCode::BAD_REQUEST, + Method::DELETE, + &disk_url, ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -616,22 +577,22 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.message, "disk cannot be deleted in state \"attaching\""); // Now, begin a detach while the disk is still being attached. - client - .make_request( - Method::POST, - &url_instance2_detach_disk, - Some(params::DiskIdentifier { disk: disk.identity.name.clone() }), - StatusCode::ACCEPTED, - ) - .await - .unwrap(); + let disk = disk_post( + client, + &url_instance2_detach_disk, + disk.identity.name.clone(), + ) + .await; + assert_eq!(disk.state, DiskState::Detaching(instance2_id.clone())); let disk: Disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Detaching(instance2_id.clone())); // It's not allowed to delete a disk that's detaching, either. - let error = NexusRequest::new( - RequestBuilder::new(client, Method::DELETE, &disk_url) - .expect_status(Some(StatusCode::BAD_REQUEST)), + let error = NexusRequest::expect_failure( + client, + StatusCode::BAD_REQUEST, + Method::DELETE, + &disk_url, ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -646,6 +607,7 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { let disk = disk_get(&client, &disk_url).await; assert_eq!(disk.state, DiskState::Detached); + // Now we can delete the disk. NexusRequest::object_delete(client, &disk_url) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -656,9 +618,18 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { assert_eq!(disks_list(&client, &disks_url).await.len(), 0); // We shouldn't find it if we request it explicitly. - let error = client - .make_request_error(Method::GET, &disk_url, StatusCode::NOT_FOUND) - .await; + let error: HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &disk_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!("not found: disk with name \"{}\"", DISK_NAME) @@ -774,7 +745,13 @@ async fn test_disk_region_creation_failure( } async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { - object_get::(client, disk_url).await + NexusRequest::object_get(client, disk_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap() } async fn disks_list(client: &ClientTestContext, list_url: &str) -> Vec { @@ -784,6 +761,24 @@ async fn disks_list(client: &ClientTestContext, list_url: &str) -> Vec { .all_items } +async fn disk_post( + client: &ClientTestContext, + url: &str, + disk_name: Name, +) -> Disk { + NexusRequest::new( + RequestBuilder::new(client, Method::POST, url) + .body(Some(¶ms::DiskIdentifier { disk: disk_name })) + .expect_status(Some(StatusCode::ACCEPTED)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap() +} + fn disks_eq(disk1: &Disk, disk2: &Disk) { identity_eq(&disk1.identity, &disk2.identity); assert_eq!(disk1.project_id, disk2.project_id); diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 7abbdbea957..950a8cfe88d 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -103,6 +103,11 @@ lazy_static! { url: &*DEMO_ORG_PROJECTS_URL, body: serde_json::to_value(&*DEMO_PROJECT_CREATE).unwrap(), }, + // Create a Disk in the Project + SetupReq { + url: &*DEMO_PROJECT_URL_DISKS, + body: serde_json::to_value(&*DEMO_DISK_CREATE).unwrap(), + }, ]; // Organization used for testing @@ -315,6 +320,15 @@ lazy_static! { ], }, + VerifyEndpoint { + url: &*DEMO_DISK_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + ], + }, + // TODO-coverage The single disk endpoint belongs here, but we've only // implemented authz for DELETE, not GET yet. From 1e1b5d55204b24a07e6cfd25a98d1a26ba380830 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 1 Feb 2022 17:03:14 -0800 Subject: [PATCH 11/17] fix more tests --- nexus/src/lib.rs | 1 + nexus/test-utils/src/resource_helpers.rs | 41 ++++++++++++++ nexus/tests/integration_tests/disks.rs | 54 ++++--------------- nexus/tests/integration_tests/unauthorized.rs | 4 +- 4 files changed, 55 insertions(+), 45 deletions(-) diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index e73533d95b1..8ade3e1868b 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -30,6 +30,7 @@ mod sagas; pub use config::Config; pub use context::ServerContext; +pub use crucible_agent_client; use external_api::http_entrypoints::external_api; use internal_api::http_entrypoints::internal_api; pub use nexus::Nexus; diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 8f1ba7df714..5973149d075 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -2,6 +2,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use crate::ControlPlaneTestContext; + use super::http_testing::dropshot_compat::objects_post; use super::http_testing::AuthnMode; use super::http_testing::NexusRequest; @@ -13,8 +15,12 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::Disk; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::VpcRouter; +use omicron_nexus::crucible_agent_client::types::State as RegionState; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views::{Organization, Project, Vpc}; +use omicron_sled_agent::sim::SledAgent; +use std::sync::Arc; +use uuid::Uuid; pub async fn objects_list_page_authz( client: &ClientTestContext, @@ -200,3 +206,38 @@ pub async fn project_get( .parsed_body() .expect("failed to parse Project") } + +pub struct DiskTest { + pub sled_agent: Arc, + pub zpool_id: Uuid, + pub zpool_size: ByteCount, + pub dataset_ids: Vec, +} + +impl DiskTest { + // Creates fake physical storage, an organization, and a project. + pub async fn new(cptestctx: &ControlPlaneTestContext) -> Self { + let sled_agent = cptestctx.sled_agent.sled_agent.clone(); + + // Create a Zpool. + let zpool_id = Uuid::new_v4(); + let zpool_size = ByteCount::from_gibibytes_u32(10); + sled_agent.create_zpool(zpool_id, zpool_size.to_bytes()).await; + + // Create multiple Datasets within that Zpool. + let dataset_count = 3; + let dataset_ids: Vec<_> = + (0..dataset_count).map(|_| Uuid::new_v4()).collect(); + for id in &dataset_ids { + sled_agent.create_crucible_dataset(zpool_id, *id).await; + + // By default, regions are created immediately. + let crucible = sled_agent.get_crucible_dataset(zpool_id, *id).await; + crucible + .set_create_callback(Box::new(|_| RegionState::Created)) + .await; + } + + Self { sled_agent, zpool_id, zpool_size, dataset_ids } + } +} diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index ceb3aa6e987..1b42bf71345 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -17,7 +17,6 @@ use omicron_common::api::external::InstanceCpuCount; use omicron_common::api::external::Name; use omicron_nexus::TestInterfaces as _; use omicron_nexus::{external_api::params, Nexus}; -use omicron_sled_agent::sim::SledAgent; use sled_agent_client::TestInterfaces as _; use std::sync::Arc; use uuid::Uuid; @@ -33,6 +32,7 @@ use nexus_test_utils::identity_eq; use nexus_test_utils::resource_helpers::create_disk; use nexus_test_utils::resource_helpers::create_organization; use nexus_test_utils::resource_helpers::create_project; +use nexus_test_utils::resource_helpers::DiskTest; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; @@ -70,52 +70,13 @@ async fn create_org_and_project(client: &ClientTestContext) -> Uuid { project.identity.id } -struct DiskTest { - sled_agent: Arc, - zpool_id: Uuid, - zpool_size: ByteCount, - dataset_ids: Vec, - project_id: Uuid, -} - -impl DiskTest { - // Creates fake physical storage, an organization, and a project. - async fn new(cptestctx: &ControlPlaneTestContext) -> Self { - let client = &cptestctx.external_client; - let sled_agent = cptestctx.sled_agent.sled_agent.clone(); - - // Create a Zpool. - let zpool_id = Uuid::new_v4(); - let zpool_size = ByteCount::from_gibibytes_u32(10); - sled_agent.create_zpool(zpool_id, zpool_size.to_bytes()).await; - - // Create multiple Datasets within that Zpool. - let dataset_count = 3; - let dataset_ids: Vec<_> = - (0..dataset_count).map(|_| Uuid::new_v4()).collect(); - for id in &dataset_ids { - sled_agent.create_crucible_dataset(zpool_id, *id).await; - - // By default, regions are created immediately. - let crucible = sled_agent.get_crucible_dataset(zpool_id, *id).await; - crucible - .set_create_callback(Box::new(|_| RegionState::Created)) - .await; - } - - // Create a project for testing. - let project_id = create_org_and_project(&client).await; - - Self { sled_agent, zpool_id, zpool_size, dataset_ids, project_id } - } -} - #[nexus_test] async fn test_disk_not_found_before_creation( cptestctx: &ControlPlaneTestContext, ) { let client = &cptestctx.external_client; DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; let disks_url = get_disks_url(); // List disks. There aren't any yet. @@ -154,7 +115,8 @@ async fn test_disk_create_attach_detach_delete( cptestctx: &ControlPlaneTestContext, ) { let client = &cptestctx.external_client; - let test = DiskTest::new(&cptestctx).await; + DiskTest::new(&cptestctx).await; + let project_id = create_org_and_project(client).await; let nexus = &cptestctx.server.apictx.nexus; let disks_url = get_disks_url(); @@ -163,7 +125,7 @@ async fn test_disk_create_attach_detach_delete( let disk = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; assert_eq!(disk.identity.name, DISK_NAME); assert_eq!(disk.identity.description, "sells rainsticks"); - assert_eq!(disk.project_id, test.project_id); + assert_eq!(disk.project_id, project_id); assert_eq!(disk.snapshot_id, None); assert_eq!(disk.size.to_whole_mebibytes(), 1024); assert_eq!(disk.state, DiskState::Creating); @@ -174,7 +136,7 @@ async fn test_disk_create_attach_detach_delete( let disk = disk_get(&client, &disk_url).await; assert_eq!(disk.identity.name, DISK_NAME); assert_eq!(disk.identity.description, "sells rainsticks"); - assert_eq!(disk.project_id, test.project_id); + assert_eq!(disk.project_id, project_id); assert_eq!(disk.snapshot_id, None); assert_eq!(disk.size.to_whole_mebibytes(), 1024); assert_eq!(disk.state, DiskState::Detached); @@ -292,6 +254,7 @@ async fn test_disk_create_disk_that_already_exists_fails( ) { let client = &cptestctx.external_client; DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; let disks_url = get_disks_url(); // Create a disk. @@ -335,6 +298,7 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; DiskTest::new(&cptestctx).await; + create_org_and_project(&client).await; let disks_url = get_disks_url(); // Create a disk. @@ -642,6 +606,7 @@ async fn test_disk_creation_region_requested_then_started( ) { let client = &cptestctx.external_client; let test = DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; // Before we create a disk, set the response from the Crucible Agent: // no matter what regions get requested, they'll always *start* as @@ -674,6 +639,7 @@ async fn test_disk_region_creation_failure( ) { let client = &cptestctx.external_client; let test = DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; // Before we create a disk, set the response from the Crucible Agent: // no matter what regions get requested, they'll always fail. diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 950a8cfe88d..c2e33e453d7 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -14,6 +14,7 @@ use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::http_testing::TestResponse; +use nexus_test_utils::resource_helpers::DiskTest; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; use omicron_common::api::external::ByteCount; @@ -54,6 +55,7 @@ use omicron_nexus::external_api::params; // from the OpenAPI spec? #[nexus_test] async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { + DiskTest::new(cptestctx).await; let client = &cptestctx.external_client; let log = &cptestctx.logctx.log; @@ -145,7 +147,7 @@ lazy_static! { static ref DEMO_DISK_CREATE: params::DiskCreate = params::DiskCreate { identity: IdentityMetadataCreateParams { - name: DEMO_PROJECT_NAME.clone(), + name: DEMO_DISK_NAME.clone(), description: "".parse().unwrap(), }, snapshot_id: None, From f580ba459b20c07132bcd31548983306c23a3523 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Feb 2022 09:39:27 -0800 Subject: [PATCH 12/17] more tests --- nexus/src/db/datastore.rs | 11 -- nexus/tests/integration_tests/unauthorized.rs | 110 ++++++++++++++++-- 2 files changed, 100 insertions(+), 21 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 86ea753dce8..52a1da8f74b 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2,17 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -// XXX TODO in this change -// - update "unauthorized" test -// - currently failing because there are no datasets to allocate the disk. -// what does the disk integration test do? -// - make sure we have coverage for the case where you try to attach or detach -// a Disk when not authorized. Make sure we actually check if a change was -// made!! (Should the rest of the "unauthorized.rs" tests also try to assess -// this? Maybe do a GET after doing all the other things and make sure -// nothing changed? Maybe do that and also have the PUT bodies actually make -// a change?) - /*! * Primary control plane interface for database read and write operations */ diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index c2e33e453d7..d41c9ce1600 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -20,6 +20,7 @@ use nexus_test_utils_macros::nexus_test; use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; +use omicron_common::api::external::InstanceCpuCount; use omicron_common::api::external::Name; use omicron_nexus::authn; use omicron_nexus::authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF; @@ -110,6 +111,11 @@ lazy_static! { url: &*DEMO_PROJECT_URL_DISKS, body: serde_json::to_value(&*DEMO_DISK_CREATE).unwrap(), }, + // Create an Instance in the Project + SetupReq { + url: &*DEMO_PROJECT_URL_INSTANCES, + body: serde_json::to_value(&*DEMO_INSTANCE_CREATE).unwrap(), + }, ]; // Organization used for testing @@ -132,6 +138,8 @@ lazy_static! { format!("{}/{}", *DEMO_ORG_PROJECTS_URL, *DEMO_PROJECT_NAME); static ref DEMO_PROJECT_URL_DISKS: String = format!("{}/disks", *DEMO_PROJECT_URL); + static ref DEMO_PROJECT_URL_INSTANCES: String = + format!("{}/instances", *DEMO_PROJECT_URL); static ref DEMO_PROJECT_CREATE: params::ProjectCreate = params::ProjectCreate { identity: IdentityMetadataCreateParams { @@ -153,6 +161,27 @@ lazy_static! { snapshot_id: None, size: ByteCount::from_gibibytes_u32(16), }; + + // Instance used for testing + static ref DEMO_INSTANCE_NAME: Name = "demo-instance".parse().unwrap(); + static ref DEMO_INSTANCE_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_INSTANCES, *DEMO_INSTANCE_NAME); + static ref DEMO_INSTANCE_DISKS_URL: String = + format!("{}/disks", *DEMO_INSTANCE_URL); + static ref DEMO_INSTANCE_DISKS_ATTACH_URL: String = + format!("{}/attach", *DEMO_INSTANCE_DISKS_URL); + static ref DEMO_INSTANCE_DISKS_DETACH_URL: String = + format!("{}/detach", *DEMO_INSTANCE_DISKS_URL); + static ref DEMO_INSTANCE_CREATE: params::InstanceCreate = + params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_DISK_NAME.clone(), + description: "".parse().unwrap(), + }, + ncpus: InstanceCpuCount(1), + memory: ByteCount::from_gibibytes_u32(16), + hostname: String::from("demo-instance"), + }; } // @@ -269,7 +298,7 @@ lazy_static! { serde_json::to_value(¶ms::OrganizationUpdate { identity: IdentityMetadataUpdateParams { name: None, - description: None, + description: Some("different".to_string()) } }).unwrap() ), @@ -304,7 +333,7 @@ lazy_static! { serde_json::to_value(params::ProjectUpdate{ identity: IdentityMetadataUpdateParams { name: None, - description: None, + description: Some("different".to_string()) }, }).unwrap() ), @@ -331,8 +360,28 @@ lazy_static! { ], }, - // TODO-coverage The single disk endpoint belongs here, but we've only - // implemented authz for DELETE, not GET yet. + VerifyEndpoint { + url: &*DEMO_INSTANCE_DISKS_ATTACH_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Post( + serde_json::to_value(params::DiskIdentifier { + disk: DEMO_DISK_NAME.clone() + }).unwrap() + ) + ], + }, + VerifyEndpoint { + url: &*DEMO_INSTANCE_DISKS_DETACH_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Post( + serde_json::to_value(params::DiskIdentifier { + disk: DEMO_DISK_NAME.clone() + }).unwrap() + ) + ], + }, VerifyEndpoint { url: "/roles", @@ -434,12 +483,25 @@ async fn verify_endpoint( // response. Otherwise, the test might later succeed by coincidence. We // might find a 404 because of something that actually doesn't exist rather // than something that's just hidden from unauthorized users. - info!(log, "test: privileged GET"); - NexusRequest::object_get(client, endpoint.url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap(); + let get_allowed = endpoint + .allowed_methods + .iter() + .any(|allowed| allowed.http_method() == Method::GET); + let resource_before: Option = if get_allowed { + info!(log, "test: privileged GET"); + Some( + NexusRequest::object_get(client, endpoint.url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(), + ) + } else { + warn!(log, "test: skipping privileged GET (method not allowed)"); + None + }; // For each of the HTTP methods we use in the API as well as TRACE, we'll // make several requests to this URL and verify the results. @@ -532,6 +594,34 @@ async fn verify_endpoint( .unwrap(); verify_response(&response); } + + // If we fetched the resource earlier, fetch it again and check the state. + // We're trying to catch cases where an endpoint correctly returns an error + // but still applied the result. + // + // This might seem gratuitous but it's an important check for resources like + // disk attachment and detachment, where Nexus reaches out to the Sled Agent + // before making a database change. If Nexus only authorized the request at + // the database query (as is our current emphasis), we could wind up making + // the change to the system even for unauthorized users (and still returning + // an "unauthorized" error)! + // TODO-coverage It would be good to check the ETag here as well, once we + // provide one. + info!(log, "test: compare current resource content with earlier"); + if let Some(resource_before) = resource_before { + let resource_after: serde_json::Value = + NexusRequest::object_get(client, endpoint.url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + resource_before, resource_after, + "resource changed after making a bunch of failed requests" + ); + } } /// Verifies the body of an HTTP response for status codes 401, 403, 404, or 405 From f0d597a44e5e393cf3e78e5dab565e0715883af1 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Feb 2022 09:50:28 -0800 Subject: [PATCH 13/17] fix oxapi_demo for disk_get --- tools/oxapi_demo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/oxapi_demo b/tools/oxapi_demo index a1e1e28fd0d..6f8a1aa6339 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -334,7 +334,7 @@ function cmd_disk_create_demo function cmd_disk_get { [[ $# != 3 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME DISK_NAME" - do_curl "/organizations/$1/projects/$2/disks/$3" + do_curl_authn "/organizations/$1/projects/$2/disks/$3" } function cmd_disk_delete From 4a26d6c6d880471601b025eec6d4fd96a477278e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Feb 2022 09:54:38 -0800 Subject: [PATCH 14/17] remove XXX (checked behavior) --- nexus/src/nexus.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 6532a1eecaa..fc106219600 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1459,9 +1459,6 @@ impl Nexus { sled_agent_client::types::DiskStateRequested::Detached, ) .await?; - // XXX it seems like there was a bug here where we would report the old - // state ("attached") instead of the new one ("detaching"). If not, I'm - // missing something about how this used to work. self.db_datastore.disk_refetch(opctx, &authz_disk).await } From 3066ca1f368f56cc0c23c1cc17125795070dc3f4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Feb 2022 09:57:02 -0800 Subject: [PATCH 15/17] another oxapi_demo fix --- tools/oxapi_demo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/oxapi_demo b/tools/oxapi_demo index 6f8a1aa6339..2eeb9a13ae1 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -231,7 +231,7 @@ function cmd_project_create_demo function cmd_project_delete { [[ $# != 2 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME" - do_curl "/organizations/$1/projects/$2" -X DELETE + do_curl_authn "/organizations/$1/projects/$2" -X DELETE } function cmd_project_get From cc084931afc7864059aae32d0cb84b2e843907c5 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Feb 2022 11:20:39 -0800 Subject: [PATCH 16/17] clarify use of test user/contexts -- not just unit tests --- nexus/src/authn/mod.rs | 2 +- nexus/src/context.rs | 14 +++++++------- nexus/src/db/datastore.rs | 2 +- nexus/src/db/saga_recovery.rs | 4 ++-- nexus/src/nexus.rs | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index fc5661866e7..3ecb9a7f2ef 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -113,7 +113,7 @@ impl Context { /// Returns an authenticated context for a specific user /// - /// This is used for unit testing the authorization rules. + /// This is used for testing. pub fn test_context_for_actor(actor_id: Uuid) -> Context { Context::context_for_actor(actor_id) } diff --git a/nexus/src/context.rs b/nexus/src/context.rs index d94957192d6..ef7627648d2 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -204,8 +204,8 @@ enum OpKind { InternalApiRequest, /// Background operations in Nexus Background, - /// Unit tests - UnitTest, + /// Automated testing (unit tests and integration tests) + Test, } impl OpContext { @@ -323,9 +323,9 @@ impl OpContext { } } - /// Returns a context suitable for automated unit tests where an OpContext - /// is needed outside of a Dropshot context - pub fn for_unit_tests( + /// Returns a context suitable for automated tests where an OpContext is + /// needed outside of a Dropshot context + pub fn for_tests( log: slog::Logger, datastore: Arc, ) -> OpContext { @@ -344,7 +344,7 @@ impl OpContext { created_instant, created_walltime, metadata: BTreeMap::new(), - kind: OpKind::UnitTest, + kind: OpKind::Test, } } @@ -429,7 +429,7 @@ mod test { let mut db = test_setup_database(&logctx.log).await; let (_, datastore) = crate::db::datastore::datastore_test(&logctx, &db).await; - let opctx = OpContext::for_unit_tests(logctx.log.new(o!()), datastore); + let opctx = OpContext::for_tests(logctx.log.new(o!()), datastore); // Like in test_background_context(), this is essentially a test of the // authorization policy. The unit tests assume this user can do diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 52a1da8f74b..cc14293d587 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2863,7 +2863,7 @@ pub async fn datastore_test( // Create an OpContext with the credentials of "test-privileged" for general // testing. let opctx = - OpContext::for_unit_tests(logctx.log.new(o!()), Arc::clone(&datastore)); + OpContext::for_tests(logctx.log.new(o!()), Arc::clone(&datastore)); (opctx, datastore) } diff --git a/nexus/src/db/saga_recovery.rs b/nexus/src/db/saga_recovery.rs index 37193199c4d..584a93f6cbc 100644 --- a/nexus/src/db/saga_recovery.rs +++ b/nexus/src/db/saga_recovery.rs @@ -481,7 +481,7 @@ mod test { let (storage, sec_client, uctx) = create_storage_sec_and_context(&log, db_datastore.clone(), sec_id); let sec_log = log.new(o!("component" => "SEC")); - let opctx = OpContext::for_unit_tests(log, Arc::clone(&db_datastore)); + let opctx = OpContext::for_tests(log, Arc::clone(&db_datastore)); // Create and start a saga. // @@ -555,7 +555,7 @@ mod test { let (storage, sec_client, uctx) = create_storage_sec_and_context(&log, db_datastore.clone(), sec_id); let sec_log = log.new(o!("component" => "SEC")); - let opctx = OpContext::for_unit_tests(log, Arc::clone(&db_datastore)); + let opctx = OpContext::for_tests(log, Arc::clone(&db_datastore)); // Create and start a saga, which we expect to complete successfully. let saga_id = SagaId(Uuid::new_v4()); diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index fc106219600..d93fc1c3ca6 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -2493,7 +2493,7 @@ impl TestInterfaces for Nexus { &self, id: &Uuid, ) -> Result, Error> { - let opctx = OpContext::for_unit_tests( + let opctx = OpContext::for_tests( self.log.new(o!()), Arc::clone(&self.db_datastore), ); From 4718726840276f22091d5042e6ea95effa66b025 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 3 Feb 2022 13:01:09 -0800 Subject: [PATCH 17/17] fix mismerge --- nexus/tests/integration_tests/unauthorized.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index a25763ead97..d41c9ce1600 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -182,8 +182,6 @@ lazy_static! { memory: ByteCount::from_gibibytes_u32(16), hostname: String::from("demo-instance"), }; -======= ->>>>>>> main } //