You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "roeap (via GitHub)" <gi...@apache.org> on 2023/12/29 20:44:24 UTC
[PR] feat(object_store): Azure url signing [arrow-rs]
roeap opened a new pull request, #5259:
URL: https://github.com/apache/arrow-rs/pull/5259
# Which issue does this PR close?
Closes #5232
# Rationale for this change
see issue.
# What changes are included in this PR?
Moves request authorization to a new `AzureAuthorizer`, analogous to the AWS implementation. Some structs are made public, such that they can be used independently. Specifically, when going via the `Signer` trait, in each request a new user delegation key is requested. If users want to sign multiple URLs, this is quite inefficient. Thus the `AzureSigner` as well as the new `get_user_delegation_key` method are `pub`, so users can also cover more elaborate signing scenarios efficiently.
# Are there any user-facing changes?
Users can now sign urls also for azure. Supported methods are via access key, or via identity auth with a user delegation key.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1441826623
##########
object_store/src/signer.rs:
##########
@@ -30,5 +30,21 @@ pub trait Signer: Send + Sync + fmt::Debug + 'static {
/// the URL should be valid, return a signed [`Url`] created with the object store
/// implementation's credentials such that the URL can be handed to something that doesn't have
/// access to the object store's credentials, to allow limited access to the object store.
- async fn signed_url(&self, method: Method, path: &Path, expires_in: Duration) -> Result<Url>;
+ async fn signed_url(&self, method: &Method, path: &Path, expires_in: Duration) -> Result<Url>;
Review Comment:
I think I would prefer to avoid making this breaking change, in practice Method should be basically free to clone, and regardless it will be cheaper than the cost of the signing computation
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#issuecomment-1872563118
> Would we want to make AWS look the same
Could do, the sign method isn't currently public so it wouldn't be a breaking change... Although making an async method to do nothing is a bit rubbish, perhaps leave it for now? I don't feel strongly either way
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1438407652
##########
object_store/src/azure/credential.rs:
##########
@@ -24,26 +24,27 @@ use crate::RetryConfig;
use async_trait::async_trait;
use base64::prelude::BASE64_STANDARD;
use base64::Engine;
-use chrono::{DateTime, Utc};
-use reqwest::header::ACCEPT;
-use reqwest::{
- header::{
- HeaderMap, HeaderName, HeaderValue, AUTHORIZATION, CONTENT_ENCODING, CONTENT_LANGUAGE,
- CONTENT_LENGTH, CONTENT_TYPE, DATE, IF_MATCH, IF_MODIFIED_SINCE, IF_NONE_MATCH,
- IF_UNMODIFIED_SINCE, RANGE,
- },
- Client, Method, RequestBuilder,
+use chrono::{DateTime, SecondsFormat, Utc};
+use reqwest::header::{
+ HeaderMap, HeaderName, HeaderValue, ACCEPT, AUTHORIZATION, CONTENT_ENCODING, CONTENT_LANGUAGE,
+ CONTENT_LENGTH, CONTENT_TYPE, DATE, IF_MATCH, IF_MODIFIED_SINCE, IF_NONE_MATCH,
+ IF_UNMODIFIED_SINCE, RANGE,
};
+use reqwest::{Client, Method, Request, RequestBuilder};
use serde::Deserialize;
use snafu::{ResultExt, Snafu};
use std::borrow::Cow;
+use std::collections::HashMap;
+use std::fmt::Debug;
use std::process::Command;
use std::str;
use std::sync::Arc;
use std::time::{Duration, Instant, SystemTime};
use url::Url;
-static AZURE_VERSION: HeaderValue = HeaderValue::from_static("2021-08-06");
+use super::client::UserDelegationKey;
+
+static AZURE_VERSION: HeaderValue = HeaderValue::from_static("2023-11-03");
Review Comment:
Bumping the azure API version should not be breaking any users I think.
##########
object_store/src/azure/client.rs:
##########
@@ -757,8 +817,7 @@ mod tests {
<NextMarker/>
</EnumerationResults>";
- let mut _list_blobs_response_internal: ListResultInternal =
- quick_xml::de::from_str(S).unwrap();
+ let _list_blobs_response_internal: ListResultInternal = quick_xml::de::from_str(S).unwrap();
Review Comment:
drive by: remove unnecessary `mut`
##########
object_store/src/azure/credential.rs:
##########
@@ -137,33 +141,47 @@ pub mod authority_hosts {
pub const AZURE_PUBLIC_CLOUD: &str = "https://login.microsoftonline.com";
}
-pub(crate) trait CredentialExt {
- /// Apply authorization to requests against azure storage accounts
- /// <https://docs.microsoft.com/en-us/rest/api/storageservices/authorize-requests-to-azure-storage>
- fn with_azure_authorization(self, credential: &AzureCredential, account: &str) -> Self;
+/// Authorize a [`Request`] with an [`AzureAuthorizer`]
+#[derive(Debug)]
+pub struct AzureAuthorizer<'a> {
Review Comment:
Initially I wanted this to look more like AWS and I expected more shared code between "regular" url signing for requests and generating SAS tokens.
In practice these functionalities could probably be separate, but kept it to be more in line with AWS.
If users would want to create multiple signed URLs with a user delegated key, they would use `AzureAuthorizer` and previously acquire a user delegated key via the client.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1441939803
##########
object_store/src/azure/credential.rs:
##########
@@ -796,4 +1030,27 @@ mod tests {
&AzureCredential::BearerToken("TOKEN".into())
);
}
+
+ #[tokio::test]
+ async fn test_service_sas() {
Review Comment:
made the test shareable, and moved it to `azure_blob_test`.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#issuecomment-1876690233
Is there anyway I can help move this along, I'd very much like for it to make the release tomorrow
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1441863035
##########
object_store/src/signer.rs:
##########
@@ -30,5 +30,21 @@ pub trait Signer: Send + Sync + fmt::Debug + 'static {
/// the URL should be valid, return a signed [`Url`] created with the object store
/// implementation's credentials such that the URL can be handed to something that doesn't have
/// access to the object store's credentials, to allow limited access to the object store.
- async fn signed_url(&self, method: Method, path: &Path, expires_in: Duration) -> Result<Url>;
+ async fn signed_url(&self, method: &Method, path: &Path, expires_in: Duration) -> Result<Url>;
Review Comment:
reverted.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#issuecomment-1872562636
@tustvold - I really like the proposed `AzureSigner` API, and am about to finish the respective changes.
Would we want to make AWS look the same, even thoght it not the same need over there for repeated signing?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#issuecomment-1877028782
@tustvold - sorry for the delay, will see that I get this done today.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1441830118
##########
object_store/src/azure/credential.rs:
##########
@@ -205,6 +286,156 @@ fn add_if_exists<'a>(h: &'a HeaderMap, key: &HeaderName) -> &'a str {
.unwrap_or_default()
}
+fn string_to_sign_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, String, String, String, String) {
+ let signed_resource = if u
+ .query()
+ .map(|q| q.contains("comp=list"))
Review Comment:
I think lets not support lists for now, we can always add it back later. I'm just thinking about some silliness where a query parameter happens to match this and we do something unintentional
##########
object_store/src/azure/client.rs:
##########
@@ -324,6 +337,74 @@ impl AzureClient {
Ok(())
}
+ /// Make a Get User Delegation Key request
+ /// <https://docs.microsoft.com/en-us/rest/api/storageservices/get-user-delegation-key>
+ async fn get_user_delegation_key(
+ &self,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+ ) -> Result<UserDelegationKey> {
+ let credential = self.get_credential().await?;
+ let url = self.config.service.clone();
+
+ let start = start.to_rfc3339_opts(chrono::SecondsFormat::Secs, true);
+ let expiry = end.to_rfc3339_opts(chrono::SecondsFormat::Secs, true);
+
+ let mut body = String::new();
+ body.push_str("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<KeyInfo>\n");
+ body.push_str(&format!(
+ "\t<Start>{start}</Start>\n\t<Expiry>{expiry}</Expiry>\n"
+ ));
+ body.push_str("</KeyInfo>");
+
+ let response = self
+ .client
+ .request(Method::POST, url)
+ .body(body)
+ .query(&[("restype", "service"), ("comp", "userdelegationkey")])
+ .with_azure_authorization(&credential, &self.config.account)
+ .send_retry(&self.config.retry_config)
+ .await
+ .context(DelegationKeyRequestSnafu)?
+ .bytes()
+ .await
+ .context(DelegationKeyResponseBodySnafu)?;
+
+ let response: UserDelegationKey =
+ quick_xml::de::from_reader(response.reader()).context(DelegationKeyResponseSnafu)?;
+
+ Ok(response)
+ }
+
+ pub async fn signer(&self, expires_in: Duration) -> Result<AzureSigner> {
Review Comment:
This could do with a doc comment explaining what it is/does
##########
object_store/src/signer.rs:
##########
@@ -30,5 +30,21 @@ pub trait Signer: Send + Sync + fmt::Debug + 'static {
/// the URL should be valid, return a signed [`Url`] created with the object store
/// implementation's credentials such that the URL can be handed to something that doesn't have
/// access to the object store's credentials, to allow limited access to the object store.
- async fn signed_url(&self, method: Method, path: &Path, expires_in: Duration) -> Result<Url>;
+ async fn signed_url(&self, method: &Method, path: &Path, expires_in: Duration) -> Result<Url>;
+
+ /// Generate signed urls for multiple paths.
+ ///
+ /// See [`Signer::signed_url`] for more details.
+ async fn signed_urls(
+ &self,
+ method: &Method,
+ paths: &[Path],
+ expires_in: Duration,
+ ) -> Result<Vec<Url>> {
+ let mut urls = Vec::with_capacity(paths.len());
+ for path in paths {
+ urls.push(self.signed_url(method, path, expires_in).await?);
+ }
+ Ok(urls)
+ }
Review Comment:
I think providing a type-erased way to sign multiple URLs makes sense to me :+1:
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1441884008
##########
object_store/src/azure/credential.rs:
##########
@@ -796,4 +1030,27 @@ mod tests {
&AzureCredential::BearerToken("TOKEN".into())
);
}
+
+ #[tokio::test]
+ async fn test_service_sas() {
Review Comment:
I think as written this could cause testing races if run concurrently with azure_blob_test. It is also hard-coding the bucket name. Perhaps we could move it into azure_blob_test?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1441811973
##########
object_store/src/azure/client.rs:
##########
@@ -324,6 +333,45 @@ impl AzureClient {
Ok(())
}
+ /// Make a Get User Delegation Key request
+ /// <https://docs.microsoft.com/en-us/rest/api/storageservices/get-user-delegation-key>
+ pub async fn get_user_delegation_key(
+ &self,
+ start: &DateTime<Utc>,
Review Comment:
this is private now ...
##########
object_store/src/azure/credential.rs:
##########
@@ -137,33 +141,86 @@ pub mod authority_hosts {
pub const AZURE_PUBLIC_CLOUD: &str = "https://login.microsoftonline.com";
}
-pub(crate) trait CredentialExt {
- /// Apply authorization to requests against azure storage accounts
- /// <https://docs.microsoft.com/en-us/rest/api/storageservices/authorize-requests-to-azure-storage>
- fn with_azure_authorization(self, credential: &AzureCredential, account: &str) -> Self;
+pub(crate) struct AzureSigner {
+ signing_key: AzureAccessKey,
Review Comment:
tried to find a way to borrow this, but found no nice solution, as the key is owned by the invoking function that creates the signer.
Since signing URLs would usually not be so high volume, i thought this might be OK.
##########
object_store/src/signer.rs:
##########
@@ -30,5 +30,21 @@ pub trait Signer: Send + Sync + fmt::Debug + 'static {
/// the URL should be valid, return a signed [`Url`] created with the object store
/// implementation's credentials such that the URL can be handed to something that doesn't have
/// access to the object store's credentials, to allow limited access to the object store.
- async fn signed_url(&self, method: Method, path: &Path, expires_in: Duration) -> Result<Url>;
+ async fn signed_url(&self, method: &Method, path: &Path, expires_in: Duration) -> Result<Url>;
+
+ /// Generate signed urls for multiple paths.
+ ///
+ /// See [`Signer::signed_url`] for more details.
+ async fn signed_urls(
+ &self,
+ method: &Method,
+ paths: &[Path],
+ expires_in: Duration,
+ ) -> Result<Vec<Url>> {
+ let mut urls = Vec::with_capacity(paths.len());
+ for path in paths {
+ urls.push(self.signed_url(method, path, expires_in).await?);
+ }
+ Ok(urls)
+ }
Review Comment:
To effectively hide the delegation key shenanigans, I though it may be best to just expose an additional method for signing multiple URLs with a default implementation.
This also introduced a breaking change as we now would take a ref of the `Method`
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1438417509
##########
object_store/src/azure/credential.rs:
##########
@@ -137,33 +141,47 @@ pub mod authority_hosts {
pub const AZURE_PUBLIC_CLOUD: &str = "https://login.microsoftonline.com";
}
-pub(crate) trait CredentialExt {
- /// Apply authorization to requests against azure storage accounts
- /// <https://docs.microsoft.com/en-us/rest/api/storageservices/authorize-requests-to-azure-storage>
- fn with_azure_authorization(self, credential: &AzureCredential, account: &str) -> Self;
+/// Authorize a [`Request`] with an [`AzureAuthorizer`]
+#[derive(Debug)]
+pub struct AzureAuthorizer<'a> {
+ credential: &'a AzureCredential,
+ delegation_key: Option<&'a UserDelegationKey>,
Review Comment:
This feels a little out of place here, as it is only used for URL signing and not the general case of request signing?
##########
object_store/src/azure/client.rs:
##########
@@ -324,6 +333,45 @@ impl AzureClient {
Ok(())
}
+ /// Make a Get User Delegation Key request
+ /// <https://docs.microsoft.com/en-us/rest/api/storageservices/get-user-delegation-key>
+ pub async fn get_user_delegation_key(
+ &self,
+ start: &DateTime<Utc>,
Review Comment:
I think I prefer the approach on AwsAuthorizer where the date is stored on the authorizer and used for testing, but the public methods take an expiry.
##########
object_store/src/azure/credential.rs:
##########
@@ -205,6 +286,156 @@ fn add_if_exists<'a>(h: &'a HeaderMap, key: &HeaderName) -> &'a str {
.unwrap_or_default()
}
+fn string_to_sign_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, String, String, String, String) {
+ let signed_resource = if u
+ .query()
+ .map(|q| q.contains("comp=list"))
+ .unwrap_or_default()
+ {
+ "c"
+ } else {
+ "b"
+ }
+ .to_string();
+
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#permissions-for-a-directory-container-or-blob
+ let signed_permissions = match *method {
+ // read and list permissions
+ Method::GET => match signed_resource.as_str() {
+ "c" => "rl",
+ "b" => "r",
+ _ => unreachable!(),
+ },
+ // write permissions (also allows crating a new blob in a sub-key)
+ Method::PUT => "w",
+ // delete permissions
+ Method::DELETE => "d",
+ // other methods are not used in any of the current operations
+ _ => "",
+ }
+ .to_string();
+ let signed_start = start.to_rfc3339_opts(SecondsFormat::Secs, true);
+ let signed_expiry = end.to_rfc3339_opts(SecondsFormat::Secs, true);
+ let canonicalized_resource = if u.host_str().unwrap_or_default().contains(account) {
+ format!("/blob/{}{}", account, u.path())
+ } else {
+ // NOTE: in case of the emulator, the account name is not part of the host
+ // but the path starts with the account name
+ format!("/blob{}", u.path())
+ };
+
+ (
+ signed_resource,
+ signed_permissions,
+ signed_start,
+ signed_expiry,
+ canonicalized_resource,
+ )
+}
+
+fn string_to_sign_service_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, HashMap<&'static str, String>) {
+ let (signed_resource, signed_permissions, signed_start, signed_expiry, canonicalized_resource) =
+ string_to_sign_sas(u, method, account, start, end);
+
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#version-2020-12-06-and-later
+ let string_to_sign = format!(
+ "{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}\n{}",
+ signed_permissions,
+ signed_start,
+ signed_expiry,
+ canonicalized_resource,
+ "", // signed identifier
+ "", // signed ip
+ "", // signed protocol
+ &AZURE_VERSION.to_str().unwrap(), // signed version
+ signed_resource, // signed resource
+ "", // signed snapshot time
+ "", // signed encryption scope
+ "", // rscc - response header: Cache-Control
+ "", // rscd - response header: Content-Disposition
+ "", // rsce - response header: Content-Encoding
+ "", // rscl - response header: Content-Language
+ "", // rsct - response header: Content-Type
+ );
+
+ let mut pairs = HashMap::new();
+ pairs.insert("sv", AZURE_VERSION.to_str().unwrap().to_string());
+ pairs.insert("sp", signed_permissions);
+ pairs.insert("st", signed_start);
+ pairs.insert("se", signed_expiry);
+ pairs.insert("sr", signed_resource);
+
+ (string_to_sign, pairs)
+}
+
+fn string_to_sign_user_delegation_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+ delegation_key: &UserDelegationKey,
+) -> (String, HashMap<&'static str, String>) {
+ let (signed_resource, signed_permissions, signed_start, signed_expiry, canonicalized_resource) =
+ string_to_sign_sas(u, method, account, start, end);
+
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas#version-2020-12-06-and-later
Review Comment:
Same as above, I think a doc comment is more discoverable
##########
object_store/src/azure/credential.rs:
##########
@@ -205,6 +286,156 @@ fn add_if_exists<'a>(h: &'a HeaderMap, key: &HeaderName) -> &'a str {
.unwrap_or_default()
}
+fn string_to_sign_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, String, String, String, String) {
+ let signed_resource = if u
+ .query()
+ .map(|q| q.contains("comp=list"))
Review Comment:
Does signing a list request even make sense?
##########
object_store/src/azure/client.rs:
##########
@@ -600,6 +648,18 @@ impl BlockList {
}
}
+#[derive(Debug, Clone, PartialEq, Deserialize)]
+#[serde(rename_all = "PascalCase")]
+pub struct UserDelegationKey {
+ pub signed_oid: String,
+ pub signed_tid: String,
+ pub signed_start: String,
+ pub signed_expiry: String,
+ pub signed_service: String,
+ pub signed_version: String,
+ pub value: String,
Review Comment:
Do these fields need to be public? Do we expect users to use this directly?
##########
object_store/src/azure/credential.rs:
##########
@@ -205,6 +286,156 @@ fn add_if_exists<'a>(h: &'a HeaderMap, key: &HeaderName) -> &'a str {
.unwrap_or_default()
}
+fn string_to_sign_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, String, String, String, String) {
+ let signed_resource = if u
+ .query()
+ .map(|q| q.contains("comp=list"))
+ .unwrap_or_default()
+ {
+ "c"
+ } else {
+ "b"
+ }
+ .to_string();
+
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#permissions-for-a-directory-container-or-blob
+ let signed_permissions = match *method {
+ // read and list permissions
+ Method::GET => match signed_resource.as_str() {
+ "c" => "rl",
+ "b" => "r",
+ _ => unreachable!(),
+ },
+ // write permissions (also allows crating a new blob in a sub-key)
+ Method::PUT => "w",
+ // delete permissions
+ Method::DELETE => "d",
+ // other methods are not used in any of the current operations
+ _ => "",
+ }
+ .to_string();
+ let signed_start = start.to_rfc3339_opts(SecondsFormat::Secs, true);
+ let signed_expiry = end.to_rfc3339_opts(SecondsFormat::Secs, true);
+ let canonicalized_resource = if u.host_str().unwrap_or_default().contains(account) {
+ format!("/blob/{}{}", account, u.path())
+ } else {
+ // NOTE: in case of the emulator, the account name is not part of the host
+ // but the path starts with the account name
+ format!("/blob{}", u.path())
+ };
+
+ (
+ signed_resource,
+ signed_permissions,
+ signed_start,
+ signed_expiry,
+ canonicalized_resource,
+ )
+}
+
+fn string_to_sign_service_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, HashMap<&'static str, String>) {
+ let (signed_resource, signed_permissions, signed_start, signed_expiry, canonicalized_resource) =
+ string_to_sign_sas(u, method, account, start, end);
+
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#version-2020-12-06-and-later
Review Comment:
Perhaps we could move this to a doc comments on the method, to make it easier to find
##########
object_store/src/azure/client.rs:
##########
@@ -324,6 +333,45 @@ impl AzureClient {
Ok(())
}
+ /// Make a Get User Delegation Key request
+ /// <https://docs.microsoft.com/en-us/rest/api/storageservices/get-user-delegation-key>
+ pub async fn get_user_delegation_key(
Review Comment:
Note that AzureClient is not public, and so neither is this method. Perhaps we could expose it on AzureAuthorizer?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(object_store): Azure url signing [arrow-rs]
Posted by "roeap (via GitHub)" <gi...@apache.org>.
roeap commented on code in PR #5259:
URL: https://github.com/apache/arrow-rs/pull/5259#discussion_r1438437333
##########
object_store/src/azure/credential.rs:
##########
@@ -205,6 +286,156 @@ fn add_if_exists<'a>(h: &'a HeaderMap, key: &HeaderName) -> &'a str {
.unwrap_or_default()
}
+fn string_to_sign_sas(
+ u: &Url,
+ method: &Method,
+ account: &str,
+ start: &DateTime<Utc>,
+ end: &DateTime<Utc>,
+) -> (String, String, String, String, String) {
+ let signed_resource = if u
+ .query()
+ .map(|q| q.contains("comp=list"))
Review Comment:
hmm... i did think about the more general case where one may just want to share to a full table, by allowing access to a group of keys.
Thinking about it though, I guess keeping the API small and focussing on the sharing access to individual blobs makes sense. All actual use cases I have on my mind would work like that anyhow...
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org