From 33bf999ae890a9cf0138fcd126d04bf401710e5e Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Mon, 11 Apr 2022 10:11:23 -0600 Subject: [PATCH 01/16] SSH public keys --- common/src/api/external/error.rs | 6 + common/src/api/external/mod.rs | 1 + common/src/sql/dbinit.sql | 27 +++ nexus/src/authz/api_resources.rs | 24 +- nexus/src/authz/omicron.polar | 23 +- nexus/src/authz/oso_generic.rs | 1 + nexus/src/db/datastore.rs | 127 +++++++++- nexus/src/db/lookup.rs | 54 ++++- nexus/src/db/model.rs | 28 ++- nexus/src/db/schema.rs | 13 + nexus/src/external_api/http_entrypoints.rs | 117 ++++++++- nexus/src/external_api/params.rs | 13 + nexus/src/external_api/tag-config.json | 6 + nexus/src/external_api/views.rs | 23 ++ nexus/src/nexus.rs | 52 ++++ nexus/tests/integration_tests/endpoints.rs | 34 +++ nexus/tests/output/nexus_tags.txt | 7 + openapi/nexus.json | 261 +++++++++++++++++++++ 18 files changed, 792 insertions(+), 25 deletions(-) diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index 04e7f9dae3..15a56b6c63 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -93,6 +93,12 @@ impl From<&Name> for LookupType { } } +impl From for LookupType { + fn from(uuid: Uuid) -> Self { + LookupType::ById(uuid) + } +} + impl Error { /// Returns whether the error is likely transient and could reasonably be /// retried diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 8ecbf493db..bfecbc3359 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -516,6 +516,7 @@ pub enum ResourceType { Fleet, Silo, SiloUser, + SshKey, ConsoleSession, Organization, Project, diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 5d7b9f7e71..f093c194ac 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -216,6 +216,33 @@ CREATE TABLE omicron.public.silo_user ( time_deleted TIMESTAMPTZ ); +/* + * Users' public SSH keys, per RFD 44 + */ +CREATE TABLE omicron.public.ssh_key ( + id UUID PRIMARY KEY, + name STRING(63) NOT NULL, + description STRING(512) NOT NULL, + time_created TIMESTAMPTZ NOT NULL, + time_modified TIMESTAMPTZ NOT NULL, + time_deleted TIMESTAMPTZ, + + /* FK into silo_user table */ + silo_user_id UUID NOT NULL, + + /* + * A 4096 bit RSA key without comment encodes to 726 ASCII characters. + * A (256 bit) Ed25519 key w/o comment encodes to 82 ASCII characters. + */ + public_key STRING(1023) NOT NULL +); + +CREATE UNIQUE INDEX ON omicron.public.ssh_key ( + silo_user_id, + name +) WHERE + time_deleted IS NULL; + /* * Organizations */ diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index a1c6f1e114..f57e39305e 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -334,14 +334,6 @@ authz_resource! { polar_snippet = FleetChild, } -authz_resource! { - name = "SiloUser", - parent = "Fleet", - primary_key = Uuid, - roles_allowed = false, - polar_snippet = FleetChild, -} - authz_resource! { name = "RoleBuiltin", parent = "Fleet", @@ -374,6 +366,22 @@ authz_resource! { polar_snippet = Custom, } +authz_resource! { + name = "SiloUser", + parent = "Silo", + primary_key = Uuid, + roles_allowed = false, + polar_snippet = Custom, +} + +authz_resource! { + name = "SshKey", + parent = "SiloUser", + primary_key = Uuid, + roles_allowed = false, + polar_snippet = Custom, +} + authz_resource! { name = "Sled", parent = "Fleet", diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 633cada063..2e6974949d 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -230,8 +230,29 @@ has_relation(fleet: Fleet, "parent_fleet", collection: ConsoleSessionList) # read silo users and modify their sessions. This is necessary for login to # work. has_permission(actor: AuthenticatedActor, "read", user: SiloUser) - if has_role(actor, "external-authenticator", user.fleet); + if has_role(actor, "external-authenticator", user.silo.fleet); has_permission(actor: AuthenticatedActor, "read", session: ConsoleSession) if has_role(actor, "external-authenticator", session.fleet); has_permission(actor: AuthenticatedActor, "modify", session: ConsoleSession) if has_role(actor, "external-authenticator", session.fleet); + +resource SiloUser { + permissions = [ "read", "modify", "create_child" ]; + relations = { parent_silo: Silo }; + + "read" if "read" on "parent_silo"; + "modify" if "modify" on "parent_silo"; + "create_child" if "create_child" on "parent_silo"; +} +has_relation(silo: Silo, "parent_silo", user: SiloUser) + if user.silo = silo; + +resource SshKey { + permissions = [ "read", "modify" ]; + relations = { silo_user: SiloUser }; + + "read" if "read" on "silo_user"; + "modify" if "modify" on "silo_user"; +} +has_relation(user: SiloUser, "silo_user", ssh_key: SshKey) + if ssh_key.silo_user = user; diff --git a/nexus/src/authz/oso_generic.rs b/nexus/src/authz/oso_generic.rs index 755f84e77b..860bbe71f4 100644 --- a/nexus/src/authz/oso_generic.rs +++ b/nexus/src/authz/oso_generic.rs @@ -65,6 +65,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { ConsoleSession::init(), Rack::init(), RoleBuiltin::init(), + SshKey::init(), Silo::init(), SiloUser::init(), Sled::init(), diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 706a56d3a5..aeaa15f4ea 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -42,8 +42,8 @@ use crate::db::{ Name, NetworkInterface, Organization, OrganizationUpdate, OximeterInfo, ProducerEndpoint, Project, ProjectUpdate, Region, RoleAssignmentBuiltin, RoleBuiltin, RouterRoute, RouterRouteUpdate, - Silo, SiloUser, Sled, UpdateAvailableArtifact, UserBuiltin, Volume, - Vpc, VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet, + Silo, SiloUser, Sled, SshKey, UpdateAvailableArtifact, UserBuiltin, + Volume, Vpc, VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet, VpcSubnetUpdate, VpcUpdate, Zpool, }, pagination::paginated, @@ -2679,6 +2679,70 @@ impl DataStore { }), } } + + // SSH public keys + + pub async fn ssh_keys_list( + &self, + silo_user_id: Uuid, + page_params: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + use db::schema::ssh_key::dsl; + + paginated(dsl::ssh_key, dsl::id, page_params) + .filter(dsl::silo_user_id.eq(silo_user_id)) + .filter(dsl::time_deleted.is_null()) + .select(SshKey::as_select()) + .load_async(self.pool()) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + + /// Create a new SSH public key for a user. + pub async fn ssh_key_create( + &self, + ssh_key: SshKey, + ) -> CreateResult { + use db::schema::ssh_key::dsl; + + diesel::insert_into(dsl::ssh_key) + .values(ssh_key) + .returning(SshKey::as_returning()) + .get_result_async(self.pool()) + .await + .map_err(|e| { + Error::internal_error(&format!( + "error creating SSH key: {:?}", + e + )) + }) + } + + /// Delete an existing SSH public key. + pub async fn ssh_key_delete( + &self, + opctx: &OpContext, + authz_ssh_key: &authz::SshKey, + ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_ssh_key).await?; + + use db::schema::ssh_key::dsl; + + let now = Utc::now(); + diesel::update(dsl::ssh_key) + .filter(dsl::id.eq(authz_ssh_key.id())) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(now)) + .execute_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_ssh_key), + ) + })?; + Ok(()) + } } /// Constructs a DataStore for use in test suites that has preloaded the @@ -2801,7 +2865,7 @@ mod test { // Associate silo with user let silo_user = datastore .silo_user_create(SiloUser::new( - Uuid::new_v4(), /* silo id */ + *SILO_ID, silo_user_id, )) .await @@ -3268,4 +3332,61 @@ mod test { let _ = db.cleanup().await; logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_ssh_keys() { + let logctx = dev::test_setup_log("test_ssh_keys"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + // Create a new Silo user so that we can lookup their keys. + let silo_user_id = Uuid::new_v4(); + let silo_user = datastore + .silo_user_create(SiloUser::new(*SILO_ID, silo_user_id)) + .await + .unwrap(); + assert_eq!(silo_user.id(), silo_user_id); + + // Create a new SSH public key for the new user. + let public_key = "ssh-test AAAAAAAAKEY".to_string(); + let ssh_key = SshKey::new( + silo_user_id, + params::SshKeyCreate { + identity: IdentityMetadataCreateParams { + name: "sshkey".parse().unwrap(), + description: "my SSH public key".to_string(), + }, + public_key, + }, + ); + let created = datastore.ssh_key_create(ssh_key.clone()).await.unwrap(); + assert_eq!(created.silo_user_id, ssh_key.silo_user_id); + assert_eq!(created.public_key, ssh_key.public_key); + + // Lookup the key we just created. + let (authz_silo, authz_silo_user, authz_ssh_key, found) = + LookupPath::new(&opctx, &datastore) + .ssh_key_id(ssh_key.id()) + .fetch() + .await + .unwrap(); + assert_eq!(authz_silo.id(), *SILO_ID); + assert_eq!(authz_silo_user.id(), silo_user_id); + assert_eq!(found.silo_user_id, ssh_key.silo_user_id); + assert_eq!(found.public_key, ssh_key.public_key); + + // Trying to insert the same one again fails. + let duplicate = datastore.ssh_key_create(ssh_key.clone()).await; + assert!(matches!( + duplicate, + Err(Error::InternalError { internal_message: _ }) + )); + + // Delete the key we just created. + datastore.ssh_key_delete(&opctx, &authz_ssh_key).await.unwrap(); + + // Clean up. + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 5860c1a64e..eccf63fd92 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -304,7 +304,7 @@ impl<'a> LookupPath<'a> { Silo { key: SiloKey::PrimaryKey(Root { lookup_root: self }, id) } } - /// Select a resource of type Silo, identified by its id + /// Select a resource of type Silo, identified by its name pub fn silo_name<'b, 'c>(self, name: &'b Name) -> Silo<'c> where 'a: 'c, @@ -325,6 +325,31 @@ impl<'a> LookupPath<'a> { Sled { key: SledKey::PrimaryKey(Root { lookup_root: self }, id) } } + /// Select a resource of type SshKey, identified by its id + pub fn ssh_key_id(self, id: Uuid) -> SshKey<'a> { + SshKey { key: SshKeyKey::PrimaryKey(Root { lookup_root: self }, id) } + } + + /// Select a resource of type SshKey, identified by its name + pub fn ssh_key_name<'b, 'c>(self, name: &'b Name) -> SshKey<'c> + where + 'a: 'c, + 'b: 'c, + { + let key = match self.opctx.authn.actor_required() { + Ok(actor) => { + let root = Root { lookup_root: self }; + let silo_user_key = SiloUserKey::PrimaryKey(root, actor.id); + SshKeyKey::Name(SiloUser { key: silo_user_key }, name) + } + Err(error) => { + let root = Root { lookup_root: self }; + SshKeyKey::Error(root, error) + } + }; + SshKey { key } + } + /// Select a resource of type UpdateAvailableArtifact, identified by its /// `(name, version, kind)` tuple pub fn update_available_artifact_tuple( @@ -382,6 +407,24 @@ lookup_resource! { primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] } +lookup_resource! { + name = "SiloUser", + ancestors = [ "Silo" ], + children = [ "SshKey" ], + lookup_by_name = false, + soft_deletes = true, + primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] +} + +lookup_resource! { + name = "SshKey", + ancestors = [ "Silo", "SiloUser" ], + children = [], + lookup_by_name = true, + soft_deletes = true, + primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] +} + lookup_resource! { name = "Organization", ancestors = [ "Silo" ], @@ -488,15 +531,6 @@ lookup_resource! { ] } -lookup_resource! { - name = "SiloUser", - ancestors = [], - children = [], - lookup_by_name = false, - soft_deletes = true, - primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] -} - lookup_resource! { name = "Sled", ancestors = [], diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index d3aaa8be70..70f828bf6e 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -11,7 +11,7 @@ use crate::db::schema::{ console_session, dataset, disk, image, instance, metric_producer, network_interface, organization, oximeter, project, rack, region, role_assignment_builtin, role_builtin, router_route, silo, silo_user, sled, - snapshot, update_available_artifact, user_builtin, volume, vpc, + snapshot, ssh_key, update_available_artifact, user_builtin, volume, vpc, vpc_firewall_rule, vpc_router, vpc_subnet, zpool, }; use crate::defaults; @@ -997,6 +997,32 @@ impl SiloUser { } } +/// Describes a user's public SSH key within the database. +#[derive(Clone, Debug, Insertable, Queryable, Resource, Selectable)] +#[table_name = "ssh_key"] +pub struct SshKey { + #[diesel(embed)] + identity: SshKeyIdentity, + + pub silo_user_id: Uuid, + pub public_key: String, +} + +impl SshKey { + pub fn new(silo_user_id: Uuid, params: params::SshKeyCreate) -> Self { + Self::new_with_id(Uuid::new_v4(), silo_user_id, params) + } + + + pub fn new_with_id(id: Uuid, silo_user_id: Uuid, params: params::SshKeyCreate) -> Self { + Self { + identity: SshKeyIdentity::new(id, params.identity), + silo_user_id, + public_key: params.public_key, + } + } +} + /// Describes an organization within the database. #[derive(Queryable, Insertable, Debug, Resource, Selectable)] #[table_name = "organization"] diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index b0f7860bc9..a5687fa838 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -137,6 +137,19 @@ table! { } } +table! { + ssh_key (id) { + id -> Uuid, + name -> Text, + description -> Text, + time_created -> Timestamptz, + time_modified -> Timestamptz, + time_deleted -> Nullable, + silo_user_id -> Uuid, + public_key -> Text, + } +} + table! { organization (id) { id -> Uuid, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 90046ff4c8..5a80ca55f1 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -11,8 +11,8 @@ use crate::ServerContext; use super::{ console_api, params, views::{ - Image, Organization, Project, Rack, Role, Silo, Sled, Snapshot, User, - Vpc, VpcRouter, VpcSubnet, + Image, Organization, Project, Rack, Role, Silo, Sled, Snapshot, SshKey, + User, Vpc, VpcRouter, VpcSubnet, }, }; use crate::context::OpContext; @@ -174,6 +174,11 @@ pub fn external_api() -> NexusApiDescription { api.register(roles_get)?; api.register(roles_get_role)?; + api.register(sshkeys_get)?; + api.register(sshkeys_get_key)?; + api.register(sshkeys_post)?; + api.register(sshkeys_delete_key)?; + api.register(console_api::spoof_login)?; api.register(console_api::spoof_login_form)?; api.register(console_api::login_redirect)?; @@ -2853,6 +2858,114 @@ async fn roles_get_role( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +// Per-user SSH public keys + +/// List the current user's SSH public keys +#[endpoint { + method = GET, + path = "/session/me/sshkeys", + tags = ["sshkeys"], +}] +async fn sshkeys_get( + rqctx: Arc>>, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let query = query_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let &actor = opctx.authn.actor_required()?; + let silo_user_id = actor.id; + let ssh_keys = nexus + .ssh_keys_list( + &opctx, + silo_user_id, + &data_page_params_for(&rqctx, &query)? + ) + .await? + .into_iter() + .map(SshKey::from) + .collect::>(); + Ok(HttpResponseOk(ScanById::results_page(&query, ssh_keys)?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Create a new SSH public key for the current user +#[endpoint { + method = POST, + path = "/session/me/sshkeys", + tags = ["sshkeys"], +}] +async fn sshkeys_post( + rqctx: Arc>>, + new_key: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let &actor = opctx.authn.actor_required()?; + let silo_user_id = actor.id; + let ssh_key = nexus + .ssh_key_create(&opctx, silo_user_id, new_key.into_inner()) + .await?; + Ok(HttpResponseOk(ssh_key.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Path parameters for SSH key requests by name +#[derive(Deserialize, JsonSchema)] +struct SshKeyPathParams { + ssh_key_name: Name, +} + +/// Get (by name) an SSH public key belonging to the current user +#[endpoint { + method = GET, + path = "/session/me/sshkeys/{ssh_key_name}", + tags = ["sshkeys"], +}] +async fn sshkeys_get_key( + rqctx: Arc>>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let ssh_key_name = &path.ssh_key_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let ssh_key = nexus.ssh_key_fetch(&opctx, ssh_key_name).await?; + Ok(HttpResponseOk(ssh_key.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Delete (by name) an SSH public key belonging to the current user +#[endpoint { + method = DELETE, + path = "/session/me/sshkeys/{ssh_key_name}", + tags = ["sshkeys"], +}] +async fn sshkeys_delete_key( + rqctx: Arc>>, + path_params: Path, +) -> Result { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let ssh_key_name = &path.ssh_key_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + nexus.ssh_key_delete(&opctx, ssh_key_name).await?; + Ok(HttpResponseDeleted()) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + #[cfg(test)] mod test { use super::external_api; diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 9a83bab95c..5606c4ab32 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -433,6 +433,19 @@ pub struct UserBuiltinCreate { pub identity: IdentityMetadataCreateParams, } +// SSH PUBLIC KEYS +// +// The SSH key mangement endpoints are currently under `/session/me`, +// and so have an implicit silo user ID which must be passed seperately +// to the creation routine. Note that this disagrees with RFD 44. + +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SshKeyCreate { + #[serde(flatten)] + pub identity: IdentityMetadataCreateParams, + pub public_key: String, // e.g., "ssh-ed25519 AAAAC3NzaC..." +} + #[cfg(test)] mod test { use super::*; diff --git a/nexus/src/external_api/tag-config.json b/nexus/src/external_api/tag-config.json index dab68d58d2..5ba81fac1c 100644 --- a/nexus/src/external_api/tag-config.json +++ b/nexus/src/external_api/tag-config.json @@ -104,6 +104,12 @@ "url": "http://oxide.computer/docs/#xxx" } }, + "sshkeys": { + "description": "Public SSH keys for an individual user", + "external_docs": { + "url": "http://oxide.computer/docs/#xxx" + } + }, "subnets": { "description": "This tag should be moved into a generic network tag", "external_docs": { diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index 5fc4b22a39..9817abbec2 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -281,3 +281,26 @@ impl From for Role { } } } + +// SSH KEYS + +/// Client view of a [`SshKey`] +#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SshKey { + #[serde(flatten)] + pub identity: IdentityMetadata, + /// The user to whom this key belongs + pub silo_user_id: Uuid, + /// The SSH public key + pub public_key: String, +} + +impl From for SshKey { + fn from(ssh_key: model::SshKey) -> Self { + Self { + identity: ssh_key.identity(), + silo_user_id: ssh_key.silo_user_id, + public_key: ssh_key.public_key, + } + } +} diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 98caab8a5f..56b300b691 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -15,6 +15,7 @@ use crate::db::model::DatasetKind; use crate::db::model::Name; use crate::db::model::RouterRoute; use crate::db::model::SiloUser; +use crate::db::model::SshKey; use crate::db::model::UpdateArtifactKind; use crate::db::model::VpcRouter; use crate::db::model::VpcRouterKind; @@ -3666,6 +3667,57 @@ impl Nexus { .await?; Ok(db_silo_user) } + + // SSH public keys + + /// Following GitHub, we do not bother to authorize this request. + /// After all, they are *public* keys. + pub async fn ssh_keys_list( + &self, + _opctx: &OpContext, + silo_user_id: Uuid, + page_params: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + self.db_datastore.ssh_keys_list(silo_user_id, page_params).await + } + + pub async fn ssh_key_fetch( + &self, + opctx: &OpContext, + ssh_key_name: &Name, + ) -> LookupResult { + let (.., ssh_key) = LookupPath::new(opctx, &self.datastore()) + .ssh_key_name(ssh_key_name) + .fetch() + .await?; + assert_eq!(ssh_key.name(), ssh_key_name); + Ok(ssh_key) + } + + pub async fn ssh_key_create( + &self, + _opctx: &OpContext, + silo_user_id: Uuid, + params: params::SshKeyCreate, + ) -> CreateResult { + let ssh_key = db::model::SshKey::new(silo_user_id, params); + Ok(self.db_datastore.ssh_key_create(ssh_key).await?) + } + + pub async fn ssh_key_delete( + &self, + opctx: &OpContext, + ssh_key_name: &Name, + ) -> DeleteResult { + let (.., authz_ssh_key, ssh_key) = + LookupPath::new(opctx, &self.datastore()) + .ssh_key_name(ssh_key_name) + .fetch() + .await?; + assert_eq!(ssh_key.name(), ssh_key_name); + + self.db_datastore.ssh_key_delete(opctx, &authz_ssh_key).await + } } /// For unimplemented endpoints, indicates whether the resource identified diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 0cac4b1ca9..0f978a7694 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -242,6 +242,21 @@ lazy_static! { }, disk: DEMO_DISK_NAME.clone(), }; + + // SSH public keys + pub static ref SSH_KEYS_URL: String = + "/session/me/sshkeys".to_string(); + pub static ref DEMO_SSH_KEY_NAME: Name = "demo-ssh-key".parse().unwrap(); + pub static ref DEMO_SSH_KEY_URL: String = + format!("{}/{}", *SSH_KEYS_URL, *DEMO_SSH_KEY_NAME); + pub static ref DEMO_SSH_KEY_CREATE: params::SshKeyCreate = + params::SshKeyCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_SSH_KEY_NAME.clone(), + description: String::from("my SSH public key"), + }, + public_key: "ssh-test AAAAAAAAA".to_string(), + }; } /// Describes an API endpoint to be verified by the "unauthorized" test @@ -790,6 +805,25 @@ lazy_static! { allowed_methods: vec![AllowedMethod::Get], }, + VerifyEndpoint { + url: &*SSH_KEYS_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_SSH_KEY_CREATE).unwrap() + ), + ], + }, + VerifyEndpoint { + url: &*DEMO_SSH_KEY_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + ], + }, + /* Hardware */ VerifyEndpoint { diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 8ef1eaee3f..f6fc57bdfb 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -118,6 +118,13 @@ project_snapshots_get /organizations/{organization_name}/proj project_snapshots_get_snapshot /organizations/{organization_name}/projects/{project_name}/snapshots/{snapshot_name} project_snapshots_post /organizations/{organization_name}/projects/{project_name}/snapshots +API operations found with tag "sshkeys" +OPERATION ID URL PATH +sshkeys_delete_key /session/me/sshkeys/{ssh_key_name} +sshkeys_get /session/me/sshkeys +sshkeys_get_key /session/me/sshkeys/{ssh_key_name} +sshkeys_post /session/me/sshkeys + API operations found with tag "subnets" OPERATION ID URL PATH subnet_network_interfaces_get /organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/network-interfaces diff --git a/openapi/nexus.json b/openapi/nexus.json index 4fd7f6787a..4189ed0384 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4400,6 +4400,168 @@ } } }, + "/session/me/sshkeys": { + "get": { + "tags": [ + "sshkeys" + ], + "summary": "List the current user's SSH public keys", + "operationId": "sshkeys_get", + "parameters": [ + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + }, + "style": "form" + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retreive the subsequent page", + "schema": { + "nullable": true, + "type": "string" + }, + "style": "form" + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/IdSortMode" + }, + "style": "form" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SshKeyResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + }, + "post": { + "tags": [ + "sshkeys" + ], + "summary": "Create a new SSH public key for the current user", + "operationId": "sshkeys_post", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SshKeyCreate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SshKey" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/session/me/sshkeys/{ssh_key_name}": { + "get": { + "tags": [ + "sshkeys" + ], + "summary": "Get (by name) an SSH public key belonging to the current user", + "operationId": "sshkeys_get_key", + "parameters": [ + { + "in": "path", + "name": "ssh_key_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SshKey" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "delete": { + "tags": [ + "sshkeys" + ], + "summary": "Delete (by name) an SSH public key belonging to the current user", + "operationId": "sshkeys_delete_key", + "parameters": [ + { + "in": "path", + "name": "ssh_key_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "204": { + "description": "successful deletion" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/silos": { "get": { "tags": [ @@ -6844,6 +7006,98 @@ "items" ] }, + "SshKey": { + "description": "Client view of a [`SshKey`]", + "type": "object", + "properties": { + "description": { + "description": "human-readable free-form text about a resource", + "type": "string" + }, + "id": { + "description": "unique, immutable, system-controlled identifier for each resource", + "type": "string", + "format": "uuid" + }, + "name": { + "description": "unique, mutable, user-controlled identifier for each resource", + "allOf": [ + { + "$ref": "#/components/schemas/Name" + } + ] + }, + "public_key": { + "description": "The SSH public key", + "type": "string" + }, + "silo_user_id": { + "description": "The user to whom this key belongs", + "type": "string", + "format": "uuid" + }, + "time_created": { + "description": "timestamp when this resource was created", + "type": "string", + "format": "date-time" + }, + "time_modified": { + "description": "timestamp when this resource was last modified", + "type": "string", + "format": "date-time" + } + }, + "required": [ + "description", + "id", + "name", + "public_key", + "silo_user_id", + "time_created", + "time_modified" + ] + }, + "SshKeyCreate": { + "description": "Create-time identity-related parameters", + "type": "object", + "properties": { + "description": { + "type": "string" + }, + "name": { + "$ref": "#/components/schemas/Name" + }, + "public_key": { + "type": "string" + } + }, + "required": [ + "description", + "name", + "public_key" + ] + }, + "SshKeyResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/SshKey" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, "TimeseriesName": { "title": "The name of a timeseries", "description": "Names are constructed by concatenating the target and metric names with ':'. Target and metric names must be lowercase alphanumeric characters with '_' separating words.", @@ -7978,6 +8232,13 @@ "url": "http://oxide.computer/docs/#xxx" } }, + { + "name": "sshkeys", + "description": "Public SSH keys for an individual user", + "externalDocs": { + "url": "http://oxide.computer/docs/#xxx" + } + }, { "name": "subnets", "description": "This tag should be moved into a generic network tag", From 9c0d76114168c254aa4fd17805a47881f5ce6294 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 20 Apr 2022 12:42:56 -0600 Subject: [PATCH 02/16] fmt --- nexus/src/db/datastore.rs | 5 +---- nexus/src/db/model.rs | 7 +++++-- nexus/src/external_api/http_entrypoints.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index aeaa15f4ea..8668469402 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2864,10 +2864,7 @@ mod test { // Associate silo with user let silo_user = datastore - .silo_user_create(SiloUser::new( - *SILO_ID, - silo_user_id, - )) + .silo_user_create(SiloUser::new(*SILO_ID, silo_user_id)) .await .unwrap(); diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 70f828bf6e..a1d043c771 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1013,8 +1013,11 @@ impl SshKey { Self::new_with_id(Uuid::new_v4(), silo_user_id, params) } - - pub fn new_with_id(id: Uuid, silo_user_id: Uuid, params: params::SshKeyCreate) -> Self { + pub fn new_with_id( + id: Uuid, + silo_user_id: Uuid, + params: params::SshKeyCreate, + ) -> Self { Self { identity: SshKeyIdentity::new(id, params.identity), silo_user_id, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 5a80ca55f1..8e4ddfdfc4 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2881,7 +2881,7 @@ async fn sshkeys_get( .ssh_keys_list( &opctx, silo_user_id, - &data_page_params_for(&rqctx, &query)? + &data_page_params_for(&rqctx, &query)?, ) .await? .into_iter() From 9809417631d677fb87dd314551937eb010ee7b0d Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 20 Apr 2022 14:08:07 -0600 Subject: [PATCH 03/16] Integration test for SSH public keys Also change pagination from by-ID to by-name. --- nexus/src/db/datastore.rs | 4 +- nexus/src/external_api/http_entrypoints.rs | 16 ++- nexus/src/nexus.rs | 2 +- nexus/tests/integration_tests/mod.rs | 1 + nexus/tests/integration_tests/ssh_keys.rs | 122 +++++++++++++++++++++ openapi/nexus.json | 6 +- 6 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 nexus/tests/integration_tests/ssh_keys.rs diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 8668469402..6317603615 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2685,11 +2685,11 @@ impl DataStore { pub async fn ssh_keys_list( &self, silo_user_id: Uuid, - page_params: &DataPageParams<'_, Uuid>, + page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { use db::schema::ssh_key::dsl; - paginated(dsl::ssh_key, dsl::id, page_params) + paginated(dsl::ssh_key, dsl::name, page_params) .filter(dsl::silo_user_id.eq(silo_user_id)) .filter(dsl::time_deleted.is_null()) .select(SshKey::as_select()) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 8e4ddfdfc4..8305685e59 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2868,7 +2868,7 @@ async fn roles_get_role( }] async fn sshkeys_get( rqctx: Arc>>, - query_params: Query, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -2877,17 +2877,15 @@ async fn sshkeys_get( let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; let silo_user_id = actor.id; + let page_params = + data_page_params_for(&rqctx, &query)?.map_name(Name::ref_cast); let ssh_keys = nexus - .ssh_keys_list( - &opctx, - silo_user_id, - &data_page_params_for(&rqctx, &query)?, - ) + .ssh_keys_list(&opctx, silo_user_id, &page_params) .await? .into_iter() .map(SshKey::from) .collect::>(); - Ok(HttpResponseOk(ScanById::results_page(&query, ssh_keys)?)) + Ok(HttpResponseOk(ScanByName::results_page(&query, ssh_keys)?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2901,7 +2899,7 @@ async fn sshkeys_get( async fn sshkeys_post( rqctx: Arc>>, new_key: TypedBody, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let handler = async { @@ -2911,7 +2909,7 @@ async fn sshkeys_post( let ssh_key = nexus .ssh_key_create(&opctx, silo_user_id, new_key.into_inner()) .await?; - Ok(HttpResponseOk(ssh_key.into())) + Ok(HttpResponseCreated(ssh_key.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 56b300b691..66a856a766 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -3676,7 +3676,7 @@ impl Nexus { &self, _opctx: &OpContext, silo_user_id: Uuid, - page_params: &DataPageParams<'_, Uuid>, + page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { self.db_datastore.ssh_keys_list(silo_user_id, page_params).await } diff --git a/nexus/tests/integration_tests/mod.rs b/nexus/tests/integration_tests/mod.rs index 6c9ba5a904..9254e849b7 100644 --- a/nexus/tests/integration_tests/mod.rs +++ b/nexus/tests/integration_tests/mod.rs @@ -16,6 +16,7 @@ mod projects; mod roles_builtin; mod router_routes; mod silos; +mod ssh_keys; mod subnet_allocation; mod timeseries; mod unauthorized; diff --git a/nexus/tests/integration_tests/ssh_keys.rs b/nexus/tests/integration_tests/ssh_keys.rs new file mode 100644 index 0000000000..05ce6e1ec8 --- /dev/null +++ b/nexus/tests/integration_tests/ssh_keys.rs @@ -0,0 +1,122 @@ +//! Sanity-tests for public SSH keys + +use http::{method::Method, StatusCode}; + +use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; +use nexus_test_utils::resource_helpers::objects_list_page_authz; +use nexus_test_utils::ControlPlaneTestContext; +use nexus_test_utils_macros::nexus_test; + +use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_nexus::external_api::params::SshKeyCreate; +use omicron_nexus::external_api::views::SshKey; + +#[nexus_test] +async fn test_ssh_keys(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + + // Ensure we start with an empty list of SSH keys. + let keys = objects_list_page_authz::(client, "/session/me/sshkeys") + .await + .items; + assert_eq!(keys.len(), 0); + + // Ensure GET fails on non-existent keys. + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + "/session/me/sshkeys/nonexistent", + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make GET request"); + + // Ensure we can POST new keys. + let new_keys = vec![ + ("key1", "an SSH public key", "ssh-test AAAAAAAA"), + ("key2", "another SSH public key", "ssh-test BBBBBBBB"), + ("key3", "yet another public key", "ssh-test CCCCCCCC"), + ]; + for (name, description, public_key) in &new_keys { + let new_key: SshKey = NexusRequest::objects_post( + client, + "/session/me/sshkeys", + &SshKeyCreate { + identity: IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: description.to_string(), + }, + public_key: public_key.to_string(), + }, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make POST request") + .parsed_body() + .unwrap(); + assert_eq!(new_key.identity.name.as_str(), *name); + assert_eq!(new_key.identity.description, *description); + assert_eq!(new_key.public_key, *public_key); + } + + // Ensure we can GET one of the keys we just posted. + let key1: SshKey = NexusRequest::object_get( + client, + &format!("/session/me/sshkeys/{}", new_keys[0].0), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make GET request") + .parsed_body() + .unwrap(); + assert_eq!(key1.identity.name.as_str(), new_keys[0].0); + assert_eq!(key1.identity.description, new_keys[0].1); + assert_eq!(key1.public_key, new_keys[0].2); + + // Ensure we can GET the list of keys we just posted. + // TODO-coverage: pagination + let keys: Vec = NexusRequest::iter_collection_authn( + client, + "/session/me/sshkeys", + "sort_by=name-ascending", + Some(new_keys.len()), + ) + .await + .expect("failed to list keys") + .all_items; + assert_eq!(keys.len(), new_keys.len()); + for (key, (name, description, public_key)) in + keys.iter().zip(new_keys.iter()) + { + assert_eq!(key.identity.name.as_str(), *name); + assert_eq!(key.identity.description, *description); + assert_eq!(key.public_key, *public_key); + } + + // Ensure we can DELETE a key. + let deleted_key_name = new_keys[0].0; + NexusRequest::object_delete( + client, + &format!("/session/me/sshkeys/{}", deleted_key_name), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to DELETE key"); + + // Ensure that we can't GET the key we just deleted. + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &format!("/session/me/sshkeys/{}", deleted_key_name), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make GET request"); +} diff --git a/openapi/nexus.json b/openapi/nexus.json index 4189ed0384..0d0c9b66de 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4434,7 +4434,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/IdSortMode" + "$ref": "#/components/schemas/NameSortMode" }, "style": "form" } @@ -4476,8 +4476,8 @@ "required": true }, "responses": { - "200": { - "description": "successful operation", + "201": { + "description": "successful creation", "content": { "application/json": { "schema": { From 95114e05607d153a74a800de59534667bf240487 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Thu, 21 Apr 2022 10:49:17 -0600 Subject: [PATCH 04/16] More authz for SSH public keys --- nexus/src/authz/omicron.polar | 23 ++++++++++--- nexus/src/db/datastore.rs | 38 +++++++++++++++------- nexus/src/external_api/http_entrypoints.rs | 8 ++--- nexus/src/nexus.rs | 30 ++++++++--------- 4 files changed, 63 insertions(+), 36 deletions(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 2e6974949d..13944d9813 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -237,12 +237,25 @@ has_permission(actor: AuthenticatedActor, "modify", session: ConsoleSession) if has_role(actor, "external-authenticator", session.fleet); resource SiloUser { - permissions = [ "read", "modify", "create_child" ]; - relations = { parent_silo: 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"; - "read" if "read" on "parent_silo"; - "modify" if "modify" on "parent_silo"; - "create_child" if "create_child" on "parent_silo"; + relations = { parent_silo: Silo }; + "admin" if "admin" on "parent_silo"; + "collaborator" if "collaborator" on "parent_silo"; + "viewer" if "viewer" on "parent_silo"; } has_relation(silo: Silo, "parent_silo", user: SiloUser) if user.silo = silo; diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 6317603615..e9cdcf233d 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2684,16 +2684,16 @@ impl DataStore { pub async fn ssh_keys_list( &self, - silo_user_id: Uuid, + opctx: &OpContext, + authz_user: &authz::SiloUser, page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { use db::schema::ssh_key::dsl; - paginated(dsl::ssh_key, dsl::name, page_params) - .filter(dsl::silo_user_id.eq(silo_user_id)) + .filter(dsl::silo_user_id.eq(authz_user.id())) .filter(dsl::time_deleted.is_null()) .select(SshKey::as_select()) - .load_async(self.pool()) + .load_async(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } @@ -2701,14 +2701,18 @@ impl DataStore { /// Create a new SSH public key for a user. pub async fn ssh_key_create( &self, + opctx: &OpContext, + authz_user: &authz::SiloUser, ssh_key: SshKey, ) -> CreateResult { - use db::schema::ssh_key::dsl; + assert_eq!(authz_user.id(), ssh_key.silo_user_id); + opctx.authorize(authz::Action::CreateChild, authz_user).await?; + use db::schema::ssh_key::dsl; diesel::insert_into(dsl::ssh_key) .values(ssh_key) .returning(SshKey::as_returning()) - .get_result_async(self.pool()) + .get_result_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { Error::internal_error(&format!( @@ -2727,13 +2731,12 @@ impl DataStore { opctx.authorize(authz::Action::Delete, authz_ssh_key).await?; use db::schema::ssh_key::dsl; - - let now = Utc::now(); diesel::update(dsl::ssh_key) .filter(dsl::id.eq(authz_ssh_key.id())) .filter(dsl::time_deleted.is_null()) - .set(dsl::time_deleted.eq(now)) - .execute_async(self.pool_authorized(opctx).await?) + .set(dsl::time_deleted.eq(Utc::now())) + .check_if_exists::(authz_ssh_key.id()) + .execute_and_check(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( @@ -3344,6 +3347,13 @@ mod test { .unwrap(); assert_eq!(silo_user.id(), silo_user_id); + let (.., authz_user) = LookupPath::new(&opctx, &datastore) + .silo_user_id(silo_user_id) + .lookup_for(authz::Action::CreateChild) + .await + .unwrap(); + assert_eq!(authz_user.id(), silo_user_id); + // Create a new SSH public key for the new user. let public_key = "ssh-test AAAAAAAAKEY".to_string(); let ssh_key = SshKey::new( @@ -3356,7 +3366,10 @@ mod test { public_key, }, ); - let created = datastore.ssh_key_create(ssh_key.clone()).await.unwrap(); + let created = datastore + .ssh_key_create(&opctx, &authz_user, ssh_key.clone()) + .await + .unwrap(); assert_eq!(created.silo_user_id, ssh_key.silo_user_id); assert_eq!(created.public_key, ssh_key.public_key); @@ -3373,7 +3386,8 @@ mod test { assert_eq!(found.public_key, ssh_key.public_key); // Trying to insert the same one again fails. - let duplicate = datastore.ssh_key_create(ssh_key.clone()).await; + let duplicate = + datastore.ssh_key_create(&opctx, &authz_user, ssh_key.clone()).await; assert!(matches!( duplicate, Err(Error::InternalError { internal_message: _ }) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 8305685e59..4bc869ab05 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2876,11 +2876,11 @@ async fn sshkeys_get( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; - let silo_user_id = actor.id; + let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; let page_params = data_page_params_for(&rqctx, &query)?.map_name(Name::ref_cast); let ssh_keys = nexus - .ssh_keys_list(&opctx, silo_user_id, &page_params) + .ssh_keys_list(&opctx, &authz_user, &page_params) .await? .into_iter() .map(SshKey::from) @@ -2905,9 +2905,9 @@ async fn sshkeys_post( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; - let silo_user_id = actor.id; + let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; let ssh_key = nexus - .ssh_key_create(&opctx, silo_user_id, new_key.into_inner()) + .ssh_key_create(&opctx, &authz_user, new_key.into_inner()) .await?; Ok(HttpResponseCreated(ssh_key.into())) }; diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 66a856a766..1c73ba53db 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -3660,25 +3660,25 @@ impl Nexus { &self, opctx: &OpContext, silo_user_id: Uuid, - ) -> LookupResult { - let (.., db_silo_user) = LookupPath::new(opctx, &self.db_datastore) - .silo_user_id(silo_user_id) - .fetch() - .await?; - Ok(db_silo_user) + ) -> LookupResult<(authz::SiloUser, db::model::SiloUser)> { + let (.., authz_user, db_silo_user) = + LookupPath::new(opctx, &self.db_datastore) + .silo_user_id(silo_user_id) + .fetch() + .await?; + Ok((authz_user, db_silo_user)) } // SSH public keys - /// Following GitHub, we do not bother to authorize this request. - /// After all, they are *public* keys. pub async fn ssh_keys_list( &self, - _opctx: &OpContext, - silo_user_id: Uuid, + opctx: &OpContext, + authz_user: &authz::SiloUser, page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { - self.db_datastore.ssh_keys_list(silo_user_id, page_params).await + opctx.authorize(authz::Action::ListChildren, authz_user).await?; + self.db_datastore.ssh_keys_list(opctx, authz_user, page_params).await } pub async fn ssh_key_fetch( @@ -3696,12 +3696,12 @@ impl Nexus { pub async fn ssh_key_create( &self, - _opctx: &OpContext, - silo_user_id: Uuid, + opctx: &OpContext, + authz_user: &authz::SiloUser, params: params::SshKeyCreate, ) -> CreateResult { - let ssh_key = db::model::SshKey::new(silo_user_id, params); - Ok(self.db_datastore.ssh_key_create(ssh_key).await?) + let ssh_key = db::model::SshKey::new(authz_user.id(), params); + Ok(self.db_datastore.ssh_key_create(opctx, authz_user, ssh_key).await?) } pub async fn ssh_key_delete( From 3f4d9cd8af6fb938a50918e2a015dbeb82bae8ee Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Thu, 21 Apr 2022 11:22:18 -0600 Subject: [PATCH 05/16] Drop superfluous SSH key lookup methods --- nexus/src/db/datastore.rs | 11 ++++++---- nexus/src/db/lookup.rs | 25 ---------------------- nexus/src/external_api/http_entrypoints.rs | 9 ++++++-- nexus/src/nexus.rs | 4 ++++ 4 files changed, 18 insertions(+), 31 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index e9cdcf233d..2871b5ff92 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3355,12 +3355,13 @@ mod test { assert_eq!(authz_user.id(), silo_user_id); // Create a new SSH public key for the new user. + let key_name = Name::try_from(String::from("sshkey")).unwrap(); let public_key = "ssh-test AAAAAAAAKEY".to_string(); let ssh_key = SshKey::new( silo_user_id, params::SshKeyCreate { identity: IdentityMetadataCreateParams { - name: "sshkey".parse().unwrap(), + name: key_name.clone(), description: "my SSH public key".to_string(), }, public_key, @@ -3376,7 +3377,8 @@ mod test { // Lookup the key we just created. let (authz_silo, authz_silo_user, authz_ssh_key, found) = LookupPath::new(&opctx, &datastore) - .ssh_key_id(ssh_key.id()) + .silo_user_id(silo_user_id) + .ssh_key_name(&key_name.into()) .fetch() .await .unwrap(); @@ -3386,8 +3388,9 @@ mod test { assert_eq!(found.public_key, ssh_key.public_key); // Trying to insert the same one again fails. - let duplicate = - datastore.ssh_key_create(&opctx, &authz_user, ssh_key.clone()).await; + let duplicate = datastore + .ssh_key_create(&opctx, &authz_user, ssh_key.clone()) + .await; assert!(matches!( duplicate, Err(Error::InternalError { internal_message: _ }) diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index eccf63fd92..957de60226 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -325,31 +325,6 @@ impl<'a> LookupPath<'a> { Sled { key: SledKey::PrimaryKey(Root { lookup_root: self }, id) } } - /// Select a resource of type SshKey, identified by its id - pub fn ssh_key_id(self, id: Uuid) -> SshKey<'a> { - SshKey { key: SshKeyKey::PrimaryKey(Root { lookup_root: self }, id) } - } - - /// Select a resource of type SshKey, identified by its name - pub fn ssh_key_name<'b, 'c>(self, name: &'b Name) -> SshKey<'c> - where - 'a: 'c, - 'b: 'c, - { - let key = match self.opctx.authn.actor_required() { - Ok(actor) => { - let root = Root { lookup_root: self }; - let silo_user_key = SiloUserKey::PrimaryKey(root, actor.id); - SshKeyKey::Name(SiloUser { key: silo_user_key }, name) - } - Err(error) => { - let root = Root { lookup_root: self }; - SshKeyKey::Error(root, error) - } - }; - SshKey { key } - } - /// Select a resource of type UpdateAvailableArtifact, identified by its /// `(name, version, kind)` tuple pub fn update_available_artifact_tuple( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 4bc869ab05..fc30d5e1e8 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2936,7 +2936,10 @@ async fn sshkeys_get_key( let ssh_key_name = &path.ssh_key_name; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let ssh_key = nexus.ssh_key_fetch(&opctx, ssh_key_name).await?; + let &actor = opctx.authn.actor_required()?; + let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; + let ssh_key = + nexus.ssh_key_fetch(&opctx, &authz_user, ssh_key_name).await?; Ok(HttpResponseOk(ssh_key.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -2958,7 +2961,9 @@ async fn sshkeys_delete_key( let ssh_key_name = &path.ssh_key_name; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - nexus.ssh_key_delete(&opctx, ssh_key_name).await?; + let &actor = opctx.authn.actor_required()?; + let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; + nexus.ssh_key_delete(&opctx, &authz_user, ssh_key_name).await?; Ok(HttpResponseDeleted()) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 1c73ba53db..271cd97666 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -3684,9 +3684,11 @@ impl Nexus { pub async fn ssh_key_fetch( &self, opctx: &OpContext, + authz_user: &authz::SiloUser, ssh_key_name: &Name, ) -> LookupResult { let (.., ssh_key) = LookupPath::new(opctx, &self.datastore()) + .silo_user_id(authz_user.id()) .ssh_key_name(ssh_key_name) .fetch() .await?; @@ -3707,10 +3709,12 @@ impl Nexus { pub async fn ssh_key_delete( &self, opctx: &OpContext, + authz_user: &authz::SiloUser, ssh_key_name: &Name, ) -> DeleteResult { let (.., authz_ssh_key, ssh_key) = LookupPath::new(opctx, &self.datastore()) + .silo_user_id(authz_user.id()) .ssh_key_name(ssh_key_name) .fetch() .await?; From af0b7691722e1932cb89755db07e6143207a0288 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Thu, 21 Apr 2022 11:36:39 -0600 Subject: [PATCH 06/16] Add SetupReq for test SSH public key --- nexus/tests/integration_tests/unauthorized.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index ff5fc64787..93dced9616 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -169,6 +169,11 @@ lazy_static! { url: &*DEMO_PROJECT_URL_INSTANCES, body: serde_json::to_value(&*DEMO_INSTANCE_CREATE).unwrap(), }, + // Create an SSH public key + SetupReq { + url: &*SSH_KEYS_URL, + body: serde_json::to_value(&*DEMO_SSH_KEY_CREATE).unwrap(), + }, ]; } From ac2af403831306aa452396cd98882ce24f7825d0 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Thu, 21 Apr 2022 11:51:06 -0600 Subject: [PATCH 07/16] Docstrings for SSH key creation paramters --- nexus/src/external_api/params.rs | 5 ++++- nexus/src/external_api/views.rs | 4 +++- openapi/nexus.json | 5 +++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 5606c4ab32..0620241171 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -439,11 +439,14 @@ pub struct UserBuiltinCreate { // and so have an implicit silo user ID which must be passed seperately // to the creation routine. Note that this disagrees with RFD 44. +/// Create-time parameters for an [`SshKey`](crate::external_api::views::SshKey) #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SshKeyCreate { #[serde(flatten)] pub identity: IdentityMetadataCreateParams, - pub public_key: String, // e.g., "ssh-ed25519 AAAAC3NzaC..." + + /// SSH public key, e.g., `"ssh-ed25519 AAAAC3NzaC..."` + pub public_key: String, } #[cfg(test)] diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index 9817abbec2..1fe982ed64 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -289,9 +289,11 @@ impl From for Role { pub struct SshKey { #[serde(flatten)] pub identity: IdentityMetadata, + /// The user to whom this key belongs pub silo_user_id: Uuid, - /// The SSH public key + + /// SSH public key, e.g., `"ssh-ed25519 AAAAC3NzaC..."` pub public_key: String, } diff --git a/openapi/nexus.json b/openapi/nexus.json index 0d0c9b66de..619f93d724 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7028,7 +7028,7 @@ ] }, "public_key": { - "description": "The SSH public key", + "description": "SSH public key, e.g., `\"ssh-ed25519 AAAAC3NzaC...\"`", "type": "string" }, "silo_user_id": { @@ -7058,7 +7058,7 @@ ] }, "SshKeyCreate": { - "description": "Create-time identity-related parameters", + "description": "Create-time parameters for an [`SshKey`](crate::external_api::views::SshKey)", "type": "object", "properties": { "description": { @@ -7068,6 +7068,7 @@ "$ref": "#/components/schemas/Name" }, "public_key": { + "description": "SSH public key, e.g., `\"ssh-ed25519 AAAAC3NzaC...\"`", "type": "string" } }, From 5955ee203e6822e72d2c753feb419663906b09f7 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 10:20:02 -0600 Subject: [PATCH 08/16] Fix test_silos by replacing improperly-authzed code with a TODO --- nexus/src/db/db-macros/src/lookup.rs | 2 +- nexus/tests/integration_tests/silos.rs | 13 ++++--------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/nexus/src/db/db-macros/src/lookup.rs b/nexus/src/db/db-macros/src/lookup.rs index af1b05743c..7182e2244c 100644 --- a/nexus/src/db/db-macros/src/lookup.rs +++ b/nexus/src/db/db-macros/src/lookup.rs @@ -340,7 +340,7 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { if let Err(_) = &maybe_silo { error!( log, - "unexpected successful lookup of siloed resource\ + "unexpected successful lookup of siloed resource \ {:?} with no silo in OpContext", #resource_name_str, ); diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index ab74ffc923..b704b6d0c6 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -66,7 +66,7 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { assert_eq!(silos.len(), 1); assert_eq!(silos[0].identity.name, "discoverable"); - // Create a new user in the discoverable silo, then create a console session + // Create a new user in the discoverable silo let new_silo_user = nexus .silo_user_create( silos[0].identity.id, /* silo id */ @@ -75,9 +75,10 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { .await .unwrap(); + // TODO-coverage, TODO-security: Add test for Silo-local session + // when we can use users in another Silo. + let authn_opctx = nexus.opctx_external_authn(); - let session = - nexus.session_create(authn_opctx, new_silo_user.id()).await.unwrap(); // Create organization with built-in user auth // Note: this currently goes to the built-in silo! @@ -136,10 +137,4 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { .silo_user_fetch(authn_opctx, new_silo_user.id()) .await .expect_err("unexpected success"); - - // Verify new user's console session isn't valid anymore. - nexus - .session_fetch(authn_opctx, session.token.clone()) - .await - .expect_err("unexpected success"); } From 04d6b8ec7e36f6a16112c807c0ed64470d7eefbb Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 10:50:22 -0600 Subject: [PATCH 09/16] Don't test SSH key endpoints in test_unauthorized Even unprivileged users are authorized to see their own SSH public keys. --- nexus/tests/integration_tests/endpoints.rs | 19 ------------------- nexus/tests/integration_tests/unauthorized.rs | 5 ----- .../output/uncovered-authz-endpoints.txt | 4 ++++ 3 files changed, 4 insertions(+), 24 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index b221c24c8a..460501df90 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -806,25 +806,6 @@ lazy_static! { allowed_methods: vec![AllowedMethod::Get], }, - VerifyEndpoint { - url: &*SSH_KEYS_URL, - visibility: Visibility::Protected, - allowed_methods: vec![ - AllowedMethod::Get, - AllowedMethod::Post( - serde_json::to_value(&*DEMO_SSH_KEY_CREATE).unwrap() - ), - ], - }, - VerifyEndpoint { - url: &*DEMO_SSH_KEY_URL, - visibility: Visibility::Protected, - allowed_methods: vec![ - AllowedMethod::Get, - AllowedMethod::Delete, - ], - }, - /* Hardware */ VerifyEndpoint { diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 3fd16db2a9..7697f4e326 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -193,11 +193,6 @@ lazy_static! { url: "/images", body: serde_json::to_value(&*DEMO_IMAGE_CREATE).unwrap(), }, - // Create an SSH public key - SetupReq { - url: &*SSH_KEYS_URL, - body: serde_json::to_value(&*DEMO_SSH_KEY_CREATE).unwrap(), - }, ]; } diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index 48197f499b..89a0dcc71e 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,4 +1,8 @@ API endpoints with no coverage in authz tests: +sshkeys_delete_key (delete "/session/me/sshkeys/{ssh_key_name}") session_me (get "/session/me") +sshkeys_get (get "/session/me/sshkeys") +sshkeys_get_key (get "/session/me/sshkeys/{ssh_key_name}") spoof_login (post "/login") logout (post "/logout") +sshkeys_post (post "/session/me/sshkeys") From 69f3154f9753ff2ed2eaba2ce7ff8537a4944f3e Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 11:03:22 -0600 Subject: [PATCH 10/16] Don't unwrap results of printing control codes to the terminal --- nexus/tests/integration_tests/unauthorized.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 7697f4e326..b1eca1fa74 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -522,11 +522,14 @@ fn record_operation(whichtest: WhichTest<'_>) { // Note that this likely still writes the color-changing control // characters to the real stdout, even without "--nocapture". That // sucks, but at least you don't see them. - term.fg(term::color::GREEN).unwrap(); - term.flush().unwrap(); + // + // We also don't unwrap() the results of printing control codes + // in case the terminal doesn't support them. + let _ = term.fg(term::color::GREEN); + let _ = term.flush(); print!("{}", c); - term.reset().unwrap(); - term.flush().unwrap(); + let _ = term.reset(); + let _ = term.flush(); } else { print!("{}", c); } From 2923e1e8c44352691bca7240db2ff361d1067de2 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 11:55:20 -0600 Subject: [PATCH 11/16] Missing spaces in error message --- nexus/src/db/db-macros/src/lookup.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/db/db-macros/src/lookup.rs b/nexus/src/db/db-macros/src/lookup.rs index 7182e2244c..c9f972139a 100644 --- a/nexus/src/db/db-macros/src/lookup.rs +++ b/nexus/src/db/db-macros/src/lookup.rs @@ -352,8 +352,8 @@ fn generate_misc_helpers(config: &Config) -> TokenStream { use crate::authz::ApiResourceError; error!( log, - "unexpected successful lookup of siloed resource\ - {:?} in a different Silo from current actor (resource\ + "unexpected successful lookup of siloed resource \ + {:?} in a different Silo from current actor (resource \ Silo {}, actor Silo {})", #resource_name_str, resource_silo_id, From 3ed430261d862736aea75da44fc74b65c069ea8d Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 11:56:45 -0600 Subject: [PATCH 12/16] Elide now-unused SSH endpoint test params --- nexus/tests/integration_tests/endpoints.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 460501df90..d79af1e519 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -243,21 +243,6 @@ lazy_static! { }, disk: DEMO_DISK_NAME.clone(), }; - - // SSH public keys - pub static ref SSH_KEYS_URL: String = - "/session/me/sshkeys".to_string(); - pub static ref DEMO_SSH_KEY_NAME: Name = "demo-ssh-key".parse().unwrap(); - pub static ref DEMO_SSH_KEY_URL: String = - format!("{}/{}", *SSH_KEYS_URL, *DEMO_SSH_KEY_NAME); - pub static ref DEMO_SSH_KEY_CREATE: params::SshKeyCreate = - params::SshKeyCreate { - identity: IdentityMetadataCreateParams { - name: DEMO_SSH_KEY_NAME.clone(), - description: String::from("my SSH public key"), - }, - public_key: "ssh-test AAAAAAAAA".to_string(), - }; } /// Describes an API endpoint to be verified by the "unauthorized" test From 4036a060dce033da3fa13e752281b9c62ea413e5 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 12:07:16 -0600 Subject: [PATCH 13/16] Remove superfluous roles on SiloUser --- nexus/src/authz/omicron.polar | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 12b95e9e7d..b5ee29a9cb 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -260,19 +260,12 @@ resource SiloUser { "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_silo: Silo }; - "admin" if "admin" on "parent_silo"; - "collaborator" if "collaborator" on "parent_silo"; - "viewer" if "viewer" on "parent_silo"; + + "list_children" if "viewer" on "parent_silo"; + "read" if "viewer" on "parent_silo"; + "modify" if "admin" on "parent_silo"; + "create_child" if "admin" on "parent_silo"; } has_relation(silo: Silo, "parent_silo", user: SiloUser) if user.silo = silo; From 9b858abf69d7338316421395ac6c625490277544 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 14:00:37 -0600 Subject: [PATCH 14/16] Move authz check for list SSH keys to datastore method --- nexus/src/db/datastore.rs | 2 ++ nexus/src/nexus.rs | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 42cbff3267..83126f60e7 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2779,6 +2779,8 @@ impl DataStore { authz_user: &authz::SiloUser, page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, authz_user).await?; + use db::schema::ssh_key::dsl; paginated(dsl::ssh_key, dsl::name, page_params) .filter(dsl::silo_user_id.eq(authz_user.id())) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 3443542c7c..df073df746 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -3836,7 +3836,6 @@ impl Nexus { authz_user: &authz::SiloUser, page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { - opctx.authorize(authz::Action::ListChildren, authz_user).await?; self.db_datastore.ssh_keys_list(opctx, authz_user, page_params).await } From 79d482a1105ed049138802996bc1eec82197e2a1 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 14:58:07 -0600 Subject: [PATCH 15/16] Pass IDs to Nexus methods, not authz objects --- nexus/src/external_api/http_entrypoints.rs | 12 ++---- nexus/src/nexus.rs | 48 +++++++++++++--------- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index bc6f7daf61..92ee274ad5 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2877,11 +2877,10 @@ async fn sshkeys_get( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; - let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; let page_params = data_page_params_for(&rqctx, &query)?.map_name(Name::ref_cast); let ssh_keys = nexus - .ssh_keys_list(&opctx, &authz_user, &page_params) + .ssh_keys_list(&opctx, actor.id, &page_params) .await? .into_iter() .map(SshKey::from) @@ -2906,9 +2905,8 @@ async fn sshkeys_post( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; - let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; let ssh_key = nexus - .ssh_key_create(&opctx, &authz_user, new_key.into_inner()) + .ssh_key_create(&opctx, actor.id, new_key.into_inner()) .await?; Ok(HttpResponseCreated(ssh_key.into())) }; @@ -2938,9 +2936,8 @@ async fn sshkeys_get_key( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; - let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; let ssh_key = - nexus.ssh_key_fetch(&opctx, &authz_user, ssh_key_name).await?; + nexus.ssh_key_fetch(&opctx, actor.id, ssh_key_name).await?; Ok(HttpResponseOk(ssh_key.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -2963,8 +2960,7 @@ async fn sshkeys_delete_key( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; - let (authz_user, _) = nexus.silo_user_fetch(&opctx, actor.id).await?; - nexus.ssh_key_delete(&opctx, &authz_user, ssh_key_name).await?; + nexus.ssh_key_delete(&opctx, actor.id, ssh_key_name).await?; Ok(HttpResponseDeleted()) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index df073df746..139f0a62ce 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -3819,13 +3819,12 @@ impl Nexus { &self, opctx: &OpContext, silo_user_id: Uuid, - ) -> LookupResult<(authz::SiloUser, db::model::SiloUser)> { - let (.., authz_user, db_silo_user) = - LookupPath::new(opctx, &self.db_datastore) - .silo_user_id(silo_user_id) - .fetch() - .await?; - Ok((authz_user, db_silo_user)) + ) -> LookupResult { + let (.., db_silo_user) = LookupPath::new(opctx, &self.datastore()) + .silo_user_id(silo_user_id) + .fetch() + .await?; + Ok(db_silo_user) } // SSH public keys @@ -3833,20 +3832,25 @@ impl Nexus { pub async fn ssh_keys_list( &self, opctx: &OpContext, - authz_user: &authz::SiloUser, + silo_user_id: Uuid, page_params: &DataPageParams<'_, Name>, ) -> ListResultVec { - self.db_datastore.ssh_keys_list(opctx, authz_user, page_params).await + let (.., authz_user) = LookupPath::new(opctx, &self.datastore()) + .silo_user_id(silo_user_id) + .lookup_for(authz::Action::ListChildren) + .await?; + assert_eq!(authz_user.id(), silo_user_id); + self.db_datastore.ssh_keys_list(opctx, &authz_user, page_params).await } pub async fn ssh_key_fetch( &self, opctx: &OpContext, - authz_user: &authz::SiloUser, + silo_user_id: Uuid, ssh_key_name: &Name, ) -> LookupResult { let (.., ssh_key) = LookupPath::new(opctx, &self.datastore()) - .silo_user_id(authz_user.id()) + .silo_user_id(silo_user_id) .ssh_key_name(ssh_key_name) .fetch() .await?; @@ -3857,27 +3861,31 @@ impl Nexus { pub async fn ssh_key_create( &self, opctx: &OpContext, - authz_user: &authz::SiloUser, + silo_user_id: Uuid, params: params::SshKeyCreate, ) -> CreateResult { - let ssh_key = db::model::SshKey::new(authz_user.id(), params); - Ok(self.db_datastore.ssh_key_create(opctx, authz_user, ssh_key).await?) + let ssh_key = db::model::SshKey::new(silo_user_id, params); + let (.., authz_user) = LookupPath::new(opctx, &self.datastore()) + .silo_user_id(silo_user_id) + .lookup_for(authz::Action::CreateChild) + .await?; + assert_eq!(authz_user.id(), silo_user_id); + Ok(self.db_datastore.ssh_key_create(opctx, &authz_user, ssh_key).await?) } pub async fn ssh_key_delete( &self, opctx: &OpContext, - authz_user: &authz::SiloUser, + silo_user_id: Uuid, ssh_key_name: &Name, ) -> DeleteResult { - let (.., authz_ssh_key, ssh_key) = + let (.., authz_user, authz_ssh_key) = LookupPath::new(opctx, &self.datastore()) - .silo_user_id(authz_user.id()) + .silo_user_id(silo_user_id) .ssh_key_name(ssh_key_name) - .fetch() + .lookup_for(authz::Action::Delete) .await?; - assert_eq!(ssh_key.name(), ssh_key_name); - + assert_eq!(authz_user.id(), silo_user_id); self.db_datastore.ssh_key_delete(opctx, &authz_ssh_key).await } } From c44bf25ee033a999963bb38734ac35cd9db5d2a1 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 22 Apr 2022 15:03:16 -0600 Subject: [PATCH 16/16] fmt --- nexus/src/nexus.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 139f0a62ce..b6c4dfb3e3 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -3870,7 +3870,10 @@ impl Nexus { .lookup_for(authz::Action::CreateChild) .await?; assert_eq!(authz_user.id(), silo_user_id); - Ok(self.db_datastore.ssh_key_create(opctx, &authz_user, ssh_key).await?) + Ok(self + .db_datastore + .ssh_key_create(opctx, &authz_user, ssh_key) + .await?) } pub async fn ssh_key_delete(