Skip to content

use pool_authorized() for all lookups #873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2022
Merged

use pool_authorized() for all lookups #873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions nexus/src/authz/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator Author

@davepacheco davepacheco Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a small but important change.

Prior to this, if you failed an authz check, the logic went like this:

  1. Pick a "fallback" error to signify "you didn't have permissions to do this". This would be 401 if you were unauthenticated and 403 if you're authenticated.
  2. Check with the resource impl (via the call to on_unauthorized at L109). The only interesting impl is this one, which is used for most API resources. Pass the fallback error, but return whatever the resource says to return.

The intent of the on_unauthorized call is to determine if you're allowed to even know this resource exists. If so, the "fallback error" (401 or 403) is fine. If not, return a 404 instead. The on_unauthorized impl I linked above is what actually does this.

After this change, the 401 always takes precedence. If you fail the authz check and you're not authenticated, you always get a 401 -- we don't invoke on_unauthorized() at all. If you're authenticated, then we do the same as before: 403 if you're allowed to read the resource, 404 otherwise.

This change isn't strictly necessary. I did this for consistency of the public API. If we just change lookups to use pool_authorized(), then all the endpoints that do a lookup behave this way without this change here. But the endpoints that don't do lookups for whatever reason (see #845) would still return 404s in some cases for unauthenticated users that fail an authz check. I thought it better to put this logic here for consistency. If you ever add a new endpoint (whether it hits the database or not), and the user fails an authz check, and they're not authenticated, they'll get a 401, instead of having it be dependent on how the endpoint was implemented.

// 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,
)
})
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ConnectionManager<DbConnection>> {
fn pool(&self) -> &bb8::Pool<ConnectionManager<DbConnection>> {
self.pool.pool()
}

Expand Down
19 changes: 5 additions & 14 deletions nexus/src/db/db-macros/src/lookup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())) },
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
9 changes: 3 additions & 6 deletions nexus/tests/integration_tests/unauthorized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down