add authz checks for top-level Organization endpoints #592

davepacheco · 2022-01-13T00:45:45Z

This change adds authz checks for:

GET /organizations/$org
PUT /organizations/$org
DELETE /organizations/$org

(POST /organizations and GET /organizations already have authz checks.)

In doing this, I've refactored the DataStore's functions for looking up an organization. From first principles, there are a few different use cases:

I'm looking up an Organization in order to do something with it directly (i.e., read all its properties, update it, or delete it). This requires an appropriate authorization check on the Organization resource itself.
I'm looking up an Organization solely as part of looking up some more deeply nested resource (e.g., a Project, Disk, Instance, etc.). In this case, there's no authz check to be done at the Organization level. You're always allowed to do this lookup, provided that you're eventually going to look up something inside the Organization. If that lookup fails (e.g., the Project wasn't found), then the client should get back a 404 and they shouldn't be able to tell if it was the Organization or Project that was missing.

I found the existing functions confusing. I'll note details inline in the code. To summarize:

organization_fetch(name) fetches the whole db::model::Organization, does an authz check, and returns an authz::Organization so that you can do more authz checks as needed. This is intended for the case where you're doing something directly with the Organization (see above).
organization_lookup_id(name) now fetches the organization's id as an authz::Organization for subsequent authz checks. This is intended for the use case where you're looking up the Organization in order to look up something else inside it (see above).
organization_lookup_noauthz(name): internal function that fetches the db::model::Organization and produces an authz::Organization, but which does no authz check of its own. This is private to the DataStore -- it's exposed through these other two functions instead, which make it easier to do the appropriate authz checks.

I'm planning to use this same pattern for other kinds of objects. Eventually, I hope to refactor the _noauthz() versions of these functions in a way that will make it harder to misuse them. But that's hard to do while there are still so many callers that haven't been updated for authz.

davepacheco · 2022-01-13T00:47:37Z

nexus/src/db/datastore.rs

+    ///
+    /// 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.
+    pub async fn organization_lookup_id(


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

davepacheco · 2022-01-13T00:48:44Z

nexus/src/db/datastore.rs

+    }
+
+    /// Lookup an organization by name.
+    pub async fn organization_fetch(


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.

davepacheco · 2022-01-13T00:55:32Z

nexus/src/nexus.rs

@@ -574,8 +581,9 @@ impl Nexus {
    ) -> LookupResult<db::model::Project> {
        let organization_id = self
            .db_datastore
-            .organization_lookup_id_by_name(organization_name)
-            .await?;
+            .organization_lookup_id(organization_name)


In this file, there were a whole bunch of callers of organization_lookup_id_by_name(name), which returned a plain Uuid for the Organization. Rather than keep that confusing extra function, since the transformation is so easy, I just updated all the callers to use organization_lookup_id(name) instead.

Most (all?) of these callers are doing this lookup in order to look up something else inside the Organization. We will eventually want to change those lookup functions to accept an authz::Organization rather than an Organization uuid so that when you finally reach the thing you're looking for (say, an authz::Project), you can do the appropriate access check. I'm trying to do this incrementally, so for now, I've not changed the behavior of these call sites.

david-crespo · 2022-01-13T18:12:32Z

nexus/src/nexus.rs

-            .await?;
+            .organization_lookup_id(organization_name)
+            .await?
+            .id();


straightforward enough

david-crespo · 2022-01-13T18:22:37Z

nexus/src/db/datastore.rs

+    /// 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.


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.

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.)

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.

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.

smklein

Apologies for the delay in reviewing this - looks good, with just a couple comments.

smklein · 2022-01-17T18:56:08Z

nexus/src/db/datastore.rs

+    ///   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


"record is not record"?

Nice catch. Fixed in #616.

smklein · 2022-01-17T19:04:36Z

nexus/src/db/datastore.rs

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


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.

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.

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

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.)

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.

smklein · 2022-01-17T19:09:57Z

nexus/src/db/datastore.rs

        name: &Name,
        updates: OrganizationUpdate,
    ) -> UpdateResult<Organization> {
        use db::schema::organization::dsl;

+        let (authz_org, _) = self.organization_lookup_noauthz(name).await?;


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.

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

davepacheco added 6 commits January 12, 2022 15:42

protect organization API endpoints

33b1624

clean up some XXXs

0b0bb87

clean up organization lookup interfaces

b7f0b85

typo

70d6dd0

update oxapi_demo

760718e

self-review nit

b9845ea

davepacheco commented Jan 13, 2022

View reviewed changes

davepacheco marked this pull request as ready for review January 13, 2022 00:57

davepacheco requested review from david-crespo and smklein January 13, 2022 00:57

davepacheco mentioned this pull request Jan 13, 2022

tracking issue for authz work #419

Closed

71 tasks

david-crespo reviewed Jan 13, 2022

View reviewed changes

nexus/src/nexus.rs

.await?;

.organization_lookup_id(organization_name)

.await?

.id();

Copy link

Contributor

david-crespo Jan 13, 2022

Choose a reason for hiding this comment

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

straightforward enough

david-crespo reviewed Jan 13, 2022

View reviewed changes

david-crespo approved these changes Jan 13, 2022

View reviewed changes

davepacheco merged commit ed94b52 into main Jan 14, 2022

davepacheco deleted the authz-orgs branch January 14, 2022 00:11

david-crespo mentioned this pull request Jan 15, 2022

return 404 rather than 401/403 when user can't even see the object #600

Merged

smklein reviewed Jan 17, 2022

View reviewed changes

This was referenced Jan 21, 2022

fixups from #592 #616

Merged

authz: protect GET for Projects (and prepare for more) #618

Merged

davepacheco mentioned this pull request Feb 1, 2022

authz: update disk lookup and protect disk get/attach/detach #662

Merged

add authz checks for top-level Organization endpoints #592

add authz checks for top-level Organization endpoints #592

Uh oh!

Conversation

davepacheco commented Jan 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-crespo Jan 22, 2022 •

edited

Loading