diff --git a/nexus/src/authz/context.rs b/nexus/src/authz/context.rs index dffc32c47c..bc024dc4f8 100644 --- a/nexus/src/authz/context.rs +++ b/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, } 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(); + } } diff --git a/nexus/src/authz/oso_generic.rs b/nexus/src/authz/oso_generic.rs index 80cad8b4a1..adf9ae3e0d 100644 --- a/nexus/src/authz/oso_generic.rs +++ b/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, +} + +/// Manages initialization of Oso environment, including Polar snippets and +/// Polar classes +pub struct OsoInitBuilder { + log: slog::Logger, + oso: Oso, + class_names: BTreeSet, + 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 { + 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.snippets.push(init.polar_snippet); + self.register_class(init.polar_class) + } + + pub fn build(mut self) -> Result { + 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 { - let mut oso = Oso::new(); +pub fn make_omicron_oso(log: &slog::Logger) -> Result { + 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 { 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 { GlobalImage::init(), ]; - let polar_config = std::iter::once(OMICRON_AUTHZ_CONFIG_BASE) - .chain(generated_inits.iter().map(|init| init.polar_snippet)) - .collect::>() - .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(); + } +} diff --git a/nexus/src/context.rs b/nexus/src/context.rs index 28ea5caab8..5de6d44c26 100644 --- a/nexus/src/context.rs +++ b/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 diff --git a/nexus/src/db/datastore/role.rs b/nexus/src/db/datastore/role.rs index 08a2e6cd17..d2d06974ed 100644 --- a/nexus/src/db/datastore/role.rs +++ b/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], ) -> ListResultVec 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.