From 04474a452ba0bc0d30bb0fcadf2be1f7af886bfe Mon Sep 17 00:00:00 2001 From: sdairs Date: Fri, 28 Aug 2026 10:53:46 +0100 Subject: [PATCH 1/2] Validate private endpoint IDs before registering them `cloud service private-endpoint create --endpoint-id` and `cloud service update --add-private-endpoint-id` accepted any string. The API does not check the value either, so a typo registered a dud endpoint org-wide that had to be unpicked from both the service and the organization. Add a clap `value_parser` on both add-flags so a malformed ID fails as a usage error (exit 2) before any request is sent. The provider is unknown at parse time and the three formats are unrelated (AWS `vpce-` VPC endpoint ID, GCP numeric PSC connection ID, Azure private endpoint Resource ID or resourceGuid), so only provider-independent mistakes are rejected: empty values, whitespace, and any value carrying `vpce-` that is not exactly `vpce-` plus 8 or 17 lowercase hex characters (catching truncated, uppercased, ARN- and service-name-pasted IDs). Azure Resource IDs are exempt from the AWS check because such a resource may itself be named `vpce-...`. Removal flags are deliberately unchecked so an already-registered bogus ID stays removable. Ownership/existence is not validated: that is not checkable client-side and remains an upstream API gap. Tests: clap accept/reject cases for both flags plus the remove-flag escape hatch, unit tests for the validator across all three provider formats, and wiremock subprocess tests asserting exit 2 with no request issued for a malformed ID and verbatim forwarding of valid AWS/GCP/Azure IDs. Fixes #611 Co-Authored-By: Claude Fable 5 --- README.md | 5 +- crates/clickhousectl/src/cloud/services.rs | 251 +++++++++++++++++- .../tests/cli_request_shape_test.rs | 148 +++++++++++ 3 files changed, 393 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 8cb3116a..cfa175be 100644 --- a/README.md +++ b/README.md @@ -614,7 +614,8 @@ clickhousectl cloud service query-endpoint create \ clickhousectl cloud service query-endpoint delete # Private endpoint management -clickhousectl cloud service private-endpoint create --endpoint-id vpce-123 +clickhousectl cloud service private-endpoint create \ + --endpoint-id vpce-0123456789abcdef0 clickhousectl cloud service private-endpoint get-config # Backup configuration @@ -640,6 +641,8 @@ Use `clickhousectl cloud service create --help` for the complete option list. If `--query` and `--queries-file` are mutually exclusive. If neither is supplied, `cloud service query` reads SQL from stdin; `--queries-file -` also reads stdin explicitly. +Private endpoint IDs supplied to `private-endpoint create --endpoint-id` and `service update --add-private-endpoint-id` are format-checked before the request is sent, because adding one registers it for the whole organization and a typo has to be unpicked from both the service and the organization. Each provider uses its own format — AWS a `vpce-` VPC endpoint ID, GCP the numeric Private Service Connect connection ID, Azure the private endpoint Resource ID or `resourceGuid` — and the provider is not known when the flag is parsed, so only provider-independent mistakes are rejected (exit code `2`): empty values, values containing whitespace, and any value carrying `vpce-` that is not exactly a well-formed AWS VPC endpoint ID (`vpce-` plus 8 or 17 lowercase hex characters) — which also catches a pasted VPC endpoint ARN or endpoint service name. Azure Resource IDs (values starting with `/`) are exempt from that check, since an Azure resource may itself be named `vpce-...`. Whether the endpoint actually exists and belongs to you is not validated by the CLI or, currently, by the Cloud API. Removal flags are never format-checked, so an already-registered bogus ID stays removable. + #### Query API auth modes `cloud service query` is the canonical way to run SQL against a cloud service — over HTTP, with no `clickhouse` binary and no service password required. It works with both credential modes: diff --git a/crates/clickhousectl/src/cloud/services.rs b/crates/clickhousectl/src/cloud/services.rs index 52ddb59d..c9435dac 100644 --- a/crates/clickhousectl/src/cloud/services.rs +++ b/crates/clickhousectl/src/cloud/services.rs @@ -276,7 +276,7 @@ CONTEXT FOR AGENTS: remove_ip_allow: Vec, /// Add a private endpoint ID to the service - #[arg(long = "add-private-endpoint-id")] + #[arg(long = "add-private-endpoint-id", value_parser = parse_private_endpoint_id_arg)] add_private_endpoint_id: Vec, /// Remove a private endpoint ID from the service @@ -554,8 +554,9 @@ pub enum PrivateEndpointCommands { /// Service ID service_id: String, - /// Private endpoint ID (VPC endpoint ID) - #[arg(long)] + /// Private endpoint ID (AWS VPC endpoint ID, GCP PSC connection ID, or + /// Azure private endpoint Resource ID / resourceGuid) + #[arg(long, value_parser = parse_private_endpoint_id_arg)] endpoint_id: String, /// Description @@ -916,6 +917,79 @@ fn parse_ip_access_list_patch(add: &[String], remove: &[String]) -> Option std::result::Result { + validate_private_endpoint_id(value).map(|()| value.to_string()) +} + +/// The three providers use unrelated endpoint ID formats, and the provider is +/// not known at parse time, so only checks that hold for every provider are +/// applied: +/// +/// * AWS PrivateLink IDs are `vpce-` plus 8 or 17 lowercase hex characters. The +/// prefix is unmistakable, so a value carrying it anywhere must be exactly one +/// well-formed AWS ID — that catches truncated IDs, uppercased IDs and a +/// pasted VPC endpoint ARN. +/// * GCP uses a numeric Private Service Connect connection ID and Azure uses a +/// private endpoint Resource ID or `resourceGuid`. Neither has a marker that +/// distinguishes a typo from a valid value, so they are only checked for +/// emptiness and whitespace (illegal in all three formats). An Azure Resource +/// ID is an absolute path and can name a resource `vpce-...`, so it is exempt +/// from the AWS check. +/// +/// Whether the endpoint exists and belongs to the caller is not checkable from +/// the client; that remains an upstream API gap. +fn validate_private_endpoint_id(value: &str) -> std::result::Result<(), String> { + if value.trim().is_empty() { + return Err("private endpoint ID must not be empty".to_string()); + } + if value.chars().any(char::is_whitespace) { + return Err(format!( + "invalid private endpoint ID '{value}': IDs cannot contain whitespace \ + (pass each endpoint ID as its own flag value)" + )); + } + // Azure Resource IDs are absolute paths and can legitimately name a + // resource `vpce-...`, so they are exempt from the AWS-shaped check below. + if value.starts_with('/') { + return Ok(()); + } + if value.to_ascii_lowercase().contains(AWS_VPC_ENDPOINT_PREFIX) + && !is_aws_vpc_endpoint_id(value) + { + return Err(format!( + "invalid AWS VPC endpoint ID '{value}': expected exactly '{AWS_VPC_ENDPOINT_PREFIX}' \ + followed by 17 lowercase hex characters (for example vpce-0123456789abcdef0). GCP \ + uses the numeric Private Service Connect connection ID and Azure the private \ + endpoint Resource ID or resourceGuid" + )); + } + Ok(()) +} + +/// `vpce-` plus 8 (legacy) or 17 (current) lowercase hex characters, and nothing +/// else. +fn is_aws_vpc_endpoint_id(value: &str) -> bool { + let Some(suffix) = value.strip_prefix(AWS_VPC_ENDPOINT_PREFIX) else { + return false; + }; + matches!(suffix.len(), 8 | 17) + && suffix + .chars() + .all(|c| c.is_ascii_digit() || matches!(c, 'a'..='f')) +} + fn parse_private_endpoint_ids_patch( add: &[String], remove: &[String], @@ -3014,7 +3088,7 @@ mod tests { "create", "svc-1", "--endpoint-id", - "vpce-1", + "vpce-0123456789abcdef0", ]); let crate::cloud::cli::ServiceCommands::PrivateEndpoint { command } = private_endpoint else { @@ -3805,7 +3879,7 @@ mod tests { "create", "svc-1", "--endpoint-id", - "vpce-1", + "vpce-0123456789abcdef0", "--description", "production", "--org-id", @@ -3824,11 +3898,167 @@ mod tests { panic!("expected private-endpoint create"); }; assert_eq!(service_id, "svc-1"); - assert_eq!(endpoint_id, "vpce-1"); + assert_eq!(endpoint_id, "vpce-0123456789abcdef0"); assert_eq!(description.as_deref(), Some("production")); assert_eq!(org_id.as_deref(), Some("org-1")); } + /// Non-AWS endpoint IDs have no marker that separates a typo from a valid + /// value, so they must pass through untouched (issue #611). + #[test] + fn parses_non_aws_private_endpoint_ids_unchanged() { + for id in [ + // GCP Private Service Connect connection ID. + "102600141743718403", + // Azure private endpoint Resource ID. + "/subscriptions/11111111-2222-3333-4444-555555555555/resourceGroups/rg/providers/\ + Microsoft.Network/privateEndpoints/pe-demo", + // Azure resourceGuid (legacy form). + "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + // Legacy 8-character AWS VPC endpoint ID. + "vpce-0123abcd", + ] { + let command = parse_service(&[ + "clickhousectl", + "cloud", + "service", + "private-endpoint", + "create", + "svc-1", + "--endpoint-id", + id, + ]); + let crate::cloud::cli::ServiceCommands::PrivateEndpoint { command } = command else { + panic!("expected private-endpoint command"); + }; + let crate::cloud::cli::PrivateEndpointCommands::Create { endpoint_id, .. } = command + else { + panic!("expected private-endpoint create"); + }; + assert_eq!(endpoint_id, id, "{id} must be accepted verbatim"); + } + } + + #[test] + fn rejects_malformed_private_endpoint_create_endpoint_id() { + for id in ["vpce-1", "vpce-bogus", "VPCE-0123456789ABCDEF0", "", " "] { + let error = Cli::try_parse_from([ + "clickhousectl", + "cloud", + "service", + "private-endpoint", + "create", + "svc-1", + "--endpoint-id", + id, + ]) + .err() + .unwrap_or_else(|| panic!("--endpoint-id {id:?} must be rejected")); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ValueValidation, + "--endpoint-id {id:?} must fail as a usage error" + ); + } + } + + #[test] + fn rejects_malformed_added_private_endpoint_id_on_update() { + let error = Cli::try_parse_from([ + "clickhousectl", + "cloud", + "service", + "update", + "svc-1", + "--add-private-endpoint-id", + "vpce-nope", + ]) + .err() + .expect("malformed --add-private-endpoint-id must be rejected"); + assert_eq!(error.kind(), clap::error::ErrorKind::ValueValidation); + } + + /// Removing an ID must never be format-checked: a bogus ID that is already + /// registered has to stay removable (issue #611). + #[test] + fn accepts_malformed_removed_private_endpoint_id_on_update() { + let command = parse_service(&[ + "clickhousectl", + "cloud", + "service", + "update", + "svc-1", + "--remove-private-endpoint-id", + "vpce-nope", + ]); + let crate::cloud::cli::ServiceCommands::Update { + remove_private_endpoint_id, + .. + } = command + else { + panic!("expected service update"); + }; + assert_eq!(remove_private_endpoint_id, vec!["vpce-nope"]); + } + + #[test] + fn validate_private_endpoint_id_accepts_every_provider_format() { + for id in [ + "vpce-0123456789abcdef0", + "vpce-0123abcd", + "102600141743718403", + "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/privateEndpoints/pe", + // An Azure resource may itself be named `vpce-...`. + "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/privateEndpoints/\ + vpce-mine", + ] { + assert!( + validate_private_endpoint_id(id).is_ok(), + "{id} must be accepted" + ); + } + } + + #[test] + fn validate_private_endpoint_id_rejects_empty_values() { + for id in ["", " ", "\t"] { + let error = validate_private_endpoint_id(id).expect_err("empty must be rejected"); + assert!(error.contains("must not be empty"), "{error}"); + } + } + + #[test] + fn validate_private_endpoint_id_rejects_malformed_values() { + for id in [ + // Two IDs squeezed into one quoted flag value. + "vpce-0123456789abcdef0 vpce-0123abcd", + "vpce-", + "vpce-1", + "vpce-xyz", + // 16 hex characters: one short of the current AWS length. + "vpce-0123456789abcdef", + // 18 hex characters: one too many. + "vpce-0123456789abcdef01", + // Non-hex character inside an otherwise correct-length ID. + "vpce-0123456789abcdefg", + // AWS never issues uppercase IDs, so this would register a dud. + "VPCE-0123456789ABCDEF0", + // Endpoint *service* name pasted instead of the endpoint ID. + "com.amazonaws.vpce.us-east-1.vpce-svc-0123456789abcdef0", + // Full ARN pasted instead of the endpoint ID. + "arn:aws:ec2:us-east-1:123456789012:vpc-endpoint/vpce-0123456789abcdef0", + ] { + let error = validate_private_endpoint_id(id) + .expect_err(&format!("{id} must be rejected, not accepted")); + // The message names the offending value so the fix is obvious. + assert!( + error.contains(id), + "error for {id} should quote the value: {error}" + ); + } + } + #[test] fn parses_service_identity_lifecycle_and_prometheus_options() { let command = parse_service(&[ @@ -4985,17 +5215,18 @@ mod tests { #[test] fn build_private_endpoint_create_request_supports_minimal_fields() { - let request = build_private_endpoint_create_request("vpce-1", None); + let request = build_private_endpoint_create_request("vpce-0123456789abcdef0", None); - assert_eq!(request.id, "vpce-1"); + assert_eq!(request.id, "vpce-0123456789abcdef0"); assert!(request.description.is_empty()); } #[test] fn build_private_endpoint_create_request_supports_maximal_fields() { - let request = build_private_endpoint_create_request("vpce-1", Some("production")); + let request = + build_private_endpoint_create_request("vpce-0123456789abcdef0", Some("production")); - assert_eq!(request.id, "vpce-1"); + assert_eq!(request.id, "vpce-0123456789abcdef0"); assert_eq!(request.description, "production"); } diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 315c70d6..e6b067e6 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -6475,3 +6475,151 @@ async fn service_update_skips_get_when_no_removals_requested() { String::from_utf8_lossy(&output.stderr) ); } + +// ── Private endpoint ID format validation (issue #611) ───────────────────── + +#[tokio::test] +async fn private_endpoint_create_sends_well_formed_endpoint_id() { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path( + "/v1/organizations/org-1/services/svc-1/privateEndpoint", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": { "id": "vpce-0123456789abcdef0", "description": "prod" }, + "status": 200, + "requestId": "stub-private-endpoint-create", + }))) + .expect(1) + .mount(&mock) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "private-endpoint", + "create", + "svc-1", + "--org-id", + "org-1", + "--endpoint-id", + "vpce-0123456789abcdef0", + ], + ); + + assert_success(&output); + let requests = mock.received_requests().await.unwrap(); + let body: Value = serde_json::from_slice(&requests[0].body).unwrap(); + assert_eq!(body["id"], "vpce-0123456789abcdef0"); +} + +/// A malformed ID must fail as a clap usage error (exit 2) before any request +/// is sent: registering one is org-wide and has to be unpicked by hand. +#[tokio::test] +async fn private_endpoint_create_rejects_malformed_endpoint_id_without_calling_api() { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path( + "/v1/organizations/org-1/services/svc-1/privateEndpoint", + )) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&mock) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "private-endpoint", + "create", + "svc-1", + "--org-id", + "org-1", + "--endpoint-id", + "vpce-bogus", + ], + ); + + assert_eq!(output.status.code(), Some(2)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("invalid AWS VPC endpoint ID 'vpce-bogus'"), + "stderr should explain the format: {stderr}" + ); + assert!( + mock.received_requests().await.unwrap().is_empty(), + "a rejected endpoint ID must not reach the API" + ); +} + +#[tokio::test] +async fn service_update_rejects_malformed_added_private_endpoint_id_without_calling_api() { + let mock = MockServer::start().await; + Mock::given(method("PATCH")) + .and(path("/v1/organizations/org-1/services/svc-1")) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&mock) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "update", + "svc-1", + "--org-id", + "org-1", + "--add-private-endpoint-id", + "vpce-bogus", + ], + ); + + assert_eq!(output.status.code(), Some(2)); + assert!(mock.received_requests().await.unwrap().is_empty()); +} + +/// GCP (numeric PSC connection ID) and Azure (Resource ID) formats are not +/// AWS-shaped and must still be forwarded verbatim. +#[tokio::test] +async fn private_endpoint_create_forwards_non_aws_endpoint_ids() { + for endpoint_id in [ + "102600141743718403", + "/subscriptions/11111111-2222-3333-4444-555555555555/resourceGroups/rg/providers/Microsoft.Network/privateEndpoints/pe-demo", + ] { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path( + "/v1/organizations/org-1/services/svc-1/privateEndpoint", + )) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": { "id": endpoint_id, "description": "" }, + "status": 200, + "requestId": "stub-private-endpoint-create", + }))) + .expect(1) + .mount(&mock) + .await; + + let output = invoke_cli_with_cloud_credentials( + &mock, + &[ + "service", + "private-endpoint", + "create", + "svc-1", + "--org-id", + "org-1", + "--endpoint-id", + endpoint_id, + ], + ); + + assert_success(&output); + let requests = mock.received_requests().await.unwrap(); + let body: Value = serde_json::from_slice(&requests[0].body).unwrap(); + assert_eq!(body["id"], endpoint_id); + } +} From 6113dbabf668b8a773791d6f61e910c8efa99447 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 1 Sep 2026 21:08:11 +0100 Subject: [PATCH 2/2] Match the AWS VPC endpoint ID error to the 8-or-17 rule it enforces The validator accepts both the 8- and 17-character hex forms and the README says so, but the rejection message only named 17. Co-Authored-By: Claude Fable 5.1 --- crates/clickhousectl/src/cloud/services.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/clickhousectl/src/cloud/services.rs b/crates/clickhousectl/src/cloud/services.rs index c9435dac..71330957 100644 --- a/crates/clickhousectl/src/cloud/services.rs +++ b/crates/clickhousectl/src/cloud/services.rs @@ -970,7 +970,7 @@ fn validate_private_endpoint_id(value: &str) -> std::result::Result<(), String> { return Err(format!( "invalid AWS VPC endpoint ID '{value}': expected exactly '{AWS_VPC_ENDPOINT_PREFIX}' \ - followed by 17 lowercase hex characters (for example vpce-0123456789abcdef0). GCP \ + followed by 8 or 17 lowercase hex characters (for example vpce-0123456789abcdef0). GCP \ uses the numeric Private Service Connect connection ID and Azure the private \ endpoint Resource ID or resourceGuid" ));