From eafba91efa42d348141881f01c8c072c430b74fd Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 5 Aug 2026 14:13:58 +0100 Subject: [PATCH 1/4] Delete auto-provisioned query keys with services (#332) --- README.md | 4 +- crates/clickhousectl/src/cloud/client.rs | 50 +++- crates/clickhousectl/src/cloud/commands.rs | 53 +++- crates/clickhousectl/src/cloud/credentials.rs | 20 +- .../clickhousectl/src/cloud/service_query.rs | 53 +++- .../tests/cli_request_shape_test.rs | 252 ++++++++++++++++++ 6 files changed, 407 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index c31697ec..f6bdda02 100644 --- a/README.md +++ b/README.md @@ -534,12 +534,12 @@ clickhousectl cloud service delete --force `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: -- **API key auth** (read + write SQL): the first time `cloud service query` runs against a service without a stored key, it provisions a Query API endpoint for that service and creates a dedicated API key bound to it. The key (`keyId`, `keySecret`, and `endpointId`) is stored in `.clickhouse/credentials.json` under `service_query_keys.`, alongside any user-level API key. Subsequent queries use that key. It is scoped to a single service, so it can read and write (SELECT, INSERT, DDL) against that service but cannot reach any other service in the org. Pass `--no-auto-enable` to fail instead of provisioning. +- **API key auth** (read + write SQL): the first time `cloud service query` runs against a service without a stored key, it provisions a Query API endpoint for that service and creates a dedicated API key bound to it. The query credentials, endpoint ID, and exact management API key ID are stored in `.clickhouse/credentials.json` under `service_query_keys.`, alongside any user-level API key. Subsequent queries use that key. It is scoped to a single service, so it can read and write (SELECT, INSERT, DDL) against that service but cannot reach any other service in the org. Pass `--no-auto-enable` to fail instead of provisioning. - **OAuth** (`cloud auth login`): the query runs as your own identity — the CLI sends your bearer token straight to the Query API, which grants **read-only** SQL access (SELECT and other read statements only; no INSERT, DDL, or other writes). No Query API key is provisioned or stored, and no query endpoint needs to be configured on the service. Use API key auth if you need to write. `--no-auto-enable` has no effect in this mode. Provisioning happens lazily (rather than at `service create` time) because the endpoint can only be bound once the service has finished provisioning, which can take several minutes — `service create` returns immediately instead of blocking on it. -Per-service scoping is enforced at the query endpoint binding, which is created with role `sql_console_admin` (read + write inside the bound service only). The API key itself has no org-level roles, so the binding is the only thing that grants it any access. `cloud service delete` removes the stored key from `credentials.json`. +Per-service scoping is enforced at the query endpoint binding, which is created with role `sql_console_admin` (read + write inside the bound service only). The API key itself has no org-level roles, so the binding is the only thing that grants it any access. `cloud service delete` deletes an auto-provisioned key by its stored management ID before deleting the service, then removes the local record. Legacy records without that ID remain readable, but service deletion will not guess at a cloud key by name. Querying an **idled** service wakes it automatically in both auth modes — under OAuth the Query API first asks for a wake confirmation, which the CLI sends after printing a notice to stderr (the first query may take a minute while the service wakes). A **stopped** service is never woken: the query fails with a hint to run `cloud service start`. diff --git a/crates/clickhousectl/src/cloud/client.rs b/crates/clickhousectl/src/cloud/client.rs index 5f549219..2bc98d74 100644 --- a/crates/clickhousectl/src/cloud/client.rs +++ b/crates/clickhousectl/src/cloud/client.rs @@ -500,6 +500,18 @@ impl CloudClient { Self::unwrap_response(response) } + pub async fn get_service_if_exists( + &self, + org_id: &str, + service_id: &str, + ) -> Result> { + match self.api().instance_get(org_id, service_id).await { + Ok(response) => Self::unwrap_response(response).map(Some), + Err(clickhouse_cloud_api::Error::Api { status: 404, .. }) => Ok(None), + Err(error) => Err(self.convert_error_for_organization(error, org_id)), + } + } + pub async fn create_service( &self, org_id: &str, @@ -513,16 +525,19 @@ impl CloudClient { Self::unwrap_response(response) } - pub async fn delete_service(&self, org_id: &str, service_id: &str) -> Result { - let response = self - .api() - .instance_delete(org_id, service_id) - .await - .map_err(|e| self.convert_error_for_organization(e, org_id))?; - Ok(DeleteResponse { - status: response.status, - request_id: response.request_id, - }) + pub async fn delete_service_if_exists( + &self, + org_id: &str, + service_id: &str, + ) -> Result> { + match self.api().instance_delete(org_id, service_id).await { + Ok(response) => Ok(Some(DeleteResponse { + status: response.status, + request_id: response.request_id, + })), + Err(clickhouse_cloud_api::Error::Api { status: 404, .. }) => Ok(None), + Err(error) => Err(self.convert_error_for_organization(error, org_id)), + } } pub async fn change_service_state( @@ -916,6 +931,21 @@ impl CloudClient { }) } + pub async fn delete_api_key_if_exists( + &self, + org_id: &str, + key_id: &str, + ) -> Result> { + match self.api().openapi_key_delete(org_id, key_id).await { + Ok(response) => Ok(Some(DeleteResponse { + status: response.status, + request_id: response.request_id, + })), + Err(clickhouse_cloud_api::Error::Api { status: 404, .. }) => Ok(None), + Err(error) => Err(self.convert_error_for_organization(error, org_id)), + } + } + // Phase 6 - Activity endpoints pub async fn list_activities( &self, diff --git a/crates/clickhousectl/src/cloud/commands.rs b/crates/clickhousectl/src/cloud/commands.rs index 73e3917c..5363b78b 100644 --- a/crates/clickhousectl/src/cloud/commands.rs +++ b/crates/clickhousectl/src/cloud/commands.rs @@ -993,6 +993,39 @@ fn service_delete_error(error: CloudError, force: bool, service_id: &str) -> Clo } } +/// Delete only a query API key whose exact management resource ID was saved +/// when clickhousectl provisioned it. A legacy record is not enough evidence +/// to select a cloud key, so it is deliberately left alone. +async fn cleanup_service_query_key( + client: &CloudClient, + org_id: &str, + service_id: &str, +) -> Result> { + let Some(key) = credentials::get_service_query_key(service_id) else { + return Ok(false); + }; + let Some(api_key_id) = key.api_key_id else { + eprintln!( + "Warning: the stored query key for service {service_id} predates exact API key IDs; \ + service deletion will continue without unsafe name-based key cleanup." + ); + return Ok(false); + }; + + client + .delete_api_key_if_exists(org_id, &api_key_id) + .await + .map_err(|mut error| { + error.message = format!( + "failed to delete the auto-provisioned query API key for service \ + {service_id}: {}", + error.message + ); + error + })?; + Ok(true) +} + pub async fn service_delete( client: &CloudClient, service_id: &str, @@ -1003,10 +1036,13 @@ pub async fn service_delete( let org_id = resolve_org_id(client, org_id).await?; if force { - let svc = client.get_service(&org_id, service_id).await?; + let svc = client.get_service_if_exists(&org_id, service_id).await?; // An absent state matches nothing: skip the stop and let the delete // call decide, rather than guessing the service is running. - let state = or_absent(svc.state.as_ref()); + let state = svc + .as_ref() + .map(|service| or_absent(service.state.as_ref())) + .unwrap_or_default(); if matches!(state.as_str(), "running" | "idle" | "starting") { eprintln!("Stopping service {} before deletion...", service_id); client @@ -1025,13 +1061,22 @@ pub async fn service_delete( } } + // The service must not disappear while its auto-provisioned key remains + // active. A cleanup failure therefore stops before the service DELETE and + // leaves the local record intact for retry. + if cleanup_service_query_key(client, &org_id, service_id).await? { + credentials::remove_service_query_key(service_id)?; + } + let response = client - .delete_service(&org_id, service_id) + .delete_service_if_exists(&org_id, service_id) .await .map_err(|error| service_delete_error(error, force, service_id))?; - let _ = credentials::remove_service_query_key(service_id); + credentials::remove_service_query_key(service_id)?; if json { println!("{}", serde_json::to_string_pretty(&response)?); + } else if response.is_none() { + println!("Service {} is already absent", service_id); } else { println!("Service {} deletion initiated", service_id); } diff --git a/crates/clickhousectl/src/cloud/credentials.rs b/crates/clickhousectl/src/cloud/credentials.rs index 7126b80d..2fbf08ed 100644 --- a/crates/clickhousectl/src/cloud/credentials.rs +++ b/crates/clickhousectl/src/cloud/credentials.rs @@ -16,6 +16,12 @@ pub struct Credentials { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ServiceQueryKey { + /// Management API resource ID used to delete this exact key. + /// + /// Records written before this field was introduced remain usable for + /// queries, but cannot be safely cleaned up by service deletion. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub api_key_id: Option, pub key_id: String, pub key_secret: String, /// The query endpoint the key is bound to, when the upsert echoed it. @@ -110,6 +116,7 @@ mod tests { creds.service_query_keys.insert( "svc-1".into(), ServiceQueryKey { + api_key_id: Some("api-key-uuid".into()), key_id: "kid".into(), key_secret: "sec".into(), endpoint_id: Some("ep".into()), @@ -121,8 +128,10 @@ mod tests { ); let s = serde_json::to_string(&creds).unwrap(); + assert!(s.contains("\"api_key_id\":\"api-key-uuid\"")); let back: Credentials = serde_json::from_str(&s).unwrap(); let key = back.service_query_keys.get("svc-1").unwrap(); + assert_eq!(key.api_key_id.as_deref(), Some("api-key-uuid")); assert_eq!(key.key_id, "kid"); assert_eq!(key.key_secret, "sec"); assert_eq!(key.endpoint_id.as_deref(), Some("ep")); @@ -135,6 +144,7 @@ mod tests { creds.service_query_keys.insert( "svc-1".into(), ServiceQueryKey { + api_key_id: Some("api-key-uuid".into()), key_id: "kid".into(), key_secret: "sec".into(), endpoint_id: None, @@ -155,14 +165,18 @@ mod tests { } #[test] - fn existing_credentials_files_with_an_endpoint_id_still_deserialize() { - // Files written before `endpoint_id` became optional carry it as a - // bare string and must keep loading. + fn existing_query_keys_without_an_api_key_id_still_deserialize() { + // Existing files contain query credentials and an endpoint ID, but + // not the management resource ID added for exact cloud-side cleanup. let raw = r#"{"service_query_keys":{"svc-1":{"key_id":"kid","key_secret":"sec", "endpoint_id":"ep","service_name":"demo","created_at":"2026-05-11T12:00:00Z"}}}"#; let creds: Credentials = serde_json::from_str(raw).unwrap(); let key = creds.service_query_keys.get("svc-1").unwrap(); + assert_eq!(key.api_key_id, None); assert_eq!(key.endpoint_id.as_deref(), Some("ep")); assert_eq!(key.key_id, "kid"); + + let written = serde_json::to_string(&creds).unwrap(); + assert!(!written.contains("api_key_id")); } } diff --git a/crates/clickhousectl/src/cloud/service_query.rs b/crates/clickhousectl/src/cloud/service_query.rs index ee613349..d154f193 100644 --- a/crates/clickhousectl/src/cloud/service_query.rs +++ b/crates/clickhousectl/src/cloud/service_query.rs @@ -8,7 +8,7 @@ use crate::cloud::client::CloudClient; use crate::cloud::credentials::{self, ServiceQueryKey}; -use chrono::Utc; +use chrono::{DateTime, Utc}; use clickhouse_cloud_api::models::{ ApiKeyPostRequest, ApiKeyPostRequestState, ApiKeyPostResponse, InstanceServiceQueryApiEndpointsPostRequest, IpAccessListEntry, @@ -41,6 +41,24 @@ fn require_credential_pair( Ok((key_id, key_secret)) } +fn build_service_query_key( + api_key_id: String, + key_id: String, + key_secret: String, + endpoint_id: Option, + service_name: &str, + created_at: DateTime, +) -> ServiceQueryKey { + ServiceQueryKey { + api_key_id: Some(api_key_id), + key_id, + key_secret, + endpoint_id, + service_name: service_name.to_string(), + created_at, + } +} + /// Discard the API key created for a provisioning attempt that then failed, /// so a later retry doesn't leave an orphaned key behind per attempt. Best /// effort: the caller is already returning an error, and a key we couldn't @@ -118,13 +136,14 @@ pub async fn ensure_service_query_setup( // `id` is diagnostic only, never an auth input: persist the record // without it rather than deleting a working credential and leaving a // dangling UUID in the endpoint's `openApiKeys`. - let stored = ServiceQueryKey { + let stored = build_service_query_key( + api_key_uuid, key_id, key_secret, - endpoint_id: endpoint.id, - service_name: service_name.to_string(), - created_at: Utc::now(), - }; + endpoint.id, + service_name, + Utc::now(), + ); credentials::set_service_query_key(service_id, stored.clone())?; Ok(stored) @@ -225,6 +244,28 @@ mod tests { ); } + #[test] + fn stored_query_key_keeps_the_management_resource_id() { + let created_at = DateTime::parse_from_rfc3339("2026-05-11T12:00:00Z") + .unwrap() + .with_timezone(&Utc); + let key = build_service_query_key( + "api-key-uuid".into(), + "query-key-id".into(), + "query-key-secret".into(), + Some("endpoint-id".into()), + "demo", + created_at, + ); + + assert_eq!(key.api_key_id.as_deref(), Some("api-key-uuid")); + assert_eq!(key.key_id, "query-key-id"); + assert_eq!(key.key_secret, "query-key-secret"); + assert_eq!(key.endpoint_id.as_deref(), Some("endpoint-id")); + assert_eq!(key.service_name, "demo"); + assert_eq!(key.created_at, created_at); + } + fn endpoint( open_api_keys: Option>, ) -> clickhouse_cloud_api::models::ServiceQueryAPIEndpoint { diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 93f7102b..73660bd7 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -352,6 +352,257 @@ async fn service_delete_running_conflict_suggests_force() { ); } +const DELETE_TEST_SERVICE_ID: &str = "11111111-2222-3333-4444-555555555555"; +const DELETE_TEST_API_KEY_ID: &str = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + +fn write_service_query_key(root: &Path, api_key_id: Option<&str>) { + let credentials_dir = root.join(".clickhouse"); + std::fs::create_dir_all(&credentials_dir).unwrap(); + let mut key = serde_json::json!({ + "key_id": "query-key-id", + "key_secret": "query-key-secret", + "endpoint_id": "endpoint-id", + "service_name": "demo", + "created_at": "2026-05-11T12:00:00Z", + }); + if let Some(api_key_id) = api_key_id { + key["api_key_id"] = Value::String(api_key_id.to_string()); + } + std::fs::write( + credentials_dir.join("credentials.json"), + serde_json::to_vec(&serde_json::json!({ + "service_query_keys": { DELETE_TEST_SERVICE_ID: key }, + })) + .unwrap(), + ) + .unwrap(); +} + +fn invoke_service_delete( + mock: &MockServer, + project_dir: &Path, + force: bool, +) -> std::process::Output { + let url = mock.uri(); + let mut args = vec![ + "cloud", + "--url", + &url, + "--json", + "service", + "delete", + DELETE_TEST_SERVICE_ID, + "--org-id", + "org-1", + ]; + if force { + args.push("--force"); + } + Command::new(clickhousectl_binary()) + .env("DO_NOT_TRACK", "1") + .env("HOME", project_dir.join("home")) + .env("CLICKHOUSE_CLOUD_API_KEY", "fake-key-for-tests") + .env("CLICKHOUSE_CLOUD_API_SECRET", "fake-secret-for-tests") + .current_dir(project_dir) + .args(args) + .output() + .expect("failed to spawn clickhousectl") +} + +fn successful_delete_response(request_id: &str) -> ResponseTemplate { + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "status": 200, + "requestId": request_id, + })) +} + +#[tokio::test] +async fn service_delete_removes_the_exact_stored_query_key_before_the_service() { + let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}" + ))) + .respond_with(successful_delete_response("stub-key-delete")) + .expect(1) + .mount(&mock) + .await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(successful_delete_response("stub-service-delete")) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + write_service_query_key(dir.path(), Some(DELETE_TEST_API_KEY_ID)); + let output = invoke_service_delete(&mock, dir.path(), false); + assert_success(&output); + + let requests = mock.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2); + assert_eq!( + requests[0].url.path(), + format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") + ); + assert_eq!( + requests[1].url.path(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ); + assert!(requests.iter().all(|request| { + request.method == wiremock::http::Method::DELETE && request.body.is_empty() + })); + + let stored: Value = serde_json::from_slice( + &std::fs::read(dir.path().join(".clickhouse/credentials.json")).unwrap(), + ) + .unwrap(); + assert!(stored.get("service_query_keys").is_none()); +} + +#[tokio::test] +async fn service_delete_without_a_stored_query_key_only_deletes_the_service() { + let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(successful_delete_response("stub-service-delete")) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + let output = invoke_service_delete(&mock, dir.path(), false); + assert_success(&output); + + let requests = mock.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + assert_eq!(requests[0].method, wiremock::http::Method::DELETE); + assert_eq!( + requests[0].url.path(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ); + assert!(requests[0].body.is_empty()); +} + +#[tokio::test] +async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { + let mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({ + "status": 404, + "error": "NOT_FOUND", + }))) + .expect(1) + .mount(&mock) + .await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}" + ))) + .respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({ + "status": 404, + "error": "NOT_FOUND", + }))) + .expect(1) + .mount(&mock) + .await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(ResponseTemplate::new(404).set_body_json(serde_json::json!({ + "status": 404, + "error": "NOT_FOUND", + }))) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + write_service_query_key(dir.path(), Some(DELETE_TEST_API_KEY_ID)); + let output = invoke_service_delete(&mock, dir.path(), true); + assert_success(&output); + assert_eq!(String::from_utf8_lossy(&output.stdout), "null\n"); + + let requests = mock.received_requests().await.unwrap(); + let request_shape = requests + .iter() + .map(|request| { + ( + request.method.as_str().to_string(), + request.url.path().to_string(), + ) + }) + .collect::>(); + assert_eq!( + request_shape, + vec![ + ( + "GET".to_string(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ), + ( + "DELETE".to_string(), + format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") + ), + ( + "DELETE".to_string(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ), + ] + ); +} + +#[tokio::test] +async fn service_delete_cleanup_failure_preserves_state_and_skips_service_delete() { + let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}" + ))) + .respond_with(ResponseTemplate::new(500).set_body_json(serde_json::json!({ + "status": 500, + "error": "cleanup failed", + }))) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + write_service_query_key(dir.path(), Some(DELETE_TEST_API_KEY_ID)); + let output = invoke_service_delete(&mock, dir.path(), false); + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!( + "Error: failed to delete the auto-provisioned query API key for service \ + {DELETE_TEST_SERVICE_ID}: cleanup failed\n" + ) + ); + + let requests = mock.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].url.path(), + format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") + ); + let stored: Value = serde_json::from_slice( + &std::fs::read(dir.path().join(".clickhouse/credentials.json")).unwrap(), + ) + .unwrap(); + assert_eq!( + stored["service_query_keys"][DELETE_TEST_SERVICE_ID]["api_key_id"], + DELETE_TEST_API_KEY_ID + ); +} + #[tokio::test] async fn org_prometheus_auto_detects_the_only_organization() { let mock = start_mock_org_auto_detection_api().await; @@ -1994,6 +2245,7 @@ async fn service_query_keeps_the_key_when_the_endpoint_response_omits_the_id() { ) .unwrap(); let key = &stored["service_query_keys"][QUERY_TEST_SERVICE_ID]; + assert_eq!(key["api_key_id"], QUERY_TEST_KEY_UUID); assert_eq!(key["key_id"], "provisioned-key-id"); assert_eq!(key["key_secret"], "provisioned-key-secret"); assert!( From 91a8b6d571ed203f15b0fd6df42c7dd1d329abfd Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 5 Aug 2026 18:49:13 +0100 Subject: [PATCH 2/4] Make query key cleanup retry-safe --- README.md | 4 +- crates/clickhousectl/src/cloud/client.rs | 7 +- crates/clickhousectl/src/cloud/commands.rs | 55 ++++--- crates/clickhousectl/src/cloud/credentials.rs | 17 +- .../clickhousectl/src/cloud/service_query.rs | 7 +- .../tests/cli_request_shape_test.rs | 155 ++++++++++++++++-- 6 files changed, 206 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index f6bdda02..b52fdbc3 100644 --- a/README.md +++ b/README.md @@ -534,12 +534,12 @@ clickhousectl cloud service delete --force `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: -- **API key auth** (read + write SQL): the first time `cloud service query` runs against a service without a stored key, it provisions a Query API endpoint for that service and creates a dedicated API key bound to it. The query credentials, endpoint ID, and exact management API key ID are stored in `.clickhouse/credentials.json` under `service_query_keys.`, alongside any user-level API key. Subsequent queries use that key. It is scoped to a single service, so it can read and write (SELECT, INSERT, DDL) against that service but cannot reach any other service in the org. Pass `--no-auto-enable` to fail instead of provisioning. +- **API key auth** (read + write SQL): the first time `cloud service query` runs against a service without a stored key, it provisions a Query API endpoint for that service and creates a dedicated API key bound to it. The query credentials, endpoint ID, exact management API key ID, and provisioning organization ID are stored in `.clickhouse/credentials.json` under `service_query_keys.`, alongside any user-level API key. Subsequent queries use that key. It is scoped to a single service, so it can read and write (SELECT, INSERT, DDL) against that service but cannot reach any other service in the org. Pass `--no-auto-enable` to fail instead of provisioning. - **OAuth** (`cloud auth login`): the query runs as your own identity — the CLI sends your bearer token straight to the Query API, which grants **read-only** SQL access (SELECT and other read statements only; no INSERT, DDL, or other writes). No Query API key is provisioned or stored, and no query endpoint needs to be configured on the service. Use API key auth if you need to write. `--no-auto-enable` has no effect in this mode. Provisioning happens lazily (rather than at `service create` time) because the endpoint can only be bound once the service has finished provisioning, which can take several minutes — `service create` returns immediately instead of blocking on it. -Per-service scoping is enforced at the query endpoint binding, which is created with role `sql_console_admin` (read + write inside the bound service only). The API key itself has no org-level roles, so the binding is the only thing that grants it any access. `cloud service delete` deletes an auto-provisioned key by its stored management ID before deleting the service, then removes the local record. Legacy records without that ID remain readable, but service deletion will not guess at a cloud key by name. +Per-service scoping is enforced at the query endpoint binding, which is created with role `sql_console_admin` (read + write inside the bound service only). The API key itself has no org-level roles, so the binding is the only thing that grants it any access. After deleting a service, `cloud service delete` deletes an auto-provisioned key by its stored management and organization IDs, then removes the local record. Legacy records without that metadata remain readable, but service deletion will not guess at a cloud key by name. Querying an **idled** service wakes it automatically in both auth modes — under OAuth the Query API first asks for a wake confirmation, which the CLI sends after printing a notice to stderr (the first query may take a minute while the service wakes). A **stopped** service is never woken: the query fails with a hint to run `cloud service start`. diff --git a/crates/clickhousectl/src/cloud/client.rs b/crates/clickhousectl/src/cloud/client.rs index 2bc98d74..c2e4163a 100644 --- a/crates/clickhousectl/src/cloud/client.rs +++ b/crates/clickhousectl/src/cloud/client.rs @@ -535,7 +535,12 @@ impl CloudClient { status: response.status, request_id: response.request_id, })), - Err(clickhouse_cloud_api::Error::Api { status: 404, .. }) => Ok(None), + Err(clickhouse_cloud_api::Error::Api { status: 404, .. }) => { + // The service and its organization share the same 404 shape. + // Confirm the request scope before declaring the service absent. + self.get_organization(org_id).await?; + Ok(None) + } Err(error) => Err(self.convert_error_for_organization(error, org_id)), } } diff --git a/crates/clickhousectl/src/cloud/commands.rs b/crates/clickhousectl/src/cloud/commands.rs index 5363b78b..4b431eb1 100644 --- a/crates/clickhousectl/src/cloud/commands.rs +++ b/crates/clickhousectl/src/cloud/commands.rs @@ -993,27 +993,45 @@ fn service_delete_error(error: CloudError, force: bool, service_id: &str) -> Clo } } -/// Delete only a query API key whose exact management resource ID was saved -/// when clickhousectl provisioned it. A legacy record is not enough evidence -/// to select a cloud key, so it is deliberately left alone. -async fn cleanup_service_query_key( - client: &CloudClient, +/// Return a query API key only when its exact resource and organization IDs +/// were saved during provisioning. A legacy record is not enough evidence to +/// select a cloud key, so it is deliberately left alone. +fn service_query_key_id_for_cleanup( org_id: &str, service_id: &str, -) -> Result> { +) -> Result, Box> { let Some(key) = credentials::get_service_query_key(service_id) else { - return Ok(false); + return Ok(None); }; - let Some(api_key_id) = key.api_key_id else { + let (Some(key_org_id), Some(api_key_id)) = (key.organization_id, key.api_key_id) else { eprintln!( - "Warning: the stored query key for service {service_id} predates exact API key IDs; \ - service deletion will continue without unsafe name-based key cleanup." + "Warning: the stored query key for service {service_id} predates exact API key \ + ownership metadata; service deletion will continue without unsafe cloud key cleanup." ); - return Ok(false); + return Ok(None); + }; + if key_org_id != org_id { + return Err(format!( + "the stored query key for service {service_id} belongs to organization {key_org_id}, \ + not {org_id}; refusing to delete either resource" + ) + .into()); + } + Ok(Some(api_key_id)) +} + +async fn cleanup_service_query_key( + client: &CloudClient, + org_id: &str, + service_id: &str, + api_key_id: Option<&str>, +) -> Result<(), Box> { + let Some(api_key_id) = api_key_id else { + return Ok(()); }; client - .delete_api_key_if_exists(org_id, &api_key_id) + .delete_api_key_if_exists(org_id, api_key_id) .await .map_err(|mut error| { error.message = format!( @@ -1023,7 +1041,7 @@ async fn cleanup_service_query_key( ); error })?; - Ok(true) + Ok(()) } pub async fn service_delete( @@ -1034,6 +1052,7 @@ pub async fn service_delete( json: bool, ) -> Result<(), Box> { let org_id = resolve_org_id(client, org_id).await?; + let query_key_id = service_query_key_id_for_cleanup(&org_id, service_id)?; if force { let svc = client.get_service_if_exists(&org_id, service_id).await?; @@ -1061,17 +1080,13 @@ pub async fn service_delete( } } - // The service must not disappear while its auto-provisioned key remains - // active. A cleanup failure therefore stops before the service DELETE and - // leaves the local record intact for retry. - if cleanup_service_query_key(client, &org_id, service_id).await? { - credentials::remove_service_query_key(service_id)?; - } - let response = client .delete_service_if_exists(&org_id, service_id) .await .map_err(|error| service_delete_error(error, force, service_id))?; + // Delete the key only after the service is gone. If cleanup fails, retain + // its exact IDs locally so repeating service delete can retry safely. + cleanup_service_query_key(client, &org_id, service_id, query_key_id.as_deref()).await?; credentials::remove_service_query_key(service_id)?; if json { println!("{}", serde_json::to_string_pretty(&response)?); diff --git a/crates/clickhousectl/src/cloud/credentials.rs b/crates/clickhousectl/src/cloud/credentials.rs index 2fbf08ed..debe90ff 100644 --- a/crates/clickhousectl/src/cloud/credentials.rs +++ b/crates/clickhousectl/src/cloud/credentials.rs @@ -16,10 +16,13 @@ pub struct Credentials { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ServiceQueryKey { + /// Organization in which the management API key was provisioned. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub organization_id: Option, /// Management API resource ID used to delete this exact key. /// - /// Records written before this field was introduced remain usable for - /// queries, but cannot be safely cleaned up by service deletion. + /// Records written before the cleanup metadata was introduced remain + /// usable for queries, but cannot be safely cleaned up by service deletion. #[serde(default, skip_serializing_if = "Option::is_none")] pub api_key_id: Option, pub key_id: String, @@ -116,6 +119,7 @@ mod tests { creds.service_query_keys.insert( "svc-1".into(), ServiceQueryKey { + organization_id: Some("org-1".into()), api_key_id: Some("api-key-uuid".into()), key_id: "kid".into(), key_secret: "sec".into(), @@ -128,9 +132,11 @@ mod tests { ); let s = serde_json::to_string(&creds).unwrap(); + assert!(s.contains("\"organization_id\":\"org-1\"")); assert!(s.contains("\"api_key_id\":\"api-key-uuid\"")); let back: Credentials = serde_json::from_str(&s).unwrap(); let key = back.service_query_keys.get("svc-1").unwrap(); + assert_eq!(key.organization_id.as_deref(), Some("org-1")); assert_eq!(key.api_key_id.as_deref(), Some("api-key-uuid")); assert_eq!(key.key_id, "kid"); assert_eq!(key.key_secret, "sec"); @@ -144,6 +150,7 @@ mod tests { creds.service_query_keys.insert( "svc-1".into(), ServiceQueryKey { + organization_id: Some("org-1".into()), api_key_id: Some("api-key-uuid".into()), key_id: "kid".into(), key_secret: "sec".into(), @@ -165,18 +172,20 @@ mod tests { } #[test] - fn existing_query_keys_without_an_api_key_id_still_deserialize() { + fn existing_query_keys_without_cleanup_metadata_still_deserialize() { // Existing files contain query credentials and an endpoint ID, but - // not the management resource ID added for exact cloud-side cleanup. + // not the ownership metadata added for exact cloud-side cleanup. let raw = r#"{"service_query_keys":{"svc-1":{"key_id":"kid","key_secret":"sec", "endpoint_id":"ep","service_name":"demo","created_at":"2026-05-11T12:00:00Z"}}}"#; let creds: Credentials = serde_json::from_str(raw).unwrap(); let key = creds.service_query_keys.get("svc-1").unwrap(); + assert_eq!(key.organization_id, None); assert_eq!(key.api_key_id, None); assert_eq!(key.endpoint_id.as_deref(), Some("ep")); assert_eq!(key.key_id, "kid"); let written = serde_json::to_string(&creds).unwrap(); + assert!(!written.contains("organization_id")); assert!(!written.contains("api_key_id")); } } diff --git a/crates/clickhousectl/src/cloud/service_query.rs b/crates/clickhousectl/src/cloud/service_query.rs index d154f193..a9e49173 100644 --- a/crates/clickhousectl/src/cloud/service_query.rs +++ b/crates/clickhousectl/src/cloud/service_query.rs @@ -42,6 +42,7 @@ fn require_credential_pair( } fn build_service_query_key( + organization_id: &str, api_key_id: String, key_id: String, key_secret: String, @@ -50,6 +51,7 @@ fn build_service_query_key( created_at: DateTime, ) -> ServiceQueryKey { ServiceQueryKey { + organization_id: Some(organization_id.to_string()), api_key_id: Some(api_key_id), key_id, key_secret, @@ -137,6 +139,7 @@ pub async fn ensure_service_query_setup( // without it rather than deleting a working credential and leaving a // dangling UUID in the endpoint's `openApiKeys`. let stored = build_service_query_key( + org_id, api_key_uuid, key_id, key_secret, @@ -245,11 +248,12 @@ mod tests { } #[test] - fn stored_query_key_keeps_the_management_resource_id() { + fn stored_query_key_keeps_the_management_resource_ownership() { let created_at = DateTime::parse_from_rfc3339("2026-05-11T12:00:00Z") .unwrap() .with_timezone(&Utc); let key = build_service_query_key( + "org-1", "api-key-uuid".into(), "query-key-id".into(), "query-key-secret".into(), @@ -258,6 +262,7 @@ mod tests { created_at, ); + assert_eq!(key.organization_id.as_deref(), Some("org-1")); assert_eq!(key.api_key_id.as_deref(), Some("api-key-uuid")); assert_eq!(key.key_id, "query-key-id"); assert_eq!(key.key_secret, "query-key-secret"); diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 73660bd7..809c7827 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -355,7 +355,7 @@ async fn service_delete_running_conflict_suggests_force() { const DELETE_TEST_SERVICE_ID: &str = "11111111-2222-3333-4444-555555555555"; const DELETE_TEST_API_KEY_ID: &str = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; -fn write_service_query_key(root: &Path, api_key_id: Option<&str>) { +fn write_service_query_key(root: &Path, organization_id: Option<&str>, api_key_id: Option<&str>) { let credentials_dir = root.join(".clickhouse"); std::fs::create_dir_all(&credentials_dir).unwrap(); let mut key = serde_json::json!({ @@ -368,6 +368,9 @@ fn write_service_query_key(root: &Path, api_key_id: Option<&str>) { if let Some(api_key_id) = api_key_id { key["api_key_id"] = Value::String(api_key_id.to_string()); } + if let Some(organization_id) = organization_id { + key["organization_id"] = Value::String(organization_id.to_string()); + } std::fs::write( credentials_dir.join("credentials.json"), serde_json::to_vec(&serde_json::json!({ @@ -417,7 +420,7 @@ fn successful_delete_response(request_id: &str) -> ResponseTemplate { } #[tokio::test] -async fn service_delete_removes_the_exact_stored_query_key_before_the_service() { +async fn service_delete_removes_the_exact_stored_query_key_after_the_service() { let mock = MockServer::start().await; Mock::given(method("DELETE")) .and(path(format!( @@ -437,7 +440,7 @@ async fn service_delete_removes_the_exact_stored_query_key_before_the_service() .await; let dir = tempfile::tempdir().unwrap(); - write_service_query_key(dir.path(), Some(DELETE_TEST_API_KEY_ID)); + write_service_query_key(dir.path(), Some("org-1"), Some(DELETE_TEST_API_KEY_ID)); let output = invoke_service_delete(&mock, dir.path(), false); assert_success(&output); @@ -445,11 +448,11 @@ async fn service_delete_removes_the_exact_stored_query_key_before_the_service() assert_eq!(requests.len(), 2); assert_eq!( requests[0].url.path(), - format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") ); assert_eq!( requests[1].url.path(), - format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") ); assert!(requests.iter().all(|request| { request.method == wiremock::http::Method::DELETE && request.body.is_empty() @@ -513,6 +516,16 @@ async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { .expect(1) .mount(&mock) .await; + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "result": {}, + "status": 200, + "requestId": "stub-org-get", + }))) + .expect(1) + .mount(&mock) + .await; Mock::given(method("DELETE")) .and(path(format!( "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" @@ -526,7 +539,7 @@ async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { .await; let dir = tempfile::tempdir().unwrap(); - write_service_query_key(dir.path(), Some(DELETE_TEST_API_KEY_ID)); + write_service_query_key(dir.path(), Some("org-1"), Some(DELETE_TEST_API_KEY_ID)); let output = invoke_service_delete(&mock, dir.path(), true); assert_success(&output); assert_eq!(String::from_utf8_lossy(&output.stdout), "null\n"); @@ -550,19 +563,28 @@ async fn forced_service_delete_treats_an_absent_key_and_service_as_success() { ), ( "DELETE".to_string(), - format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") ), + ("GET".to_string(), "/v1/organizations/org-1".to_string()), ( "DELETE".to_string(), - format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") ), ] ); } #[tokio::test] -async fn service_delete_cleanup_failure_preserves_state_and_skips_service_delete() { +async fn service_delete_cleanup_failure_preserves_credentials_for_retry() { let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(successful_delete_response("stub-service-delete")) + .expect(1) + .mount(&mock) + .await; Mock::given(method("DELETE")) .and(path(format!( "/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}" @@ -576,7 +598,7 @@ async fn service_delete_cleanup_failure_preserves_state_and_skips_service_delete .await; let dir = tempfile::tempdir().unwrap(); - write_service_query_key(dir.path(), Some(DELETE_TEST_API_KEY_ID)); + write_service_query_key(dir.path(), Some("org-1"), Some(DELETE_TEST_API_KEY_ID)); let output = invoke_service_delete(&mock, dir.path(), false); assert_eq!(output.status.code(), Some(1)); assert_eq!( @@ -588,9 +610,13 @@ async fn service_delete_cleanup_failure_preserves_state_and_skips_service_delete ); let requests = mock.received_requests().await.unwrap(); - assert_eq!(requests.len(), 1); + assert_eq!(requests.len(), 2); assert_eq!( requests[0].url.path(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ); + assert_eq!( + requests[1].url.path(), format!("/v1/organizations/org-1/keys/{DELETE_TEST_API_KEY_ID}") ); let stored: Value = serde_json::from_slice( @@ -603,6 +629,112 @@ async fn service_delete_cleanup_failure_preserves_state_and_skips_service_delete ); } +#[tokio::test] +async fn service_delete_failure_preserves_the_query_key_without_cleanup() { + let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(ResponseTemplate::new(500).set_body_json(serde_json::json!({ + "status": 500, + "error": "service delete failed", + }))) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + write_service_query_key(dir.path(), Some("org-1"), Some(DELETE_TEST_API_KEY_ID)); + let output = invoke_service_delete(&mock, dir.path(), false); + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "Error: service delete failed\n" + ); + + let requests = mock.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].url.path(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ); + let stored: Value = serde_json::from_slice( + &std::fs::read(dir.path().join(".clickhouse/credentials.json")).unwrap(), + ) + .unwrap(); + assert_eq!( + stored["service_query_keys"][DELETE_TEST_SERVICE_ID]["api_key_id"], + DELETE_TEST_API_KEY_ID + ); +} + +#[tokio::test] +async fn service_delete_rejects_query_key_from_another_organization() { + let mock = MockServer::start().await; + let dir = tempfile::tempdir().unwrap(); + write_service_query_key(dir.path(), Some("org-2"), Some(DELETE_TEST_API_KEY_ID)); + + let output = invoke_service_delete(&mock, dir.path(), false); + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!( + "Error: the stored query key for service {DELETE_TEST_SERVICE_ID} belongs to \ + organization org-2, not org-1; refusing to delete either resource\n" + ) + ); + assert!(mock.received_requests().await.unwrap().is_empty()); + + let stored: Value = serde_json::from_slice( + &std::fs::read(dir.path().join(".clickhouse/credentials.json")).unwrap(), + ) + .unwrap(); + assert_eq!( + stored["service_query_keys"][DELETE_TEST_SERVICE_ID]["organization_id"], + "org-2" + ); +} + +#[tokio::test] +async fn service_delete_does_not_treat_a_missing_organization_as_an_absent_service() { + let mock = MockServer::start().await; + let not_found = ResponseTemplate::new(404).set_body_json(serde_json::json!({ + "status": 404, + "error": "NOT_FOUND", + })); + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(not_found.clone()) + .expect(1) + .mount(&mock) + .await; + Mock::given(method("GET")) + .and(path("/v1/organizations/org-1")) + .respond_with(not_found) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + let output = invoke_service_delete(&mock, dir.path(), false); + assert_eq!(output.status.code(), Some(1)); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + "Error: NOT_FOUND: request scoped to organization org-1\n" + ); + + let requests = mock.received_requests().await.unwrap(); + assert_eq!(requests.len(), 2); + assert_eq!( + requests[0].url.path(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ); + assert_eq!(requests[1].url.path(), "/v1/organizations/org-1"); +} + #[tokio::test] async fn org_prometheus_auto_detects_the_only_organization() { let mock = start_mock_org_auto_detection_api().await; @@ -2245,6 +2377,7 @@ async fn service_query_keeps_the_key_when_the_endpoint_response_omits_the_id() { ) .unwrap(); let key = &stored["service_query_keys"][QUERY_TEST_SERVICE_ID]; + assert_eq!(key["organization_id"], "org-1"); assert_eq!(key["api_key_id"], QUERY_TEST_KEY_UUID); assert_eq!(key["key_id"], "provisioned-key-id"); assert_eq!(key["key_secret"], "provisioned-key-secret"); From c2425149b919e6ec550d35c71909431de46fc30a Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 5 Aug 2026 18:59:23 +0100 Subject: [PATCH 3/4] Preserve partial query key metadata --- README.md | 2 +- crates/clickhousectl/src/cloud/commands.rs | 34 +++++++++------ .../tests/cli_request_shape_test.rs | 41 +++++++++++++++++++ 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index b52fdbc3..cdeaf583 100644 --- a/README.md +++ b/README.md @@ -539,7 +539,7 @@ clickhousectl cloud service delete --force Provisioning happens lazily (rather than at `service create` time) because the endpoint can only be bound once the service has finished provisioning, which can take several minutes — `service create` returns immediately instead of blocking on it. -Per-service scoping is enforced at the query endpoint binding, which is created with role `sql_console_admin` (read + write inside the bound service only). The API key itself has no org-level roles, so the binding is the only thing that grants it any access. After deleting a service, `cloud service delete` deletes an auto-provisioned key by its stored management and organization IDs, then removes the local record. Legacy records without that metadata remain readable, but service deletion will not guess at a cloud key by name. +Per-service scoping is enforced at the query endpoint binding, which is created with role `sql_console_admin` (read + write inside the bound service only). The API key itself has no org-level roles, so the binding is the only thing that grants it any access. After deleting a service, `cloud service delete` deletes an auto-provisioned key by its stored management and organization IDs, then removes the local record. Legacy records without that metadata remain readable, but service deletion will not guess at a cloud key by name; a partial record with a management ID is retained for manual recovery. Querying an **idled** service wakes it automatically in both auth modes — under OAuth the Query API first asks for a wake confirmation, which the CLI sends after printing a notice to stderr (the first query may take a minute while the service wakes). A **stopped** service is never woken: the query fails with a hint to run `cloud service start`. diff --git a/crates/clickhousectl/src/cloud/commands.rs b/crates/clickhousectl/src/cloud/commands.rs index 4b431eb1..7628336d 100644 --- a/crates/clickhousectl/src/cloud/commands.rs +++ b/crates/clickhousectl/src/cloud/commands.rs @@ -994,21 +994,29 @@ fn service_delete_error(error: CloudError, force: bool, service_id: &str) -> Clo } /// Return a query API key only when its exact resource and organization IDs -/// were saved during provisioning. A legacy record is not enough evidence to -/// select a cloud key, so it is deliberately left alone. -fn service_query_key_id_for_cleanup( +/// were saved during provisioning. The boolean indicates that partial cleanup +/// metadata must remain on disk because discarding it would lose the key ID. +fn service_query_key_cleanup( org_id: &str, service_id: &str, -) -> Result, Box> { +) -> Result<(Option, bool), Box> { let Some(key) = credentials::get_service_query_key(service_id) else { - return Ok(None); + return Ok((None, false)); }; - let (Some(key_org_id), Some(api_key_id)) = (key.organization_id, key.api_key_id) else { + let Some(api_key_id) = key.api_key_id else { eprintln!( - "Warning: the stored query key for service {service_id} predates exact API key \ - ownership metadata; service deletion will continue without unsafe cloud key cleanup." + "Warning: the stored query key for service {service_id} predates exact management \ + API key IDs; service deletion will continue without unsafe cloud key cleanup." ); - return Ok(None); + return Ok((None, false)); + }; + let Some(key_org_id) = key.organization_id else { + eprintln!( + "Warning: the stored query key for service {service_id} has a management API key ID \ + but no provisioning organization; cloud key cleanup was skipped and the local \ + record was retained." + ); + return Ok((None, true)); }; if key_org_id != org_id { return Err(format!( @@ -1017,7 +1025,7 @@ fn service_query_key_id_for_cleanup( ) .into()); } - Ok(Some(api_key_id)) + Ok((Some(api_key_id), false)) } async fn cleanup_service_query_key( @@ -1052,7 +1060,7 @@ pub async fn service_delete( json: bool, ) -> Result<(), Box> { let org_id = resolve_org_id(client, org_id).await?; - let query_key_id = service_query_key_id_for_cleanup(&org_id, service_id)?; + let (query_key_id, retain_query_key) = service_query_key_cleanup(&org_id, service_id)?; if force { let svc = client.get_service_if_exists(&org_id, service_id).await?; @@ -1087,7 +1095,9 @@ pub async fn service_delete( // Delete the key only after the service is gone. If cleanup fails, retain // its exact IDs locally so repeating service delete can retry safely. cleanup_service_query_key(client, &org_id, service_id, query_key_id.as_deref()).await?; - credentials::remove_service_query_key(service_id)?; + if !retain_query_key { + credentials::remove_service_query_key(service_id)?; + } if json { println!("{}", serde_json::to_string_pretty(&response)?); } else if response.is_none() { diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index 809c7827..c619b9ca 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -696,6 +696,47 @@ async fn service_delete_rejects_query_key_from_another_organization() { ); } +#[tokio::test] +async fn service_delete_retains_a_key_id_without_organization_metadata() { + let mock = MockServer::start().await; + Mock::given(method("DELETE")) + .and(path(format!( + "/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}" + ))) + .respond_with(successful_delete_response("stub-service-delete")) + .expect(1) + .mount(&mock) + .await; + + let dir = tempfile::tempdir().unwrap(); + write_service_query_key(dir.path(), None, Some(DELETE_TEST_API_KEY_ID)); + let output = invoke_service_delete(&mock, dir.path(), false); + assert_success(&output); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!( + "Warning: the stored query key for service {DELETE_TEST_SERVICE_ID} has a management \ + API key ID but no provisioning organization; cloud key cleanup was skipped and the \ + local record was retained.\n" + ) + ); + + let requests = mock.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].url.path(), + format!("/v1/organizations/org-1/services/{DELETE_TEST_SERVICE_ID}") + ); + let stored: Value = serde_json::from_slice( + &std::fs::read(dir.path().join(".clickhouse/credentials.json")).unwrap(), + ) + .unwrap(); + assert_eq!( + stored["service_query_keys"][DELETE_TEST_SERVICE_ID]["api_key_id"], + DELETE_TEST_API_KEY_ID + ); +} + #[tokio::test] async fn service_delete_does_not_treat_a_missing_organization_as_an_absent_service() { let mock = MockServer::start().await; From 2618e3ff142de0a6f251991a37847bec65713794 Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 5 Aug 2026 19:08:25 +0100 Subject: [PATCH 4/4] Fail closed on unreadable query credentials --- crates/clickhousectl/src/cloud/commands.rs | 2 +- crates/clickhousectl/src/cloud/credentials.rs | 16 ++++++++++++++++ .../tests/cli_request_shape_test.rs | 18 ++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/crates/clickhousectl/src/cloud/commands.rs b/crates/clickhousectl/src/cloud/commands.rs index 7628336d..7dc24e4e 100644 --- a/crates/clickhousectl/src/cloud/commands.rs +++ b/crates/clickhousectl/src/cloud/commands.rs @@ -1000,7 +1000,7 @@ fn service_query_key_cleanup( org_id: &str, service_id: &str, ) -> Result<(Option, bool), Box> { - let Some(key) = credentials::get_service_query_key(service_id) else { + let Some(key) = credentials::try_get_service_query_key(service_id)? else { return Ok((None, false)); }; let Some(api_key_id) = key.api_key_id else { diff --git a/crates/clickhousectl/src/cloud/credentials.rs b/crates/clickhousectl/src/cloud/credentials.rs index debe90ff..a93fc16d 100644 --- a/crates/clickhousectl/src/cloud/credentials.rs +++ b/crates/clickhousectl/src/cloud/credentials.rs @@ -76,6 +76,22 @@ pub fn get_service_query_key(service_id: &str) -> Option { creds.service_query_keys.get(service_id).cloned() } +pub fn try_get_service_query_key( + service_id: &str, +) -> Result, Box> { + let path = credentials_path(); + let data = match std::fs::read_to_string(&path) { + Ok(data) => data, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(error) => { + return Err(format!("failed to read {}: {error}", path.display()).into()); + } + }; + let creds: Credentials = serde_json::from_str(&data) + .map_err(|error| format!("failed to parse {}: {error}", path.display()))?; + Ok(creds.service_query_keys.get(service_id).cloned()) +} + pub fn set_service_query_key( service_id: &str, key: ServiceQueryKey, diff --git a/crates/clickhousectl/tests/cli_request_shape_test.rs b/crates/clickhousectl/tests/cli_request_shape_test.rs index c619b9ca..c65c18b5 100644 --- a/crates/clickhousectl/tests/cli_request_shape_test.rs +++ b/crates/clickhousectl/tests/cli_request_shape_test.rs @@ -776,6 +776,24 @@ async fn service_delete_does_not_treat_a_missing_organization_as_an_absent_servi assert_eq!(requests[1].url.path(), "/v1/organizations/org-1"); } +#[tokio::test] +async fn service_delete_aborts_when_query_key_credentials_are_malformed() { + let mock = MockServer::start().await; + let dir = tempfile::tempdir().unwrap(); + let credentials_dir = dir.path().join(".clickhouse"); + std::fs::create_dir_all(&credentials_dir).unwrap(); + std::fs::write(credentials_dir.join("credentials.json"), "{").unwrap(); + + let output = invoke_service_delete(&mock, dir.path(), false); + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.starts_with("Error: failed to parse ") + && stderr.contains(".clickhouse/credentials.json") + ); + assert!(mock.received_requests().await.unwrap().is_empty()); +} + #[tokio::test] async fn org_prometheus_auto_detects_the_only_organization() { let mock = start_mock_org_auto_detection_api().await;