From c8d98d3593cf85d1d677b4390ac7afde6b8c50ce Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 4 Apr 2022 13:56:13 -0700 Subject: [PATCH 01/11] use pool_authorized() for all lookups --- nexus/src/authz/context.rs | 23 ++++++++++++------- nexus/src/db/datastore.rs | 2 +- nexus/src/db/db-macros/src/lookup.rs | 19 ++++----------- nexus/tests/integration_tests/unauthorized.rs | 9 +++----- 4 files changed, 24 insertions(+), 29 deletions(-) diff --git a/nexus/src/authz/context.rs b/nexus/src/authz/context.rs index 3de7b74bac9..378a224434c 100644 --- a/nexus/src/authz/context.rs +++ b/nexus/src/authz/context.rs @@ -93,19 +93,26 @@ impl Context { error ))), Ok(false) => { - let error = if is_authn { - Error::Forbidden - } else { - // If the user did not authenticate successfully, this will - // become a 401 rather than a 403. + Err(if !is_authn { + // If we failed an authz check, and the user did not + // authenticate at all, we report a 401. Error::Unauthenticated { internal_message: String::from( "authorization failed for unauthenticated request", ), } - }; - - Err(resource.on_unauthorized(&self.authz, error, actor, action)) + } else { + // Otherwise, we normally think of this as a 403 + // "Forbidden". However, the resource impl may choose to + // override that with a 404 to avoid leaking information + // about the resource existing. + resource.on_unauthorized( + &self.authz, + Error::Forbidden, + actor, + action, + ) + }) } } } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index fcbc4560127..f1893f4c25f 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -121,7 +121,7 @@ impl DataStore { // the database. Eventually, this function should only be used for doing // authentication in the first place (since we can't do an authz check in // that case). - pub(super) fn pool(&self) -> &bb8::Pool> { + fn pool(&self) -> &bb8::Pool> { self.pool.pool() } diff --git a/nexus/src/db/db-macros/src/lookup.rs b/nexus/src/db/db-macros/src/lookup.rs index 0acceef5031..32192bd8125 100644 --- a/nexus/src/db/db-macros/src/lookup.rs +++ b/nexus/src/db/db-macros/src/lookup.rs @@ -476,7 +476,7 @@ fn generate_database_functions(config: &Config) -> TokenStream { quote! { let (#(#ancestors_authz_names,)* _) = #parent_resource_name::lookup_by_id_no_authz( - _opctx, datastore, db_row.#parent_id + opctx, datastore, db_row.#parent_id ).await?; }, quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) }, @@ -519,21 +519,19 @@ fn generate_database_functions(config: &Config) -> TokenStream { /// this module, you want `fetch()` or `lookup_for(authz::Action)`. // Do NOT make this function public. async fn lookup_by_name_no_authz( - _opctx: &OpContext, + opctx: &OpContext, datastore: &DataStore, #parent_lookup_arg_formal name: &Name, ) -> LookupResult<(authz::#resource_name, model::#resource_name)> { use db::schema::#resource_as_snake::dsl; - // TODO-security See the note about pool_authorized() below. - let conn = datastore.pool(); dsl::#resource_as_snake .filter(dsl::time_deleted.is_null()) .filter(dsl::name.eq(name.clone())) #lookup_filter .select(model::#resource_name::as_select()) - .get_result_async(conn) + .get_result_async(datastore.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( @@ -584,24 +582,17 @@ fn generate_database_functions(config: &Config) -> TokenStream { /// this module, you want `fetch()` or `lookup_for(authz::Action)`. // Do NOT make this function public. async fn lookup_by_id_no_authz( - _opctx: &OpContext, + opctx: &OpContext, datastore: &DataStore, id: Uuid, ) -> LookupResult<(#(authz::#path_types,)* model::#resource_name)> { use db::schema::#resource_as_snake::dsl; - // TODO-security This could use pool_authorized() instead. - // However, it will change the response code for this case: - // unauthenticated users will get a 401 rather than a 404 - // because we'll kick them out sooner than we used to -- they - // won't even be able to make this database query. That's a - // good thing but this change can be deferred to a follow-up PR. - let conn = datastore.pool(); let db_row = dsl::#resource_as_snake .filter(dsl::time_deleted.is_null()) .filter(dsl::id.eq(id)) .select(model::#resource_name::as_select()) - .get_result_async(conn) + .get_result_async(datastore.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 8f2d8ac1367..3fde028fea3 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -227,12 +227,9 @@ async fn verify_endpoint( 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. - let unauthn_status = match endpoint.visibility { - Visibility::Public => StatusCode::UNAUTHORIZED, - Visibility::Protected => StatusCode::NOT_FOUND, - }; + // When the user is not authenticated, failing any authz check results in a + // "401 Unauthorized" status code. + let unauthn_status = StatusCode::UNAUTHORIZED; // Determine the expected status code for authenticated, unauthorized // requests, based on the endpoint's visibility. From 6889373e526e4fe56d2a989a7a528561df79ba3b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 5 Apr 2022 16:33:24 -0700 Subject: [PATCH 02/11] stub endpoints should still have working authz + coverage --- common/src/api/external/mod.rs | 2 + nexus/src/nexus.rs | 224 ++++++++++++++---- nexus/tests/integration_tests/endpoints.rs | 42 +++- nexus/tests/integration_tests/unauthorized.rs | 69 ++++-- .../unauthorized_coverage.rs | 3 +- .../output/uncovered-authz-endpoints.txt | 4 - 6 files changed, 267 insertions(+), 77 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 527327450aa..3ec0c44ec24 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -521,11 +521,13 @@ pub enum ResourceType { Project, Dataset, Disk, + Image, Instance, NetworkInterface, Rack, Sled, SagaDbg, + Snapshot, Volume, Vpc, VpcFirewallRule, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index b96c0072d63..aad21fcf2e8 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -273,6 +273,59 @@ impl Nexus { } } + /// Used as the body of an unimplemented API endpoint (that we intend to + /// implement) + /// + /// The body of this function doesn't matter very much, since we're + /// eventually going to replace it with a real implementation. We do a + /// superuser-level authz check to ensure the correct behavior This function + /// checks for superuser privilege, really just so that + // XXX-dap TODO-doc review the above comment + // XXX-dap TODO-doc give examples in case the internal usages are removed by + // the time someone needs to use this + async fn unimplemented_todo( + &self, + opctx: &OpContext, + visibility: Unimpl, + ) -> Error { + // Do a super-user-level authz check first. This is really for our + // convenience: it causes this endpoint to do the expected thing for + // unauthenticated and unprivileged users, which in turn allows the + // authz integration test to cover this endpoint. When we do implement + // the endpoint, it will need to correctly authenticate and authorize + // the request. + match opctx.authorize(authz::Action::Modify, &authz::FLEET).await { + Err(error @ Error::Forbidden) => { + if let Unimpl::ProtectedLookup(lookup_error) = visibility { + lookup_error + } else { + error + } + } + Err(error) => error, + Ok(_) => { + // In the event that a superuser actually gets this far, produce + // a server error. + // + // It's tempting to use other status codes here: + // + // "501 Not Implemented" is specifically when we don't recognize + // the HTTP method and cannot implement it on _any_ resource. + // + // "405 Method Not Allowed" is specifically when an HTTP method + // isn't supported. That doesn't feel quite right either -- + // this is usually interpreted to mean "not part of the API", + // which it obviously _is_, since the client found it in the API + // spec. + // + // Neither of these is true: this HTTP method on this HTTP + // resource is part of the API, and it will be supported by the + // server, but it doesn't work yet. + Error::internal_error("endpoint is not implemented") + } + } + } + // TODO-robustness we should have a limit on how many sled agents there can // be (for graceful degradation at large scale). pub async fn upsert_sled( @@ -846,114 +899,178 @@ impl Nexus { pub async fn list_images( &self, - _opctx: &OpContext, + opctx: &OpContext, _pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - unimplemented!(); + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn create_image( self: &Arc, - _opctx: &OpContext, + opctx: &OpContext, _params: ¶ms::ImageCreate, ) -> CreateResult { - unimplemented!(); + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn image_fetch( &self, - _opctx: &OpContext, - _image_name: &Name, + opctx: &OpContext, + image_name: &Name, ) -> LookupResult { - unimplemented!(); + let lookup_type = LookupType::ByName(image_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Image); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } pub async fn delete_image( self: &Arc, - _opctx: &OpContext, - _image_name: &Name, + opctx: &OpContext, + image_name: &Name, ) -> DeleteResult { - unimplemented!(); + let lookup_type = LookupType::ByName(image_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Image); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } pub async fn project_list_images( &self, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn project_create_image( self: &Arc, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _params: ¶ms::ImageCreate, ) -> CreateResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn project_image_fetch( &self, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, - _image_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + image_name: &Name, ) -> LookupResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + let lookup_type = LookupType::ByName(image_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Image); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } pub async fn project_delete_image( self: &Arc, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, - _image_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + image_name: &Name, ) -> DeleteResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + let lookup_type = LookupType::ByName(image_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Image); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } pub async fn project_create_snapshot( self: &Arc, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _params: ¶ms::SnapshotCreate, ) -> CreateResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn project_list_snapshots( &self, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn snapshot_fetch( &self, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, - _snapshot_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + snapshot_name: &Name, ) -> LookupResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + let lookup_type = LookupType::ByName(snapshot_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Snapshot); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } pub async fn project_delete_snapshot( self: &Arc, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, - _snapshot_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + snapshot_name: &Name, ) -> DeleteResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + let lookup_type = LookupType::ByName(snapshot_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Snapshot); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } // Instances @@ -3337,6 +3454,21 @@ impl Nexus { } } +/// For unimplemented endpoints, indicates whether the resource identified +/// by this endpoint will always be publicly visible or not +/// +/// For example, the resource "/images" is well-known (it's part of the +/// API). Being unauthorized to list images will result in a "403 +/// Forbidden". It's `UnimplResourceVisibility::Public'. +/// +/// By contrast, the resource "/images/some-image" is not publicly-known. +/// If you're not authorized to view it, you'll get a "404 Not Found". It's +/// `Unimpl::ProtectedLookup(LookupType::ByName("some-image"))`. +enum Unimpl { + Public, + ProtectedLookup(Error), +} + fn generate_session_token() -> String { // TODO: "If getrandom is unable to provide secure entropy this method will panic." // Should we explicitly handle that? diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 28314c98d0b..10a1988afaf 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -194,6 +194,19 @@ lazy_static! { subnet_name: DEMO_VPC_SUBNET_NAME.clone(), ip: None, }; + + // Images + pub static ref DEMO_IMAGE_NAME: Name = "demo-image".parse().unwrap(); + pub static ref DEMO_IMAGE_URL: String = + format!("/images/{}", *DEMO_IMAGE_NAME); + pub static ref DEMO_IMAGE_CREATE: params::ImageCreate = + params::ImageCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_IMAGE_NAME.clone(), + description: String::from(""), + }, + source: params::ImageSource::Url(String::from("dummy")) + }; } /// Describes an API endpoint to be verified by the "unauthorized" test @@ -257,6 +270,10 @@ pub enum AllowedMethod { /// that we define here or uuids that we control in the test suite (e.g., /// the rack and sled uuids). GetNonexistent, + /// HTTP "GET" method that is not yet implemented + /// + /// This should be a transient state, used only for stub APIs + GetUnimplemented, /// HTTP "POST" method, with sample input (which should be valid input for /// this endpoint) Post(serde_json::Value), @@ -272,6 +289,7 @@ impl AllowedMethod { AllowedMethod::Delete => &Method::DELETE, AllowedMethod::Get => &Method::GET, AllowedMethod::GetNonexistent => &Method::GET, + AllowedMethod::GetUnimplemented => &Method::GET, AllowedMethod::Post(_) => &Method::POST, AllowedMethod::Put(_) => &Method::PUT, } @@ -285,7 +303,8 @@ impl AllowedMethod { match self { AllowedMethod::Delete | AllowedMethod::Get - | AllowedMethod::GetNonexistent => None, + | AllowedMethod::GetNonexistent + | AllowedMethod::GetUnimplemented => None, AllowedMethod::Post(body) => Some(&body), AllowedMethod::Put(body) => Some(&body), } @@ -719,5 +738,26 @@ lazy_static! { serde_json::Value::Null )], }, + + /* Images */ + VerifyEndpoint { + url: "/images", + visibility: Visibility::Public, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IMAGE_CREATE).unwrap() + ), + ], + }, + + VerifyEndpoint { + url: &*DEMO_IMAGE_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Delete, + ], + }, ]; } diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 8f2d8ac1367..13cbe363bf2 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -77,13 +77,11 @@ SUMMARY OF REQUESTS MADE KEY, USING HEADER AND EXAMPLE ROW: - +----------------------------> privileged GET (expects 200) - | (digit = last digit of 200-level - | response) + +----------------------------> privileged GET (expects 200 or 500) + | (digit = last digit of status code) | | +-> privileged GET (expects same as above) - | | (digit = last digit of 200-level - | | response) + | | (digit = last digit of status code) | | ('-' => skipped (N/A)) ^ ^ HEADER: G GET PUT POST DEL TRCE G URL @@ -245,26 +243,47 @@ 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. - let get_allowed = endpoint - .allowed_methods - .iter() - .any(|allowed| matches!(allowed, AllowedMethod::Get)); - let resource_before: Option = if get_allowed { - info!(log, "test: privileged GET"); - record_operation(WhichTest::PrivilegedGet(Some(&http::StatusCode::OK))); - 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)"); - record_operation(WhichTest::PrivilegedGet(None)); - None + let get_allowed = endpoint.allowed_methods.iter().find(|allowed| { + matches!(allowed, AllowedMethod::Get | AllowedMethod::GetUnimplemented) + }); + let resource_before = match get_allowed { + Some(AllowedMethod::Get) => { + info!(log, "test: privileged GET"); + record_operation(WhichTest::PrivilegedGet(Some( + &http::StatusCode::OK, + ))); + Some( + NexusRequest::object_get(client, endpoint.url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(), + ) + } + Some(AllowedMethod::GetUnimplemented) => { + info!(log, "test: privileged GET (unimplemented)"); + let expected_status = http::StatusCode::INTERNAL_SERVER_ERROR; + record_operation(WhichTest::PrivilegedGet(Some(&expected_status))); + NexusRequest::expect_failure( + client, + expected_status, + http::Method::GET, + endpoint.url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); + None + } + Some(_) => unimplemented!(), + None => { + warn!(log, "test: skipping privileged GET (method not allowed)"); + record_operation(WhichTest::PrivilegedGet(None)); + None + } }; print!(" "); diff --git a/nexus/tests/integration_tests/unauthorized_coverage.rs b/nexus/tests/integration_tests/unauthorized_coverage.rs index b76727d7447..6a2e0f1ef48 100644 --- a/nexus/tests/integration_tests/unauthorized_coverage.rs +++ b/nexus/tests/integration_tests/unauthorized_coverage.rs @@ -126,7 +126,8 @@ fn test_unauthorized_coverage() { // If you're here because this assertion failed, check that if you've added // any API operations to Nexus, you've also added a corresponding test in // "unauthorized.rs" so that it will automatically be checked for its - // behavior for unauthenticated and unauthorized users. + // behavior for unauthenticated and unauthorized users. DO NOT SKIP THIS. + // Even if you're just adding a stub, see [`Nexus::unimplemented_todo()`]. assert_contents( "tests/output/uncovered-authz-endpoints.txt", &uncovered_endpoints, diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index ecb7a4d63d4..7952c25beed 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,10 +1,7 @@ API endpoints with no coverage in authz tests: -images_delete_image (delete "/images/{image_name}") project_images_delete_image (delete "/organizations/{organization_name}/projects/{project_name}/images/{image_name}") project_snapshots_delete_snapshot (delete "/organizations/{organization_name}/projects/{project_name}/snapshots/{snapshot_name}") silos_delete_silo (delete "/silos/{silo_name}") -images_get (get "/images") -images_get_image (get "/images/{image_name}") project_images_get (get "/organizations/{organization_name}/projects/{project_name}/images") project_images_get_image (get "/organizations/{organization_name}/projects/{project_name}/images/{image_name}") project_snapshots_get (get "/organizations/{organization_name}/projects/{project_name}/snapshots") @@ -12,7 +9,6 @@ project_snapshots_get_snapshot (get "/organizations/{organization_n session_me (get "/session/me") silos_get (get "/silos") silos_get_silo (get "/silos/{silo_name}") -images_post (post "/images") spoof_login (post "/login") logout (post "/logout") project_images_post (post "/organizations/{organization_name}/projects/{project_name}/images") From 74aeea2895033f7a00a848edf2222675009d2e38 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 09:51:08 -0700 Subject: [PATCH 03/11] add coverage tests for unimplemented endpoints --- nexus/tests/integration_tests/endpoints.rs | 64 ++++++++++++++++++- .../output/uncovered-authz-endpoints.txt | 8 --- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 10a1988afaf..e26c8b08aa3 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -53,8 +53,12 @@ lazy_static! { format!("{}/{}", *DEMO_ORG_PROJECTS_URL, *DEMO_PROJECT_NAME); pub static ref DEMO_PROJECT_URL_DISKS: String = format!("{}/disks", *DEMO_PROJECT_URL); + pub static ref DEMO_PROJECT_URL_IMAGES: String = + format!("{}/images", *DEMO_PROJECT_URL); pub static ref DEMO_PROJECT_URL_INSTANCES: String = format!("{}/instances", *DEMO_PROJECT_URL); + pub static ref DEMO_PROJECT_URL_SNAPSHOTS: String = + format!("{}/snapshots", *DEMO_PROJECT_URL); pub static ref DEMO_PROJECT_URL_VPCS: String = format!("{}/vpcs", *DEMO_PROJECT_URL); pub static ref DEMO_PROJECT_CREATE: params::ProjectCreate = @@ -199,6 +203,8 @@ lazy_static! { pub static ref DEMO_IMAGE_NAME: Name = "demo-image".parse().unwrap(); pub static ref DEMO_IMAGE_URL: String = format!("/images/{}", *DEMO_IMAGE_NAME); + pub static ref DEMO_PROJECT_IMAGE_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_IMAGES, *DEMO_IMAGE_NAME); pub static ref DEMO_IMAGE_CREATE: params::ImageCreate = params::ImageCreate { identity: IdentityMetadataCreateParams { @@ -207,6 +213,19 @@ lazy_static! { }, source: params::ImageSource::Url(String::from("dummy")) }; + + // Snapshots + pub static ref DEMO_SNAPSHOT_NAME: Name = "demo-snapshot".parse().unwrap(); + pub static ref DEMO_SNAPSHOT_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_SNAPSHOTS, *DEMO_SNAPSHOT_NAME); + pub static ref DEMO_SNAPSHOT_CREATE: params::SnapshotCreate = + params::SnapshotCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_SNAPSHOT_NAME.clone(), + description: String::from(""), + }, + disk: DEMO_DISK_NAME.clone(), + }; } /// Describes an API endpoint to be verified by the "unauthorized" test @@ -583,6 +602,49 @@ lazy_static! { ], }, + /* Project images */ + + VerifyEndpoint { + url: &*DEMO_PROJECT_URL_IMAGES, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IMAGE_CREATE).unwrap() + ), + ], + }, + + VerifyEndpoint { + url: &*DEMO_PROJECT_IMAGE_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Delete, + ], + }, + + /* Snapshots */ + + VerifyEndpoint { + url: &*DEMO_PROJECT_URL_SNAPSHOTS, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Post( + serde_json::to_value(DEMO_SNAPSHOT_CREATE.clone()).unwrap(), + ) + ] + }, + VerifyEndpoint { + url: &*DEMO_SNAPSHOT_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Delete, + ] + }, + /* Instances */ VerifyEndpoint { url: &*DEMO_PROJECT_URL_INSTANCES, @@ -740,6 +802,7 @@ lazy_static! { }, /* Images */ + VerifyEndpoint { url: "/images", visibility: Visibility::Public, @@ -750,7 +813,6 @@ lazy_static! { ), ], }, - VerifyEndpoint { url: &*DEMO_IMAGE_URL, visibility: Visibility::Protected, diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index 7952c25beed..37134686de2 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,16 +1,8 @@ API endpoints with no coverage in authz tests: -project_images_delete_image (delete "/organizations/{organization_name}/projects/{project_name}/images/{image_name}") -project_snapshots_delete_snapshot (delete "/organizations/{organization_name}/projects/{project_name}/snapshots/{snapshot_name}") silos_delete_silo (delete "/silos/{silo_name}") -project_images_get (get "/organizations/{organization_name}/projects/{project_name}/images") -project_images_get_image (get "/organizations/{organization_name}/projects/{project_name}/images/{image_name}") -project_snapshots_get (get "/organizations/{organization_name}/projects/{project_name}/snapshots") -project_snapshots_get_snapshot (get "/organizations/{organization_name}/projects/{project_name}/snapshots/{snapshot_name}") session_me (get "/session/me") silos_get (get "/silos") silos_get_silo (get "/silos/{silo_name}") spoof_login (post "/login") logout (post "/logout") -project_images_post (post "/organizations/{organization_name}/projects/{project_name}/images") -project_snapshots_post (post "/organizations/{organization_name}/projects/{project_name}/snapshots") silos_post (post "/silos") From b3827374228055aa2f52f065100f91cd9cd3d6a0 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 09:56:21 -0700 Subject: [PATCH 04/11] stop using expectorate for coverage allowlist --- nexus/tests/integration_tests/unauthorized_coverage.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized_coverage.rs b/nexus/tests/integration_tests/unauthorized_coverage.rs index 6a2e0f1ef48..9636abbfb13 100644 --- a/nexus/tests/integration_tests/unauthorized_coverage.rs +++ b/nexus/tests/integration_tests/unauthorized_coverage.rs @@ -128,10 +128,12 @@ fn test_unauthorized_coverage() { // "unauthorized.rs" so that it will automatically be checked for its // behavior for unauthenticated and unauthorized users. DO NOT SKIP THIS. // Even if you're just adding a stub, see [`Nexus::unimplemented_todo()`]. - assert_contents( - "tests/output/uncovered-authz-endpoints.txt", - &uncovered_endpoints, - ); + // If you _added_ a test that covered an endpoint from the allowlist -- + // hooray! Just delete the corresponding line from this file. + let expected_uncovered_endpoints = + std::fs::read_to_string("tests/output/uncovered-authz-endpoints.txt") + .expect("failed to load file of allowed uncovered endpoints"); + assert_eq!(expected_uncovered_endpoints, uncovered_endpoints); } #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] From 4eec5a65895fb5373d4961beb7d0f61e6271dfdc Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 12:00:46 -0700 Subject: [PATCH 05/11] document unimplemented_todo() better --- nexus/src/lib.rs | 2 +- nexus/src/nexus.rs | 178 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 162 insertions(+), 18 deletions(-) diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 07f3ed06693..1da73d6a968 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -21,7 +21,7 @@ pub mod db; // Public for documentation examples mod defaults; pub mod external_api; // Public for testing pub mod internal_api; // Public for testing -mod nexus; +pub mod nexus; // Public for documentatione xamples mod populate; mod saga_interface; mod sagas; diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index aad21fcf2e8..35cacee868c 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -273,29 +273,173 @@ impl Nexus { } } - /// Used as the body of an unimplemented API endpoint (that we intend to - /// implement) + /// Used as the body of a "stub" endpoint -- one that's currently + /// unimplemented but that we eventually intend to implement /// - /// The body of this function doesn't matter very much, since we're - /// eventually going to replace it with a real implementation. We do a - /// superuser-level authz check to ensure the correct behavior This function - /// checks for superuser privilege, really just so that - // XXX-dap TODO-doc review the above comment - // XXX-dap TODO-doc give examples in case the internal usages are removed by - // the time someone needs to use this - async fn unimplemented_todo( + /// Even though an endpoint is unimplemented, it's useful if it implements + /// the correct authn/authz behaviors behaviors for unauthenticated and + /// authenticated, unauthorized requests. This allows us to maintain basic + /// authn/authz test coverage for stub endpoints, which in turn helps us + /// ensure that all endpoints are covered. + /// + /// In order to implement the correct authn/authz behavior, we need to know + /// a little about the endpoint. This is given by the `visibility` + /// argument. See the examples below. + /// + /// # Examples + /// + /// ## A top-level API endpoint (always visible) + /// + /// For example, "/my-new-kind-of-resource". The assumption is that the + /// _existence_ of this endpoint is not a secret. Use: + /// + /// ``` + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// + /// async fn my_things_list( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// ) -> Result<(), Error> + /// { + /// Err(nexus.unimplemented_todo(opctx, Unimpl::Public).await) + /// } + /// ``` + /// + /// ## An authz-protected resource under the top level + /// + /// For example, "/my-new-kind-of-resource/demo" (where "demo" is the name + /// of a specific resource of type "my-new-kind-of-resource"). Use: + /// + /// ``` + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::model::Name; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// use omicron_common::api::external::LookupType; + /// use omicron_common::api::external::ResourceType; + /// + /// async fn my_thing_fetch( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// the_name: &Name, + /// ) -> Result<(), Error> + /// { + /// // You will want to have defined your OWN ResourceType for this + /// // resource, even though it's still a stub. + /// let resource_type = ResourceType::Snapshot; + /// let lookup_type = LookupType::ByName(the_name.to_string()); + /// let not_found_error = lookup_type.into_not_found(resource_type); + /// let unimp = Unimpl::ProtectedLookup(not_found_error); + /// Err(nexus.unimplemented_todo(opctx, unimp).await) + /// } + /// ``` + /// + /// This does the bare minimum to produce an appropriate 404 "Not Found" + /// error for authenticated, unauthorized users. + /// + /// ## An authz-protected API endpoint under some other (non-stub) resource + /// + /// For example, "/organizations/my-org/my-new-kind-of-resource" or + /// "/organizations/my-org/my-new-kind-of-resource/demo", where "my-org" is + /// the name of an Organization and "demo" is the name of a specific + /// resource of my-new-kind-of-resource. + /// + /// In this case, your function should do whatever lookup of the non-stub + /// resource that the function will eventually do, and then treat it like + /// one of the first two examples. If the URL describes a specific + /// resource, use `Unimpl::ProtectedLookup` as in the second example above. + /// Otherwise, use `Unimpl::Public` as in the first example above. + /// + /// Here's an example stub for the "list" endpoint for a new resource + /// underneath Organizations: + /// + /// ``` + /// use omicron_nexus::authz; + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::lookup::LookupPath; + /// use omicron_nexus::db::model::Name; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// + /// async fn organization_list_my_thing( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// organization_name: &Name, + /// ) -> Result<(), Error> + /// { + /// let (.., _authz_org) = LookupPath::new(opctx, datastore) + /// .organization_name(organization_name) + /// .lookup_for(authz::Action::ListChildren) + /// .await?; + /// Err(nexus.unimplemented_todo(opctx, Unimpl::Public).await) + /// } + /// ``` + /// + /// Here's an example stub for the "get" endpoint for that same resource: + /// + /// ``` + /// use omicron_nexus::authz; + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::lookup::LookupPath; + /// use omicron_nexus::db::model::Name; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// use omicron_common::api::external::LookupType; + /// use omicron_common::api::external::ResourceType; + /// + /// async fn my_thing_fetch( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// organization_name: &Name, + /// the_name: &Name, + /// ) -> Result<(), Error> + /// { + /// let _ = LookupPath::new(opctx, datastore) + /// .organization_name(organization_name) + /// .lookup_for(authz::Action::ListChildren) + /// .await?; + /// // You will want to have defined your OWN ResourceType for this + /// // resource, even though it's still a stub. + /// let resource_type = ResourceType::Snapshot; + /// let lookup_type = LookupType::ByName(the_name.to_string()); + /// let not_found_error = lookup_type.into_not_found(resource_type); + /// let unimp = Unimpl::ProtectedLookup(not_found_error); + /// Err(nexus.unimplemented_todo(opctx, unimp).await) + /// } + /// ``` + /// + /// This isn't _quite_ the lookup that this endpoint will eventually do, + /// but the real one cannot be done until you actually implement the + /// resource. This lookup is close, and sufficient to kick out unauthorized + /// requests with the correct 404. + pub async fn unimplemented_todo( &self, opctx: &OpContext, visibility: Unimpl, ) -> Error { - // Do a super-user-level authz check first. This is really for our - // convenience: it causes this endpoint to do the expected thing for - // unauthenticated and unprivileged users, which in turn allows the - // authz integration test to cover this endpoint. When we do implement - // the endpoint, it will need to correctly authenticate and authorize - // the request. + // Deny access to non-super-users. This is really just for the benefit + // of the authz coverage tests. By requiring (and testing) correct + // authz behavior for stubs, we ensure that that behavior is preserved + // when the stub's implementation is fleshed out. match opctx.authorize(authz::Action::Modify, &authz::FLEET).await { Err(error @ Error::Forbidden) => { + // Emulate the behavior of `Authz::authorize()`: if this is a + // non-public resource, then the user should get a 404, not a + // 403, when authorization fails. if let Unimpl::ProtectedLookup(lookup_error) = visibility { lookup_error } else { @@ -3464,7 +3608,7 @@ impl Nexus { /// By contrast, the resource "/images/some-image" is not publicly-known. /// If you're not authorized to view it, you'll get a "404 Not Found". It's /// `Unimpl::ProtectedLookup(LookupType::ByName("some-image"))`. -enum Unimpl { +pub enum Unimpl { Public, ProtectedLookup(Error), } From dced3d2052040086a3e157d653a7274bf4331245 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 12:12:30 -0700 Subject: [PATCH 06/11] simplify subresource get/delete stubs --- nexus/src/nexus.rs | 94 +++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 63 deletions(-) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 35cacee868c..0685b8afe92 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -347,16 +347,12 @@ impl Nexus { /// /// ## An authz-protected API endpoint under some other (non-stub) resource /// - /// For example, "/organizations/my-org/my-new-kind-of-resource" or - /// "/organizations/my-org/my-new-kind-of-resource/demo", where "my-org" is - /// the name of an Organization and "demo" is the name of a specific - /// resource of my-new-kind-of-resource. + /// ### ... when the endpoint never returns 404 (e.g., "list", "create") /// - /// In this case, your function should do whatever lookup of the non-stub - /// resource that the function will eventually do, and then treat it like - /// one of the first two examples. If the URL describes a specific - /// resource, use `Unimpl::ProtectedLookup` as in the second example above. - /// Otherwise, use `Unimpl::Public` as in the first example above. + /// For example, "/organizations/my-org/my-new-kind-of-resource". In this + /// case, your function should do whatever lookup of the non-stub resource + /// that the function will eventually do, and then treat it like the first + /// example. /// /// Here's an example stub for the "list" endpoint for a new resource /// underneath Organizations: @@ -386,7 +382,10 @@ impl Nexus { /// } /// ``` /// - /// Here's an example stub for the "get" endpoint for that same resource: + /// ### ... when the endpoint can return 404 (e.g., "get", "delete") + /// + /// You can treat this exactly like the second example above. Here's an + /// example stub for the "get" endpoint for that same resource: /// /// ``` /// use omicron_nexus::authz; @@ -408,10 +407,6 @@ impl Nexus { /// the_name: &Name, /// ) -> Result<(), Error> /// { - /// let _ = LookupPath::new(opctx, datastore) - /// .organization_name(organization_name) - /// .lookup_for(authz::Action::ListChildren) - /// .await?; /// // You will want to have defined your OWN ResourceType for this /// // resource, even though it's still a stub. /// let resource_type = ResourceType::Snapshot; @@ -421,11 +416,6 @@ impl Nexus { /// Err(nexus.unimplemented_todo(opctx, unimp).await) /// } /// ``` - /// - /// This isn't _quite_ the lookup that this endpoint will eventually do, - /// but the real one cannot be done until you actually implement the - /// resource. This lookup is close, and sufficient to kick out unauthorized - /// requests with the correct 404. pub async fn unimplemented_todo( &self, opctx: &OpContext, @@ -1114,39 +1104,27 @@ impl Nexus { pub async fn project_image_fetch( &self, opctx: &OpContext, - organization_name: &Name, - project_name: &Name, + _organization_name: &Name, + _project_name: &Name, image_name: &Name, ) -> LookupResult { - let _ = LookupPath::new(opctx, &self.db_datastore) - .organization_name(organization_name) - .project_name(project_name) - .lookup_for(authz::Action::ListChildren) - .await?; let lookup_type = LookupType::ByName(image_name.to_string()); - let error = lookup_type.into_not_found(ResourceType::Image); - Err(self - .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) - .await) + let not_found_error = lookup_type.into_not_found(ResourceType::Image); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } pub async fn project_delete_image( self: &Arc, opctx: &OpContext, - organization_name: &Name, - project_name: &Name, + _organization_name: &Name, + _project_name: &Name, image_name: &Name, ) -> DeleteResult { - let _ = LookupPath::new(opctx, &self.db_datastore) - .organization_name(organization_name) - .project_name(project_name) - .lookup_for(authz::Action::ListChildren) - .await?; let lookup_type = LookupType::ByName(image_name.to_string()); - let error = lookup_type.into_not_found(ResourceType::Image); - Err(self - .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) - .await) + let not_found_error = lookup_type.into_not_found(ResourceType::Image); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } pub async fn project_create_snapshot( @@ -1182,39 +1160,29 @@ impl Nexus { pub async fn snapshot_fetch( &self, opctx: &OpContext, - organization_name: &Name, - project_name: &Name, + _organization_name: &Name, + _project_name: &Name, snapshot_name: &Name, ) -> LookupResult { - let _ = LookupPath::new(opctx, &self.db_datastore) - .organization_name(organization_name) - .project_name(project_name) - .lookup_for(authz::Action::ListChildren) - .await?; let lookup_type = LookupType::ByName(snapshot_name.to_string()); - let error = lookup_type.into_not_found(ResourceType::Snapshot); - Err(self - .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) - .await) + let not_found_error = + lookup_type.into_not_found(ResourceType::Snapshot); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } pub async fn project_delete_snapshot( self: &Arc, opctx: &OpContext, - organization_name: &Name, - project_name: &Name, + _organization_name: &Name, + _project_name: &Name, snapshot_name: &Name, ) -> DeleteResult { - let _ = LookupPath::new(opctx, &self.db_datastore) - .organization_name(organization_name) - .project_name(project_name) - .lookup_for(authz::Action::ListChildren) - .await?; let lookup_type = LookupType::ByName(snapshot_name.to_string()); - let error = lookup_type.into_not_found(ResourceType::Snapshot); - Err(self - .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) - .await) + let not_found_error = + lookup_type.into_not_found(ResourceType::Snapshot); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } // Instances From 6cfb88347315718745b71efdb59f6ffacc29c0ac Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 13:41:06 -0700 Subject: [PATCH 07/11] nit: function naming convention --- nexus/src/external_api/http_entrypoints.rs | 6 +++--- nexus/src/nexus.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 6a7f5be5ab1..677f9a3a38c 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1201,7 +1201,7 @@ async fn images_get( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let images = nexus - .list_images( + .images_list( &opctx, &data_page_params_for(&rqctx, &query)? .map_name(|n| Name::ref_cast(n)), @@ -1233,7 +1233,7 @@ async fn images_post( let new_image_params = &new_image.into_inner(); let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let image = nexus.create_image(&opctx, &new_image_params).await?; + let image = nexus.image_create(&opctx, &new_image_params).await?; Ok(HttpResponseCreated(image.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -1289,7 +1289,7 @@ async fn images_delete_image( let image_name = &path.image_name; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - nexus.delete_image(&opctx, &image_name).await?; + nexus.image_delete(&opctx, &image_name).await?; Ok(HttpResponseDeleted()) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 0685b8afe92..ca90ff664fd 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1031,7 +1031,7 @@ impl Nexus { Ok(()) } - pub async fn list_images( + pub async fn images_list( &self, opctx: &OpContext, _pagparams: &DataPageParams<'_, Name>, @@ -1039,7 +1039,7 @@ impl Nexus { Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } - pub async fn create_image( + pub async fn image_create( self: &Arc, opctx: &OpContext, _params: ¶ms::ImageCreate, @@ -1059,7 +1059,7 @@ impl Nexus { .await) } - pub async fn delete_image( + pub async fn image_delete( self: &Arc, opctx: &OpContext, image_name: &Name, From 90924fe49034139e510ca9661898bd93a9da069e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 13:42:34 -0700 Subject: [PATCH 08/11] nit: authz check --- nexus/src/nexus.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index ca90ff664fd..1c53da38bfc 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1096,7 +1096,7 @@ impl Nexus { let _ = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) .project_name(project_name) - .lookup_for(authz::Action::ListChildren) + .lookup_for(authz::Action::CreateChild) .await?; Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } From 97c9c084ba9c395871a029666a3f1acfb57e2d54 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 13:43:47 -0700 Subject: [PATCH 09/11] nit: don't use such a concrete example that will be wrong when copy/pasted --- nexus/src/nexus.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 1c53da38bfc..8d4741f8218 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -332,9 +332,9 @@ impl Nexus { /// the_name: &Name, /// ) -> Result<(), Error> /// { - /// // You will want to have defined your OWN ResourceType for this - /// // resource, even though it's still a stub. - /// let resource_type = ResourceType::Snapshot; + /// // You will want to have defined your own ResourceType variant for + /// // this resource, even though it's still a stub. + /// let resource_type: ResourceType = todo!(); /// let lookup_type = LookupType::ByName(the_name.to_string()); /// let not_found_error = lookup_type.into_not_found(resource_type); /// let unimp = Unimpl::ProtectedLookup(not_found_error); @@ -407,9 +407,9 @@ impl Nexus { /// the_name: &Name, /// ) -> Result<(), Error> /// { - /// // You will want to have defined your OWN ResourceType for this - /// // resource, even though it's still a stub. - /// let resource_type = ResourceType::Snapshot; + /// // You will want to have defined your own ResourceType variant for + /// // this resource, even though it's still a stub. + /// let resource_type: ResourceType = todo!(); /// let lookup_type = LookupType::ByName(the_name.to_string()); /// let not_found_error = lookup_type.into_not_found(resource_type); /// let unimp = Unimpl::ProtectedLookup(not_found_error); From 595e715b6892142f481222705fb5a8ddf8a6a1c5 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 6 Apr 2022 14:17:14 -0700 Subject: [PATCH 10/11] fix typo Co-authored-by: Justin Bennett --- nexus/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 1da73d6a968..befbf7102d3 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -21,7 +21,7 @@ pub mod db; // Public for documentation examples mod defaults; pub mod external_api; // Public for testing pub mod internal_api; // Public for testing -pub mod nexus; // Public for documentatione xamples +pub mod nexus; // Public for documentation examples mod populate; mod saga_interface; mod sagas; From c4d8e1fbd217a323522893f218a191f47c375f71 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 7 Apr 2022 15:51:35 -0700 Subject: [PATCH 11/11] better comments --- nexus/tests/integration_tests/endpoints.rs | 12 +++++++++++- .../tests/integration_tests/unauthorized_coverage.rs | 5 ++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index e26c8b08aa3..b46f098dd16 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -288,10 +288,20 @@ pub enum AllowedMethod { /// be uncommon. In most cases, resources are identified either by names /// that we define here or uuids that we control in the test suite (e.g., /// the rack and sled uuids). + /// + /// This is not necessary for methods other than `GET`. We only need this + /// to configure the test's expectation for *privileged* requests. For the + /// other HTTP methods, we only make unprivileged requests, and they should + /// always fail in the correct way. GetNonexistent, /// HTTP "GET" method that is not yet implemented /// - /// This should be a transient state, used only for stub APIs + /// This should be a transient state, used only for stub APIs. + /// + /// This is not necessary for methods other than `GET`. We only need this + /// to configure the test's expectation for *privileged* requests. For the + /// other HTTP methods, we only make unprivileged requests, and they should + /// always fail in the correct way. GetUnimplemented, /// HTTP "POST" method, with sample input (which should be valid input for /// this endpoint) diff --git a/nexus/tests/integration_tests/unauthorized_coverage.rs b/nexus/tests/integration_tests/unauthorized_coverage.rs index 9636abbfb13..e92a5af98be 100644 --- a/nexus/tests/integration_tests/unauthorized_coverage.rs +++ b/nexus/tests/integration_tests/unauthorized_coverage.rs @@ -129,7 +129,10 @@ fn test_unauthorized_coverage() { // behavior for unauthenticated and unauthorized users. DO NOT SKIP THIS. // Even if you're just adding a stub, see [`Nexus::unimplemented_todo()`]. // If you _added_ a test that covered an endpoint from the allowlist -- - // hooray! Just delete the corresponding line from this file. + // hooray! Just delete the corresponding line from this file. (Why is this + // not `expectorage::assert_contents`? Because we only expect this file to + // ever shrink, which is easy enough to fix by hand, and we don't want to + // make it easy to accidentally add things to the allowlist.) let expected_uncovered_endpoints = std::fs::read_to_string("tests/output/uncovered-authz-endpoints.txt") .expect("failed to load file of allowed uncovered endpoints");