From 539af12d21567a39a074d3a73c893d98275c70d4 Mon Sep 17 00:00:00 2001 From: trinity-1686a Date: Mon, 19 May 2025 18:07:04 +0200 Subject: [PATCH 1/4] allow punnycode in bucket name --- src/api/admin/bucket.rs | 4 ++-- src/api/s3/bucket.rs | 2 +- src/garage/admin/bucket.rs | 2 +- src/model/bucket_alias_table.rs | 18 ++++++++---------- src/model/helper/locked.rs | 7 +++---- src/util/config.rs | 4 ++++ 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index 2537bfc9..6cc21938 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -277,7 +277,7 @@ pub async fn handle_create_bucket( let helper = garage.locked_helper().await; if let Some(ga) = &req.global_alias { - if !is_valid_bucket_name(ga) { + if !is_valid_bucket_name(ga, garage.config.allow_punnycode) { return Err(Error::bad_request(format!( "{}: {}", ga, INVALID_BUCKET_NAME_MESSAGE @@ -292,7 +292,7 @@ pub async fn handle_create_bucket( } if let Some(la) = &req.local_alias { - if !is_valid_bucket_name(&la.alias) { + if !is_valid_bucket_name(&la.alias, garage.config.allow_punnycode) { return Err(Error::bad_request(format!( "{}: {}", la.alias, INVALID_BUCKET_NAME_MESSAGE diff --git a/src/api/s3/bucket.rs b/src/api/s3/bucket.rs index 3a09e769..d2a36c18 100644 --- a/src/api/s3/bucket.rs +++ b/src/api/s3/bucket.rs @@ -172,7 +172,7 @@ pub async fn handle_create_bucket( } // Create the bucket! - if !is_valid_bucket_name(&bucket_name) { + if !is_valid_bucket_name(&bucket_name, garage.config.allow_punnycode) { return Err(Error::bad_request(format!( "{}: {}", bucket_name, INVALID_BUCKET_NAME_MESSAGE diff --git a/src/garage/admin/bucket.rs b/src/garage/admin/bucket.rs index 1bdc6086..1ed0ebd8 100644 --- a/src/garage/admin/bucket.rs +++ b/src/garage/admin/bucket.rs @@ -126,7 +126,7 @@ impl AdminRpcHandler { #[allow(clippy::ptr_arg)] async fn handle_create_bucket(&self, name: &String) -> Result { - if !is_valid_bucket_name(name) { + if !is_valid_bucket_name(name, self.garage.config.allow_punnycode) { return Err(Error::BadRequest(format!( "{}: {}", name, INVALID_BUCKET_NAME_MESSAGE diff --git a/src/model/bucket_alias_table.rs b/src/model/bucket_alias_table.rs index 8bbe4118..04d808e8 100644 --- a/src/model/bucket_alias_table.rs +++ b/src/model/bucket_alias_table.rs @@ -22,14 +22,10 @@ mod v08 { pub use v08::*; impl BucketAlias { - pub fn new(name: String, ts: u64, bucket_id: Option) -> Option { - if !is_valid_bucket_name(&name) { - None - } else { - Some(BucketAlias { - name, - state: crdt::Lww::raw(ts, bucket_id), - }) + pub fn new(name: String, ts: u64, bucket_id: Option) -> Self { + BucketAlias { + name, + state: crdt::Lww::raw(ts, bucket_id), } } @@ -80,7 +76,7 @@ impl TableSchema for BucketAliasTable { /// In the case of Garage, bucket names must not be hex-encoded /// 32 byte string, which is excluded thanks to the /// maximum length of 63 bytes given in the spec. -pub fn is_valid_bucket_name(n: &str) -> bool { +pub fn is_valid_bucket_name(n: &str, punny: bool) -> bool { // Bucket names must be between 3 and 63 characters n.len() >= 3 && n.len() <= 63 // Bucket names must be composed of lowercase letters, numbers, @@ -92,7 +88,9 @@ pub fn is_valid_bucket_name(n: &str) -> bool { // Bucket names must not be formatted as an IP address && n.parse::().is_err() // Bucket names must not start with "xn--" - && !n.starts_with("xn--") + && (!n.starts_with("xn--") || punny) + // We are a bit stricter, to properly restrict punnycode in all labels + && (!n.contains(".xn--") || punny) // Bucket names must not end with "-s3alias" && !n.ends_with("-s3alias") } diff --git a/src/model/helper/locked.rs b/src/model/helper/locked.rs index 482e91b0..16b0bafc 100644 --- a/src/model/helper/locked.rs +++ b/src/model/helper/locked.rs @@ -57,7 +57,7 @@ impl<'a> LockedHelper<'a> { bucket_id: Uuid, alias_name: &String, ) -> Result<(), Error> { - if !is_valid_bucket_name(alias_name) { + if !is_valid_bucket_name(alias_name, self.0.config.allow_punnycode) { return Err(Error::InvalidBucketName(alias_name.to_string())); } @@ -88,8 +88,7 @@ impl<'a> LockedHelper<'a> { // writes are now done and all writes use timestamp alias_ts let alias = match alias { - None => BucketAlias::new(alias_name.clone(), alias_ts, Some(bucket_id)) - .ok_or_else(|| Error::InvalidBucketName(alias_name.clone()))?, + None => BucketAlias::new(alias_name.clone(), alias_ts, Some(bucket_id)), Some(mut a) => { a.state = Lww::raw(alias_ts, Some(bucket_id)); a @@ -218,7 +217,7 @@ impl<'a> LockedHelper<'a> { ) -> Result<(), Error> { let key_helper = KeyHelper(self.0); - if !is_valid_bucket_name(alias_name) { + if !is_valid_bucket_name(alias_name, self.0.config.allow_punnycode) { return Err(Error::InvalidBucketName(alias_name.to_string())); } diff --git a/src/util/config.rs b/src/util/config.rs index 73fc4ff4..f128177b 100644 --- a/src/util/config.rs +++ b/src/util/config.rs @@ -135,6 +135,10 @@ pub struct Config { /// Configuration for the admin API endpoint #[serde(default = "Default::default")] pub admin: AdminConfig, + + /// Allow punnycode in bucket names + #[serde(default)] + pub allow_punnycode: bool, } /// Value for data_dir: either a single directory or a list of dirs with attributes From a605a8080659b73939f6b3ff60bc0847ed0fb3c5 Mon Sep 17 00:00:00 2001 From: trinity-1686a Date: Mon, 19 May 2025 18:11:55 +0200 Subject: [PATCH 2/4] support punnycode in api/web endpoint --- Cargo.lock | 43 +-------------------------------------- Cargo.toml | 1 - src/api/common/Cargo.toml | 1 - src/api/common/helpers.rs | 3 +-- 4 files changed, 2 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bd5a1f9f..e65778cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1300,7 +1300,6 @@ dependencies = [ "http-body-util", "hyper 1.6.0", "hyper-util", - "idna 0.5.0", "md-5", "nom", "opentelemetry", @@ -2170,16 +2169,6 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" -[[package]] -name = "idna" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "634d9b1461af396cad843f47fdba5597a4f9e6ddd4bfb6ff5d85028c25cb12f6" -dependencies = [ - "unicode-bidi", - "unicode-normalization", -] - [[package]] name = "idna" version = "1.0.3" @@ -4252,21 +4241,6 @@ dependencies = [ "zerovec", ] -[[package]] -name = "tinyvec" -version = "1.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "09b3661f17e86524eccd4371ab0429194e0d7c008abb45f7a7495b1719463c71" -dependencies = [ - "tinyvec_macros", -] - -[[package]] -name = "tinyvec_macros" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" - [[package]] name = "tokio" version = "1.44.1" @@ -4587,27 +4561,12 @@ version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2896d95c02a80c6d6a5d6e953d479f5ddf2dfdb6a244441010e373ac0fb88971" -[[package]] -name = "unicode-bidi" -version = "0.3.18" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c1cb5db39152898a79168971543b1cb5020dff7fe43c8dc468b0885f5e29df5" - [[package]] name = "unicode-ident" version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5a5f39404a5da50712a4c1eecf25e90dd62b613502b7e925fd4e4d19b5c96512" -[[package]] -name = "unicode-normalization" -version = "0.1.24" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5033c97c4262335cded6d6fc3e5c18ab755e1a3dc96376350f3d8e9f009ad956" -dependencies = [ - "tinyvec", -] - [[package]] name = "unicode-segmentation" version = "1.12.0" @@ -4655,7 +4614,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32f8b686cadd1473f4bd0117a5d28d36b1ade384ea9b5069a1c40aefed7fda60" dependencies = [ "form_urlencoded", - "idna 1.0.3", + "idna", "percent-encoding", ] diff --git a/Cargo.toml b/Cargo.toml index 732f6f05..400c1840 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,7 +58,6 @@ git-version = "0.3.4" hex = "0.4" hexdump = "0.1" hmac = "0.12" -idna = "0.5" itertools = "0.12" ipnet = "2.9.0" lazy_static = "1.4" diff --git a/src/api/common/Cargo.toml b/src/api/common/Cargo.toml index 6d906423..b1a8b47a 100644 --- a/src/api/common/Cargo.toml +++ b/src/api/common/Cargo.toml @@ -28,7 +28,6 @@ err-derive.workspace = true hex.workspace = true hmac.workspace = true md-5.workspace = true -idna.workspace = true tracing.workspace = true nom.workspace = true pin-project.workspace = true diff --git a/src/api/common/helpers.rs b/src/api/common/helpers.rs index c8586de4..6fc4aa13 100644 --- a/src/api/common/helpers.rs +++ b/src/api/common/helpers.rs @@ -8,7 +8,6 @@ use hyper::{ body::{Body, Bytes}, Request, Response, }; -use idna::domain_to_unicode; use serde::{Deserialize, Serialize}; use garage_model::bucket_table::BucketParams; @@ -97,7 +96,7 @@ pub fn authority_to_host(authority: &str) -> Result { authority ))), }; - authority.map(|h| domain_to_unicode(h).0) + authority.map(|h| h.to_ascii_lowercase()) } /// Extract the bucket name and the key name from an HTTP path and possibly a bucket provided in From bba9202f310b257ed52d1a82052f05532495c62e Mon Sep 17 00:00:00 2001 From: trinity-1686a Date: Mon, 19 May 2025 20:36:03 +0200 Subject: [PATCH 3/4] add test for punycode --- src/api/admin/bucket.rs | 4 +- src/api/s3/bucket.rs | 2 +- src/garage/admin/bucket.rs | 2 +- src/garage/tests/common/garage.rs | 2 + src/garage/tests/s3/website.rs | 73 +++++++++++++++++++++++++++++++ src/model/bucket_alias_table.rs | 8 ++-- src/model/helper/locked.rs | 4 +- src/util/config.rs | 4 +- 8 files changed, 87 insertions(+), 12 deletions(-) diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index 6cc21938..f8bd1eb5 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -277,7 +277,7 @@ pub async fn handle_create_bucket( let helper = garage.locked_helper().await; if let Some(ga) = &req.global_alias { - if !is_valid_bucket_name(ga, garage.config.allow_punnycode) { + if !is_valid_bucket_name(ga, garage.config.allow_punycode) { return Err(Error::bad_request(format!( "{}: {}", ga, INVALID_BUCKET_NAME_MESSAGE @@ -292,7 +292,7 @@ pub async fn handle_create_bucket( } if let Some(la) = &req.local_alias { - if !is_valid_bucket_name(&la.alias, garage.config.allow_punnycode) { + if !is_valid_bucket_name(&la.alias, garage.config.allow_punycode) { return Err(Error::bad_request(format!( "{}: {}", la.alias, INVALID_BUCKET_NAME_MESSAGE diff --git a/src/api/s3/bucket.rs b/src/api/s3/bucket.rs index d2a36c18..23cceb84 100644 --- a/src/api/s3/bucket.rs +++ b/src/api/s3/bucket.rs @@ -172,7 +172,7 @@ pub async fn handle_create_bucket( } // Create the bucket! - if !is_valid_bucket_name(&bucket_name, garage.config.allow_punnycode) { + if !is_valid_bucket_name(&bucket_name, garage.config.allow_punycode) { return Err(Error::bad_request(format!( "{}: {}", bucket_name, INVALID_BUCKET_NAME_MESSAGE diff --git a/src/garage/admin/bucket.rs b/src/garage/admin/bucket.rs index 1ed0ebd8..073329c1 100644 --- a/src/garage/admin/bucket.rs +++ b/src/garage/admin/bucket.rs @@ -126,7 +126,7 @@ impl AdminRpcHandler { #[allow(clippy::ptr_arg)] async fn handle_create_bucket(&self, name: &String) -> Result { - if !is_valid_bucket_name(name, self.garage.config.allow_punnycode) { + if !is_valid_bucket_name(name, self.garage.config.allow_punycode) { return Err(Error::BadRequest(format!( "{}: {}", name, INVALID_BUCKET_NAME_MESSAGE diff --git a/src/garage/tests/common/garage.rs b/src/garage/tests/common/garage.rs index 8d71504f..2b0a381c 100644 --- a/src/garage/tests/common/garage.rs +++ b/src/garage/tests/common/garage.rs @@ -63,6 +63,8 @@ rpc_bind_addr = "127.0.0.1:{rpc_port}" rpc_public_addr = "127.0.0.1:{rpc_port}" rpc_secret = "{secret}" +allow_punycode = true + [s3_api] s3_region = "{region}" api_bind_addr = "127.0.0.1:{s3_port}" diff --git a/src/garage/tests/s3/website.rs b/src/garage/tests/s3/website.rs index 9a9e29f2..6d37eee8 100644 --- a/src/garage/tests/s3/website.rs +++ b/src/garage/tests/s3/website.rs @@ -533,3 +533,76 @@ async fn test_website_check_domain() { }) ); } + +#[tokio::test] +async fn test_website_puny() { + const BCKT_NAME: &str = "xn--pda.eu"; + let ctx = common::context(); + let bucket = ctx.create_bucket(BCKT_NAME); + + let data = ByteStream::from_static(BODY); + + ctx.client + .put_object() + .bucket(&bucket) + .key("index.html") + .body(data) + .send() + .await + .unwrap(); + + let client = Client::builder(TokioExecutor::new()).build_http(); + + let req = |suffix| { + Request::builder() + .method("GET") + .uri(format!("http://127.0.0.1:{}/", ctx.garage.web_port)) + .header("Host", format!("{}{}", BCKT_NAME, suffix)) + .body(Body::new(Bytes::new())) + .unwrap() + }; + + ctx.garage + .command() + .args(["bucket", "website", "--allow", BCKT_NAME]) + .quiet() + .expect_success_status("Could not allow website on bucket"); + + let mut resp = client.request(req("")).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + BODY.as_ref() + ); + + resp = client.request(req(".web.garage")).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + BODY.as_ref() + ); + + for bname in [ + BCKT_NAME.to_string(), + format!("{BCKT_NAME}.web.garage"), + format!("{BCKT_NAME}.s3.garage"), + ] { + let admin_req = || { + Request::builder() + .method("GET") + .uri(format!( + "http://127.0.0.1:{0}/check?domain={1}", + ctx.garage.admin_port, bname + )) + .body(Body::new(Bytes::new())) + .unwrap() + }; + + let admin_resp = client.request(admin_req()).await.unwrap(); + assert_eq!(admin_resp.status(), StatusCode::OK); + assert_eq!( + admin_resp.into_body().collect().await.unwrap().to_bytes(), + format!("Domain '{bname}' is managed by Garage").as_bytes() + ); + } +} diff --git a/src/model/bucket_alias_table.rs b/src/model/bucket_alias_table.rs index 04d808e8..276d0d1c 100644 --- a/src/model/bucket_alias_table.rs +++ b/src/model/bucket_alias_table.rs @@ -76,7 +76,7 @@ impl TableSchema for BucketAliasTable { /// In the case of Garage, bucket names must not be hex-encoded /// 32 byte string, which is excluded thanks to the /// maximum length of 63 bytes given in the spec. -pub fn is_valid_bucket_name(n: &str, punny: bool) -> bool { +pub fn is_valid_bucket_name(n: &str, puny: bool) -> bool { // Bucket names must be between 3 and 63 characters n.len() >= 3 && n.len() <= 63 // Bucket names must be composed of lowercase letters, numbers, @@ -88,9 +88,9 @@ pub fn is_valid_bucket_name(n: &str, punny: bool) -> bool { // Bucket names must not be formatted as an IP address && n.parse::().is_err() // Bucket names must not start with "xn--" - && (!n.starts_with("xn--") || punny) - // We are a bit stricter, to properly restrict punnycode in all labels - && (!n.contains(".xn--") || punny) + && (!n.starts_with("xn--") || puny) + // We are a bit stricter, to properly restrict punycode in all labels + && (!n.contains(".xn--") || puny) // Bucket names must not end with "-s3alias" && !n.ends_with("-s3alias") } diff --git a/src/model/helper/locked.rs b/src/model/helper/locked.rs index 16b0bafc..a5821f77 100644 --- a/src/model/helper/locked.rs +++ b/src/model/helper/locked.rs @@ -57,7 +57,7 @@ impl<'a> LockedHelper<'a> { bucket_id: Uuid, alias_name: &String, ) -> Result<(), Error> { - if !is_valid_bucket_name(alias_name, self.0.config.allow_punnycode) { + if !is_valid_bucket_name(alias_name, self.0.config.allow_punycode) { return Err(Error::InvalidBucketName(alias_name.to_string())); } @@ -217,7 +217,7 @@ impl<'a> LockedHelper<'a> { ) -> Result<(), Error> { let key_helper = KeyHelper(self.0); - if !is_valid_bucket_name(alias_name, self.0.config.allow_punnycode) { + if !is_valid_bucket_name(alias_name, self.0.config.allow_punycode) { return Err(Error::InvalidBucketName(alias_name.to_string())); } diff --git a/src/util/config.rs b/src/util/config.rs index f128177b..c74029e7 100644 --- a/src/util/config.rs +++ b/src/util/config.rs @@ -136,9 +136,9 @@ pub struct Config { #[serde(default = "Default::default")] pub admin: AdminConfig, - /// Allow punnycode in bucket names + /// Allow punycode in bucket names #[serde(default)] - pub allow_punnycode: bool, + pub allow_punycode: bool, } /// Value for data_dir: either a single directory or a list of dirs with attributes From 2dc3a6dbbe88a3498a9fc39c50aeb94124c39781 Mon Sep 17 00:00:00 2001 From: trinity-1686a Date: Thu, 22 May 2025 14:08:06 +0200 Subject: [PATCH 4/4] document allow_punycode configuration option --- doc/book/reference-manual/configuration.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/doc/book/reference-manual/configuration.md b/doc/book/reference-manual/configuration.md index e0fc17bc..09ce8d24 100644 --- a/doc/book/reference-manual/configuration.md +++ b/doc/book/reference-manual/configuration.md @@ -46,6 +46,7 @@ bootstrap_peers = [ "212fd62eeaca72c122b45a7f4fa0f55e012aa5e24ac384a72a3016413fa724ff@[fc00:F::1]:3901", ] +allow_punycode = false [consul_discovery] api = "catalog" @@ -115,6 +116,7 @@ Top-level configuration options: [`rpc_public_addr`](#rpc_public_addr), [`rpc_public_addr_subnet`](#rpc_public_addr_subnet) [`rpc_secret`/`rpc_secret_file`](#rpc_secret). +[`allow_punycode`](#allow_punycode). The `[consul_discovery]` section: [`api`](#consul_api), @@ -604,7 +606,7 @@ be obtained by running `garage node id` and then included directly in the key will be returned by `garage node id` and you will have to add the IP yourself. -### `allow_world_readable_secrets` or `GARAGE_ALLOW_WORLD_READABLE_SECRETS` (env) {#allow_world_readable_secrets} +#### `allow_world_readable_secrets` or `GARAGE_ALLOW_WORLD_READABLE_SECRETS` (env) {#allow_world_readable_secrets} Garage checks the permissions of your secret files to make sure they're not world-readable. In some cases, the check might fail and consider your files as @@ -616,6 +618,13 @@ permission verification. Alternatively, you can set the `GARAGE_ALLOW_WORLD_READABLE_SECRETS` environment variable to `true` to bypass the permissions check. +#### `allow_punycode` {#allow_punycode} + +Allow creating buckets with names containing punycode. When used for buckets served +as websites, this allows using almost any unicode character in the domain name. + +Default to `false`. + ### The `[consul_discovery]` section Garage supports discovering other nodes of the cluster using Consul. For this