Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use the correct Silo for resource lookups #937

Merged
merged 6 commits into from
Apr 19, 2022
Merged

use the correct Silo for resource lookups #937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9db0fcf
first cut
davepacheco Apr 15, 2022
ccf976f
fixes
davepacheco Apr 15, 2022
93ae1d1
check the resource's silo
davepacheco Apr 18, 2022
ddd3cc5
organization_create should accept and check silo cred
davepacheco Apr 18, 2022
2e751f0
rustfmt
davepacheco Apr 18, 2022
774d9d2
Merge branch 'main' into use-the-silo
davepacheco Apr 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/src/api/external/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use uuid::Uuid;
/// General best practices for error design apply here. Where possible, we want
/// to reuse existing variants rather than inventing new ones to distinguish
/// cases that no programmatic consumer needs to distinguish.
#[derive(Debug, Deserialize, thiserror::Error, PartialEq, Serialize)]
#[derive(Clone, Debug, Deserialize, thiserror::Error, PartialEq, Serialize)]
pub enum Error {
/// An object needed as part of this operation was not found.
#[error("Object (of type {lookup_type:?}) not found: {type_name}")]
Expand Down
12 changes: 10 additions & 2 deletions nexus/src/authn/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ pub use crate::db::fixed_data::user_builtin::USER_TEST_PRIVILEGED;
pub use crate::db::fixed_data::user_builtin::USER_TEST_UNPRIVILEGED;
use crate::db::model::ConsoleSession;

use crate::authz;
use omicron_common::api::external::LookupType;
use serde::Deserialize;
use serde::Serialize;
use uuid::Uuid;
Expand Down Expand Up @@ -81,8 +83,14 @@ impl Context {

pub fn silo_required(
&self,
) -> Result<Uuid, omicron_common::api::external::Error> {
self.actor_required().map(|actor| actor.silo_id)
) -> Result<authz::Silo, omicron_common::api::external::Error> {
self.actor_required().map(|actor| {
authz::Silo::new(
authz::FLEET,
actor.silo_id,
LookupType::ById(actor.silo_id),
)
})
}

/// Returns the list of schemes tried, in order
Expand Down
38 changes: 24 additions & 14 deletions nexus/src/authz/actor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use super::roles::RoleSet;
use crate::authn;
use omicron_common::api::external::LookupType;
use omicron_common::api::external::ResourceType;
use uuid::Uuid;

Expand All @@ -14,18 +15,15 @@ use uuid::Uuid;
#[derive(Clone, Debug)]
pub struct AnyActor {
authenticated: bool,
actor_id: Option<Uuid>,
actor_info: Option<(Uuid, Uuid)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want a little struct with fields silo_id and actor_id here, since it'd be very easy to mix up which UUID is which (actor vs. Silo) in this tuple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was on the fence about it. I generally agree. But since all uses are within a few lines of this one, I thought it'd be less clear to have another named struct just for it. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I agree with @plotnick

roles: RoleSet,
}

impl AnyActor {
pub fn new(authn: &authn::Context, roles: RoleSet) -> Self {
let actor = authn.actor();
AnyActor {
authenticated: actor.is_some(),
actor_id: actor.map(|a| a.id),
roles,
}
let actor_info = actor.map(|a| (a.id, a.silo_id));
AnyActor { authenticated: actor.is_some(), actor_info, roles }
}
}

Expand All @@ -36,8 +34,9 @@ impl oso::PolarClass for AnyActor {
a.authenticated
})
.add_attribute_getter("authn_actor", |a: &AnyActor| {
a.actor_id.map(|actor_id| AuthenticatedActor {
a.actor_info.map(|(actor_id, silo_id)| AuthenticatedActor {
actor_id,
silo_id,
roles: a.roles.clone(),
})
})
Expand All @@ -48,6 +47,7 @@ impl oso::PolarClass for AnyActor {
#[derive(Clone, Debug)]
pub struct AuthenticatedActor {
actor_id: Uuid,
silo_id: Uuid,
roles: RoleSet,
}

Expand All @@ -73,12 +73,22 @@ impl Eq for AuthenticatedActor {}

impl oso::PolarClass for AuthenticatedActor {
fn get_polar_class_builder() -> oso::ClassBuilder<Self> {
oso::Class::builder().with_equality_check().add_constant(
AuthenticatedActor {
actor_id: authn::USER_DB_INIT.id,
roles: RoleSet::new(),
},
"USER_DB_INIT",
)
oso::Class::builder()
.with_equality_check()
.add_constant(
AuthenticatedActor {
actor_id: authn::USER_DB_INIT.id,
silo_id: authn::USER_DB_INIT.silo_id,
roles: RoleSet::new(),
},
"USER_DB_INIT",
)
.add_attribute_getter("silo", |a: &AuthenticatedActor| {
super::Silo::new(
super::FLEET,
a.silo_id,
LookupType::ById(a.silo_id),
)
})
}
}
4 changes: 2 additions & 2 deletions nexus/src/authz/api_resources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ impl AuthorizedResource for ConsoleSessionList {

authz_resource! {
name = "Organization",
parent = "Fleet",
parent = "Silo",
primary_key = Uuid,
roles_allowed = true,
polar_snippet = Custom,
Expand Down Expand Up @@ -371,7 +371,7 @@ authz_resource! {
parent = "Fleet",
primary_key = Uuid,
roles_allowed = true,
polar_snippet = FleetChild,
polar_snippet = Custom,
}

authz_resource! {
Expand Down
41 changes: 35 additions & 6 deletions nexus/src/authz/omicron.polar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource)

#
# Permissions and predefined roles for resources in the
# Fleet/Organization/Project hierarchy
# Fleet/Silo/Organization/Project hierarchy
#
# For now, we define the following permissions for most resources in the system:
#
Expand Down Expand Up @@ -79,8 +79,11 @@ has_role(actor: AuthenticatedActor, role: String, resource: Resource)
# The complete set of predefined roles:
#
# - fleet.admin (superuser for the whole system)
# - fleet.collaborator (can create and own orgs)
# - fleet.collaborator (can create and own silos)
# - fleet.viewer (can read fleet-wide data)
# - silo.admin (superuser for the silo)
# - silo.collaborator (can create and own orgs)
# - silo.viewer (can read silo-wide data)
# - organization.admin (complete control over an organization)
# - organization.collaborator (can create, modify, and delete projects)
# - project.admin (complete control over a project)
Expand Down Expand Up @@ -125,6 +128,32 @@ resource Fleet {
"modify" if "admin";
}

resource Silo {
permissions = [
"list_children",
"modify",
"read",
"create_child",
];
roles = [ "admin", "collaborator", "viewer" ];

"list_children" if "viewer";
"read" if "viewer";

"viewer" if "collaborator";
"create_child" if "collaborator";
"collaborator" if "admin";
"modify" if "admin";
relations = { parent_fleet: Fleet };
"admin" if "admin" on "parent_fleet";
"collaborator" if "collaborator" on "parent_fleet";
"viewer" if "viewer" on "parent_fleet";
}
has_relation(fleet: Fleet, "parent_fleet", silo: Silo)
if silo.fleet = fleet;
has_role(actor: AuthenticatedActor, "viewer", silo: Silo)
if actor.silo = silo;

resource Organization {
permissions = [
"list_children",
Expand All @@ -148,11 +177,11 @@ resource Organization {
"collaborator" if "admin";
"modify" if "admin";

relations = { parent_fleet: Fleet };
"admin" if "admin" on "parent_fleet";
relations = { parent_silo: Silo };
"admin" if "admin" on "parent_silo";
}
has_relation(fleet: Fleet, "parent_fleet", organization: Organization)
if organization.fleet = fleet;
has_relation(silo: Silo, "parent_silo", organization: Organization)
if organization.silo = silo;

resource Project {
permissions = [
Expand Down
58 changes: 32 additions & 26 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,14 +516,15 @@ impl DataStore {
pub async fn organization_create(
&self,
opctx: &OpContext,
organization: Organization,
organization: &params::OrganizationCreate,
) -> CreateResult<Organization> {
use db::schema::organization::dsl;

opctx.authorize(authz::Action::CreateChild, &authz::FLEET).await?;
let authz_silo = opctx.authn.silo_required()?;
opctx.authorize(authz::Action::CreateChild, &authz_silo).await?;

use db::schema::organization::dsl;
let silo_id = authz_silo.id();
let organization = Organization::new(organization.clone(), silo_id);
let name = organization.name().as_str().to_string();
let silo_id = organization.silo_id();

Silo::insert_resource(
silo_id,
Expand All @@ -533,7 +534,11 @@ impl DataStore {
.await
.map_err(|e| match e {
AsyncInsertError::CollectionNotFound => Error::InternalError {
internal_message: format!("attempting to create an organization under non-existent silo {}", silo_id),
internal_message: format!(
"attempting to create an \
organization under non-existent silo {}",
silo_id
),
},
AsyncInsertError::DatabaseError(e) => {
public_error_from_diesel_pool(
Expand Down Expand Up @@ -603,10 +608,13 @@ impl DataStore {
opctx: &OpContext,
pagparams: &DataPageParams<'_, Uuid>,
) -> ListResultVec<Organization> {
let authz_silo = opctx.authn.silo_required()?;
opctx.authorize(authz::Action::ListChildren, &authz_silo).await?;

use db::schema::organization::dsl;
opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?;
paginated(dsl::organization, dsl::id, pagparams)
.filter(dsl::time_deleted.is_null())
.filter(dsl::silo_id.eq(authz_silo.id()))
.select(Organization::as_select())
.load_async::<Organization>(self.pool_authorized(opctx).await?)
.await
Expand All @@ -618,10 +626,13 @@ impl DataStore {
opctx: &OpContext,
pagparams: &DataPageParams<'_, Name>,
) -> ListResultVec<Organization> {
use db::schema::organization::dsl;
let authz_silo = opctx.authn.silo_required()?;
opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?;

use db::schema::organization::dsl;
paginated(dsl::organization, dsl::name, pagparams)
.filter(dsl::time_deleted.is_null())
.filter(dsl::silo_id.eq(authz_silo.id()))
.select(Organization::as_select())
.load_async::<Organization>(self.pool_authorized(opctx).await?)
.await
Expand Down Expand Up @@ -2726,9 +2737,7 @@ mod test {
use crate::db::explain::ExplainableAsync;
use crate::db::identity::Resource;
use crate::db::lookup::LookupPath;
use crate::db::model::{
ConsoleSession, DatasetKind, Organization, Project,
};
use crate::db::model::{ConsoleSession, DatasetKind, Project};
use crate::external_api::params;
use chrono::{Duration, Utc};
use nexus_test_utils::db::test_setup_database;
Expand All @@ -2746,18 +2755,15 @@ mod test {
let logctx = dev::test_setup_log("test_project_creation");
let mut db = test_setup_database(&logctx.log).await;
let (opctx, datastore) = datastore_test(&logctx, &db).await;
let organization = Organization::new(
params::OrganizationCreate {
identity: IdentityMetadataCreateParams {
name: "org".parse().unwrap(),
description: "desc".to_string(),
},
let organization = params::OrganizationCreate {
identity: IdentityMetadataCreateParams {
name: "org".parse().unwrap(),
description: "desc".to_string(),
},
*SILO_ID,
);
};

let organization =
datastore.organization_create(&opctx, organization).await.unwrap();
datastore.organization_create(&opctx, &organization).await.unwrap();

let project = Project::new(
organization.id(),
Expand All @@ -2768,12 +2774,12 @@ mod test {
},
},
);
let org = authz::Organization::new(
authz::FLEET,
organization.id(),
LookupType::ById(organization.id()),
);
datastore.project_create(&opctx, &org, project).await.unwrap();
let (.., authz_org) = LookupPath::new(&opctx, &datastore)
.organization_id(organization.id())
.lookup_for(authz::Action::CreateChild)
.await
.unwrap();
datastore.project_create(&opctx, &authz_org, project).await.unwrap();

let (.., organization_after_project_create) =
LookupPath::new(&opctx, &datastore)
Expand Down
Loading