Small changes in key model and refactoringnew-buckets

author: Alex Auvolat <alex@adnab.me> 2022-01-04 18:59:17 +0100
committer: Alex Auvolat <alex@adnab.me> 2022-01-04 18:59:17 +0100
commit: 677ab60cc117677bf53dc4887a6ff1c094e17cd0 (patch)
tree: deed8c5e3a0e22fa0d1d987d4ca1313fd5c54fb7
parent: df35feba18787cac06b2a87e3426752fb78da2d5 (diff)
download: garage-new-buckets.tar.gz
garage-new-buckets.zip
7 files changed, 119 insertions, 103 deletions
diff --git a/src/api/s3_bucket.rs b/src/api/s3_bucket.rs
index 50aeb173..27208ffa 100644
--- a/src/api/s3_bucket.rs
+++ b/src/api/s3_bucket.rs
@@ -38,8 +38,8 @@ pub fn handle_get_bucket_versioning() -> Result<Response<Body>, Error> {
 }
 
 pub async fn handle_list_buckets(garage: &Garage, api_key: &Key) -> Result<Response<Body>, Error> {
-	let key_state = api_key.state.as_option().ok_or_internal_error(
-		"Key should not be in deleted state at this point (internal error)",
+	let key_p = api_key.params().ok_or_internal_error(
+		"Key should not be in deleted state at this point (in handle_list_buckets)",
 	)?;
 
 	// Collect buckets user has access to
@@ -74,7 +74,7 @@ pub async fn handle_list_buckets(garage: &Garage, api_key: &Key) -> Result<Respo
 		}
 	}
 
