[nexus] Split authn/authz and db-fixed-data into new crates #5849

smklein · 2024-05-31T23:16:31Z

As a part of the ongoing effort to split Nexus into smaller pieces, this PR splits out two new crates:

nexus-auth takes the contents of nexus/db-queries/src/auth{n,z}, as well as nexus/db-queries/src/context.rs, and separates this logic into a new bespoke crate. Although this crate does have a dependency on the datastore itself, it only actually invokes a single method, and can be abstracted via a new trait, defined in nexus/auth/storage.
nexus-db-fixed-data takes the contents of nexus/db-queries/src/db/fixed-data and separates this logic into a new crate.

smklein · 2024-05-31T23:18:09Z

nexus/auth/src/authn/silos.rs

@@ -68,57 +62,6 @@ pub enum IdentityProviderType {
    Saml(SamlIdentityProvider),
 }

-impl IdentityProviderType {


This method was moved into nexus/db-queries/src/db/datastore/identity_provider.rs - it doesn't need to be defined here, and is tightly coupled with the datastore, not auth logic.

nexus/auth/src/authz/api_resources.rs

smklein · 2024-05-31T23:21:21Z

nexus/auth/src/authz/context.rs

@@ -51,7 +52,6 @@ impl Authz {
        self.oso.is_allowed(actor.clone(), action, resource.clone())
    }

-    #[cfg(test)]


The policy_test (look for the files changed in this PR) was previously a "unit test" in db-queries that used auth and datastore methods extensively.

It has stayed in db-queries to access the database methods, but still needs to access methods defined in nexus-auth. As a result, some cfg(test) methods are exposed as pub to make this work. This is one such example.

smklein · 2024-05-31T23:22:29Z

nexus/auth/src/authz/context.rs

@@ -66,18 +66,22 @@ impl Authz {
 pub struct Context {
    authn: Arc<authn::Context>,
    authz: Arc<Authz>,
-    datastore: Arc<DataStore>,
+    datastore: Arc<dyn Storage>,


This is another common pattern through this PR: Wherever nexus/auth depends on the DataStore, I basically use dependency injection via dyn Storage to not directly depend on the datastore. Then, the implementation of Storage allows nexus/auth to access a very limited subset of the datastore.

Turns out, that "limited interface" is only a single method. See: nexus/auth/src/storage.rs

smklein · 2024-05-31T23:36:52Z

nexus/auth/src/authz/context.rs

+    struct FakeStorage {}
+
+    impl FakeStorage {
+        fn new() -> Arc<dyn crate::storage::Storage> {
+            Arc::new(Self {})
+        }
+    }
+
+    #[async_trait::async_trait]
+    impl crate::storage::Storage for FakeStorage {
+        async fn role_asgn_list_for(
+            &self,
+            _opctx: &OpContext,
+            _identity_type: IdentityType,
+            _identity_id: Uuid,
+            _resource_type: ResourceType,
+            _resource_id: Uuid,
+        ) -> Result<Vec<RoleAssignment>, Error> {
+            todo!();
+        }
+    }


This is an example of a test that actively does not need access to the database, and can be replaced by a stub.

smklein · 2024-05-31T23:38:44Z

nexus/auth/src/context.rs

+    struct FakeStorage {}
+
+    impl FakeStorage {
+        fn new() -> Arc<dyn crate::storage::Storage> {
+            Arc::new(Self {})
+        }
+    }
+
+    #[async_trait::async_trait]
+    impl crate::storage::Storage for FakeStorage {
+        async fn role_asgn_list_for(
+            &self,
+            _opctx: &OpContext,
+            _identity_type: IdentityType,
+            _identity_id: Uuid,
+            _resource_type: ResourceType,
+            _resource_id: Uuid,
+        ) -> Result<Vec<RoleAssignment>, Error> {
+            todo!();
+        }
+    }


Another example of a test that previously required a whole database, but actually, never queries anything from the database

smklein · 2024-05-31T23:40:30Z

nexus/auth/src/storage.rs

+#[async_trait::async_trait]
+pub trait Storage: Send + Sync {


I'd be interested in figuring out a way to make this not use async-trait.

I basically went down this route because:

authz::Context contains one of these objects

If it uses generics, then OpContext also needs a generic parameter

That changes a lot of code

... so I went down the dyn Storage route instead, as the DataStore is already wrapped in an Arc

smklein · 2024-05-31T23:40:55Z

nexus/db-fixed-data/src/project.rs

 use nexus_types::external_api::params;
 use omicron_common::api::external::IdentityMetadataCreateParams;
 use once_cell::sync::Lazy;

+/// The name of the built-in Project and VPC for Oxide services.
+pub const SERVICES_DB_NAME: &str = "oxide-services";


This is just moved, not new

smklein · 2024-05-31T23:41:41Z

nexus/db-queries/src/db/datastore/auth.rs

+impl Storage for super::DataStore {
+    /// Return the built-in roles that the given built-in user has for the given
+    /// resource
+    async fn role_asgn_list_for(


This method was moved exactly from nexus/db-queries/src/db/datastore/role.rs

nit: could spell out assign, but I also understand not wanting to introduce that to the diff if this is used in a lot of places.

Grepping for role_asgn, this appears to be a pattern used beyond this method. I agree with you that I'd rather it be fully spelled out, but I'm gonna punt on this to avoid touching several files that aren't currently part of this PR?

That works for me

smklein · 2024-05-31T23:42:55Z

nexus/db-queries/src/db/datastore/identity_provider.rs

 use omicron_common::api::external::ResourceType;
 use ref_cast::RefCast;

 impl DataStore {
+    pub async fn identity_provider_lookup(


This method was moved exactly from IdentityProvider::lookup to here (and DataStore is now a &self parameter instead of the first argument)

hawkw

Overall, this seems good to me, although I had a few small style suggestions --- take them or leave them! And, I've mostly tried to avoid commenting on the code that was moved around but hasn't actually changed.

hawkw · 2024-05-31T23:47:48Z

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

@@ -9,6 +9,7 @@ use super::SiloAuthnPolicy;
 use crate::authn;
 use async_trait::async_trait;
 use authn::Reason;
+use slog::trace;


i'm assuming this is because there used to be a `#[macro_use] somewhere?

Exactly:

omicron/nexus/db-queries/src/lib.rs

Lines 14 to 17 in b0dfd53

#[macro_use]

extern crate slog;

#[macro_use]

extern crate newtype_derive;

nexus/auth/src/authz/api_resources.rs

nexus/auth/src/authz/context.rs

hawkw · 2024-05-31T23:56:41Z

nexus/auth/src/authz/oso_generic.rs

@@ -172,8 +172,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result<OsoInit, anyhow::Error> {
 ///
 /// There's currently just one enum of Actions for all of Omicron.  We expect
 /// most objects to support mostly the same set of actions.
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
-#[cfg_attr(test, derive(strum::EnumIter))]


We could, potentially, make the strum dependency optional and make this:

#[cfg_attr(feature = "strum", derive(strum::EnumIter)]

and then have policy_test enable the strum feature in its dev-dependency on authz...but i'm not sure if that's really worth the effort, up to you.

nexus/db-fixed-data/Cargo.toml

andrewjstone

Looks great!

andrewjstone · 2024-06-01T00:12:24Z

nexus/db-queries/src/db/datastore/auth.rs

+impl Storage for super::DataStore {
+    /// Return the built-in roles that the given built-in user has for the given
+    /// resource
+    async fn role_asgn_list_for(


nit: could spell out assign, but I also understand not wanting to introduce that to the diff if this is used in a lot of places.

andrewjstone · 2024-06-01T00:14:36Z

nexus/db-queries/src/policy_test/resource_builder.rs

@@ -242,7 +242,39 @@ macro_rules! impl_dyn_authorized_resource_for_global {
    };
 }

-impl_dyn_authorized_resource_for_global!(authz::oso_generic::Database);
+impl_dyn_authorized_resource_for_resource!(authz::AddressLot);


smklein added 7 commits May 31, 2024 12:44

Split authn and authz out of db-queries (still need to do tests)

b76bacf

working through tests

e7264c4

tests converted

58e552e

Reduce deps

b274d17

Lockfile

63ca0a0

hakari, less pub interfaces

805c090

Rely on FakeStorage more

a1868ac

smklein commented May 31, 2024

View reviewed changes

smklein marked this pull request as ready for review May 31, 2024 23:43

smklein requested review from andrewjstone, jgallagher and hawkw May 31, 2024 23:46

oso is only a dev-dependency of nexus/db-queries now

9967156

hawkw approved these changes May 31, 2024

View reviewed changes

review feedback

deb573e

andrewjstone approved these changes Jun 1, 2024

View reviewed changes

smklein enabled auto-merge (squash) June 1, 2024 01:41

smklein merged commit 450f906 into main Jun 1, 2024
21 checks passed

smklein deleted the authn-authz-splitup branch June 1, 2024 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nexus] Split authn/authz and db-fixed-data into new crates #5849

[nexus] Split authn/authz and db-fixed-data into new crates #5849

smklein commented May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

smklein May 31, 2024

andrewjstone Jun 1, 2024

smklein Jun 1, 2024

andrewjstone Jun 1, 2024

smklein May 31, 2024

hawkw left a comment

hawkw May 31, 2024

smklein Jun 1, 2024

hawkw May 31, 2024

andrewjstone left a comment

andrewjstone Jun 1, 2024

andrewjstone Jun 1, 2024

	#[macro_use]
	extern crate slog;
	#[macro_use]
	extern crate newtype_derive;

[nexus] Split authn/authz and db-fixed-data into new crates #5849

[nexus] Split authn/authz and db-fixed-data into new crates #5849

Conversation

smklein commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment