admin docs: reformatting, key admin: add check

5 files changed, 69 insertions, 64 deletions

diff --git a/doc/drafts/admin-api.md b/doc/drafts/admin-api.md
index cb491945..572595db 100644
--- a/doc/drafts/admin-api.md
+++ b/doc/drafts/admin-api.md
@@ -363,6 +363,7 @@ existing layout in the cluster.
 This returns the new cluster layout with all changes reverted,
 as returned by GetClusterLayout.
 
+
 ### Access key operations
 
 #### ListKeys `GET /v1/key`
@@ -384,37 +385,6 @@ Example response:
 ]
 ```
 
-#### CreateKey `POST /v1/key`
-
-Creates a new API access key.
-
-Request body format:
-
-```json
-{
-    "name": "NameOfMyKey"
-}
-```
-
-This returns the key info, including the created secret key,
-in the same format as the result of GetKeyInfo.
-
-#### ImportKey `POST /v1/key/import`
-
-Imports an existing API key.
-
-Request body format:
-
-```json
-{
-    "accessKeyId": "GK31c2f218a2e44f485b94239e",
-    "secretAccessKey": "b892c0665f0ada8a4755dae98baa3b133590e11dae3bcc1f9d769d67f16c3835",
-    "name": "NameOfMyKey"
-}
-```
-
-This returns the key info in the same format as the result of GetKeyInfo.
-
 #### GetKeyInfo `GET /v1/key?id=<acces key id>`
 #### GetKeyInfo `GET /v1/key?search=<pattern>`
 
@@ -490,9 +460,38 @@ Example response:
 }
 ```
 
-#### DeleteKey `DELETE /v1/key?id=<acces key id>`
+#### CreateKey `POST /v1/key`
 
-Deletes an API access key.
+Creates a new API access key.
+
+Request body format:
+
+```json
+{
+    "name": "NameOfMyKey"
+}
+```
+
+This returns the key info, including the created secret key,
+in the same format as the result of GetKeyInfo.
+
+#### ImportKey `POST /v1/key/import`
+
+Imports an existing API key.
+This will check that the imported key is in the valid format, i.e.
+is a key that could have been generated by Garage.
+
+Request body format:
+
+```json
+{
+    "accessKeyId": "GK31c2f218a2e44f485b94239e",
+    "secretAccessKey": "b892c0665f0ada8a4755dae98baa3b133590e11dae3bcc1f9d769d67f16c3835",
+    "name": "NameOfMyKey"
+}
+```
+
+This returns the key info in the same format as the result of GetKeyInfo.
 
 #### UpdateKey `POST /v1/key?id=<acces key id>`
 
@@ -516,6 +515,11 @@ The possible flags in `allow` and `deny` are: `createBucket`.
 
 This returns the key info in the same format as the result of GetKeyInfo.
 
+#### DeleteKey `DELETE /v1/key?id=<acces key id>`
+
+Deletes an API access key.
+
+
 ### Bucket operations
 
 #### ListBuckets `GET /v1/bucket`
@@ -644,12 +648,6 @@ or no alias at all.
 Technically, you can also specify both `globalAlias` and `localAlias` and that would create
 two aliases, but I don't see why you would want to do that.
 
-#### DeleteBucket `DELETE /v1/bucket?id=<bucket id>`
-
-Deletes a storage bucket. A bucket cannot be deleted if it is not empty.
-
-Warning: this will delete all aliases associated with the bucket!
-
 #### UpdateBucket `PUT /v1/bucket?id=<bucket id>`
 
 Updates configuration of the given bucket.
@@ -682,6 +680,13 @@ In `quotas`: new values of `maxSize` and `maxObjects` must both be specified, or
 to remove the quotas. An absent value will be considered the same as a `null`. It is not possible
 to change only one of the two quotas.
 
+#### DeleteBucket `DELETE /v1/bucket?id=<bucket id>`
+
+Deletes a storage bucket. A bucket cannot be deleted if it is not empty.
+
+Warning: this will delete all aliases associated with the bucket!
+
+
 ### Operations on permissions for keys on buckets
 
 #### BucketAllowKey `POST /v1/bucket/allow`
diff --git a/src/api/admin/cluster.rs b/src/api/admin/cluster.rs
index ae4a1cc3..c8107b82 100644
--- a/src/api/admin/cluster.rs
+++ b/src/api/admin/cluster.rs
@@ -1,7 +1,7 @@
 use std::net::SocketAddr;
 use std::sync::Arc;
 
-use hyper::{Body, Request, Response, StatusCode};
+use hyper::{Body, Request, Response};
 use serde::{Deserialize, Serialize};
 
 use garage_util::crdt::*;
