diff options
| author | Alex <alex@adnab.me> | 2023-05-18 09:33:03 +0000 |
|---|---|---|
| committer | Alex <alex@adnab.me> | 2023-05-18 09:33:03 +0000 |
| commit | 03efc191c1697140d24c431e88bd4964c77823e5 (patch) | |
| tree | 103270ccca9d9a26c220fce528b9a3c397fb2703 | |
| parent | c26a4308b4454e2f36e2824280b9f587a6918fa9 (diff) | |
| parent | 4420db731020bf7123f2bb80b076da581836f900 (diff) | |
| download | garage-03efc191c1697140d24c431e88bd4964c77823e5.tar.gz garage-03efc191c1697140d24c431e88bd4964c77823e5.zip | |
Merge pull request 'K2V: double urlencoding' (#574) from fix-k2v-urlencoding into main
Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/574
| -rw-r--r-- | Cargo.lock | 4 | ||||
| -rw-r--r-- | Cargo.nix | 12 | ||||
| -rw-r--r-- | src/api/signature/payload.rs | 40 | ||||
| -rw-r--r-- | src/garage/tests/common/custom_requester.rs | 1 | ||||
| -rw-r--r-- | src/k2v-client/Cargo.toml | 6 | ||||
| -rw-r--r-- | src/k2v-client/bin/k2v-cli.rs | 9 |
6 files changed, 63 insertions, 9 deletions
@@ -1891,7 +1891,7 @@ dependencies = [ [[package]] name = "k2v-client" -version = "0.0.2" +version = "0.0.3" dependencies = [ "base64 0.21.0", "clap 4.2.7", @@ -1906,6 +1906,8 @@ dependencies = [ "serde_json", "thiserror", "tokio", + "tracing", + "tracing-subscriber", ] [[package]] @@ -33,7 +33,7 @@ args@{ ignoreLockHash, }: let - nixifiedLockHash = "b57cac5992bf6a665ce564a43f629f165268920af9035aa11fda2a8dfe9738f6"; + nixifiedLockHash = "73f681ad7edf7fe539579e25d09d2ad962b9b7d64b018d421951d3cde43f5f77"; workspaceSrc = if args.workspaceSrc == null then ./. else args.workspaceSrc; currentLockHash = builtins.hashFile "sha256" (workspaceSrc + /Cargo.lock); lockHashIgnored = if ignoreLockHash @@ -67,7 +67,7 @@ in garage_web = rustPackages.unknown.garage_web."0.8.2"; garage = rustPackages.unknown.garage."0.8.2"; format_table = rustPackages.unknown.format_table."0.1.1"; - k2v-client = rustPackages.unknown.k2v-client."0.0.2"; + k2v-client = rustPackages.unknown.k2v-client."0.0.3"; }; "registry+https://github.com/rust-lang/crates.io-index".addr2line."0.19.0" = overridableMkRustCrate (profileName: rec { name = "addr2line"; @@ -2637,15 +2637,17 @@ in }; }); - "unknown".k2v-client."0.0.2" = overridableMkRustCrate (profileName: rec { + "unknown".k2v-client."0.0.3" = overridableMkRustCrate (profileName: rec { name = "k2v-client"; - version = "0.0.2"; + version = "0.0.3"; registry = "unknown"; src = fetchCrateLocal (workspaceSrc + "/src/k2v-client"); features = builtins.concatLists [ (lib.optional (rootFeatures' ? "k2v-client/clap" || rootFeatures' ? "k2v-client/cli") "clap") (lib.optional (rootFeatures' ? "k2v-client/cli") "cli") (lib.optional (rootFeatures' ? "k2v-client/cli" || rootFeatures' ? "k2v-client/format_table") "format_table") + (lib.optional (rootFeatures' ? "k2v-client/cli" || rootFeatures' ? "k2v-client/tracing") "tracing") + (lib.optional (rootFeatures' ? "k2v-client/cli" || rootFeatures' ? "k2v-client/tracing-subscriber") "tracing-subscriber") ]; dependencies = { base64 = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".base64."0.21.0" { inherit profileName; }).out; @@ -2661,6 +2663,8 @@ in serde_json = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".serde_json."1.0.91" { inherit profileName; }).out; thiserror = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".thiserror."1.0.38" { inherit profileName; }).out; tokio = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".tokio."1.28.0" { inherit profileName; }).out; + ${ if rootFeatures' ? "k2v-client/cli" || rootFeatures' ? "k2v-client/tracing" then "tracing" else null } = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".tracing."0.1.37" { inherit profileName; }).out; + ${ if rootFeatures' ? "k2v-client/cli" || rootFeatures' ? "k2v-client/tracing-subscriber" then "tracing_subscriber" else null } = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".tracing-subscriber."0.3.16" { inherit profileName; }).out; }; }); diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 4c7934e5..c945ab11 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -19,7 +19,7 @@ use crate::signature::error::*; pub async fn check_payload_signature( garage: &Garage, - service: &str, + service: &'static str, request: &Request<Body>, ) -> Result<(Option<Key>, Option<Hash>), Error> { let mut headers = HashMap::new(); @@ -51,6 +51,7 @@ pub async fn check_payload_signature( }; let canonical_request = canonical_request( + service, request.method(), request.uri(), &headers, @@ -231,15 +232,50 @@ pub fn string_to_sign(datetime: &DateTime<Utc>, scope_string: &str, canonical_re } pub fn canonical_request( + service: &'static str, method: &Method, uri: &hyper::Uri, headers: &HashMap<String, String>, signed_headers: &str, content_sha256: &str, ) -> String { + // There seems to be evidence that in AWSv4 signatures, the path component is url-encoded + // a second time when building the canonical request, as specified in this documentation page: + // -> https://docs.aws.amazon.com/rolesanywhere/latest/userguide/authentication-sign-process.html + // However this documentation page is for a specific service ("roles anywhere"), and + // in the S3 service we know for a fact that there is no double-urlencoding, because all of + // the tests we made with external software work without it. + // + // The theory is that double-urlencoding occurs for all services except S3, + // which is what is implemented in rusoto_signature: + // -> https://docs.rs/rusoto_signature/latest/src/rusoto_signature/signature.rs.html#464 + // + // Digging into the code of the official AWS Rust SDK, we learn that double-URI-encoding can + // be set or unset on a per-request basis (the signature crates, aws-sigv4 and aws-sig-auth, + // are agnostic to this). Grepping the codebase confirms that S3 is the only API for which + // double_uri_encode is set to false, meaning it is true (its default value) for all other + // AWS services. We will therefore implement this behavior in Garage as well. + // + // Note that this documentation page, which is touted as the "authoritative reference" on + // AWSv4 signatures, makes no mention of either single- or double-urlencoding: + // -> https://docs.aws.amazon.com/IAM/latest/UserGuide/create-signed-request.html + // This page of the S3 documentation does also not mention anything specific: + // -> https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html + // + // Note that there is also the issue of path normalization, which I hope is unrelated to the + // one of URI-encoding. At least in aws-sigv4 both parameters can be set independently, + // and rusoto_signature does not seem to do any effective path normalization, even though + // it mentions it in the comments (same link to the souce code as above). + // We make the explicit choice of NOT normalizing paths in the K2V API because doing so + // would make non-normalized paths invalid K2V partition keys, and we don't want that. + let path: std::borrow::Cow<str> = if service != "s3" { + uri_encode(uri.path(), false).into() + } else { + uri.path().into() + }; [ method.as_str(), - uri.path(), + &path, &canonical_query_string(uri), &canonical_header_string(headers, signed_headers), "", diff --git a/src/garage/tests/common/custom_requester.rs b/src/garage/tests/common/custom_requester.rs index 609eda97..4133bb8b 100644 --- a/src/garage/tests/common/custom_requester.rs +++ b/src/garage/tests/common/custom_requester.rs @@ -209,6 +209,7 @@ impl<'a> RequestBuilder<'a> { all_headers.extend(self.unsigned_headers.clone()); let canonical_request = signature::payload::canonical_request( + self.service, &self.method, &Uri::try_from(&uri).unwrap(), &all_headers, diff --git a/src/k2v-client/Cargo.toml b/src/k2v-client/Cargo.toml index 4212a00a..79af5242 100644 --- a/src/k2v-client/Cargo.toml +++ b/src/k2v-client/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "k2v-client" -version = "0.0.2" +version = "0.0.3" authors = ["Trinity Pointard <trinity.pointard@gmail.com>", "Alex Auvolat <alex@adnab.me>"] edition = "2018" license = "AGPL-3.0" @@ -24,10 +24,12 @@ tokio = { version = "1.0", default-features = false, features = ["rt", "rt-multi # cli deps clap = { version = "4.1", optional = true, features = ["derive", "env"] } format_table = { workspace = true, optional = true } +tracing = { version = "0.1", optional = true } +tracing-subscriber = { version = "0.3", optional = true, features = ["env-filter"] } [features] -cli = ["clap", "tokio/fs", "tokio/io-std", "format_table"] +cli = ["clap", "tokio/fs", "tokio/io-std", "tracing", "tracing-subscriber", "format_table"] [lib] path = "lib.rs" diff --git a/src/k2v-client/bin/k2v-cli.rs b/src/k2v-client/bin/k2v-cli.rs index 76388752..984b4192 100644 --- a/src/k2v-client/bin/k2v-cli.rs +++ b/src/k2v-client/bin/k2v-cli.rs @@ -397,6 +397,15 @@ impl Filter { #[tokio::main] async fn main() -> Result<(), Error> { + if std::env::var("RUST_LOG").is_err() { + std::env::set_var("RUST_LOG", "warn") + } + + tracing_subscriber::fmt() + .with_writer(std::io::stderr) + .with_env_filter(tracing_subscriber::filter::EnvFilter::from_default_env()) + .init(); + let args = Args::parse(); let region = Region::Custom { |