This is an automated email from the ASF dual-hosted git repository.
JingsongLi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/paimon-rust.git
The following commit(s) were added to refs/heads/main by this push:
new a00b2fd6 fix(rest): sign the query string the client actually sends
(#821)
a00b2fd6 is described below
commit a00b2fd6eb339c64259663d58558a55cd0edee9e
Author: Jiajia Li <[email protected]>
AuthorDate: Sun Sep 13 20:06:32 2026 +0800
fix(rest): sign the query string the client actually sends (#821)
---
Cargo.lock | 8 +--
DEPENDENCIES.rust.tsv | 1 -
benchmarks/tpcds/DEPENDENCIES.rust.tsv | 1 -
bindings/c/DEPENDENCIES.rust.tsv | 1 -
bindings/go/DEPENDENCIES.rust.tsv | 1 -
bindings/python/DEPENDENCIES.rust.tsv | 1 -
crates/integration_tests/DEPENDENCIES.rust.tsv | 1 -
.../integrations/datafusion/DEPENDENCIES.rust.tsv | 1 -
crates/paimon-rest-server/DEPENDENCIES.rust.tsv | 1 -
crates/paimon/Cargo.toml | 3 +-
crates/paimon/DEPENDENCIES.rust.tsv | 1 -
crates/paimon/src/api/auth/base.rs | 10 ++-
crates/paimon/src/api/auth/dlf_signer.rs | 49 ++++++++++++-
crates/paimon/src/api/resource_paths.rs | 20 +++++-
crates/paimon/src/api/rest_api.rs | 7 +-
crates/paimon/src/api/rest_client.rs | 80 ++++++++++++++++++++++
crates/paimon/src/api/rest_util.rs | 40 +++++++++--
17 files changed, 196 insertions(+), 30 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index f8374af8..e13b4b93 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4692,6 +4692,7 @@ dependencies = [
"paimon-mosaic-core",
"paimon-vindex-core",
"parquet",
+ "percent-encoding",
"pretty_assertions",
"rand 0.8.7",
"regex",
@@ -4713,7 +4714,6 @@ dependencies = [
"typed-builder",
"unicode-segmentation",
"url",
- "urlencoding",
"uuid",
"vortex",
"zstd",
@@ -7380,12 +7380,6 @@ dependencies = [
"serde",
]
-[[package]]
-name = "urlencoding"
-version = "2.1.3"
-source = "registry+https://github.com/rust-lang/crates.io-index";
-checksum = "daf8dba3b7eb870caf1ddeed7bc9d2a049f3cfdfae7cb521b087cc33ae4c49da"
-
[[package]]
name = "utf8-ranges"
version = "1.0.5"
diff --git a/DEPENDENCIES.rust.tsv b/DEPENDENCIES.rust.tsv
index bc275a51..5bb7096b 100644
--- a/DEPENDENCIES.rust.tsv
+++ b/DEPENDENCIES.rust.tsv
@@ -631,7 +631,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected]
X X
[email protected] X
X
[email protected] X
X
diff --git a/benchmarks/tpcds/DEPENDENCIES.rust.tsv
b/benchmarks/tpcds/DEPENDENCIES.rust.tsv
index 0da96087..a58f9739 100644
--- a/benchmarks/tpcds/DEPENDENCIES.rust.tsv
+++ b/benchmarks/tpcds/DEPENDENCIES.rust.tsv
@@ -415,7 +415,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected] X
X
[email protected] X
X
diff --git a/bindings/c/DEPENDENCIES.rust.tsv b/bindings/c/DEPENDENCIES.rust.tsv
index f50b9243..47cb3e4b 100644
--- a/bindings/c/DEPENDENCIES.rust.tsv
+++ b/bindings/c/DEPENDENCIES.rust.tsv
@@ -369,7 +369,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected] X
X
[email protected] X
X
diff --git a/bindings/go/DEPENDENCIES.rust.tsv
b/bindings/go/DEPENDENCIES.rust.tsv
index f50b9243..47cb3e4b 100644
--- a/bindings/go/DEPENDENCIES.rust.tsv
+++ b/bindings/go/DEPENDENCIES.rust.tsv
@@ -369,7 +369,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected] X
X
[email protected] X
X
diff --git a/bindings/python/DEPENDENCIES.rust.tsv
b/bindings/python/DEPENDENCIES.rust.tsv
index f486406b..1957e800 100644
--- a/bindings/python/DEPENDENCIES.rust.tsv
+++ b/bindings/python/DEPENDENCIES.rust.tsv
@@ -546,7 +546,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected]
X X
[email protected] X
X
[email protected] X
X
diff --git a/crates/integration_tests/DEPENDENCIES.rust.tsv
b/crates/integration_tests/DEPENDENCIES.rust.tsv
index 565b00e5..b33de207 100644
--- a/crates/integration_tests/DEPENDENCIES.rust.tsv
+++ b/crates/integration_tests/DEPENDENCIES.rust.tsv
@@ -333,7 +333,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected] X
X
[email protected] X
X
diff --git a/crates/integrations/datafusion/DEPENDENCIES.rust.tsv
b/crates/integrations/datafusion/DEPENDENCIES.rust.tsv
index 413c2dfe..6e690bf9 100644
--- a/crates/integrations/datafusion/DEPENDENCIES.rust.tsv
+++ b/crates/integrations/datafusion/DEPENDENCIES.rust.tsv
@@ -535,7 +535,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected]
X X
[email protected] X
X
[email protected] X
X
diff --git a/crates/paimon-rest-server/DEPENDENCIES.rust.tsv
b/crates/paimon-rest-server/DEPENDENCIES.rust.tsv
index 67a57822..374c700d 100644
--- a/crates/paimon-rest-server/DEPENDENCIES.rust.tsv
+++ b/crates/paimon-rest-server/DEPENDENCIES.rust.tsv
@@ -339,7 +339,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected] X
X
[email protected] X
X
diff --git a/crates/paimon/Cargo.toml b/crates/paimon/Cargo.toml
index dc09b964..a3d116fb 100644
--- a/crates/paimon/Cargo.toml
+++ b/crates/paimon/Cargo.toml
@@ -64,6 +64,8 @@ storage-hdfs = ["dep:opendal-service-hdfs-native"]
[dependencies]
url = "2.5.2"
+# Already in the tree via `url`; direct dep for `RESTUtil::decode_string`.
+percent-encoding = "2.3"
async-trait = "0.1.81"
bytes = "1.7.1"
bitflags = "2.6.0"
@@ -129,7 +131,6 @@ twox-hash = { version = "2.1.3", default-features = false,
features = ["xxhash64
md-5 = "0.10"
regex = "1"
uuid = { version = "1", features = ["v4"] }
-urlencoding = "2.1"
paimon-mosaic-core = "0.2.0"
paimon-ftindex-core = { version = "0.1.0", optional = true }
tempfile = "3"
diff --git a/crates/paimon/DEPENDENCIES.rust.tsv
b/crates/paimon/DEPENDENCIES.rust.tsv
index 62cf71a3..9273ed30 100644
--- a/crates/paimon/DEPENDENCIES.rust.tsv
+++ b/crates/paimon/DEPENDENCIES.rust.tsv
@@ -529,7 +529,6 @@ [email protected] X
X
[email protected] X
X
[email protected]
X
[email protected] X
X
[email protected]
X
[email protected]
X X
[email protected] X
X
[email protected] X
X
diff --git a/crates/paimon/src/api/auth/base.rs
b/crates/paimon/src/api/auth/base.rs
index 444afe1b..edf97c28 100644
--- a/crates/paimon/src/api/auth/base.rs
+++ b/crates/paimon/src/api/auth/base.rs
@@ -21,6 +21,7 @@ use std::collections::HashMap;
use async_trait::async_trait;
+use crate::api::rest_util::RESTUtil;
use crate::Result;
/// Parameter for REST authentication.
@@ -40,6 +41,10 @@ pub struct RESTAuthParameter {
impl RESTAuthParameter {
/// Create a new RESTAuthParameter.
+ ///
+ /// Values are URL-encoded here, so every signer signs the bytes the
request actually
+ /// carries. Signing a raw value while sending the encoded one is a
signature mismatch for
+ /// anything outside `A-Za-z0-9` plus `*`, `-`, `.` and `_`. Keys stay
raw, as in Java.
pub fn new(
method: impl Into<String>,
path: impl Into<String>,
@@ -50,7 +55,10 @@ impl RESTAuthParameter {
method: method.into(),
path: path.into(),
data,
- parameters,
+ parameters: parameters
+ .into_iter()
+ .map(|(key, value)| (key, RESTUtil::encode_string(&value)))
+ .collect(),
}
}
diff --git a/crates/paimon/src/api/auth/dlf_signer.rs
b/crates/paimon/src/api/auth/dlf_signer.rs
index ca49a283..4873b8fd 100644
--- a/crates/paimon/src/api/auth/dlf_signer.rs
+++ b/crates/paimon/src/api/auth/dlf_signer.rs
@@ -85,6 +85,7 @@ use uuid::Uuid;
use super::base::RESTAuthParameter;
use super::dlf_provider::DLFToken;
+use crate::api::rest_util::RESTUtil;
type HmacSha256 = Hmac<Sha256>;
type HmacSha1 = Hmac<Sha1>;
@@ -486,10 +487,10 @@ impl DLFOpenApiSigner {
}
fn build_canonicalized_resource(&self, rest_auth_parameter:
&RESTAuthParameter) -> String {
- let path =
urlencoding::decode(&rest_auth_parameter.path).unwrap_or_default();
+ let path = RESTUtil::decode_string(&rest_auth_parameter.path);
if rest_auth_parameter.parameters.is_empty() {
- return path.to_string();
+ return path;
}
let mut sorted_params: Vec<_> =
rest_auth_parameter.parameters.iter().collect();
@@ -498,7 +499,7 @@ impl DLFOpenApiSigner {
let query_parts: Vec<String> = sorted_params
.iter()
.map(|(key, value)| {
- let decoded_value =
urlencoding::decode(value).unwrap_or_default();
+ let decoded_value = RESTUtil::decode_string(value);
if !decoded_value.is_empty() {
format!("{key}={decoded_value}")
} else {
@@ -648,3 +649,45 @@ impl DLFSignerFactory {
}
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::api::resource_paths::ResourcePaths;
+
+ /// Java signs the decoded path and values, so a `+` on the wire
canonicalises as a space.
+ #[test]
+ fn test_canonicalized_resource_decodes_plus_as_space() {
+ let path = ResourcePaths::new("").database("analytics db");
+ let parameter = RESTAuthParameter::for_get(
+ path,
+ HashMap::from([("principal".to_string(), "a b".to_string())]),
+ );
+
+ let resource =
DLFOpenApiSigner.build_canonicalized_resource(¶meter);
+
+ assert_eq!(resource, "/v1/databases/analytics db?principal=a b");
+ }
+
+ /// `HttpClient::get` takes its path from the caller, so a raw `=` or `&`
has to survive the
+ /// decode rather than truncate the resource that gets signed.
+ #[test]
+ fn test_canonicalized_resource_keeps_raw_delimiters_in_path() {
+ let parameter = RESTAuthParameter::for_get("/v1/a=b/databases/x&y",
HashMap::new());
+
+ let resource =
DLFOpenApiSigner.build_canonicalized_resource(¶meter);
+
+ assert_eq!(resource, "/v1/a=b/databases/x&y");
+ }
+
+ /// The signer signs the decoded path, so an encoded prefix canonicalises
back to its raw form.
+ #[test]
+ fn test_canonicalized_resource_keeps_encoded_prefix() {
+ let path = ResourcePaths::new("clg=paimon").databases();
+ let parameter = RESTAuthParameter::for_get(path, HashMap::new());
+
+ let resource =
DLFOpenApiSigner.build_canonicalized_resource(¶meter);
+
+ assert_eq!(resource, "/v1/clg=paimon/databases");
+ }
+}
diff --git a/crates/paimon/src/api/resource_paths.rs
b/crates/paimon/src/api/resource_paths.rs
index 9acea094..01351956 100644
--- a/crates/paimon/src/api/resource_paths.rs
+++ b/crates/paimon/src/api/resource_paths.rs
@@ -41,7 +41,13 @@ impl ResourcePaths {
let base_path = if prefix.is_empty() {
format!("/{}", Self::V1)
} else {
- format!("/{}/{}", Self::V1, prefix.trim_matches('/'))
+ // Encode the prefix, as Java's `ResourcePaths` constructor does:
it is
+ // server-supplied, and a raw `/`, `?` or `#` would restructure
the URL.
+ format!(
+ "/{}/{}",
+ Self::V1,
+ RESTUtil::encode_string(prefix.trim_matches('/'))
+ )
};
ResourcePaths { base_path }
}
@@ -260,6 +266,18 @@ mod tests {
);
}
+ #[test]
+ fn test_resource_paths_encodes_prefix() {
+ assert_eq!(
+ ResourcePaths::new("clg=paimon").databases(),
+ "/v1/clg%3Dpaimon/databases"
+ );
+ assert_eq!(
+ ResourcePaths::new("clg&paimon").databases(),
+ "/v1/clg%26paimon/databases"
+ );
+ }
+
#[test]
fn test_resource_paths_table() {
let paths = ResourcePaths::new("");
diff --git a/crates/paimon/src/api/rest_api.rs
b/crates/paimon/src/api/rest_api.rs
index 50605895..8cd943c9 100644
--- a/crates/paimon/src/api/rest_api.rs
+++ b/crates/paimon/src/api/rest_api.rs
@@ -143,10 +143,9 @@ impl RESTApi {
});
}
- let query_params: Vec<(&str, String)> = vec![(
- CatalogOptions::WAREHOUSE,
- RESTUtil::encode_string(warehouse),
- )];
+ // Pass the warehouse raw: the client and the signer each encode
it once.
+ let query_params: Vec<(&str, String)> =
+ vec![(CatalogOptions::WAREHOUSE, warehouse.to_string())];
let config_response: ConfigResponse = client
.get(&ResourcePaths::config(), Some(&query_params))
.await?;
diff --git a/crates/paimon/src/api/rest_client.rs
b/crates/paimon/src/api/rest_client.rs
index 1b6a636f..194d7374 100644
--- a/crates/paimon/src/api/rest_client.rs
+++ b/crates/paimon/src/api/rest_client.rs
@@ -276,3 +276,83 @@ impl HttpClient {
})
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::sync::{Arc, Mutex};
+
+ use async_trait::async_trait;
+ use axum::extract::State;
+ use axum::http::Uri;
+ use axum::routing::get;
+ use axum::{Json, Router};
+
+ use super::*;
+ use crate::api::auth::AuthProvider;
+
+ struct RecordingProvider(Arc<Mutex<Option<HashMap<String, String>>>>);
+
+ #[async_trait]
+ impl AuthProvider for RecordingProvider {
+ async fn merge_auth_header(
+ &self,
+ base_header: HashMap<String, String>,
+ parameter: &RESTAuthParameter,
+ ) -> Result<HashMap<String, String>> {
+ *self.0.lock().unwrap() = Some(parameter.parameters.clone());
+ Ok(base_header)
+ }
+ }
+
+ async fn probe(
+ State(query): State<Arc<Mutex<Option<String>>>>,
+ uri: Uri,
+ ) -> Json<serde_json::Value> {
+ *query.lock().unwrap() = uri.query().map(str::to_string);
+ Json(serde_json::json!({}))
+ }
+
+ fn canonical(pairs: impl Iterator<Item = String>) -> String {
+ let mut parts: Vec<String> = pairs.collect();
+ parts.sort();
+ parts.join("&")
+ }
+
+ #[tokio::test]
+ async fn test_query_parameters_are_signed_as_sent() {
+ let sent_query = Arc::new(Mutex::new(None));
+ let app = Router::new()
+ .route("/probe", get(probe))
+ .with_state(sent_query.clone());
+ let listener =
tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
+ let address = listener.local_addr().unwrap();
+ let _server = tokio::spawn(async move { axum::serve(listener,
app).await.unwrap() });
+
+ let signed_params = Arc::new(Mutex::new(None));
+ let auth_function = RESTAuthFunction::new(
+ HashMap::new(),
+ Box::new(RecordingProvider(signed_params.clone())),
+ );
+ let client = HttpClient::new(&format!("http://{address}";),
Some(auth_function)).unwrap();
+
+ client
+ .get::<serde_json::Value>(
+ "/probe",
+ Some(&[("principal", "acs:ram::1:role/Admin"), ("pattern",
"db%")]),
+ )
+ .await
+ .unwrap();
+
+ let signed = signed_params.lock().unwrap().clone().unwrap();
+ let sent = sent_query.lock().unwrap().clone().unwrap();
+
+ assert_eq!(
+ canonical(signed.iter().map(|(key, value)|
format!("{key}={value}"))),
+ canonical(sent.split('&').map(str::to_string)),
+ );
+ assert_eq!(
+ signed.get("principal").map(String::as_str),
+ Some("acs%3Aram%3A%3A1%3Arole%2FAdmin"),
+ );
+ }
+}
diff --git a/crates/paimon/src/api/rest_util.rs
b/crates/paimon/src/api/rest_util.rs
index 5c259fea..20cb982c 100644
--- a/crates/paimon/src/api/rest_util.rs
+++ b/crates/paimon/src/api/rest_util.rs
@@ -30,11 +30,15 @@ impl RESTUtil {
url::form_urlencoded::byte_serialize(value.as_bytes()).collect()
}
- /// URL-decode a string value.
+ /// URL-decode a string value, as Java's `URLDecoder.decode(s, UTF-8)`
does.
+ ///
+ /// Whole-string decoding: `%XX` is resolved and `+` becomes a space,
while `=` and `&` are
+ /// ordinary characters. Java throws on a malformed escape; this decodes
lossily instead,
+ /// so that a signer keeps the stray bytes rather than signing an empty
resource.
pub fn decode_string(encoded: &str) -> String {
- url::form_urlencoded::parse(encoded.as_bytes())
- .map(|(k, _)| k.to_string())
- .collect()
+ percent_encoding::percent_decode(encoded.replace('+', " ").as_bytes())
+ .decode_utf8_lossy()
+ .into_owned()
}
/// Extract all keys with a given prefix from options, returning a new
HashMap with the prefix removed.
@@ -81,6 +85,34 @@ mod tests {
assert_eq!(decoded, original);
}
+ /// Java's `RESTUtil.decodeString` is `URLDecoder.decode`: a whole-string
decoder, in which
+ /// `=` and `&` are ordinary characters rather than separators.
+ #[test]
+ fn test_decode_string_decodes_the_whole_string() {
+ assert_eq!(
+ RESTUtil::decode_string("/v1/a=b/databases/x"),
+ "/v1/a=b/databases/x"
+ );
+ assert_eq!(RESTUtil::decode_string("/v1/x&y/d"), "/v1/x&y/d");
+ assert_eq!(RESTUtil::decode_string("a+b"), "a b");
+ assert_eq!(RESTUtil::decode_string("a%2Fb"), "a/b");
+ assert_eq!(RESTUtil::decode_string("a%2Bb"), "a+b");
+ assert_eq!(
+ RESTUtil::decode_string("%E4%B8%AD%E6%96%87"),
+ "\u{4e2d}\u{6587}"
+ );
+ }
+
+ /// Malformed escapes decode lossily: Java throws, but dropping the rest
of a resource that
+ /// is about to be signed is worse than keeping the stray bytes.
+ #[test]
+ fn test_decode_string_passes_through_malformed_escapes() {
+ assert_eq!(RESTUtil::decode_string("100%"), "100%");
+ assert_eq!(RESTUtil::decode_string("a%zzb"), "a%zzb");
+ assert_eq!(RESTUtil::decode_string("a%2"), "a%2");
+ assert_eq!(RESTUtil::decode_string("a%FFb"), "a\u{FFFD}b");
+ }
+
#[test]
fn test_extract_prefix_map() {
let mut options = Options::new();
