Repair stale stored query keys - #508
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let replacement_request = endpoint.request(replacement_keys); | ||
| let rollback_request = endpoint.request(endpoint.open_api_keys.clone()); | ||
| let replacement_endpoint = match client | ||
| .create_query_endpoint(org_id, service_id, &replacement_request) |
There was a problem hiding this comment.
🟠 High cloud/service_query.rs:297
A concurrent change to the service query endpoint is overwritten by the repair, so an update made by the Cloud console or another machine to openApiKeys, roles, or allowedOrigins can be lost. The local credentials lock does not cover those clients, and this full configuration upsert has no version/ETag precondition; use a conditional update (or detect a conflict and re-read/retry) before replacing the stale configuration.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/cloud/service_query.rs around line 297:
A concurrent change to the service query endpoint is overwritten by the repair, so an update made by the Cloud console or another machine to `openApiKeys`, `roles`, or `allowedOrigins` can be lost. The local credentials lock does not cover those clients, and this full configuration upsert has no version/ETag precondition; use a conditional update (or detect a conflict and re-read/retry) before replacing the stale configuration.
d1598a0 to
1ccc501
Compare
| if let Some(stored_endpoint_id) = stored_endpoint_id { | ||
| let current_endpoint_id = endpoint.id.as_deref().ok_or_else(|| { | ||
| CloudError::new( | ||
| "the query endpoint response omitted its id; refusing to repair a stored endpoint binding without confirming ownership", | ||
| ) | ||
| })?; | ||
| if current_endpoint_id != stored_endpoint_id { | ||
| return Err(CloudError::new(format!( | ||
| "the stored query key belongs to endpoint {stored_endpoint_id}, but the service now reports endpoint {current_endpoint_id}; refusing to modify the replacement endpoint" | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium cloud/service_query.rs:218
When stored_endpoint_id is None, repair proceeds against whichever endpoint currently contains the saved API-key UUID, so a replaced endpoint can be modified without proving it owns the stored binding. Require a stored endpoint ID and a returned matching ID before continuing.
| if let Some(stored_endpoint_id) = stored_endpoint_id { | |
| let current_endpoint_id = endpoint.id.as_deref().ok_or_else(|| { | |
| CloudError::new( | |
| "the query endpoint response omitted its id; refusing to repair a stored endpoint binding without confirming ownership", | |
| ) | |
| })?; | |
| if current_endpoint_id != stored_endpoint_id { | |
| return Err(CloudError::new(format!( | |
| "the stored query key belongs to endpoint {stored_endpoint_id}, but the service now reports endpoint {current_endpoint_id}; refusing to modify the replacement endpoint" | |
| ))); | |
| } | |
| } | |
| let stored_endpoint_id = stored_endpoint_id.ok_or_else(|| { | |
| CloudError::new( | |
| "the stored query key has no endpoint id; refusing to repair without confirming endpoint ownership", | |
| ) | |
| })?; | |
| let current_endpoint_id = endpoint.id.as_deref().ok_or_else(|| { | |
| CloudError::new( | |
| "the query endpoint response omitted its id; refusing to repair a stored endpoint binding without confirming ownership", | |
| ) | |
| })?; | |
| if current_endpoint_id != stored_endpoint_id { | |
| return Err(CloudError::new(format!( | |
| "the stored query key belongs to endpoint {stored_endpoint_id}, but the service now reports endpoint {current_endpoint_id}; refusing to modify the replacement endpoint" | |
| ))); | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/cloud/service_query.rs around lines 218-229:
When `stored_endpoint_id` is `None`, repair proceeds against whichever endpoint currently contains the saved API-key UUID, so a replaced endpoint can be modified without proving it owns the stored binding. Require a stored endpoint ID and a returned matching ID before continuing.
| .collect(); | ||
| let replacement_request = endpoint.request(replacement_keys); | ||
| let rollback_request = endpoint.request(endpoint.open_api_keys.clone()); | ||
| let replacement_endpoint = match client |
There was a problem hiding this comment.
🟠 High cloud/service_query.rs:323
A response/read error from create_query_endpoint can occur after the server has applied the replacement, but this branch immediately deletes key.api_key_id without restoring rollback_request. The endpoint can therefore retain a binding to a deleted key, causing subsequent queries with the stored credentials to fail. Roll back the endpoint first and delete the new key only after rollback succeeds; if rollback also fails, retain the key and report both errors.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/cloud/service_query.rs around line 323:
A response/read error from `create_query_endpoint` can occur after the server has applied the replacement, but this branch immediately deletes `key.api_key_id` without restoring `rollback_request`. The endpoint can therefore retain a binding to a deleted key, causing subsequent queries with the stored credentials to fail. Roll back the endpoint first and delete the new key only after rollback succeeds; if rollback also fails, retain the key and report both errors.
|
This is over-engineered. The ability to create a key and associated it to a QE is already possible. This should simply be described in the output when it occurs, baking the functionality into another flag is uncessary. |
|
Addressed in Verified with:
|
| status: 401 | 403, | ||
| message, | ||
| } if !message.starts_with("SQL error ") => Some(CloudError::new(format!( | ||
| "the stored Query API key for service {service_id} was rejected and may be stale: {message}\n\nNo credentials were changed. Create a replacement key, then associate its resource ID (`key.id` in the JSON response) with this service's Query API endpoint:\n clickhousectl cloud api-key create --name clickhousectl-query-{service_id} --org-id {org_id} --json\n clickhousectl cloud service query-endpoint get {service_id} --org-id {org_id}\n clickhousectl cloud service query-endpoint create {service_id} --org-id {org_id} --role sql_console_admin --open-api-key <new-key.id>\n\n`query-endpoint create` replaces the complete endpoint configuration. Repeat every existing role and API key from `query-endpoint get`, and preserve its allowed origin with `--allowed-origins`, if set." |
There was a problem hiding this comment.
🟠 High cloud/services.rs:1790
Following the recovery commands in this message does not replace service_query_keys.<service-id>, so the next cloud service query still loads the revoked key_id/key_secret via get_service_query_key and fails with 401/403 again. Update the guidance to use cloud service query --repair-query-key (or otherwise explicitly replace the locally stored credential) after creating the replacement key.
Also found in 1 other location(s)
README.md:597
The documented recovery workflow does not replace the credential in
service_query_keys.<service-id>. Afterapi-key createandquery-endpoint create,cloud service querystill preferentially loads the old stored key and retries its revoked secret, so users following this guidance continue receiving 401/403. The documentation should direct users to the actual stored-key repair mechanism (or include the required local credential replacement step).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/cloud/services.rs around line 1790:
Following the recovery commands in this message does not replace `service_query_keys.<service-id>`, so the next `cloud service query` still loads the revoked `key_id`/`key_secret` via `get_service_query_key` and fails with 401/403 again. Update the guidance to use `cloud service query --repair-query-key` (or otherwise explicitly replace the locally stored credential) after creating the replacement key.
Also found in 1 other location(s):
- README.md:597 -- The documented recovery workflow does not replace the credential in `service_query_keys.<service-id>`. After `api-key create` and `query-endpoint create`, `cloud service query` still preferentially loads the old stored key and retries its revoked secret, so users following this guidance continue receiving 401/403. The documentation should direct users to the actual stored-key repair mechanism (or include the required local credential replacement step).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6cc7dc3. Configure here.
| status: 401 | 403, | ||
| message, | ||
| } if !message.starts_with("SQL error ") => Some(CloudError::new(format!( | ||
| "the stored Query API key for service {service_id} was rejected and may be stale: {message}\n\nNo credentials were changed. Create a replacement key, then associate its resource ID (`key.id` in the JSON response) with this service's Query API endpoint:\n clickhousectl cloud api-key create --name clickhousectl-query-{service_id} --org-id {org_id} --json\n clickhousectl cloud service query-endpoint get {service_id} --org-id {org_id}\n clickhousectl cloud service query-endpoint create {service_id} --org-id {org_id} --role sql_console_admin --open-api-key <new-key.id>\n\n`query-endpoint create` replaces the complete endpoint configuration. Repeat every existing role and API key from `query-endpoint get`, and preserve its allowed origin with `--allowed-origins`, if set." |
There was a problem hiding this comment.
Stale-key recovery leaves queries broken
High Severity
The new stale-key guidance only covers creating a cloud key and binding it on the query endpoint. cloud service query keeps authenticating with the revoked key_id/key_secret from credentials.json, and nothing updates that local record. Following the documented recovery still fails with the same 401/403 loop.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 6cc7dc3. Configure here.
1 participant
Summary
cloud service query --repair-query-keyreplacement for one stored service credentialTests
cargo test -p clickhousectlcargo test -p clickhousectl --test cli_request_shape_test service_query_repair_replaces_only_the_exact_owned_key_and_binding -- --exactcargo test -p clickhousectl cloud::services::tests::parses_service_querycargo fmt --all --checkcargo clippy -p clickhousectl --all-targets -- -D warningsSubprocess coverage pins stored-key 401 and 403 guidance with no writes, exact binding replacement when the old management key is already absent, unrelated credential preservation, and safe legacy-record refusal.
Stack
This is the child of
issue-453-query-endpoint-readiness(PR #504) in gh-stack #505. It targets the parent branch; PR #504 should merge first.Closes #454
Stack created with GitHub Stacks CLI • Give Feedback