report clearer error when authz fails on unregistered resource (#1472)

nexus/src/authz/context.rs

@@ -13,13 +13,16 @@ use crate::context::OpContext;
 use crate::db::DataStore;
 use futures::future::BoxFuture;
 use omicron_common::api::external::Error;
+use omicron_common::bail_unless;
 use oso::Oso;
 use oso::OsoError;
+use std::collections::BTreeSet;
 use std::sync::Arc;
 
 /// Server-wide authorization context
 pub struct Authz {
     oso: Oso,
+    class_names: BTreeSet<String>,
 }
 
 impl Authz {
@@ -30,8 +33,9 @@ impl Authz {
     /// This function panics if we could not load the compiled-in Polar
     /// configuration.  That should be impossible outside of development.
     pub fn new(log: &slog::Logger) -> Authz {
-        let oso = oso_generic::make_omicron_oso(log).expect("initializing Oso");
-        Authz { oso }
+        let oso_init =
+            oso_generic::make_omicron_oso(log).expect("initializing Oso");
+        Authz { oso: oso_init.oso, class_names: oso_init.class_names }
     }
 
     // TODO-cleanup This should not be exposed outside the `authz` module.
@@ -77,8 +81,29 @@ impl Context {
         resource: Resource,
     ) -> Result<(), Error>
     where
-        Resource: AuthorizedResource + Clone,
+        Resource: AuthorizedResource + oso::PolarClass + Clone,
     {
+        // If we're given a resource whose PolarClass was never registered with
+        // Oso, then the call to `is_allowed()` below will always return false
+        // (indicating that the actor does not have permissions).  That will
+        // cause this function to return an authz failure error (401, 403, or
+        // 404, depending on the context).  This is never what we intend.
+        // What's likely happened is that somebody forgot to register the class
+        // with Oso.  This failure mode is very hard to debug because the Rust
+        // code generates a valid Polar snippet and there's a working PolarClass
+        // impl -- it's just that neither was ever given to Oso.  Make this
+        // failure mode more debuggable by reporting a 500 with a clear error
+        // message.  After all, this is a bug.  (We could panic, since it's more
+        // of a programmer error than an operational error.  But unlike most
+        // programmer errors, the nature of the problem and the blast radius are
+        // well understood, so we may as well avoid crashing.)
+        let class_name = &Resource::get_polar_class().name;
+        bail_unless!(
+            self.authz.class_names.contains(class_name),
+            "attempted authz check on unregistered resource: {:?}",
+            class_name
+        );
+
         let mut roles = RoleSet::new();
         resource
             .load_roles(opctx, &self.datastore, &self.authn, &mut roles)
@@ -282,4 +307,72 @@ mod test {
         db.cleanup().await.unwrap();
         logctx.cleanup_successful();
     }
+
+    #[tokio::test]
+    async fn test_unregistered_resource() {
+        let logctx = dev::test_setup_log("test_unregistered_resource");
+        let mut db = test_setup_database(&logctx.log).await;
+        let (opctx, datastore) =
+            crate::db::datastore::datastore_test(&logctx, &db).await;
+
+        // Define a resource that we "forget" to register with Oso.
+        use super::AuthorizedResource;
+        use crate::authz::actor::AnyActor;
+        use crate::authz::roles::RoleSet;
+        use crate::context::OpContext;
+        use omicron_common::api::external::Error;
+        use oso::PolarClass;
+        #[derive(Clone, PolarClass)]
+        struct UnregisteredResource;
+        impl AuthorizedResource for UnregisteredResource {
+            fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>(
+                &'a self,
+                _: &'b OpContext,
+                _: &'c DataStore,
+                _: &'d authn::Context,
+                _: &'e mut RoleSet,
+            ) -> futures::future::BoxFuture<'f, Result<(), Error>>
+            where
+                'a: 'f,
+                'b: 'f,
+                'c: 'f,
+                'd: 'f,
+                'e: 'f,
+            {
+                // authorize() shouldn't get far enough to call this.
+                unimplemented!();
+            }
+
+            fn on_unauthorized(
+                &self,
+                _: &Authz,
+                _: Error,
+                _: AnyActor,
+                _: Action,
+            ) -> Error {
+                // authorize() shouldn't get far enough to call this.
+                unimplemented!();
+            }
+        }
+
+        // Make sure an authz check with this resource fails with a clear
+        // message.
+        let unregistered_resource = UnregisteredResource {};
+        let authz_privileged = authz_context_for_actor(
+            &logctx.log,
+            authn::Context::privileged_test_user(),
+            Arc::clone(&datastore),
+        );
+        let error = authz_privileged
+            .authorize(&opctx, Action::Read, unregistered_resource)
+            .await;
+        println!("{:?}", error);
+        assert!(matches!(error, Err(Error::InternalError {
+            internal_message
+        }) if internal_message == "attempted authz check \
+            on unregistered resource: \"UnregisteredResource\""));
+
+        db.cleanup().await.unwrap();
+        logctx.cleanup_successful();
+    }
 }

nexus/src/authz/oso_generic.rs

@@ -13,12 +13,14 @@ use super::Authz;
 use crate::authn;
 use crate::context::OpContext;
 use crate::db::DataStore;
+use anyhow::ensure;
 use anyhow::Context;
 use futures::future::BoxFuture;
 use futures::FutureExt;
 use omicron_common::api::external::Error;
 use oso::Oso;
 use oso::PolarClass;
+use std::collections::BTreeSet;
 use std::fmt;
 
 /// Base Polar configuration describing control plane authorization rules
@@ -32,10 +34,70 @@ pub(super) struct Init {
     pub polar_class: oso::Class,
 }
 
+/// Represents an initialized Oso environment
+pub struct OsoInit {
+    pub oso: Oso,
+    pub class_names: BTreeSet<String>,
+}
+
+/// Manages initialization of Oso environment, including Polar snippets and
+/// Polar classes
+pub struct OsoInitBuilder {
+    log: slog::Logger,
+    oso: Oso,
+    class_names: BTreeSet<String>,
+    snippets: Vec<&'static str>,
+}
+
+impl OsoInitBuilder {
+    pub fn new(log: slog::Logger) -> OsoInitBuilder {
+        OsoInitBuilder {
+            log,
+            oso: Oso::new(),
+            class_names: BTreeSet::new(),
+            snippets: vec![OMICRON_AUTHZ_CONFIG_BASE],
+        }
+    }
+
+    /// Registers a class that either has no associated Polar snippet or whose
+    /// snippet is part of the base file
+    pub fn register_class(
+        mut self,
+        c: oso::Class,
+    ) -> Result<Self, anyhow::Error> {
+        info!(self.log, "registering Oso class"; "class" => &c.name);
+        let name = c.name.clone();
+        let new_element = self.class_names.insert(name.clone());
+        ensure!(new_element, "Oso class was already registered: {:?}", &name);
+        self.oso
+            .register_class(c)
+            .with_context(|| format!("registering Oso class {:?}", name))?;
+        Ok(self)
+    }
+
+    /// Registers a class with associated Polar snippet
+    pub(super) fn register_class_with_snippet(
+        mut self,
+        init: Init,
+    ) -> Result<Self, anyhow::Error> {
+        self.snippets.push(init.polar_snippet);
+        self.register_class(init.polar_class)
+    }
+
+    pub fn build(mut self) -> Result<OsoInit, anyhow::Error> {
+        let polar_config = self.snippets.join("\n");
+        info!(self.log, "full Oso configuration"; "config" => &polar_config);
+        self.oso
+            .load_str(&polar_config)
+            .context("loading Polar (Oso) config")?;
+        Ok(OsoInit { oso: self.oso, class_names: self.class_names })
+    }
+}
+
 /// Returns an Oso handle suitable for authorizing using Omicron's authorization
 /// rules
-pub fn make_omicron_oso(log: &slog::Logger) -> Result<Oso, anyhow::Error> {
-    let mut oso = Oso::new();
+pub fn make_omicron_oso(log: &slog::Logger) -> Result<OsoInit, anyhow::Error> {
+    let mut oso_builder = OsoInitBuilder::new(log.clone());
     let classes = [
         // Hand-written classes
         Action::get_polar_class(),
@@ -49,8 +111,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result<Oso, anyhow::Error> {
         DeviceAuthRequestList::get_polar_class(),
     ];
     for c in classes {
-        info!(log, "registering Oso class"; "class" => &c.name);
-        oso.register_class(c).context("registering class")?;
+        oso_builder = oso_builder.register_class(c)?;
     }
 
     // Generated by the `authz_resource!` macro
@@ -82,19 +143,11 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result<Oso, anyhow::Error> {
         GlobalImage::init(),
     ];
 
-    let polar_config = std::iter::once(OMICRON_AUTHZ_CONFIG_BASE)
-        .chain(generated_inits.iter().map(|init| init.polar_snippet))
-        .collect::<Vec<&'static str>>()
-        .join("\n");
-
     for init in generated_inits {
-        info!(log, "registering Oso class"; "class" => &init.polar_class.name);
-        oso.register_class(init.polar_class).context("registering class")?;
+        oso_builder = oso_builder.register_class_with_snippet(init)?;
     }
 
-    info!(log, "full Oso configuration"; "config" => &polar_config);
-    oso.load_str(&polar_config).context("loading Polar (Oso) config")?;
-    Ok(oso)
+    oso_builder.build()
 }
 
 /// Describes an action being authorized
@@ -235,3 +288,30 @@ impl AuthorizedResource for Database {
         error
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::OsoInitBuilder;
+    use crate::authz::Action;
+    use omicron_test_utils::dev;
+    use oso::PolarClass;
+
+    #[test]
+    fn test_duplicate_polar_classes() {
+        let logctx = dev::test_setup_log("test_duplicate_polar_classes");
+        let oso_builder = OsoInitBuilder::new(logctx.log.clone());
+        let oso_builder =
+            oso_builder.register_class(Action::get_polar_class()).unwrap();
+        match oso_builder.register_class(Action::get_polar_class()) {
+            Ok(_) => panic!("failed to detect duplicate class"),
+            Err(error) => {
+                println!("{:#}", error);
+                assert_eq!(
+                    error.to_string(),
+                    "Oso class was already registered: \"Action\""
+                );
+            }
+        };
+        logctx.cleanup_successful();
+    }
+}

nexus/src/context.rs

@@ -453,7 +453,7 @@ impl OpContext {
         resource: &Resource,
     ) -> Result<(), Error>
     where
-        Resource: AuthorizedResource + Debug + Clone,
+        Resource: AuthorizedResource + Debug + Clone + oso::PolarClass,
     {
         // TODO-cleanup In an ideal world, Oso would consume &Action and
         // &Resource.  Instead, it consumes owned types.  As a result, they're

nexus/src/db/datastore/role.rs

@@ -157,7 +157,7 @@ impl DataStore {
     // is mitigated because we cap the number of role assignments per resource
     // pretty tightly.
     pub async fn role_assignment_fetch_visible<
-        T: authz::ApiResourceWithRoles + Clone,
+        T: authz::ApiResourceWithRoles + Clone + oso::PolarClass,
     >(
         &self,
         opctx: &OpContext,
@@ -203,7 +203,7 @@ impl DataStore {
         new_assignments: &[shared::RoleAssignment<T::AllowedRoles>],
     ) -> ListResultVec<db::model::RoleAssignment>
     where
-        T: authz::ApiResourceWithRolesType + Clone,
+        T: authz::ApiResourceWithRolesType + Clone + oso::PolarClass,
     {
         // TODO-security We should carefully review what permissions are
         // required for modifying the policy of a resource.