authz: first cut by davepacheco · Pull Request #346 · oxidecomputer/omicron

davepacheco · 2021-10-28T23:38:58Z

This change:

supersedes prototype OpContext #331. This one does something more useful with the OpContext than what was there. (I left out the part of prototype OpContext #331 that uses the OpContext in saga recovery, but I'd like to do that in a follow-on PR -- and have it actually authorize all of the database queries!)
introduces a small subsystem authorization within Nexus based on Oso
adds authorization for POST /organizations, just as an example

More details:

adds OpContext, with an easy way to construct one from a Dropshot request. This authenticates the request.
adds OpContext::authorize, which I think is an ergonomic way to do authz checks
This function is built on authz::Context, a per-operation authorization context that bundles an authn::Context and authz::Authz, the server-wide handle used for authorization (which is basically just a wrapper around Oso)

For a quick summary of what it looks like to protect an endpoint, take a look at these diffs:
https://github.com/oxidecomputer/omicron/pull/346/files#diff-40c86e07f5a9940ab4c92d785e701e507538a87d7f3c19045ef47a3885ad27e5L230-R234
https://github.com/oxidecomputer/omicron/pull/346/files#diff-21bcb54baeab3bca25f4c5bd8a2754d6d9bedb7347995dc1e1cbc487673c64a3R397-R403
https://github.com/oxidecomputer/omicron/pull/346/files#diff-f5f8df6872a0c2cc4cbc022eb186649c66c8aee85c51329b639736a82424d305R147-R162

Tasks:

basic infrastructure for authorizing requests
protect one endpoint
logging of authz attempts and results
update oxapi_demo to use spoof authn for endpoints that now require it
automated tests
follow-ups:
- issue/PR to use OpContext in saga recovery
- figure out how to structure this so that we return 404 for resources that users aren't supposed to be able to see. Current thinking: if authz fails, we do a second check for the "read" action, and if that fails, we produce a 404 instead of a 403. This only works if we use "read" everywhere for this purpose, which I think makes sense to do.
- figure out the set of resources, roles, and policies we need for v1
- figure out the data structures we need in the database to implement ^
- figure out how to protect read, list, update, and delete operations in the absence of Oso's data filtering

davepacheco · 2021-10-28T23:50:41Z

+    }
+}
+
+// Define newtypes for the various types that we want to impl PolarClass for.


I'm not sure the best way to deal with this. What I've done here works, but is a bunch of boilerplate per-resource. However, the PolarClass derive that Oso provides is pretty limited. For example, it doesn't call set_equality_check for types that impl PartialEq and Eq. We may want to write our own proc macro and apply it directly to the database types.

davepacheco · 2021-10-29T00:14:43Z

This definitely needs some work but I'm interested in feedback from anybody who wants to take a look before I go too far down this path. (Maybe @smklein?)

If it looks okay, I'll plan to add some tests, integrate it, and then iterate on it.

andrewjstone

@davepacheco This looks really good. It nicely wraps up authz and serves as a good intro to Oso. I think it's fine to continue with.

My only partial concern is the createXXX actions. Something about them feels wrong to me, but I could be off. Perhaps we could ask an Oso engineer about this.

andrewjstone · 2021-11-02T17:45:07Z

+# "admin" role on Organizations they create.  They cannot necessarily even see
+# Organizations created by other Fleet administrators.)
+resource Fleet {
+	permissions = [ "create_organization" ];


This permission seems overly specific to me, but you may be correct here, because I can't see how you could make it more general on the Fleet resource. My gut tells me you want just a create permission, perhaps on an organization, which would then be authorized by the parent fleet, but I'm not sure if that's possible.

I just haven't seen any specific examples in Oso docs that are this specific or that actually create things in polar. I just looked around for a bit.

Yeah, this is a good question. There's a lot about the policy I'm not sure about. The reason I went with separate "create" permissions is mainly for consistency with Projects, where you can create Instances, Disks, VPCs, and other things that someone might want to control separately.

andrewjstone · 2021-11-02T17:46:36Z

+/// Polar configuration describing control plane authorization rules
+pub const OMICRON_AUTHZ_CONFIG: &str = include_str!("omicron.polar");
+
+/// Returns an Oso handle suitable for authorizing using to Omicron's


typo: using to

Fixed in 7ef635e.

bnaecker · 2021-11-02T19:30:17Z

 	(set -o xtrace; curl -sSi "$OXAPI_URL$path" "$@" | json -ga)
 }

+function do_curl_authz


Seems like this should be do_curl_authn?

Ha -- fair point. I thought of this as "do_curl for an endpoint that's protected by authorization", but yeah, it makes more sense to think of it as "do_curl with authn". Fixed in 7ef635e.

luqmana · 2021-11-03T12:48:34Z

 #![allow(clippy::style)]

 pub mod authn; // Public only for testing
+mod authz; // Public only for testing


Old comment?

Thanks! Fixed in 7ef635e.

luqmana

Thanks Dave! Oso is pretty cool from what I can see so far! (Barring the whole, needing to clone thing I suppose). Seems like a good starting point for authz support.

davepacheco · 2021-11-03T15:53:48Z

Thanks @andrewjstone @bnaecker and @luqmana for taking a look! I'll plan to sync up, add some tests, and integrate soon.

davepacheco · 2021-11-04T16:27:37Z

I'm just about ready to land this. Relative to my last comment, I made these changes:

sync'd up with "main"
first-classed the idea of two test-only users: one with all privileges and one with none. I wouldn't say this is final but it's a way to move forward with testing of authz itself as well as testing endpoints that are protected by authz.
fixed existing organization tests by using the new privileged test user
added some tests for the newly-authorized "POST /organizations" endpoint that check what happens when you pass no credentials, bad credentials, or valid credentials for an unauthorized user
fixed a bug uncovered by those tests (we were returning 403 when a user didn't provide creds, which is wrong)
added a basic unit test for the new policy. (I don't want to go too far here because we're going to be iterating a lot, but at least we have a place to put some assertions now.)
add tests to OpContext that do a basic sanity check of the privileges for internal contexts

I'd be interested in folks' feedback on these parts too. At the same time, with people feeling above like the change is a step in the right direction, I think it's important to get this into "main" sooner rather than later. If you've got feedback on this after I land it, please let me know!

david-crespo · 2021-11-04T16:45:48Z

-    .await
+    };
+    let authn_header =
+        http::HeaderValue::from_static(omicron_nexus::authn::TEST_USER_UUID);


This is in keeping with making everything work the same and ignore authz, which is appropriate for now, but I have a feeling we'll eventually want to specify this from outside in tests, not build it into the post helper. We'll have to find a way to make it ergonomic in the caller. Could be as simple as adding a headers arg here and making a headers var at the top of the test to use a bunch of times (in cases where we don't want to specify anything unusual.)

I see you've done something like this with try_create_organization. Might be worth trying to unify the two using an optional headers arg. This is related to the general need for better test helpers. oxidecomputer/dropshot#165

Hm. Seeing now how it's used, I can see we rely on creating an org for setup practically everywhere, and we definitely don't want to worry about auth in those contexts. That gives me a couple more ideas:

If we rely on orgs and projects in basically every test, I wonder if it makes sense to bundle up seeding some basic set of resources into a single helper, which would obviate the question of making it ergonomic to create orgs one at a time in tests.

If we don't want to do that, guess we should make it a convention that unqualified create_* helpers will just work

I think these are good points, and I'd like to see some more examples (i.e., convert some more endpoints to use authz) before trying to decide on an answer.

david-crespo · 2021-11-04T16:48:29Z

+/// User id reserved for a test user that's granted many privileges for the
+/// purpose of running automated tests.
+// "4007" looks a bit like "root".
+pub const TEST_USER_UUID: &str = "001de000-05e4-0000-0000-000000004007";


love the first class test UUIDs. I'd rather have something in the name here about this being intended to be a privileged user. the fact that it's not UNPRIVILEGED isn't enough unless you're seeing both together

david-crespo · 2021-11-04T16:58:14Z

+        let apictx = rqctx.context();
+        let authn = Arc::new(apictx.external_authn.authn_request(rqctx).await?);
+        let authz =
+            authz::Context::new(Arc::clone(&authn), Arc::clone(&apictx.authz));


I like the idea of bundling these two calls together. I want to confirm my understanding of the behavior around failing early due to authn. Failed is an error, so we'll exit early thanks to the ?, but Unauthenticated is not:

omicron/nexus/src/authn/external/mod.rs

Line 76 in 986a8b7

Ok(authn::Context { kind: authn::Kind::Unauthenticated, schemes_tried })

That means we'll get to the authz check even if we don't have a user, which we need because there may be actions that are allowed for unauthenticated users, and if we don't let unauthenticated users through to the authz check they'll never be allowed to do anything. Correct?

https://github.com/oxidecomputer/omicron/pull/346/files#diff-9ed003ec23a0f189be4ac9885715d07037b1235420abd72f802b39701a40662dR65

I think that's all right.

david-crespo · 2021-11-04T16:59:35Z

+
+    created_instant: Instant,
+    created_walltime: SystemTime,
+    metadata: BTreeMap<String, String>,


it'll be important to avoid relying on anything in here for real logic (if my understanding is correct that's it's meant only for logging)

Yes, though it's still a bit vague to me. Right now, the intent is just to look at this in the debugger or expose it in a DTrace probe or something like that. At some point we may support authz based on various key-value conditions (that's part of RFD 43) and we may want to use this for that. But if so I think we'll want guard rails around that.

davepacheco added 9 commits October 27, 2021 13:53

premerge

2e8c946

first enforcement works!

e8a4cab

cleanup: OpContext, database perm

e0fc1d9

cleanup

65b3adb

more cleanup

a1e1899

a bit of comment cleanup

ffa341e

oxapi_demo: some help with authn

47bd051

protect org_create instead of org_get

d201966

after further consideration, punt

bc33c50

davepacheco commented Oct 28, 2021

View reviewed changes

edits

6e5d9d3

andrewjstone approved these changes Nov 2, 2021

View reviewed changes

david-crespo mentioned this pull request Nov 2, 2021

Fake creds for dev/test/qa #355

Closed

bnaecker reviewed Nov 2, 2021

View reviewed changes

luqmana reviewed Nov 3, 2021

View reviewed changes

luqmana approved these changes Nov 3, 2021

View reviewed changes

code review feedback

7ef635e

davepacheco added 10 commits November 3, 2021 08:57

Merge branch 'main' into oso

d31d10f

build moved Cargo.lock

a63f535

fix datastore unit tests

4896aae

Merge branch 'main' into oso

07d64db

fix integration tests now that creating orgs requires authn

471a93c

add automated tests

73c2d6f

remove constant literal from omicron.polar

e197322

fix formatting

7b49521

fix authn test

c0b60d5

add basic policy test

88d7ebc

davepacheco marked this pull request as ready for review November 4, 2021 16:38

david-crespo approved these changes Nov 4, 2021

View reviewed changes

rename TEST_USER to reflect that it is privileged

e7b07c9

davepacheco merged commit 2ec1cf4 into main Nov 4, 2021

davepacheco deleted the oso branch November 4, 2021 19:28

This was referenced Nov 4, 2021

use OpContext during saga recovery #366

Merged

prototype OpContext #331

Closed

davepacheco mentioned this pull request Nov 23, 2021

tracking issue for authz work #419

Closed

71 tasks

Conversation

davepacheco commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Oct 29, 2021

Uh oh!

andrewjstone 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luqmana left a comment

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Nov 3, 2021

Uh oh!

davepacheco commented Nov 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 4, 2021 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davepacheco commented Oct 28, 2021 •

edited

Loading

david-crespo Nov 4, 2021 •

edited

Loading