Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
alamb commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2047362742
##
src/client/builder.rs:
##
@@ -283,4 +293,38 @@ mod tests {
"https://f...@example.com/bananas?foo=1&bingo=foo&auth=test&t1=funky+shenanigans&a=%F0%9F%98%80";
);
}
+
+#[test]
+fn test_add_query_pairs_no_path() {
+let mut uri = Uri::from_static("https://f...@example.com";);
+add_query_pairs(&mut uri, [("foo", "1")]);
+assert_eq!(uri.to_string(), "https://f...@example.com/?foo=1";);
+}
+
+#[test]
+fn test_request_builder_query() {
Review Comment:
Thank you @AdamGS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
crepererum merged PR #334: URL: https://github.com/apache/arrow-rs-object-store/pull/334 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
AdamGS commented on PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#issuecomment-2809427840
I did, checked with 0.11.2 (works), 0.12 errors with:
```
Generic { store: "MicrosoftAzure", source: ListRequest { source: RetryError
{ method: GET, uri:
Some(https://vortexicebergsummit25.blob.core.windows.net/tpch?&restype=container&comp=list),
retries: 0, max_retries: 10, elapsed: 358.050375ms, retry_timeout: 180s,
inner: Status { status: 403, body: Some("\u{feff}AuthenticationFailedServer
failed to authenticate the request. Make sure the value of Authorization header
is formed correctly including the
signature.\nRequestId:e6bb31fc-801e-0065-80ca-aed8ec00\nTime:2025-04-16T12:27:08.3067099ZThe
MAC signature found in the HTTP request
'7hFfzazKKMmXmDkJpR9n28xZXybWPQYbFDnX580Ey2g=' is not the same as any computed
signature. Server used following string to sign: 'GET\n\n\n\n\n\nWed, 16 Apr
2025 12:27:07
GMT\n\n\n\n\n\nx-ms-version:2023-11-03\n/vortexicebergsummit25/tpch\n:\ncomp:list\nrestype:container'.") } } } }
```
and if I patch it to use this PR it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
crepererum commented on PR #334: URL: https://github.com/apache/arrow-rs-object-store/pull/334#issuecomment-2809151068 I might have missed this, but can someone test manually that this fixes the actual bug by using a real azure account? Maybe you already did that, in which case I just want a confirmation that this was the case. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
AdamGS commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2046482455
##
src/client/builder.rs:
##
@@ -252,12 +252,18 @@ where
let mut out = match parts.path_and_query {
Some(p) => match p.query() {
-Some(x) => format!("{}?{}", p.path(), x),
+Some(query) => format!("{}?{}", p.path(), query),
None => format!("{}?", p.path()),
},
None => "/?".to_string(),
};
-let mut serializer = form_urlencoded::Serializer::new(&mut out);
+let mut serializer = if out.ends_with('?') {
+let start_position = out.len();
+form_urlencoded::Serializer::for_suffix(&mut out, start_position)
+} else {
+form_urlencoded::Serializer::new(&mut out)
+};
Review Comment:
This code path had the same kind of bug, modified the relevant test to catch
it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
AdamGS commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2046482455
##
src/client/builder.rs:
##
@@ -252,12 +252,18 @@ where
let mut out = match parts.path_and_query {
Some(p) => match p.query() {
-Some(x) => format!("{}?{}", p.path(), x),
+Some(query) => format!("{}?{}", p.path(), query),
None => format!("{}?", p.path()),
},
None => "/?".to_string(),
};
-let mut serializer = form_urlencoded::Serializer::new(&mut out);
+let mut serializer = if out.ends_with('?') {
+let start_position = out.len();
+form_urlencoded::Serializer::for_suffix(&mut out, start_position)
+} else {
+form_urlencoded::Serializer::new(&mut out)
+};
Review Comment:
This code path had the same kind of "bug", modified the relevant test to
catch it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
alamb commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2045487830
##
src/client/builder.rs:
##
@@ -183,7 +183,8 @@ impl HttpRequestBuilder {
let mut error = None;
if let Ok(ref mut req) = self.request {
let mut out = format!("{}?", req.uri().path());
-let mut encoder = form_urlencoded::Serializer::new(&mut out);
+let start_position = out.len();
Review Comment:
Maybe a unit test then (aka assert what URL is sent)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
alamb commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2045487830
##
src/client/builder.rs:
##
@@ -183,7 +183,8 @@ impl HttpRequestBuilder {
let mut error = None;
if let Ok(ref mut req) = self.request {
let mut out = format!("{}?", req.uri().path());
-let mut encoder = form_urlencoded::Serializer::new(&mut out);
+let start_position = out.len();
Review Comment:
Maybe a unit test then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
AdamGS commented on PR #334: URL: https://github.com/apache/arrow-rs-object-store/pull/334#issuecomment-2807303161 merged `main` so we can run tests here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
AdamGS commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2045308481
##
src/client/builder.rs:
##
@@ -183,7 +183,8 @@ impl HttpRequestBuilder {
let mut error = None;
if let Ok(ref mut req) = self.request {
let mut out = format!("{}?", req.uri().path());
-let mut encoder = form_urlencoded::Serializer::new(&mut out);
+let start_position = out.len();
Review Comment:
yeah, after this change the URL will again be
`azurebase.com/path?comp=list&restype:container`.
Part of the problem here is that I realized that it seems like [Azurite
behaves differently](https://github.com/Azure/Azurite/issues/2548) than Azure
in this case, so tests that will fail on the real Azure (like @a10y's original
repro) would pass in the repo's test suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
alamb commented on PR #334: URL: https://github.com/apache/arrow-rs-object-store/pull/334#issuecomment-2807250737 FYI @crepererum and @tustvold since you approved / authored https://github.com/apache/arrow-rs/pull/7183 I would appreciate it if you could also give this PR a look -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix query parameter signing in Azure [arrow-rs-object-store]
alamb commented on code in PR #334:
URL:
https://github.com/apache/arrow-rs-object-store/pull/334#discussion_r2045293398
##
src/client/builder.rs:
##
@@ -183,7 +183,8 @@ impl HttpRequestBuilder {
let mut error = None;
if let Ok(ref mut req) = self.request {
let mut out = format!("{}?", req.uri().path());
-let mut encoder = form_urlencoded::Serializer::new(&mut out);
+let start_position = out.len();
Review Comment:
> [apache/arrow-rs#7183](https://github.com/apache/arrow-rs/pull/7183)
Changed the way the query string is serialized from:
>
> ```
> azurebase.com/path?comp=list&restype:container
> ```
>
> to:
>
> ```
> azurebase.com/path?&comp=list&restype:container # Note the leading & after
the ?
> ```
Does this change remove the `&` after `?`. I am having a hard time
reasoning about the change this behavior makes.
Is it possible to write some unit tests or something that can show the
before/after behavior (and prevent this code from being accidentally broken
again)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
