Skip to content

add authz checks for top-level Organization endpoints #592

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 6 commits into from
Jan 14, 2022
Merged

add authz checks for top-level Organization endpoints #592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
33b1624
protect organization API endpoints
davepacheco Jan 12, 2022
0b0bb87
clean up some XXXs
davepacheco Jan 12, 2022
b7f0b85
clean up organization lookup interfaces
davepacheco Jan 13, 2022
70d6dd0
typo
davepacheco Jan 13, 2022
760718e
update oxapi_demo
davepacheco Jan 13, 2022
b9845ea
self-review nit
davepacheco Jan 13, 2022
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
122 changes: 80 additions & 42 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,17 +263,45 @@ impl DataStore {
})
}

/// Lookup a organization by name.
pub async fn organization_fetch(
/// Fetches an Organization from the database and returns both the database
/// row and an authz::Organization for doing authz checks
///
/// There are a few different ways this can be used:
///
/// * If a code path wants to use anything in the database row _aside_ from
/// the id, it should do an authz check for `authz::Action::Read` on the
/// returned [`authz::Organization`]. `organization_fetch()` does this,
/// for example.
/// * If a code path is only doing this lookup to get the id so that it can
/// look up something else inside the Organization, then the database
/// record is not record -- and neither is an authz check on the
Copy link
Collaborator

Choose a reason for hiding this comment

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

"record is not record"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. Fixed in #616.

/// Organization. Callers usually use `organization_lookup_id()` for
/// this. That function does not expose the database row to the caller.
///
/// Callers in this bucket should still do _some_ authz check. Clients
/// must not be able to discover whether an Organization exists with a
/// particular name. It's just that we don't know at this point whether
/// they're allowed to see the Organization. It depends on whether, as we
/// look up things inside the Organization, we find something that they
/// _are_ able to see.
///
/// **This is an internal-only function.** This function cannot know what
/// authz checks are required. As a result, it should not be made
/// accessible outside the DataStore. It should always be wrapped by
/// something that does the appropriate authz check.
// TODO-security We should refactor things so that it's harder to
// accidentally mark this "pub" or otherwise expose database data without
// doing an authz check.
async fn organization_lookup_noauthz(
&self,
name: &Name,
) -> LookupResult<Organization> {
) -> LookupResult<(authz::Organization, Organization)> {
use db::schema::organization::dsl;
dsl::organization
.filter(dsl::time_deleted.is_null())
.filter(dsl::name.eq(name.clone()))
.select(Organization::as_select())
.first_async::<Organization>(self.pool())
.get_result_async::<Organization>(self.pool())
.await
.map_err(|e| {
public_error_from_diesel_pool(
Expand All @@ -282,10 +310,42 @@ impl DataStore {
LookupType::ByName(name.as_str().to_owned()),
)
})
.map(|o| (authz::FLEET.organization(o.id()), o))
}

/// Look up the id for an organization based on its name
///
/// Returns an [`authz::Organization`] (which makes the id available).
///
/// This function does no authz checks because it is not possible to know
/// just by looking up an Organization's id what privileges are required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this point. Couldn't you check for the read permission? Are you accounting for the possibility that someone could access a sub-resource without having read permission on the organization?

I wonder if that warrants another permission that's like READ_THRU that indicates the ability to use the ID to look up sub-resources. Feels like overkill, though. It would correlate one-to-one with having an actual READ/etc permission on a sub-resource, so I suppose in some sense it would be an optimization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't think we want to give you full read permissions on the Organization just because you have some permission on something inside the Organization.

We could create a read_thru like you suggest. That permission would be granted implicitly by having any role on anything inside the Organization. To know if you have that, we'd have to either (a) load all the rules that you have on all objects, and for each one, check if it's inside that Organization [which might involve several hops -- from Instance to Project to Organization, for example]. Or (b) load all the resources in the Organization and check for each one whether you have roles on it. Both of these are potentially extremely expensive. It's worse: if you're looking up an Instance, have to do all that for the Organization, then do it all again for the Project. We could denormalize the representation so that we could more quickly know what permissions people have, but that would make other operations much more expensive (e.g., moving a Project from one Organization to another).

I think not creating a permission for this more closely resembles the user experience, too: users get access to, say, a Project. That doesn't mean they can do anything with the Organization aside from seeing that it exists because it happens to be part of the Project's path. (On the other hand, maybe every user implicitly has some read access to their own parent Organization, and that lets them see some basic stuff. But if someone were to share a specific Project in another Organization with you, I think you should still be able to look it up without somebody giving you some extra other permission on the Organization.)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's all fine, seems like the optimal middle ground. We'll see how we can make it harder to accidentally make the noauth thing pub, or (maybe riskier since it does not require making it pub) to use it in some other datastore function without the appropriate auth check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. My (vague, hand-wavy) thinking here is to put the noauth functions into a submodule that itself is not exposed.

Another idea I thought of is to have the fields in the db models all private, with an authz-protected interface for getting a similar struct with the same fields "pub" or a trait that provides access. Either way, we'd do an authz check before giving an object with that data. This seems tricky and annoying (yet another type called Organization?!) but there might be something worthwhile here.

pub async fn organization_lookup_id(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the same as the old organization_lookup() function, but using the common organization_lookup_noauthz() and renamed for clarity.

&self,
name: &Name,
) -> LookupResult<authz::Organization> {
self.organization_lookup_noauthz(name).await.map(|(o, _)| o)
}

/// Lookup an organization by name.
pub async fn organization_fetch(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes from the old organization_fetch() function:

  • uses the common organization_lookup_noauthz function
  • does a "Read" authz check
  • returning an authz::Organization for subsequent authz checks.

&self,
opctx: &OpContext,
name: &Name,
) -> LookupResult<(authz::Organization, Organization)> {
let (authz_org, db_org) =
self.organization_lookup_noauthz(name).await?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm totally on-board with the "noauthz" prefix, but is it necessary to use both "fetch" and "lookup" as different verbs to access the organization?

We have:

  • Get an Organization, without authZ
  • Get an Organization ID, without authZ
  • Get an Organization, with authZ

Couldn't these be:

  • organization_lookup_noauthz
  • organization_lookup_id
  • organization_lookup (the "authz") is implied

?

I know this is kinda a nit, but given the large surface area of the DB, it seems like an as-simple-as-possible naming convention will be worthwhile investing into.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha, I think maybe what you find clearer is what I find more confusing. If it's okay, I'll think about this some more as I iterate on the other lookup functions. I expect these to evolve, potentially a lot: at the very least, we're going to be adding lookup-by-id, for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Narrowing my question, when do you view "fetch" as an appropriate verb vs "lookup"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll explain my intuition below, with the caveat that I don't think my intuition here is necessarily based on a good reason. It's probably a product of the systems I've worked on before. I can see how a reader might not have the same intuition and I'm on board for changing the names. I'm just not sure how to do it yet and I don't think it makes sense to spend a lot of time on it right now because I think a bunch of upcoming changes are likely to invalidate whatever convention we try to establish now based only on these three functions.

"fetch" to me suggests:

  • a public interface (because it's a high-level-sounding operation)
  • that it's authorization-protected (since it's a public interface)
  • that it accepts as input whatever unique identifier the caller uses to refer to objects
  • that it returns the whole record

"lookup" to me is much more general. It could be an internal or external function. I wouldn't have expectations about whether authorization is done. It might accept any kind of criteria (not even necessarily an identifier) and return any number of objects or even some field from some object.

I don't like the specific thing you suggested because the function names look so similar that I think I'd find it hard to keep track of which ones do authz. (After all, you said the "authz" is implied in organization_lookup, but it's not implied in organization_lookup_by_id, even though it doesn't have _noauthz.)

Copy link
Contributor

@david-crespo david-crespo Jan 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For the record, the first time I read through your explanation, I found it strange. Now it makes a lot of sense to me. Not sure what that means, but I find the distinction you draw—between external-facing auth-gated functions that return a whole resource and internal non-auth-gated functions that return IDs and/or auth metadata about a resource—to be a coherent one, so I think assigning the two sides names make sense. We may have to get over some prior intuitions about what these words mean but that's ok. I have a feeling lookup and fetch are about as good as it's going to get. We have the same problem coming up with words for "thing": object, resource, model, row, data, etc.

// TODO-security See the note in authz::authorize(). This needs to
// return a 404, not a 403.
opctx.authorize(authz::Action::Read, authz_org).await?;
Ok((authz_org, db_org))
}

/// Delete a organization
pub async fn organization_delete(&self, name: &Name) -> DeleteResult {
pub async fn organization_delete(
&self,
opctx: &OpContext,
name: &Name,
) -> DeleteResult {
use db::schema::organization::dsl;
use db::schema::project;

Expand All @@ -303,6 +363,11 @@ impl DataStore {
)
})?;

// TODO-cleanup TODO-security This should use a more common lookup
// function.
let authz_org = authz::FLEET.organization(id);
opctx.authorize(authz::Action::Delete, authz_org).await?;

// Make sure there are no projects present within this organization.
let project_found = diesel_pool_result_optional(
project::dsl::project
Expand Down Expand Up @@ -352,39 +417,6 @@ impl DataStore {
Ok(())
}

/// Look up an organization by name
pub async fn organization_lookup(
&self,
name: &Name,
) -> Result<authz::Organization, Error> {
use db::schema::organization::dsl;
dsl::organization
.filter(dsl::time_deleted.is_null())
.filter(dsl::name.eq(name.clone()))
.select(dsl::id)
.get_result_async::<Uuid>(self.pool())
.await
.map_err(|e| {
public_error_from_diesel_pool(
e,
ResourceType::Organization,
LookupType::ByName(name.as_str().to_owned()),
)
})
.map(|o| authz::FLEET.organization(o))
}

/// Look up the id for a organization based on its name
///
/// As endpoints move to doing authorization, they should move to
/// [`organization_lookup()`] instead of this function.
pub async fn organization_lookup_id_by_name(
&self,
name: &Name,
) -> Result<Uuid, Error> {
self.organization_lookup(name).await.map(|o| o.id())
}

pub async fn organizations_list_by_id(
&self,
opctx: &OpContext,
Expand Down Expand Up @@ -430,17 +462,21 @@ impl DataStore {
/// Updates a organization by name (clobbering update -- no etag)
pub async fn organization_update(
&self,
opctx: &OpContext,
name: &Name,
updates: OrganizationUpdate,
) -> UpdateResult<Organization> {
use db::schema::organization::dsl;

let (authz_org, _) = self.organization_lookup_noauthz(name).await?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use organization_lookup_id here?

I know it's functionally the same, but it seems like a good pattern to minimize the number of calls to organization_lookup_noauthz until after the auth check is done - at which point, we could just call organization_lookup anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes -- good call! And nice catch. Fixed in #616.

opctx.authorize(authz::Action::Modify, authz_org).await?;

diesel::update(dsl::organization)
.filter(dsl::time_deleted.is_null())
.filter(dsl::name.eq(name.clone()))
.filter(dsl::id.eq(authz_org.id()))
.set(updates)
.returning(Organization::as_returning())
.get_result_async(self.pool())
.get_result_async(self.pool_authorized(opctx).await?)
.await
.map_err(|e| {
public_error_from_diesel_pool(
Expand Down Expand Up @@ -2383,8 +2419,10 @@ mod test {
);
let org = authz::FLEET.organization(organization.id());
datastore.project_create(&opctx, &org, project).await.unwrap();
let organization_after_project_create =
datastore.organization_fetch(organization.name()).await.unwrap();
let (_, organization_after_project_create) = datastore
.organization_fetch(&opctx, organization.name())
.await
.unwrap();
assert!(organization_after_project_create.rcgen > organization.rcgen);

db.cleanup().await.unwrap();
Expand Down
9 changes: 7 additions & 2 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,9 @@ async fn organizations_get_organization(
let path = path_params.into_inner();
let organization_name = &path.organization_name;
let handler = async {
let organization = nexus.organization_fetch(&organization_name).await?;
let opctx = OpContext::for_external_api(&rqctx).await?;
let organization =
nexus.organization_fetch(&opctx, &organization_name).await?;
Ok(HttpResponseOk(organization.into()))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
Expand All @@ -311,7 +313,8 @@ async fn organizations_delete_organization(
let params = path_params.into_inner();
let organization_name = &params.organization_name;
let handler = async {
nexus.organization_delete(&organization_name).await?;
let opctx = OpContext::for_external_api(&rqctx).await?;
nexus.organization_delete(&opctx, &organization_name).await?;
Ok(HttpResponseDeleted())
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
Expand Down Expand Up @@ -340,8 +343,10 @@ async fn organizations_put_organization(
let path = path_params.into_inner();
let organization_name = &path.organization_name;
let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let new_organization = nexus
.organization_update(
&opctx,
&organization_name,
&updated_organization.into_inner(),
)
Expand Down
Loading