@@ -161,8 +161,8 @@ struct GetClusterStatusResponse {
 #[derive(Serialize)]
 #[serde(rename_all = "camelCase")]
 struct ApplyClusterLayoutResponse {
-    message: Vec<String>,
-    layout: GetClusterLayoutResponse,
+	message: Vec<String>,
+	layout: GetClusterLayoutResponse,
 }
 
 #[derive(Serialize)]
@@ -238,7 +238,7 @@ pub async fn handle_update_cluster_layout(
 	garage.system.update_cluster_layout(&layout).await?;
 
 	let res = format_cluster_layout(&layout);
-    Ok(json_ok_response(&res)?)
+	Ok(json_ok_response(&res)?)
 }
 
 pub async fn handle_apply_cluster_layout(
@@ -253,10 +253,10 @@ pub async fn handle_apply_cluster_layout(
 	garage.system.update_cluster_layout(&layout).await?;
 
 	let res = ApplyClusterLayoutResponse {
-        message: msg,
-        layout: format_cluster_layout(&layout),
-    };
-    Ok(json_ok_response(&res)?)
+		message: msg,
+		layout: format_cluster_layout(&layout),
+	};
+	Ok(json_ok_response(&res)?)
 }
 
 pub async fn handle_revert_cluster_layout(
@@ -270,7 +270,7 @@ pub async fn handle_revert_cluster_layout(
 	garage.system.update_cluster_layout(&layout).await?;
 
 	let res = format_cluster_layout(&layout);
-    Ok(json_ok_response(&res)?)
+	Ok(json_ok_response(&res)?)
 }
 
 // ----
diff --git a/src/api/admin/key.rs b/src/api/admin/key.rs
index 0d1f799b..8d1c6890 100644
--- a/src/api/admin/key.rs
+++ b/src/api/admin/key.rs
@@ -93,7 +93,8 @@ pub async fn handle_import_key(
 		&req.access_key_id,
 		&req.secret_access_key,
 		req.name.as_deref().unwrap_or("Imported key"),
-	);
+	)
+	.ok_or_bad_request("Invalid key format")?;
 	garage.key_table.insert(&imported_key).await?;
 
 	key_info_results(garage, imported_key, false).await
diff --git a/src/garage/admin/key.rs b/src/garage/admin/key.rs
index 908986fa..1c92670c 100644
--- a/src/garage/admin/key.rs
+++ b/src/garage/admin/key.rs
@@ -2,7 +2,7 @@ use std::collections::HashMap;
 
 use garage_table::*;
 
-use garage_model::helper::error::Error;
+use garage_model::helper::error::*;
 use garage_model::key_table::*;
 
 use crate::cli::*;
@@ -127,22 +127,13 @@ impl AdminRpcHandler {
 			return Err(Error::BadRequest("This command is intended to re-import keys that were previously generated by Garage. If you want to create a new key, use `garage key new` instead. Add the --yes flag if you really want to re-import a key.".to_string()));
 		}
 
-		if query.key_id.len() != 26
-			|| &query.key_id[..2] != "GK"
-			|| hex::decode(&query.key_id[2..]).is_err()
-		{
-			return Err(Error::BadRequest(format!("The specified key ID is not a valid Garage key ID (starts with `GK`, followed by 12 hex-encoded bytes)")));
-		}
-		if query.secret_key.len() != 64 || hex::decode(&query.secret_key).is_err() {
-			return Err(Error::BadRequest(format!("The specified secret key is not a valid Garage secret key (composed of 32 hex-encoded bytes)")));
-		}
-
 		let prev_key = self.garage.key_table.get(&EmptyKey, &query.key_id).await?;
 		if prev_key.is_some() {
 			return Err(Error::BadRequest(format!("Key {} already exists in data store. Even if it is deleted, we can't let you create a new key with the same ID. Sorry.", query.key_id)));
 		}
 
-		let imported_key = Key::import(&query.key_id, &query.secret_key, &query.name);
+		let imported_key = Key::import(&query.key_id, &query.secret_key, &query.name)
+			.ok_or_bad_request("Invalid key format")?;
 		self.garage.key_table.insert(&imported_key).await?;
 
 		self.key_info_result(imported_key).await
diff --git a/src/model/key_table.rs b/src/model/key_table.rs
index bb5334a3..a9762f1b 100644
--- a/src/model/key_table.rs
+++ b/src/model/key_table.rs
@@ -149,11 +149,19 @@ impl Key {
 	}
 
 	/// Import a key from it's parts
-	pub fn import(key_id: &str, secret_key: &str, name: &str) -> Self {
-		Self {
+	pub fn import(key_id: &str, secret_key: &str, name: &str) -> Result<Self, &'static str> {
+		if key_id.len() != 26 || &key_id[..2] != "GK" || hex::decode(&key_id[2..]).is_err() {
+			return Err("The specified key ID is not a valid Garage key ID (starts with `GK`, followed by 12 hex-encoded bytes)");
+		}
+
+		if secret_key.len() != 64 || hex::decode(&secret_key).is_err() {
+			return Err("The specified secret key is not a valid Garage secret key (composed of 32 hex-encoded bytes)");
+		}
+
+		Ok(Self {
 			key_id: key_id.to_string(),
 			state: crdt::Deletable::present(KeyParams::new(secret_key, name)),
-		}
+		})
 	}
 
 	/// Create a new Key which can me merged to mark an existing key deleted