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

authz: protect VPC endpoints #743

Merged
merged 13 commits into from
Mar 10, 2022
Merged

authz: protect VPC endpoints #743

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2d56033
update unauthorized test
davepacheco Mar 9, 2022
79f2385
add protection to LIST and GET VPCs
davepacheco Mar 9, 2022
cbad266
protect VPC create endpoint
davepacheco Mar 9, 2022
d601c1a
add protection to PUT VPC
davepacheco Mar 9, 2022
eedfd07
protect VPC delete
davepacheco Mar 9, 2022
e2f5460
VPC update could return 200 (fixes #742)
davepacheco Mar 9, 2022
e4934df
fix some tests
davepacheco Mar 9, 2022
0eb3f01
fix test
davepacheco Mar 9, 2022
b017370
fix test
davepacheco Mar 9, 2022
d5cb1b3
clean up XXXs
davepacheco Mar 9, 2022
ab79a3a
actually add authz check to delete
davepacheco Mar 9, 2022
3ff0cee
dead code walking
davepacheco Mar 10, 2022
460dd38
Merge branch 'main' into authz-vpc
davepacheco Mar 10, 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
1 change: 1 addition & 0 deletions nexus/src/authz/api_resources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,3 +479,4 @@ impl ApiResourceError for ProjectChild {

pub type Disk = ProjectChild;
pub type Instance = ProjectChild;
pub type Vpc = ProjectChild;
1 change: 1 addition & 0 deletions nexus/src/authz/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ pub use api_resources::FleetChild;
pub use api_resources::Instance;
pub use api_resources::Organization;
pub use api_resources::Project;
pub use api_resources::Vpc;
pub use api_resources::FLEET;

mod context;
Expand Down
135 changes: 110 additions & 25 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2012,30 +2012,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);
bnaecker marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -2050,29 +2131,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,
Expand All @@ -2098,7 +2180,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
Expand All @@ -2110,18 +2198,15 @@ impl DataStore {
let now = Utc::now();
diesel::update(dsl::vpc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

At least some of the other deletion methods use the check_if_exists method to separately handle the case where there is no such object at all, and where there is one, but it's already been deleted. In the latter case, I think we want to not update time_deleted and return a success, to make the operation idempotent. If I understand correctly, this will return an object-not-found error in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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(())
Expand Down
23 changes: 18 additions & 5 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,8 +1130,10 @@ async fn project_vpcs_get(
let organization_name = &path.organization_name;
let project_name = &path.project_name;
let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let vpcs = nexus
.project_list_vpcs(
&opctx,
&organization_name,
&project_name,
&data_page_params_for(&rqctx, &query)?
Expand Down Expand Up @@ -1176,8 +1178,9 @@ async fn project_vpcs_get_vpc(
let project_name = &path.project_name;
let vpc_name = &path.vpc_name;
let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let vpc = nexus
.project_lookup_vpc(&organization_name, &project_name, &vpc_name)
.vpc_fetch(&opctx, &organization_name, &project_name, &vpc_name)
.await?;
Ok(HttpResponseOk(vpc.into()))
};
Expand All @@ -1204,8 +1207,10 @@ async fn project_vpcs_post(
let project_name = &path.project_name;
let new_vpc_params = &new_vpc.into_inner();
let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let vpc = nexus
.project_create_vpc(
&opctx,
&organization_name,
&project_name,
&new_vpc_params,
Expand All @@ -1228,20 +1233,22 @@ async fn project_vpcs_put_vpc(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
path_params: Path<VpcPathParam>,
updated_vpc: TypedBody<params::VpcUpdate>,
) -> Result<HttpResponseUpdatedNoContent, HttpError> {
) -> Result<HttpResponseOk<Vpc>, HttpError> {
let apictx = rqctx.context();
let nexus = &apictx.nexus;
let path = path_params.into_inner();
let handler = async {
nexus
let opctx = OpContext::for_external_api(&rqctx).await?;
let newvpc = nexus
.project_update_vpc(
&opctx,
&path.organization_name,
&path.project_name,
&path.vpc_name,
&updated_vpc.into_inner(),
)
.await?;
Ok(HttpResponseUpdatedNoContent())
Ok(HttpResponseOk(newvpc.into()))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}
Expand All @@ -1265,8 +1272,14 @@ async fn project_vpcs_delete_vpc(
let project_name = &path.project_name;
let vpc_name = &path.vpc_name;
let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
nexus
.project_delete_vpc(&organization_name, &project_name, &vpc_name)
.project_delete_vpc(
&opctx,
&organization_name,
&project_name,
&vpc_name,
)
.await?;
Ok(HttpResponseDeleted())
};
Expand Down
Loading