From 4086d584fb8e823a76a41e040af1f465db72a5ac Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 27 Jan 2022 16:12:37 -0800 Subject: [PATCH 1/8] 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 2/8] 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 3/8] 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 4/8] 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 5/8] 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 6/8] 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 7/8] 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 88a08f9b66fd23c1df7935fef7ffab39a0fbe60f Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 3 Feb 2022 08:55:09 -0800 Subject: [PATCH 8/8] review feedback: magic number --- nexus/test-utils/src/http_testing.rs | 6 ++++-- nexus/tests/integration_tests/basic.rs | 10 +++++----- nexus/tests/integration_tests/disks.rs | 2 +- nexus/tests/integration_tests/projects.rs | 4 ++-- nexus/tests/integration_tests/roles_builtin.rs | 2 +- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 6bcd66ebfed..8652f7c86cf 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -514,15 +514,17 @@ impl<'a> NexusRequest<'a> { testctx: &'a ClientTestContext, collection_url: &str, initial_params: &str, - limit: usize, + limit: Option, ) -> Result, anyhow::Error> where T: Clone + serde::de::DeserializeOwned, { - let url_base = format!("{}?limit={}&", collection_url, limit); let mut npages = 0; let mut all_items = Vec::new(); let mut next_token: Option = None; + const DEFAULT_PAGE_SIZE: usize = 10; + let limit = limit.unwrap_or(DEFAULT_PAGE_SIZE); + let url_base = format!("{}?limit={}&", collection_url, limit); loop { let url = if let Some(next_token) = &next_token { diff --git a/nexus/tests/integration_tests/basic.rs b/nexus/tests/integration_tests/basic.rs index 52ffb4edf68..6047a667889 100644 --- a/nexus/tests/integration_tests/basic.rs +++ b/nexus/tests/integration_tests/basic.rs @@ -549,7 +549,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -572,7 +572,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "sort_by=name-ascending", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -595,7 +595,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "sort_by=name-descending", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -617,7 +617,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "sort_by=id-ascending", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -671,7 +671,7 @@ async fn projects_list( client: &ClientTestContext, projects_url: &str, ) -> Vec { - NexusRequest::iter_collection_authn(client, projects_url, "", 10) + NexusRequest::iter_collection_authn(client, projects_url, "", None) .await .expect("failed to list projects") .all_items diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 95377c151cd..d7af2dd525d 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -778,7 +778,7 @@ async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { } async fn disks_list(client: &ClientTestContext, list_url: &str) -> Vec { - NexusRequest::iter_collection_authn(client, list_url, "", 10) + NexusRequest::iter_collection_authn(client, list_url, "", None) .await .expect("failed to list disks") .all_items diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 3a463ffbb3c..60ffe559622 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -39,7 +39,7 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { &client, &projects_url, "", - 10, + None, ) .await .expect("failed to list projects") @@ -62,7 +62,7 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { &client, &format!("/organizations/{}/projects", org2_name), "", - 10, + None, ) .await .expect("failed to list projects") diff --git a/nexus/tests/integration_tests/roles_builtin.rs b/nexus/tests/integration_tests/roles_builtin.rs index 084f447eb7e..9faf8b1bdab 100644 --- a/nexus/tests/integration_tests/roles_builtin.rs +++ b/nexus/tests/integration_tests/roles_builtin.rs @@ -46,7 +46,7 @@ async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) { // This endpoint uses a custom pagination scheme that is easy to get wrong. // Let's test that all markers do work. let roles_paginated = - NexusRequest::iter_collection_authn(&testctx, "/roles", "", 1) + NexusRequest::iter_collection_authn(&testctx, "/roles", "", Some(1)) .await .expect("failed to iterate all roles"); assert_eq!(roles, roles_paginated.all_items);