-
Notifications
You must be signed in to change notification settings - Fork 61
authz: protect VPC endpoints #743
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.
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
Changes from all commits
2d56033
79f2385
cbad266
d601c1a
eedfd07
e2f5460
e4934df
0eb3f01
b017370
d5cb1b3
ab79a3a
3ff0cee
460dd38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2067,30 +2067,111 @@ impl DataStore { | |
|
|
||
| // VPCs | ||
|
|
||
| /// Fetches a Vpc from the database and returns both the database row | ||
| /// and an [`authz::Vpc`] for doing authz checks | ||
| /// | ||
| /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases | ||
| /// and caveats. | ||
| // TODO-security See the note on organization_lookup_noauthz(). | ||
| async fn vpc_lookup_noauthz( | ||
| &self, | ||
| authz_project: &authz::Project, | ||
| vpc_name: &Name, | ||
| ) -> LookupResult<(authz::Vpc, Vpc)> { | ||
| use db::schema::vpc::dsl; | ||
| dsl::vpc | ||
| .filter(dsl::time_deleted.is_null()) | ||
| .filter(dsl::project_id.eq(authz_project.id())) | ||
| .filter(dsl::name.eq(vpc_name.clone())) | ||
| .select(Vpc::as_select()) | ||
| .first_async(self.pool()) | ||
| .await | ||
| .map_err(|e| { | ||
| public_error_from_diesel_pool( | ||
| e, | ||
| ErrorHandler::NotFoundByLookup( | ||
| ResourceType::Vpc, | ||
| LookupType::ByName(vpc_name.as_str().to_owned()), | ||
| ), | ||
| ) | ||
| }) | ||
| .map(|d| { | ||
| ( | ||
| authz_project.child_generic( | ||
| ResourceType::Vpc, | ||
| d.id(), | ||
| LookupType::from(&vpc_name.0), | ||
| ), | ||
| d, | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| /// Look up the id for a Vpc based on its name | ||
| /// | ||
| /// Returns an [`authz::Vpc`] (which makes the id available). | ||
| /// | ||
| /// Like the other "lookup_by_path()" functions, this function does no authz | ||
| /// checks. | ||
| pub async fn vpc_lookup_by_path( | ||
| &self, | ||
| organization_name: &Name, | ||
| project_name: &Name, | ||
| vpc_name: &Name, | ||
| ) -> LookupResult<authz::Vpc> { | ||
| let authz_project = self | ||
| .project_lookup_by_path(organization_name, project_name) | ||
| .await?; | ||
| self.vpc_lookup_noauthz(&authz_project, vpc_name).await.map(|(v, _)| v) | ||
| } | ||
|
|
||
| /// Lookup a Vpc by name and return the full database record, along | ||
| /// with an [`authz::Vpc`] for subsequent authorization checks | ||
| pub async fn vpc_fetch( | ||
| &self, | ||
| opctx: &OpContext, | ||
| authz_project: &authz::Project, | ||
| name: &Name, | ||
| ) -> LookupResult<(authz::Vpc, Vpc)> { | ||
| let (authz_vpc, db_vpc) = | ||
| self.vpc_lookup_noauthz(authz_project, name).await?; | ||
| opctx.authorize(authz::Action::Read, &authz_vpc).await?; | ||
| Ok((authz_vpc, db_vpc)) | ||
| } | ||
|
|
||
| pub async fn project_list_vpcs( | ||
| &self, | ||
| project_id: &Uuid, | ||
| opctx: &OpContext, | ||
| authz_project: &authz::Project, | ||
| pagparams: &DataPageParams<'_, Name>, | ||
| ) -> ListResultVec<Vpc> { | ||
| use db::schema::vpc::dsl; | ||
| opctx.authorize(authz::Action::ListChildren, authz_project).await?; | ||
|
|
||
| use db::schema::vpc::dsl; | ||
| paginated(dsl::vpc, dsl::name, &pagparams) | ||
| .filter(dsl::time_deleted.is_null()) | ||
| .filter(dsl::project_id.eq(*project_id)) | ||
| .filter(dsl::project_id.eq(authz_project.id())) | ||
| .select(Vpc::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)) | ||
| } | ||
|
|
||
| pub async fn project_create_vpc(&self, vpc: Vpc) -> Result<Vpc, Error> { | ||
| pub async fn project_create_vpc( | ||
| &self, | ||
| opctx: &OpContext, | ||
| authz_project: &authz::Project, | ||
| vpc: Vpc, | ||
| ) -> Result<Vpc, Error> { | ||
| use db::schema::vpc::dsl; | ||
|
|
||
| assert_eq!(authz_project.id(), vpc.project_id); | ||
| opctx.authorize(authz::Action::CreateChild, authz_project).await?; | ||
|
|
||
| // TODO-correctness Shouldn't this use "insert_resource"? | ||
bnaecker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let name = vpc.name().clone(); | ||
| let vpc = diesel::insert_into(dsl::vpc) | ||
| .values(vpc) | ||
| .on_conflict(dsl::id) | ||
| .do_nothing() | ||
| .returning(Vpc::as_returning()) | ||
| .get_result_async(self.pool()) | ||
| .await | ||
|
|
@@ -2105,29 +2186,30 @@ impl DataStore { | |
|
|
||
| pub async fn project_update_vpc( | ||
| &self, | ||
| vpc_id: &Uuid, | ||
| opctx: &OpContext, | ||
| authz_vpc: &authz::Vpc, | ||
| updates: VpcUpdate, | ||
| ) -> Result<(), Error> { | ||
| use db::schema::vpc::dsl; | ||
| ) -> UpdateResult<Vpc> { | ||
| opctx.authorize(authz::Action::Modify, authz_vpc).await?; | ||
|
|
||
| use db::schema::vpc::dsl; | ||
| diesel::update(dsl::vpc) | ||
| .filter(dsl::time_deleted.is_null()) | ||
| .filter(dsl::id.eq(*vpc_id)) | ||
| .filter(dsl::id.eq(authz_vpc.id())) | ||
| .set(updates) | ||
| .execute_async(self.pool()) | ||
| .returning(Vpc::as_returning()) | ||
| .get_result_async(self.pool_authorized(opctx).await?) | ||
| .await | ||
| .map_err(|e| { | ||
| public_error_from_diesel_pool( | ||
| e, | ||
| ErrorHandler::NotFoundByLookup( | ||
| ResourceType::Vpc, | ||
| LookupType::ById(*vpc_id), | ||
| ), | ||
| ErrorHandler::NotFoundByResource(authz_vpc), | ||
| ) | ||
| })?; | ||
| Ok(()) | ||
| }) | ||
| } | ||
|
|
||
| // TODO-security TODO-cleanup Remove this function. Update callers to use | ||
| // vpc_lookup_by_path() or vpc_fetch() instead. | ||
bnaecker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pub async fn vpc_fetch_by_name( | ||
| &self, | ||
| project_id: &Uuid, | ||
|
|
@@ -2153,7 +2235,13 @@ impl DataStore { | |
| }) | ||
| } | ||
|
|
||
| pub async fn project_delete_vpc(&self, vpc_id: &Uuid) -> DeleteResult { | ||
| pub async fn project_delete_vpc( | ||
| &self, | ||
| opctx: &OpContext, | ||
| authz_vpc: &authz::Vpc, | ||
| ) -> DeleteResult { | ||
| opctx.authorize(authz::Action::Delete, authz_vpc).await?; | ||
|
|
||
| use db::schema::vpc::dsl; | ||
|
|
||
| // Note that we don't ensure the firewall rules are empty here, because | ||
|
|
@@ -2165,18 +2253,15 @@ impl DataStore { | |
| let now = Utc::now(); | ||
| diesel::update(dsl::vpc) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least some of the other deletion methods use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might be right, though I think we expect that DELETE of a resource that doesn't exist in the public API produces a 404, not a successful result. This seems worth testing more broadly, though this one seems hard to test because you will have just looked up the VPC successfully to get to this code path. Anyway, I think this is unrelated here (in that the behavior you're describing exists on "main" and isn't related to the change here). I'd like to file a separate issue to cover this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant that: if there is no such record at all, return 404; if there is a record, but it's already deleted, return success. Or by "in the public API", did you mean one that isn't deleted? As an example, when you create a VPC (so you have the ID), then delete it twice, what should we return for that second call? But that's a general question, I agree, and we should do it separately. |
||
| .filter(dsl::time_deleted.is_null()) | ||
| .filter(dsl::id.eq(*vpc_id)) | ||
| .filter(dsl::id.eq(authz_vpc.id())) | ||
| .set(dsl::time_deleted.eq(now)) | ||
| .returning(Vpc::as_returning()) | ||
| .get_result_async(self.pool()) | ||
| .get_result_async(self.pool_authorized(opctx).await?) | ||
| .await | ||
| .map_err(|e| { | ||
| public_error_from_diesel_pool( | ||
| e, | ||
| ErrorHandler::NotFoundByLookup( | ||
| ResourceType::Vpc, | ||
| LookupType::ById(*vpc_id), | ||
| ), | ||
| ErrorHandler::NotFoundByResource(authz_vpc), | ||
| ) | ||
| })?; | ||
| Ok(()) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.