-	for (alias, _, id_opt) in key_state.local_aliases.items() {
+	for (alias, _, id_opt) in key_p.local_aliases.items() {
 		if let Some(id) = id_opt {
 			aliases.insert(alias.clone(), *id);
 		}
@@ -83,7 +83,7 @@ pub async fn handle_list_buckets(garage: &Garage, api_key: &Key) -> Result<Respo
 	// Generate response
 	let list_buckets = s3_xml::ListAllMyBucketsResult {
 		owner: s3_xml::Owner {
-			display_name: s3_xml::Value(api_key.name.get().to_string()),
+			display_name: s3_xml::Value(key_p.name.get().to_string()),
 			id: s3_xml::Value(api_key.key_id.to_string()),
 		},
 		buckets: s3_xml::BucketList {
diff --git a/src/api/signature.rs b/src/api/signature.rs
index b5da7b62..c580cb3a 100644
--- a/src/api/signature.rs
+++ b/src/api/signature.rs
@@ -66,6 +66,7 @@ pub async fn check_signature(
 		.await?
 		.filter(|k| !k.state.is_deleted())
 		.ok_or_else(|| Error::Forbidden(format!("No such key: {}", authorization.key_id)))?;
+	let key_p = key.params().unwrap();
 
 	let canonical_request = canonical_request(
 		request.method(),
@@ -79,7 +80,7 @@ pub async fn check_signature(
 
 	let mut hmac = signing_hmac(
 		&date,
-		&key.secret_key,
+		&key_p.secret_key,
 		&garage.config.s3_api.s3_region,
 		"s3",
 	)
diff --git a/src/garage/admin.rs b/src/garage/admin.rs
index b1eb6915..a682075f 100644
--- a/src/garage/admin.rs
+++ b/src/garage/admin.rs
@@ -107,7 +107,13 @@ impl AdminRpcHandler {
 			.items()
 			.iter()
 		{
-			if let Some(key) = self.garage.key_table.get(&EmptyKey, k).await? {
+			if let Some(key) = self
+				.garage
+				.key_table
+				.get(&EmptyKey, k)
+				.await?
+				.filter(|k| !k.is_deleted())
+			{
 				relevant_keys.insert(k.clone(), key);
 			}
 		}
@@ -222,7 +228,7 @@ impl AdminRpcHandler {
 		// 1. delete authorization from keys that had access
 		for (key_id, _) in bucket.authorized_keys() {
 			helper
-				.set_bucket_key_permissions(bucket.id, key_id, BucketKeyPerm::no_permissions())
+				.set_bucket_key_permissions(bucket.id, key_id, BucketKeyPerm::NO_PERMISSIONS)
 				.await?;
 		}
 
@@ -439,7 +445,7 @@ impl AdminRpcHandler {
 			)
 			.await?
 			.iter()
-			.map(|k| (k.key_id.to_string(), k.name.get().clone()))
+			.map(|k| (k.key_id.to_string(), k.params().unwrap().name.get().clone()))
 			.collect::<Vec<_>>();
 		Ok(AdminRpc::KeyList(key_ids))
 	}
@@ -454,7 +460,7 @@ impl AdminRpcHandler {
 	}
 
 	async fn handle_create_key(&self, query: &KeyNewOpt) -> Result<AdminRpc, Error> {
-		let key = Key::new(query.name.clone());
+		let key = Key::new(&query.name);
 		self.garage.key_table.insert(&key).await?;
 		self.key_info_result(key).await
 	}
@@ -465,7 +471,10 @@ impl AdminRpcHandler {
 			.bucket_helper()
 			.get_existing_matching_key(&query.key_pattern)
 			.await?;
-		key.name.update(query.new_name.clone());
+		key.params_mut()
+			.unwrap()
+			.name
+			.update(query.new_name.clone());
 		self.garage.key_table.insert(&key).await?;
 		self.key_info_result(key).await
 	}
@@ -500,7 +509,7 @@ impl AdminRpcHandler {
 		// 2. Remove permissions on all authorized buckets
 		for (ab_id, _auth) in state.authorized_buckets.items().iter() {
 			helper
-				.set_bucket_key_permissions(*ab_id, &key.key_id, BucketKeyPerm::no_permissions())
+				.set_bucket_key_permissions(*ab_id, &key.key_id, BucketKeyPerm::NO_PERMISSIONS)
 				.await?;
 		}
 
diff --git a/src/garage/cli/util.rs b/src/garage/cli/util.rs
index 7a7d0e9b..365831c4 100644
--- a/src/garage/cli/util.rs
+++ b/src/garage/cli/util.rs
@@ -65,11 +65,11 @@ pub fn print_key_info(key: &Key, relevant_buckets: &HashMap<Uuid, Bucket>) {
 		"".to_string()
 	};
 
-	println!("Key name: {}", key.name.get());
-	println!("Key ID: {}", key.key_id);
-	println!("Secret key: {}", key.secret_key);
 	match &key.state {
 		Deletable::Present(p) => {
+			println!("Key name: {}", p.name.get());
+			println!("Key ID: {}", key.key_id);
+			println!("Secret key: {}", p.secret_key);
 			println!("Can create buckets: {}", p.allow_create_bucket.get());
 			println!("\nKey-specific bucket aliases:");
 			let mut table = vec![];
@@ -112,12 +112,19 @@ pub fn print_key_info(key: &Key, relevant_buckets: &HashMap<Uuid, Bucket>) {
 			format_table(table);
 		}
 		Deletable::Deleted => {
-			println!("\nKey is deleted.");
+			println!("Key {} is deleted.", key.key_id);
 		}
 	}
 }
 
 pub fn print_bucket_info(bucket: &Bucket, relevant_keys: &HashMap<String, Key>) {
+	let key_name = |k| {
+		relevant_keys
+			.get(k)
+			.map(|k| k.params().unwrap().name.get().as_str())
+			.unwrap_or("<deleted>")
+	};
+
 	println!("Bucket: {}", hex::encode(bucket.id));
 	match &bucket.state {
 		Deletable::Deleted => println!("Bucket is deleted."),
@@ -135,11 +142,7 @@ pub fn print_bucket_info(bucket: &Bucket, relevant_keys: &HashMap<String, Key>)
 			let mut table = vec![];
 			for ((key_id, alias), _, active) in p.local_aliases.items().iter() {
 				if *active {
-					let key_name = relevant_keys
-						.get(key_id)
-						.map(|k| k.name.get().as_str())
-						.unwrap_or("");
-					table.push(format!("\t{} ({})\t{}", key_id, key_name, alias));
+					table.push(format!("\t{} ({})\t{}", key_id, key_name(key_id), alias));
 				}
 			}
 			format_table(table);
@@ -150,13 +153,13 @@ pub fn print_bucket_info(bucket: &Bucket, relevant_keys: &HashMap<String, Key>)
 				let rflag = if perm.allow_read { "R" } else { " " };
 				let wflag = if perm.allow_write { "W" } else { " " };
 				let oflag = if perm.allow_owner { "O" } else { " " };
-				let key_name = relevant_keys
-					.get(k)
-					.map(|k| k.name.get().as_str())
-					.unwrap_or("");
 				table.push(format!(
 					"\t{}{}{}\t{}\t{}",
-					rflag, wflag, oflag, k, key_name
+					rflag,
+					wflag,
+					oflag,
+					k,
+					key_name(k)
 				));
 			}
 			format_table(table);
diff --git a/src/model/bucket_table.rs b/src/model/bucket_table.rs
index d687e774..db7cec18 100644
--- a/src/model/bucket_table.rs
+++ b/src/model/bucket_table.rs
@@ -97,27 +97,32 @@ impl Bucket {
 		self.state.is_deleted()
 	}
 
+	/// Returns an option representing the parameters (None if in deleted state)
+	pub fn params(&self) -> Option<&BucketParams> {
+		self.state.as_option()
+	}
+
+	/// Mutable version of `.params()`
+	pub fn params_mut(&mut self) -> Option<&mut BucketParams> {
+		self.state.as_option_mut()
+	}
+
 	/// Return the list of authorized keys, when each was updated, and the permission associated to
 	/// the key
 	pub fn authorized_keys(&self) -> &[(String, BucketKeyPerm)] {
-		match &self.state {
-			crdt::Deletable::Deleted => &[],
-			crdt::Deletable::Present(state) => state.authorized_keys.items(),
-		}
+		self.params()
+			.map(|s| s.authorized_keys.items())
+			.unwrap_or(&[])
 	}
 
 	pub fn aliases(&self) -> &[(String, u64, bool)] {
-		match &self.state {
-			crdt::Deletable::Deleted => &[],
-			crdt::Deletable::Present(state) => state.aliases.items(),
-		}
+		self.params().map(|s| s.aliases.items()).unwrap_or(&[])
 	}
 
 	pub fn local_aliases(&self) -> &[((String, String), u64, bool)] {
-		match &self.state {
-			crdt::Deletable::Deleted => &[],
-			crdt::Deletable::Present(state) => state.local_aliases.items(),
-		}
+		self.params()
+			.map(|s| s.local_aliases.items())
+			.unwrap_or(&[])
 	}
 }
 
diff --git a/src/model/key_table.rs b/src/model/key_table.rs
index c25f2da4..d5e30f3f 100644
--- a/src/model/key_table.rs
+++ b/src/model/key_table.rs
@@ -14,29 +14,37 @@ pub struct Key {
 	/// The id of the key (immutable), used as partition key
 	pub key_id: String,
 
-	/// The secret_key associated
+	/// Internal state of the key
+	pub state: crdt::Deletable<KeyParams>,
+}
+
+/// Configuration for a key
+#[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
+pub struct KeyParams {
+	/// The secret_key associated (immutable)
 	pub secret_key: String,
 
 	/// Name for the key
 	pub name: crdt::Lww<String>,
 
+	/// Flag to allow users having this key to create buckets
+	pub allow_create_bucket: crdt::Lww<bool>,
+
 	/// If the key is present: it gives some permissions,
 	/// a map of bucket IDs (uuids) to permissions.
 	/// Otherwise no permissions are granted to key
-	pub state: crdt::Deletable<KeyParams>,
-}
-
-/// Configuration for a key
-#[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
-pub struct KeyParams {
-	pub allow_create_bucket: crdt::Lww<bool>,
 	pub authorized_buckets: crdt::Map<Uuid, BucketKeyPerm>,
+
+	/// A key can have a local view of buckets names it is
+	/// the only one to see, this is the namespace for these aliases
 	pub local_aliases: crdt::LwwMap<String, Option<Uuid>>,
 }
 
 impl KeyParams {
-	pub fn new() -> Self {
+	fn new(secret_key: &str, name: &str) -> Self {
 		KeyParams {
+			secret_key: secret_key.to_string(),
+			name: crdt::Lww::new(name.to_string()),
 			allow_create_bucket: crdt::Lww::new(false),
 			authorized_buckets: crdt::Map::new(),
 			local_aliases: crdt::LwwMap::new(),
@@ -44,14 +52,9 @@ impl KeyParams {
 	}
 }
 
-impl Default for KeyParams {
-	fn default() -> Self {
-		Self::new()
-	}
-}
-
 impl Crdt for KeyParams {
 	fn merge(&mut self, o: &Self) {
+		self.name.merge(&o.name);
 		self.allow_create_bucket.merge(&o.allow_create_bucket);
 		self.authorized_buckets.merge(&o.authorized_buckets);
 		self.local_aliases.merge(&o.local_aliases);
@@ -60,14 +63,12 @@ impl Crdt for KeyParams {
 
 impl Key {
 	/// Initialize a new Key, generating a random identifier and associated secret key
-	pub fn new(name: String) -> Self {
+	pub fn new(name: &str) -> Self {
 		let key_id = format!("GK{}", hex::encode(&rand::random::<[u8; 12]>()[..]));
 		let secret_key = hex::encode(&rand::random::<[u8; 32]>()[..]);
 		Self {
 			key_id,
-			secret_key,
-			name: crdt::Lww::new(name),
-			state: crdt::Deletable::present(KeyParams::new()),
+			state: crdt::Deletable::present(KeyParams::new(&secret_key, name)),
 		}
 	}
 
@@ -75,9 +76,7 @@ impl Key {
 	pub fn import(key_id: &str, secret_key: &str, name: &str) -> Self {
 		Self {
 			key_id: key_id.to_string(),
-			secret_key: secret_key.to_string(),
-			name: crdt::Lww::new(name.to_string()),
-			state: crdt::Deletable::present(KeyParams::new()),
+			state: crdt::Deletable::present(KeyParams::new(secret_key, name)),
 		}
 	}
 
@@ -85,49 +84,47 @@ impl Key {
 	pub fn delete(key_id: String) -> Self {
 		Self {
 			key_id,
-			secret_key: "".into(),
-			name: crdt::Lww::new("".to_string()),
 			state: crdt::Deletable::Deleted,
 		}
 	}
 
+	/// Returns true if this represents a deleted bucket
+	pub fn is_deleted(&self) -> bool {
+		self.state.is_deleted()
+	}
+
+	/// Returns an option representing the params (None if in deleted state)
+	pub fn params(&self) -> Option<&KeyParams> {
+		self.state.as_option()
+	}
+
+	/// Mutable version of `.state()`
+	pub fn params_mut(&mut self) -> Option<&mut KeyParams> {
+		self.state.as_option_mut()
+	}
+
+	/// Get permissions for a bucket
+	pub fn bucket_permissions(&self, bucket: &Uuid) -> BucketKeyPerm {
+		self.params()
+			.map(|params| params.authorized_buckets.get(bucket))
+			.flatten()
+			.cloned()
+			.unwrap_or(BucketKeyPerm::NO_PERMISSIONS)
+	}
+
 	/// Check if `Key` is allowed to read in bucket
 	pub fn allow_read(&self, bucket: &Uuid) -> bool {
-		if let crdt::Deletable::Present(params) = &self.state {
-			params
-				.authorized_buckets
-				.get(bucket)
-				.map(|x| x.allow_read)
-				.unwrap_or(false)
-		} else {
-			false
-		}
+		self.bucket_permissions(bucket).allow_read
 	}
 
 	/// Check if `Key` is allowed to write in bucket
 	pub fn allow_write(&self, bucket: &Uuid) -> bool {
-		if let crdt::Deletable::Present(params) = &self.state {
-			params
-				.authorized_buckets
-				.get(bucket)
-				.map(|x| x.allow_write)
-				.unwrap_or(false)
-		} else {
-			false
-		}
+		self.bucket_permissions(bucket).allow_write
 	}
 
 	/// Check if `Key` is owner of bucket
 	pub fn allow_owner(&self, bucket: &Uuid) -> bool {
-		if let crdt::Deletable::Present(params) = &self.state {
-			params
-				.authorized_buckets
-				.get(bucket)
-				.map(|x| x.allow_owner)
-				.unwrap_or(false)
-		} else {
-			false
-		}
+		self.bucket_permissions(bucket).allow_owner
 	}
 }
 
@@ -142,7 +139,6 @@ impl Entry<EmptyKey, String> for Key {
 
 impl Crdt for Key {
 	fn merge(&mut self, other: &Self) {
-		self.name.merge(&other.name);
 		self.state.merge(&other.state);
 	}
 }
@@ -168,15 +164,20 @@ impl TableSchema for KeyTable {
 			KeyFilter::Deleted(df) => df.apply(entry.state.is_deleted()),
 			KeyFilter::MatchesAndNotDeleted(pat) => {
 				let pat = pat.to_lowercase();
-				!entry.state.is_deleted()
-					&& (entry.key_id.to_lowercase().starts_with(&pat)
-						|| entry.name.get().to_lowercase() == pat)
+				entry
+					.params()
+					.map(|p| {
+						entry.key_id.to_lowercase().starts_with(&pat)
+							|| p.name.get().to_lowercase() == pat
+					})
+					.unwrap_or(false)
 			}
 		}
 	}
 
 	fn try_migrate(bytes: &[u8]) -> Option<Self::E> {
 		let old_k = rmp_serde::decode::from_read_ref::<_, old::Key>(bytes).ok()?;
+		let name = crdt::Lww::raw(old_k.name.timestamp(), old_k.name.get().clone());
 
 		let state = if old_k.deleted.get() {
 			crdt::Deletable::Deleted
@@ -185,16 +186,15 @@ impl TableSchema for KeyTable {
 			// migration is performed in specific migration code in
 			// garage/migrate.rs
 			crdt::Deletable::Present(KeyParams {
+				secret_key: old_k.secret_key,
+				name,
 				allow_create_bucket: crdt::Lww::new(false),
 				authorized_buckets: crdt::Map::new(),
 				local_aliases: crdt::LwwMap::new(),
 			})
 		};
-		let name = crdt::Lww::raw(old_k.name.timestamp(), old_k.name.get().clone());
 		Some(Key {
 			key_id: old_k.key_id,
-			secret_key: old_k.secret_key,
-			name,
 			state,
 		})
 	}
diff --git a/src/model/permission.rs b/src/model/permission.rs
index b8f7dd71..67527ed0 100644
--- a/src/model/permission.rs
+++ b/src/model/permission.rs
@@ -21,14 +21,12 @@ pub struct BucketKeyPerm {
 }
 
 impl BucketKeyPerm {
-	pub fn no_permissions() -> Self {
-		Self {
-			timestamp: 0,
-			allow_read: false,
-			allow_write: false,
-			allow_owner: false,
-		}
-	}
+	pub const NO_PERMISSIONS: Self = Self {
+		timestamp: 0,
+		allow_read: false,
+		allow_write: false,
+		allow_owner: false,
+	};
 }
 
 impl Crdt for BucketKeyPerm {
author	Alex Auvolat <alex@adnab.me>	2022-01-04 18:59:17 +0100
committer	Alex Auvolat <alex@adnab.me>	2022-01-04 18:59:17 +0100
commit	677ab60cc117677bf53dc4887a6ff1c094e17cd0 (patch)
tree	deed8c5e3a0e22fa0d1d987d4ca1313fd5c54fb7
parent	df35feba18787cac06b2a87e3426752fb78da2d5 (diff)
download	garage-new-buckets.tar.gz garage-new-buckets.zip