Skip to content

Commit

Permalink
Validate custom, dest/target pairs on route create
Browse files Browse the repository at this point in the history
  • Loading branch information
@FelixMcFelix
FelixMcFelix committed May 29, 2024
1 parent c1dc6d0 commit 0f0dcb6
Show file tree
Hide file tree
Showing 4 changed files with 234 additions and 32 deletions.
47 changes: 45 additions & 2 deletions nexus/src/app/vpc_router.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@ use omicron_common::api::external::http_pagination::PaginatedBy;
use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DeleteResult;
use omicron_common::api::external::Error;
use omicron_common::api::external::IpNet;
use omicron_common::api::external::ListResultVec;
use omicron_common::api::external::LookupResult;
use omicron_common::api::external::NameOrId;
use omicron_common::api::external::RouteDestination;
use omicron_common::api::external::RouteTarget;
use omicron_common::api::external::RouterRouteKind;
use omicron_common::api::external::UpdateResult;
use std::net::IpAddr;
use uuid::Uuid;

impl super::Nexus {
Expand Down Expand Up @@ -191,8 +195,47 @@ impl super::Nexus {
kind: &RouterRouteKind,
params: &params::RouterRouteCreate,
) -> CreateResult<db::model::RouterRoute> {
let (.., authz_router) =
router_lookup.lookup_for(authz::Action::CreateChild).await?;
let (.., authz_router, db_router) =
router_lookup.fetch_for(authz::Action::CreateChild).await?;

if db_router.kind == VpcRouterKind::System {
return Err(Error::invalid_request(
"user-provided routes cannot be added to a system router",
));
}

// Validate route destinations/targets at this stage:
// - mixed explicit v4 and v6 are disallowed.
// - users cannot specify 'Vpc' as a custom router dest/target.
// - users cannot specify 'Subnet' as a custom router target.
// - the only internet gateway we support today is 'outbound'.
match (&params.destination, &params.target) {
(RouteDestination::Ip(IpAddr::V4(_)), RouteTarget::Ip(IpAddr::V4(_)))
| (RouteDestination::Ip(IpAddr::V6(_)), RouteTarget::Ip(IpAddr::V6(_)))
| (RouteDestination::IpNet(IpNet::V4(_)), RouteTarget::Ip(IpAddr::V4(_)))
| (RouteDestination::IpNet(IpNet::V6(_)), RouteTarget::Ip(IpAddr::V6(_))) => {},

(RouteDestination::Ip(_), RouteTarget::Ip(_))
| (RouteDestination::IpNet(_), RouteTarget::Ip(_))
=> return Err(Error::invalid_request(
"cannot mix explicit IPv4 and IPv6 addresses between destination and target"
)),

(RouteDestination::Vpc(_), _) | (_, RouteTarget::Vpc(_)) => return Err(Error::invalid_request(
"VPCs cannot be used as a destination or target in custom routers"
)),

(_, RouteTarget::Subnet(_)) => return Err(Error::invalid_request(
"subnets cannot be used as a target in custom routers"
)),

(_, RouteTarget::InternetGateway(n)) if n.as_str() != "outbound" => return Err(Error::invalid_request(
"'outbound' is currently the only valid internet gateway"
)),

_ => {},
};

let id = Uuid::new_v4();
let route = db::model::RouterRoute::new(
id,
Expand Down
75 changes: 75 additions & 0 deletions nexus/test-utils/src/resource_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ use omicron_common::api::external::InstanceCpuCount;
use omicron_common::api::external::Ipv4Net;
use omicron_common::api::external::Ipv6Net;
use omicron_common::api::external::NameOrId;
use omicron_common::api::external::RouteDestination;
use omicron_common::api::external::RouteTarget;
use omicron_common::api::external::RouterRoute;
use omicron_common::disk::DiskIdentity;
use omicron_sled_agent::sim::SledAgent;
use omicron_test_utils::dev::poll::wait_for_condition;
Expand Down Expand Up @@ -613,6 +616,78 @@ pub async fn create_router(
.unwrap()
}

pub async fn create_route(
client: &ClientTestContext,
project_name: &str,
vpc_name: &str,
router_name: &str,
route_name: &str,
destination: RouteDestination,
target: RouteTarget,
) -> RouterRoute {
NexusRequest::objects_post(
&client,
format!(
"/v1/vpc-router-routes?project={}&vpc={}&router={}",
&project_name, &vpc_name, &router_name
)
.as_str(),
&params::RouterRouteCreate {
identity: IdentityMetadataCreateParams {
name: route_name.parse().unwrap(),
description: String::from("route description"),
},
target,
destination,
},
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body()
.unwrap()
}

#[allow(clippy::too_many_arguments)]
pub async fn create_route_with_error(
client: &ClientTestContext,
project_name: &str,
vpc_name: &str,
router_name: &str,
route_name: &str,
destination: RouteDestination,
target: RouteTarget,
status: StatusCode,
) -> HttpErrorResponseBody {
NexusRequest::new(
RequestBuilder::new(
client,
Method::POST,
format!(
"/v1/vpc-router-routes?project={}&vpc={}&router={}",
&project_name, &vpc_name, &router_name
)
.as_str(),
)
.body(Some(&params::RouterRouteCreate {
identity: IdentityMetadataCreateParams {
name: route_name.parse().unwrap(),
description: String::from("route description"),
},
target,
destination,
}))
.expect_status(Some(status)),
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body()
.unwrap()
}

pub async fn assert_ip_pool_utilization(
client: &ClientTestContext,
pool_name: &str,
Expand Down
91 changes: 90 additions & 1 deletion nexus/tests/integration_tests/router_routes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@

use dropshot::Method;
use http::StatusCode;
use itertools::Itertools;
use nexus_test_utils::http_testing::AuthnMode;
use nexus_test_utils::http_testing::NexusRequest;
use nexus_test_utils::identity_eq;
use nexus_test_utils::resource_helpers::create_route;
use nexus_test_utils::resource_helpers::create_route_with_error;
use nexus_test_utils::resource_helpers::objects_list_page_authz;
use nexus_test_utils_macros::nexus_test;
use nexus_types::external_api::params;
Expand All @@ -23,11 +26,15 @@ use nexus_test_utils::resource_helpers::{
create_project, create_router, create_vpc,
};

use crate::integration_tests::vpc_routers::PROJECT_NAME;

type ControlPlaneTestContext =
nexus_test_utils::ControlPlaneTestContext<omicron_nexus::Server>;

#[nexus_test]
async fn test_router_routes(cptestctx: &ControlPlaneTestContext) {
async fn test_router_routes_crud_operations(
cptestctx: &ControlPlaneTestContext,
) {
let client = &cptestctx.external_client;

let project_name = "springfield-squidport";
Expand Down Expand Up @@ -212,3 +219,85 @@ async fn test_router_routes(cptestctx: &ControlPlaneTestContext) {
.await
.unwrap();
}

#[nexus_test]
async fn test_router_routes_disallow_mixed_v4_v6(
cptestctx: &ControlPlaneTestContext,
) {
let client = &cptestctx.external_client;
let _ = create_project(&client, PROJECT_NAME).await;

let vpc_name = "default";
let router_name = "routy";
let _router =
create_router(&client, PROJECT_NAME, vpc_name, router_name).await;

// Some targets/strings refer to a mixed v4/v6 entity, e.g.,
// subnet or instance. Others refer to one kind only (ipnet, ip).
// Users should not be able to mix v4 and v6 in these latter routes
// -- route resolution will ignore them, but a helpful error message
// is more useful.
let dest_set: [RouteDestination; 5] = [
"ip:4.4.4.4".parse().unwrap(),
"ipnet:4.4.4.0/24".parse().unwrap(),
"ip:2001:4860:4860::8888".parse().unwrap(),
"ipnet:2001:4860:4860::/64".parse().unwrap(),
"subnet:named-subnet".parse().unwrap(),
];

let target_set: [RouteTarget; 5] = [
"ip:172.30.0.5".parse().unwrap(),
"ip:fd37:faf4:cc25::5".parse().unwrap(),
"instance:named-instance".parse().unwrap(),
"inetgw:outbound".parse().unwrap(),
"drop".parse().unwrap(),
];

for (i, (dest, target)) in dest_set
.into_iter()
.cartesian_product(target_set.into_iter())
.enumerate()
{
use RouteDestination as Rd;
use RouteTarget as Rt;
let allowed = match (&dest, &target) {
(Rd::Ip(IpAddr::V4(_)), Rt::Ip(IpAddr::V4(_)))
| (Rd::Ip(IpAddr::V6(_)), Rt::Ip(IpAddr::V6(_)))
| (Rd::IpNet(IpNet::V4(_)), Rt::Ip(IpAddr::V4(_)))
| (Rd::IpNet(IpNet::V6(_)), Rt::Ip(IpAddr::V6(_))) => true,
(Rd::Ip(_), Rt::Ip(_)) | (Rd::IpNet(_), Rt::Ip(_)) => false,
_ => true,
};

let route_name = format!("test-route-{i}");

if allowed {
create_route(
client,
PROJECT_NAME,
vpc_name,
router_name,
&route_name,
dest,
target,
)
.await;
} else {
let err = create_route_with_error(
client,
PROJECT_NAME,
vpc_name,
router_name,
&route_name,
dest,
target,
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(
err.message,
"cannot mix explicit IPv4 and IPv6 addresses between destination and target"
);
}
}
}
Loading

0 comments on commit 0f0dcb6

Please sign in to comment.