feat(object_store): Azure url signing #5259

roeap · 2023-12-29T20:44:22Z

Which issue does this PR close?

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.

roeap · 2023-12-29T20:50:47Z

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();


drive by: remove unnecessary mut

object_store/src/azure/credential.rs

tustvold

Thank you for this, I'll save a detailed review for when we've got the structure nailed down, but wonder what you think about of instead of making the UserDelegationKey shenanigans public, instead expose an async method on AzureAuthorizer that returns an opaque AzureSigner that can then be used to sign potentially multiple requests.

This would have a few advantages imo:

Avoids users needing to understand what a UserDelegationKey is, and when/why they would want one
Keeps the implementation details private
Preserves the spirit of the design of AwsAuthorizer
Makes it obvious how to sign multiple requests with the same set of credentials

Perhaps something like:

pub struct AzureAuthorizer<'a> {
    credential: &'a AzureCredential,
    date: Option<DateTime<Utc>>,
    account: &'a str,
}


impl AzureAuthorizer<'a> {
    pub fn new(
        credential: &'a AzureCredential,
        account: &'a str,
    ) -> Self {
        AzureAuthorizer {
            credential,
            account,
            date: None,
        }
    }


    pub fn authorize(&self, request: &mut Request) {
        ...
    }

    pub async fn signer(&self, expires_in: Duration) -> Result<AzureSigner<'_>> {
        ...
    }

}

pub struct AzureSigner<'a> {
    credential: AzureSignerCredential<'a>
    authorizer: &'a AzureAuthorizer<'a>,
}

enum AzureSignerCredential<'a> {
    AccessKey(&'a AzureAccessKey),
    UserDelegationKey(...)
}

impl AzureSigner<'a> {
    pub fn sign(&self, method: Method, path: &Path, expires_in: Duration) -> Result<Url> {
        ...
    }
}
```

tustvold · 2023-12-29T21:35:41Z

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(


Note that AzureClient is not public, and so neither is this method. Perhaps we could expose it on AzureAuthorizer?

tustvold · 2023-12-29T21:36:06Z

object_store/src/azure/client.rs

+    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,


Do these fields need to be public? Do we expect users to use this directly?

object_store/src/azure/credential.rs

object_store/src/azure/client.rs

roeap · 2023-12-30T16:43:55Z

@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?

tustvold · 2023-12-30T16:46:49Z

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

tustvold · 2024-01-04T08:22:15Z

Is there anyway I can help move this along, I'd very much like for it to make the release tomorrow

roeap · 2024-01-04T12:36:04Z

@tustvold - sorry for the delay, will see that I get this done today.

roeap

@tustvold - there probably are things to clean up, but since we want to get this in I though, getting earlier feedback is better :).

object_store/src/azure/client.rs

roeap · 2024-01-04T14:23:33Z

object_store/src/signer.rs

+    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)
+    }


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

I think providing a type-erased way to sign multiple URLs makes sense to me 👍

roeap · 2024-01-04T14:26:10Z

object_store/src/azure/credential.rs

-    /// <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,


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

tustvold

Looks good to me, just some minor comments

object_store/src/azure/client.rs

tustvold · 2024-01-04T14:30:50Z

object_store/src/signer.rs

+    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)
+    }


I think providing a type-erased way to sign multiple URLs makes sense to me 👍

object_store/src/azure/credential.rs

tustvold · 2024-01-04T15:19:10Z

object_store/src/azure/credential.rs

@@ -796,4 +1030,27 @@ mod tests {
            &AzureCredential::BearerToken("TOKEN".into())
        );
    }
+
+    #[tokio::test]
+    async fn test_service_sas() {


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?

made the test shareable, and moved it to azure_blob_test.

Robert Pack added 2 commits December 23, 2023 15:40

refactor: move current signing to new AzureAuthrizer

2684c3d

feat: generate signed urls with master key

1c0f33b

github-actions bot added the object-store Object Store Interface label Dec 29, 2023

feat: sign with user delegated keys

5d68d75

roeap force-pushed the azure-sign-url branch from 13f4865 to 5d68d75 Compare December 29, 2023 20:47

roeap commented Dec 29, 2023

View reviewed changes

chore: clippy

156ab15

roeap mentioned this pull request Dec 29, 2023

Add support for generating Azure Blob Storage signed URLs delta-incubator/delta-sharing-rs#22

Merged

tustvold reviewed Dec 29, 2023

View reviewed changes

roeap force-pushed the azure-sign-url branch from 475219a to f0e6428 Compare January 4, 2024 14:17

pr feedback

f8aa334

roeap force-pushed the azure-sign-url branch from f0e6428 to f8aa334 Compare January 4, 2024 14:19

roeap commented Jan 4, 2024

View reviewed changes

chore: clippy

9ccc92b

tustvold reviewed Jan 4, 2024

View reviewed changes

object_store/src/signer.rs Outdated Show resolved Hide resolved

tustvold approved these changes Jan 4, 2024

View reviewed changes

pr feedback II

82d3bd0

tustvold reviewed Jan 4, 2024

View reviewed changes

fix: move sigining test

60f1bf6

tustvold merged commit 2f383e7 into apache:master Jan 4, 2024
13 checks passed

roeap deleted the azure-sign-url branch January 4, 2024 16:51

tustvold mentioned this pull request Jan 5, 2024

Azure Signed URL Support #5232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(object_store): Azure url signing #5259

feat(object_store): Azure url signing #5259

roeap commented Dec 29, 2023

roeap Dec 29, 2023

tustvold left a comment •

edited

Loading

tustvold Dec 29, 2023

tustvold Dec 29, 2023

roeap commented Dec 30, 2023

tustvold commented Dec 30, 2023

tustvold commented Jan 4, 2024

roeap commented Jan 4, 2024

roeap left a comment

roeap Jan 4, 2024

tustvold Jan 4, 2024

roeap Jan 4, 2024

tustvold left a comment

tustvold Jan 4, 2024

tustvold Jan 4, 2024

roeap Jan 4, 2024

feat(object_store): Azure url signing #5259

feat(object_store): Azure url signing #5259

Conversation

roeap commented Dec 29, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap commented Dec 30, 2023

tustvold commented Dec 30, 2023

tustvold commented Jan 4, 2024

roeap commented Jan 4, 2024

roeap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold left a comment •

edited

Loading