Skip to content

Commit

Permalink
Apply dest/target validation to VPC routes on PUT (#6004)
Browse files Browse the repository at this point in the history 
VPC custom route creation applies various validations on the contents of
a route. These include preventing mixed v4/v6 addresses (helpful) and
disallowing system-only targets such as subnets/vpcs (mandatory). This
PR ensures that these checks occur on modification of Default or Custom
routes by users.

Fixes #6003. Also fixes some typos identified by @elaine-oxide.
  • Loading branch information
@FelixMcFelix
FelixMcFelix authored Jul 6, 2024
1 parent 3cb1d1d commit c471735
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 100 deletions.
82 changes: 51 additions & 31 deletions nexus/src/app/vpc_router.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,37 +204,11 @@ impl super::Nexus {
));
}

// 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"
)),

_ => {},
};
validate_user_route_targets(
&params.destination,
&params.target,
RouterRouteKind::Custom,
)?;

let id = Uuid::new_v4();
let route = db::model::RouterRoute::new(
Expand Down Expand Up @@ -297,6 +271,12 @@ impl super::Nexus {
}
}

validate_user_route_targets(
&params.destination,
&params.target,
db_route.kind.0,
)?;

let out = self
.db_datastore
.router_update_route(&opctx, &authz_route, params.clone().into())
Expand Down Expand Up @@ -356,3 +336,43 @@ impl super::Nexus {
Ok(())
}
}

/// Validate route destinations/targets for a user-specified route:
/// - mixed explicit v4 and v6 are disallowed.
/// - users cannot specify 'Vpc' as a custom/default router dest/target.
/// - users cannot specify 'Subnet' as a custom/default router target.
/// - the only internet gateway we support today is 'outbound'.
fn validate_user_route_targets(
dest: &RouteDestination,
target: &RouteTarget,
route_type: RouterRouteKind,
) -> Result<(), Error> {
match (dest, 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(
format!("users cannot specify VPCs as a destination or target in {route_type} routes")
)),

(_, RouteTarget::Subnet(_)) => return Err(Error::invalid_request(
format!("subnets cannot be used as a target in {route_type} routers")
)),

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

_ => {},
};

