From 7895f99d3afc6e97f62f52abe06a6ee8d0f0617f Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Wed, 14 Jun 2023 16:56:15 +0200 Subject: [PATCH] admin and cli: hide secret keys unless asked --- doc/drafts/admin-api.md | 3 +++ src/api/admin/api_server.rs | 9 +++++++-- src/api/admin/key.rs | 34 ++++++++++++++++++++++++---------- src/api/admin/router.rs | 14 ++++++++------ src/api/helpers.rs | 4 ++++ src/garage/admin/key.rs | 9 +++++++-- src/garage/cli/structs.rs | 7 +++++-- 7 files changed, 58 insertions(+), 22 deletions(-) diff --git a/doc/drafts/admin-api.md b/doc/drafts/admin-api.md index 79ea7e8c..340f4583 100644 --- a/doc/drafts/admin-api.md +++ b/doc/drafts/admin-api.md @@ -411,6 +411,9 @@ If `id` is set, the key is looked up using its exact identifier (faster). If `search` is set, the key is looked up using its name or prefix of identifier (slower, all keys are enumerated to do this). +Optionnally, the query parameter `showSecretKey=true` can be set to reveal the +associated secret access key. + Example response: ```json diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs index b0dfdfb7..6819e28e 100644 --- a/src/api/admin/api_server.rs +++ b/src/api/admin/api_server.rs @@ -242,8 +242,13 @@ impl ApiHandler for AdminApiServer { Endpoint::RevertClusterLayout => handle_revert_cluster_layout(&self.garage, req).await, // Keys Endpoint::ListKeys => handle_list_keys(&self.garage).await, - Endpoint::GetKeyInfo { id, search } => { - handle_get_key_info(&self.garage, id, search).await + Endpoint::GetKeyInfo { + id, + search, + show_secret_key, + } => { + let show_secret_key = show_secret_key.map(|x| x == "true").unwrap_or(false); + handle_get_key_info(&self.garage, id, search, show_secret_key).await } Endpoint::CreateKey => handle_create_key(&self.garage, req).await, Endpoint::ImportKey => handle_import_key(&self.garage, req).await, diff --git a/src/api/admin/key.rs b/src/api/admin/key.rs index 664fde4c..0d1f799b 100644 --- a/src/api/admin/key.rs +++ b/src/api/admin/key.rs @@ -10,7 +10,7 @@ use garage_model::garage::Garage; use garage_model::key_table::*; use crate::admin::error::*; -use crate::helpers::{json_ok_response, parse_json_body}; +use crate::helpers::{is_default, json_ok_response, parse_json_body}; pub async fn handle_list_keys(garage: &Arc) -> Result, Error> { let res = garage @@ -44,6 +44,7 @@ pub async fn handle_get_key_info( garage: &Arc, id: Option, search: Option, + show_secret_key: bool, ) -> Result, Error> { let key = if let Some(id) = id { garage.key_helper().get_existing_key(&id).await? @@ -56,7 +57,7 @@ pub async fn handle_get_key_info( unreachable!(); }; - key_info_results(garage, key).await + key_info_results(garage, key, show_secret_key).await } pub async fn handle_create_key( @@ -68,7 +69,7 @@ pub async fn handle_create_key( let key = Key::new(req.name.as_deref().unwrap_or("Unnamed key")); garage.key_table.insert(&key).await?; - key_info_results(garage, key).await + key_info_results(garage, key, true).await } #[derive(Deserialize)] @@ -88,10 +89,14 @@ pub async fn handle_import_key( return Err(Error::KeyAlreadyExists(req.access_key_id.to_string())); } - let imported_key = Key::import(&req.access_key_id, &req.secret_access_key, &req.name); + let imported_key = Key::import( + &req.access_key_id, + &req.secret_access_key, + req.name.as_deref().unwrap_or("Imported key"), + ); garage.key_table.insert(&imported_key).await?; - key_info_results(garage, imported_key).await + key_info_results(garage, imported_key, false).await } #[derive(Deserialize)] @@ -99,7 +104,7 @@ pub async fn handle_import_key( struct ImportKeyRequest { access_key_id: String, secret_access_key: String, - name: String, + name: Option, } pub async fn handle_update_key( @@ -129,7 +134,7 @@ pub async fn handle_update_key( garage.key_table.insert(&key).await?; - key_info_results(garage, key).await + key_info_results(garage, key, false).await } #[derive(Deserialize)] @@ -152,7 +157,11 @@ pub async fn handle_delete_key(garage: &Arc, id: String) -> Result, key: Key) -> Result, Error> { +async fn key_info_results( + garage: &Arc, + key: Key, + show_secret: bool, +) -> Result, Error> { let mut relevant_buckets = HashMap::new(); let key_state = key.state.as_option().unwrap(); @@ -181,7 +190,11 @@ async fn key_info_results(garage: &Arc, key: Key) -> Result, key: Key) -> Result, permissions: KeyPerm, buckets: Vec, } diff --git a/src/api/admin/router.rs b/src/api/admin/router.rs index 077509e3..97ad6f76 100644 --- a/src/api/admin/router.rs +++ b/src/api/admin/router.rs @@ -35,6 +35,7 @@ pub enum Endpoint { GetKeyInfo { id: Option, search: Option, + show_secret_key: Option, }, DeleteKey { id: String, @@ -104,11 +105,11 @@ impl Endpoint { POST ("/v0/layout/apply" | "/v1/layout/apply") => ApplyClusterLayout, POST ("/v0/layout/revert" | "/v1/layout/revert") => RevertClusterLayout, // API key endpoints - GET ("/v0/key" | "/v1/key") if id => GetKeyInfo (query_opt::id, query_opt::search), - GET ("/v0/key" | "/v1/key") if search => GetKeyInfo (query_opt::id, query_opt::search), - POST ("/v0/key" | "/v1/key") if id => UpdateKey (query::id), - POST ("/v0/key" | "/v1/key") => CreateKey, - POST ("/v0/key/import" | "/v1/key/import") => ImportKey, + GET "/v1/key" if id => GetKeyInfo (query_opt::id, query_opt::search, query_opt::show_secret_key), + GET "/v1/key" if search => GetKeyInfo (query_opt::id, query_opt::search, query_opt::show_secret_key), + POST "/v1/key" if id => UpdateKey (query::id), + POST "/v1/key" => CreateKey, + POST "/v1/key/import" => ImportKey, DELETE ("/v0/key" | "/v1/key") if id => DeleteKey (query::id), GET ("/v0/key" | "/v1/key") => ListKeys, // Bucket endpoints @@ -153,6 +154,7 @@ generateQueryParameters! { "search" => search, "globalAlias" => global_alias, "alias" => alias, - "accessKeyId" => access_key_id + "accessKeyId" => access_key_id, + "showSecretKey" => show_secret_key ] } diff --git a/src/api/helpers.rs b/src/api/helpers.rs index 642dbc42..1d55ebd5 100644 --- a/src/api/helpers.rs +++ b/src/api/helpers.rs @@ -152,6 +152,10 @@ pub fn json_ok_response(res: &T) -> Result, Error> .body(Body::from(resp_json))?) } +pub fn is_default(v: &T) -> bool { + *v == T::default() +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/garage/admin/key.rs b/src/garage/admin/key.rs index 8a1c02af..908986fa 100644 --- a/src/garage/admin/key.rs +++ b/src/garage/admin/key.rs @@ -41,12 +41,17 @@ impl AdminRpcHandler { Ok(AdminRpc::KeyList(key_ids)) } - async fn handle_key_info(&self, query: &KeyOpt) -> Result { - let key = self + async fn handle_key_info(&self, query: &KeyInfoOpt) -> Result { + let mut key = self .garage .key_helper() .get_existing_matching_key(&query.key_pattern) .await?; + + if !query.show_secret { + key.state.as_option_mut().unwrap().secret_key = "(redacted)".into(); + } + self.key_info_result(key).await } diff --git a/src/garage/cli/structs.rs b/src/garage/cli/structs.rs index 2547fb8d..05d2ea31 100644 --- a/src/garage/cli/structs.rs +++ b/src/garage/cli/structs.rs @@ -328,7 +328,7 @@ pub enum KeyOperation { /// Get key info #[structopt(name = "info", version = garage_version())] - Info(KeyOpt), + Info(KeyInfoOpt), /// Create new key #[structopt(name = "create", version = garage_version())] @@ -356,9 +356,12 @@ pub enum KeyOperation { } #[derive(Serialize, Deserialize, StructOpt, Debug)] -pub struct KeyOpt { +pub struct KeyInfoOpt { /// ID or name of the key pub key_pattern: String, + /// Whether to display the secret key + #[structopt(long = "show-secret")] + pub show_secret: bool, } #[derive(Serialize, Deserialize, StructOpt, Debug)]