return 404 rather than 401/403 when user can't even see the object by davepacheco · Pull Request #600 · oxidecomputer/omicron

davepacheco · 2022-01-15T00:25:56Z

When a user attempts to perform some action on an object, and they shouldn't even see that the object exists, they should get a 404 rather than a 401 or 403. (If we return a 403, then we're leaking information to a potential attacker: they can find out whether an Organization exists based on whether they get a 404 or 403 when they try to create a Project inside it.)

I think this is widely understood, but for a reference, see the Enforcement chapter of Oso's Authorization Academy, which says:

The default strategy for handling an authorization failure is to return an HTTP 403 Forbidden message, along with any details that might be helpful for the client.
However, in some cases returning an error message would be revealing information that the user should not otherwise know. For example, suppose a GitClub user visits /org/acme/repo/secret_project – a private repository – then a 403 Forbidden message might reveal to the user that such a repository exists.
Therefore, it is best practice to return an HTTP 404 NotFound message whenever the user is not even allowed to read the resource, regardless of what action they are attempting.

That's essentially what this change does. It took some refactoring to make this work well, especially because our 404 errors have type information about what you're looking up.

david-crespo · 2022-01-15T00:28:03Z

Great, I was just wondering about this while reviewing #592

davepacheco · 2022-01-15T00:28:56Z

+        actor: AnyActor,
+        action: Action,
+    ) -> Result<(), Error> {
+        if action == Action::Read {


This function is the crux of the change. It's invoked by the main authorize() function if authorization fails. Here we're saying if we were already checking for "read", and that failed, then return 404. Otherwise, do a "read" check and return 404 (if that fails) or whatever error authorize() had already picked (which is either 401 or 403 based on whether the user was authenticated).

davepacheco · 2022-01-15T00:31:04Z

+    pub fn organization(
+        &self,
+        organization_id: Uuid,
+        lookup_type: LookupType,


In order to produce the type-specific 404 errors (Error::ObjectNotFound), the authz:: resource structs now have a LookupType that describes how they were looked up in the first place.

For several types, this meant we could no longer derive Eq and PartialEq. Those traits are only used for types for which we do an equality check in the Polar file:

omicron/nexus/src/authz/omicron.polar

Lines 204 to 212 in 2e5c819

# Define relationships

has_relation(fleet: Fleet, "parent_fleet", organization: Organization)

if organization.fleet = fleet;

has_relation(organization: Organization, "parent_organization", project: Project)

if project.organization = organization;

has_relation(project: Project, "parent_project", project_child: ProjectChild)

if project_child.project = project;

has_relation(fleet: Fleet, "parent_fleet", fleet_child: FleetChild)

if fleet_child.fleet = fleet;

For some types, I removed these impls because we didn't need them. For others, I impl'd them by hand in a way that ignores the new LookupType field.

david-crespo · 2022-01-17T16:27:21Z

+        _: AnyActor,
+        _: Action,
+    ) -> Result<(), Error> {
+        Err(error)


What does passing through the error mean in practice? I gather that it means preserve the existing behavior for Fleet (and for Database), but what is the plain-language explanation of what that behavior is? Something like "directly return whatever error is returned by the auth check" ? In other words, why don't fleet and database need the special logic the resources get? Is it because they're never being fetched directly — only as a means to getting something else?

Yes, "directly return whatever error is returned by the auth check". In practice, that will either be a 401 (if credentials weren't provided) or a 403 (if user is authenticated and not authorized). By contrast, for API resources, this does an additional check for "read" to decide whether it should be a 404 instead.

david-crespo · 2022-01-17T17:12:09Z

+     * But we don't really know if you should be able to see the Organization.
+     * Right now, the only real way to tell that is if you have permissions on
+     * anything _inside_ the Organization, which is incredibly expensive to
+     * determine in general.


another point for the READ_THRU optimization (just kidding)

smklein · 2022-01-17T21:00:48Z

+        'd: 'f,
+        'e: 'f;
+
+    /// Invoked on authz failure to determine the final authz result


Why is this method a part of this trait? From what I can tell, we implement Authorize for objects that already implement AuthzApiResource + oso::PolarClass, and only actually use methods related to those objects in the implementation I saw.

Isn't this more related to the resource? Could this be a part of the AuthzApiResource trait instead?

AuthorizedResource (formerly Authorize) is also implemented for Database and Fleet. Those types do not impl ApiResource.

Gotcha - I see that dependency exists more on the callsite of on_unauthorized than the implementation.

smklein · 2022-01-17T21:07:14Z

+/// corresponding ResourceType and is stored in the database
+///
+/// This is a helper trait used to impl [`AuthzResource`].
+pub trait AuthzApiResource: Clone + Send + Sync + 'static {


Are there any objects that implement AuthzApiResource, but don't implement oso::PolarClass?

Would it simplify the traits if AuthzApiResource required the object to implement oso::PolarClass? It kinda seems like that's an expectation.

(FWIW, this currently seems possible, by adding trait AuthzApiResource: oso::PolarClass + ...)

Fixed in 484ae6b.

davepacheco · 2022-01-18T17:26:02Z

Thanks for the reviews, @david-crespo and @smklein! I believe I've addressed the feedback and this is ready for another look.

smklein · 2022-01-18T19:39:17Z

+        'd: 'f,
+        'e: 'f;
+
+    /// Invoked on authz failure to determine the final authz result


Gotcha - I see that dependency exists more on the callsite of on_unauthorized than the implementation.

davepacheco added 3 commits January 14, 2022 15:49

authz checks for things you cannot see ought to return 404s

0d24c25

clean up async functions

03277a0

better factor is_allowed (a little, anyway)

003431d

clean up comment

b568bf7

davepacheco commented Jan 15, 2022

View reviewed changes

reformat

70a0144

davepacheco marked this pull request as ready for review January 15, 2022 00:33

davepacheco requested review from david-crespo and smklein January 15, 2022 00:34

david-crespo reviewed Jan 17, 2022

View reviewed changes

Comment thread nexus/src/authz/context.rs Outdated

david-crespo reviewed Jan 17, 2022

View reviewed changes

smklein reviewed Jan 18, 2022

View reviewed changes

code review feedback

484ae6b

Merge remote-tracking branch 'origin/main' into no-read-404

4170030

smklein approved these changes Jan 18, 2022

View reviewed changes

david-crespo approved these changes Jan 18, 2022

View reviewed changes

davepacheco merged commit 3d132cc into main Jan 18, 2022

davepacheco deleted the no-read-404 branch January 18, 2022 19:55

davepacheco mentioned this pull request Jan 18, 2022

tracking issue for authz work #419

Closed

71 tasks

	# Define relationships
	has_relation(fleet: Fleet, "parent_fleet", organization: Organization)
	if organization.fleet = fleet;
	has_relation(organization: Organization, "parent_organization", project: Project)
	if project.organization = organization;
	has_relation(project: Project, "parent_project", project_child: ProjectChild)
	if project_child.project = project;
	has_relation(fleet: Fleet, "parent_fleet", fleet_child: FleetChild)
	if fleet_child.fleet = fleet;

Conversation

davepacheco commented Jan 15, 2022

Uh oh!

david-crespo commented Jan 15, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

davepacheco commented Jan 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants