Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(object_store): Azure url signing #5259

Merged
merged 8 commits into from
Jan 4, 2024
Merged

Conversation

roeap
Copy link
Contributor

@roeap roeap commented Dec 29, 2023

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.

@github-actions github-actions bot added the object-store Object Store Interface label Dec 29, 2023
@@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive by: remove unnecessary mut

object_store/src/azure/credential.rs Show resolved Hide resolved
object_store/src/azure/credential.rs Show resolved Hide resolved
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {
        ...
    }
}
```

@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +654 to +660
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

object_store/src/azure/credential.rs Outdated Show resolved Hide resolved
object_store/src/azure/credential.rs Outdated Show resolved Hide resolved
object_store/src/azure/credential.rs Outdated Show resolved Hide resolved
object_store/src/azure/credential.rs Outdated Show resolved Hide resolved
object_store/src/azure/client.rs Show resolved Hide resolved
@roeap
Copy link
Contributor Author

roeap commented Dec 30, 2023

@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
Copy link
Contributor

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
Copy link
Contributor

tustvold commented Jan 4, 2024

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

@roeap
Copy link
Contributor Author

roeap commented Jan 4, 2024

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

Copy link
Contributor Author

@roeap roeap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 Show resolved Hide resolved
Comment on lines 33 to 49
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)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// <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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just some minor comments

object_store/src/azure/client.rs Show resolved Hide resolved
Comment on lines 33 to 49
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

object_store/src/azure/credential.rs Outdated Show resolved Hide resolved
@@ -796,4 +1030,27 @@ mod tests {
&AzureCredential::BearerToken("TOKEN".into())
);
}

#[tokio::test]
async fn test_service_sas() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made the test shareable, and moved it to azure_blob_test.

@tustvold tustvold merged commit 2f383e7 into apache:master Jan 4, 2024
13 checks passed
@roeap roeap deleted the azure-sign-url branch January 4, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Signed URL Support
2 participants