Ok(())
}
163 changes: 102 additions & 61 deletions nexus/tests/integration_tests/router_routes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use dropshot::test_util::ClientTestContext;
use dropshot::HttpErrorResponseBody;
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::http_testing::RequestBuilder;
use nexus_test_utils::identity_eq;
use nexus_test_utils::resource_helpers::create_route;
use nexus_test_utils::resource_helpers::create_route_with_error;
Expand Down Expand Up @@ -437,26 +439,8 @@ async fn test_router_routes_internet_gateway_target(
let _router =
create_router(&client, PROJECT_NAME, VPC_NAME, router_name).await;

// Internet gateways are not fully supported: only 'inetgw:outbound'
// is a valid choice.
let dest: RouteDestination = "ipnet:240.0.0.0/8".parse().unwrap();

let err = create_route_with_error(
client,
PROJECT_NAME,
VPC_NAME,
&router_name,
"bad-route",
dest.clone(),
"inetgw:not-a-real-gw".parse().unwrap(),
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(
err.message,
"'outbound' is currently the only valid internet gateway"
);

// This can be used in a custom router, in addition
// to its default system spot.
let target: RouteTarget = "inetgw:outbound".parse().unwrap();
Expand Down Expand Up @@ -485,55 +469,112 @@ async fn test_router_routes_disallow_custom_targets(
let _router =
create_router(&client, PROJECT_NAME, VPC_NAME, router_name).await;

// Neither 'vpc:xxx' nor 'subnet:xxx' can be specified as route targets
// in custom routers.
let dest: RouteDestination = "ipnet:240.0.0.0/8".parse().unwrap();

let err = create_route_with_error(
let valid_dest: RouteDestination = "ipnet:240.0.0.0/8".parse().unwrap();
let pairs: &[(RouteDestination, RouteTarget, &str)] = &[
// Neither 'vpc:xxx' nor 'subnet:xxx' can be specified as route targets
// in custom routers.
(
valid_dest.clone(),
"vpc:a-vpc-name-unknown".parse().unwrap(),
"users cannot specify VPCs as a destination or target in Custom routes",
),
(
"vpc:a-vpc-name-unknown".parse().unwrap(),
RouteTarget::Drop,
"users cannot specify VPCs as a destination or target in Custom routes",
),
(
valid_dest.clone(),
"subnet:a-sub-name-unknown".parse().unwrap(),
"subnets cannot be used as a target in Custom routers",
),
// Internet gateways are not fully supported: only 'inetgw:outbound'
// is a valid choice.
(
valid_dest.clone(),
"inetgw:not-a-real-gw".parse().unwrap(),
"'outbound' is currently the only valid internet gateway",
),
];
_ = create_route(
client,
PROJECT_NAME,
VPC_NAME,
&router_name,
"bad-route",
dest.clone(),
"vpc:a-vpc-name-unknown".parse().unwrap(),
StatusCode::BAD_REQUEST,
router_name,
"good-route",
valid_dest,
"instance:madeup".parse().unwrap(),
)
.await;
assert_eq!(
err.message,
"VPCs cannot be used as a destination or target in custom routers"
);

let err = create_route_with_error(
client,
PROJECT_NAME,
VPC_NAME,
&router_name,
"bad-route",
"vpc:a-vpc-name-unknown".parse().unwrap(),
"drop".parse().unwrap(),
StatusCode::BAD_REQUEST,
)
.await;
assert_eq!(
err.message,
"VPCs cannot be used as a destination or target in custom routers"
);
for (update, (dest, target, msg)) in
[false, true].into_iter().cartesian_product(pairs.iter())
{
let err = if update {
create_route_with_error(
client,
PROJECT_NAME,
VPC_NAME,
&router_name,
"bad-route",
dest.clone(),
target.clone(),
StatusCode::BAD_REQUEST,
)
.await
} else {
update_route_with_error(
client,
PROJECT_NAME,
VPC_NAME,
&router_name,
"good-route",
dest.clone(),
target.clone(),
StatusCode::BAD_REQUEST,
)
.await
};

let err = create_route_with_error(
client,
PROJECT_NAME,
VPC_NAME,
&router_name,
"bad-route",
dest.clone(),
"subnet:a-vpc-name-unknown".parse().unwrap(),
StatusCode::BAD_REQUEST,
assert_eq!(err.message, *msg);
}
}

#[allow(clippy::too_many_arguments)]
pub async fn update_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::PUT,
format!(
"/v1/vpc-router-routes/{}?project={}&vpc={}&router={}",
route_name, project_name, vpc_name, router_name
)
.as_str(),
)
.body(Some(&params::RouterRouteUpdate {
identity: IdentityMetadataUpdateParams {
name: None,
description: None,
},
target,
destination,
}))
.expect_status(Some(status)),
)
.await;
assert_eq!(
err.message,
"subnets cannot be used as a target in custom routers"
);
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body()
.unwrap()
}
6 changes: 3 additions & 3 deletions nexus/types/src/external_api/params.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ pub struct SubnetSelector {
pub struct RouterSelector {
/// Name or ID of the project, only required if `vpc` is provided as a `Name`
pub project: Option<NameOrId>,
/// Name or ID of the VPC, only required if `subnet` is provided as a `Name`
/// Name or ID of the VPC, only required if `router` is provided as a `Name`
pub vpc: Option<NameOrId>,
/// Name or ID of the router
pub router: NameOrId,
Expand All @@ -269,7 +269,7 @@ pub struct RouterSelector {
pub struct OptionalRouterSelector {
/// Name or ID of the project, only required if `vpc` is provided as a `Name`
pub project: Option<NameOrId>,
/// Name or ID of the VPC, only required if `subnet` is provided as a `Name`
/// Name or ID of the VPC, only required if `router` is provided as a `Name`
pub vpc: Option<NameOrId>,
/// Name or ID of the router
pub router: Option<NameOrId>,
Expand All @@ -279,7 +279,7 @@ pub struct OptionalRouterSelector {
pub struct RouteSelector {
/// Name or ID of the project, only required if `vpc` is provided as a `Name`
pub project: Option<NameOrId>,
/// Name or ID of the VPC, only required if `subnet` is provided as a `Name`
/// Name or ID of the VPC, only required if `router` is provided as a `Name`
pub vpc: Option<NameOrId>,
/// Name or ID of the router, only required if `route` is provided as a `Name`
pub router: Option<NameOrId>,
Expand Down
10 changes: 5 additions & 5 deletions openapi/nexus.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8401,7 +8401,7 @@
{
"in": "query",
"name": "vpc",
"description": "Name or ID of the VPC, only required if `subnet` is provided as a `Name`",
"description": "Name or ID of the VPC, only required if `router` is provided as a `Name`",
"schema": {
"$ref": "#/components/schemas/NameOrId"
}
Expand Down Expand Up @@ -8458,7 +8458,7 @@
{
"in": "query",
"name": "vpc",
"description": "Name or ID of the VPC, only required if `subnet` is provided as a `Name`",
"description": "Name or ID of the VPC, only required if `router` is provided as a `Name`",
"schema": {
"$ref": "#/components/schemas/NameOrId"
}
Expand Down Expand Up @@ -8531,7 +8531,7 @@
{
"in": "query",
"name": "vpc",
"description": "Name or ID of the VPC, only required if `subnet` is provided as a `Name`",
"description": "Name or ID of the VPC, only required if `router` is provided as a `Name`",
"schema": {
"$ref": "#/components/schemas/NameOrId"
}
Expand Down Expand Up @@ -8591,7 +8591,7 @@
{
"in": "query",
"name": "vpc",
"description": "Name or ID of the VPC, only required if `subnet` is provided as a `Name`",
"description": "Name or ID of the VPC, only required if `router` is provided as a `Name`",
"schema": {
"$ref": "#/components/schemas/NameOrId"
}
Expand Down Expand Up @@ -8661,7 +8661,7 @@
{
"in": "query",
"name": "vpc",
"description": "Name or ID of the VPC, only required if `subnet` is provided as a `Name`",
"description": "Name or ID of the VPC, only required if `router` is provided as a `Name`",
"schema": {
"$ref": "#/components/schemas/NameOrId"
}
Expand Down

0 comments on commit c471735

Please sign in to comment.