From c8d98d3593cf85d1d677b4390ac7afde6b8c50ce Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 4 Apr 2022 13:56:13 -0700 Subject: [PATCH] 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.