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

Add support for identity SAS #153

Merged
merged 3 commits into from
Mar 9, 2020
Merged

Add support for identity SAS #153

merged 3 commits into from
Mar 9, 2020

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Jan 24, 2020

Since API version 2018-11-09, we can use Azure Active Directory credentials to authenticate with storage services which is crucial for enterprise and multi-tenancy use-cases. This pull request adds support for this functionality.

Relevant supporting content:

When running the integration tests locally, I noticed that a few of them fail with this branch. However, several of the integration tests also fail for me with the current master branch. Any help that you could provide with regards to working on the integration tests would be much appreciated.

This change also adds a new integration test for the identity SAS which requires three new environment variables to be set: AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET. These environment variables specify the Azure Active Directory identity to use for the new integration test. Note that the identity must be granted the "Storage Blob Data Contributor" and "Storage Blob Delegator" permissions on the storage account. The following bash script can be used to set up the required Azure Active Directory configuration and environment variables:

## create service principal for integration tests

service_principal_name="azure-storage-ruby-integtest"  # CHANGE ME

sp="$(az ad sp create-for-rbac --name "${service_principal_name}" --skip-assignment -o json)"
sp_tenant="$(jq -r '.tenant' <<< "${sp}")"
sp_client="$(jq -r '.appId' <<< "${sp}")"
sp_secret="$(jq -r '.password' <<< "${sp}")"

## assign permissions to service principal

storage_resource_id="$(az storage account show --name "${AZURE_STORAGE_ACCOUNT}" -o json | jq -r '.id')"

while ! az role assignment create --role "Storage Blob Data Contributor" --assignee "${sp_client}" --scope "${storage_resource_id}"; do sleep 2s; done

while ! az role assignment create --role "Storage Blob Delegator" --assignee "${sp_client}" --scope "${storage_resource_id}"; do sleep 2s; done

## export environment variables required for integration tests

export AZURE_TENANT_ID="${sp_tenant}"
export AZURE_CLIENT_ID="${sp_client}"
export AZURE_CLIENT_SECRET="${sp_secret}"

@coveralls
Copy link

coveralls commented Jan 24, 2020

Coverage Status

Coverage increased (+0.04%) to 87.092% when pulling 04e0917 on c-w:user-delegation-key into 5cf7a82 on Azure:dev.

@yaxia yaxia self-assigned this Feb 4, 2020
@yaxia yaxia changed the base branch from master to dev February 4, 2020 04:07
@yaxia
Copy link
Member

yaxia commented Mar 4, 2020

We merged the code of azure-ruby-asm-core and fixed the merge validation. Please resolve the conflicts on the latest code.
There are also several comments:

  1. Any scenario that you require the minimum Ruby version to v2.4?
  2. As the API version is upgrade to 2018-11-09, there are some server behavior changes. @katmsft will work on the mitigation based on this PR.
  3. Since the api version is bumped up from 2017-11-09, we don't have a timeline to support all the feature in 2018-11-09 and 2018-03-28.

PS: there string-to-sign in the doc: Create a user delegation SAS was wrong. I've sent a PR to fix it. If you already have code to generate the delegation SAS, please modify it accordingly (no signed identifier, singed resource type and signed blob snapshot time should be added).

Copy link
Member

@katmsft katmsft left a comment

Choose a reason for hiding this comment

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

My major concern was about dropping ruby version 2.3.0 and the version bumps up (2.0.0 instead of 1.2.0). Please rebase to the latest dev branch and change them. Thanks!
Also, we can only use target testing to verify the E2E scenario for user delegation key, please confirm in the thread that this is already covered. Thanks for your contribution!

.travis.yml Outdated
- 2.2.2
- 2.4.1
- 2.5.0
- 2.4
Copy link
Member

Choose a reason for hiding this comment

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

Why 2.3 is also dropped? Can we add it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped support for 2.3 since it's been end of life since 2019-03-31. I will add back the version.

Copy link
Contributor Author

@c-w c-w Mar 5, 2020

Choose a reason for hiding this comment

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

Added back support for 2.3 in 04e0917.

@@ -1,3 +1,7 @@
Tracking Breaking Changes in 1.2.0

* This module now supports Ruby versions to 2.4 through 2.7
Copy link
Member

Choose a reason for hiding this comment

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

2.3 would be still in support matrix for latest Nokogiri.

Copy link
Contributor Author

@c-w c-w Mar 5, 2020

Choose a reason for hiding this comment

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

Added back support for 2.3 in 04e0917.

@@ -1,3 +1,7 @@
2020.2 - version 1.2.0
* This module now supports Ruby versions to 2.4 through 2.7
Copy link
Member

Choose a reason for hiding this comment

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

Also, 2.3 :)

Copy link
Contributor Author

@c-w c-w Mar 5, 2020

Choose a reason for hiding this comment

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

Added back support for 2.3 in 04e0917.

blob/README.md Outdated
@@ -9,12 +9,12 @@ This project provides a Ruby package that makes it easy to access and manage Mic

# Supported Ruby Versions

* Ruby 1.9.3 to 2.5
* Ruby 2.4 to 2.7
Copy link
Member

Choose a reason for hiding this comment

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

2.3 :)

Copy link
Contributor Author

@c-w c-w Mar 5, 2020

Choose a reason for hiding this comment

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

Added back support for 2.3 in 04e0917.

@@ -38,10 +38,10 @@ Gem::Specification.new do |s|
s.license = "MIT"
s.files = `git ls-files ./lib/azure/storage/blob/`.split("\n") << "./lib/azure/storage/blob.rb"

s.required_ruby_version = ">= 1.9.3"
s.required_ruby_version = ">= 2.4.0"
Copy link
Member

Choose a reason for hiding this comment

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

2.3

Copy link
Contributor Author

@c-w c-w Mar 5, 2020

Choose a reason for hiding this comment

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

Added back support for 2.3 in 04e0917.


s.add_runtime_dependency("azure-core", "~> 0.1.13")
s.add_runtime_dependency("azure-storage-common", "~> 1.0")
s.add_runtime_dependency("azure-storage-common", "~> 1.2")
Copy link
Member

Choose a reason for hiding this comment

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

Because this is breaking change, common will have to bump up to 2.0.0 among all other services.

Copy link
Contributor Author

@c-w c-w Mar 5, 2020

Choose a reason for hiding this comment

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

Bumped versions to 2.0.0 in 04e0917.

@c-w
Copy link
Contributor Author

c-w commented Mar 5, 2020

@katmsft

My major concern was about dropping ruby version 2.3.0 and the version bumps up (2.0.0 instead of 1.2.0). Please rebase to the latest dev branch and change them. Thanks!

I updated the pull request to use Ruby 2.3 as the minimum version and bumped the versions of azure-storage-blob and azure-storage-common to 2.0.0

Also, we can only use target testing to verify the E2E scenario for user delegation key, please confirm in the thread that this is already covered. Thanks for your contribution!

For testing, test/integration/auth/user_delegation_key_shared_access_signature_test.rb implements and end-to-end test that reaches out to a real Azure Storage account to create a shared access signature via user delegation key. The test worked when I submitted this pull request. Could you confirm that the test accurately reflects the new functionality and that it's working during your validation?

You can run the test by setting the environment variables AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET in addition to AZURE_STORAGE_ACCOUNT and AZURE_STORAGE_ACCESS_KEY required by the rest of the integration tests.

@yaxia

We merged the code of azure-ruby-asm-core and fixed the merge validation. Please resolve the conflicts on the latest code.

Done. I rebased with the Azure:dev branch and force-pushed to this branch.

Any scenario that you require the minimum Ruby version to v2.4?

I dropped support for 2.3 since it's been end of life since 2019-03-31. I can add back the version since the code should still work on 2.3.

As the API version is upgrade to 2018-11-09, there are some server behavior changes.

As per the comment in common/lib/azure/storage/common/default.rb, the API version upgrade in this pull request should only affect the SAS generator. Is it necessary to also update all the other versions or would it be possible to stay on 2017-11-09 for everything and only use 2018-11-09 for SAS generation?

The string-to-sign in the doc: Create a user delegation SAS was wrong. If you already have code to generate the delegation SAS, please modify it accordingly (no signed identifier, singed resource type and signed blob snapshot time should be added).

Since the docs were incorrect when I was working on this, I adapted the implementation of the signature from the Python SDK (see code) so the code in this pull request should already be correct.

Copy link
Member

@katmsft katmsft left a comment

Choose a reason for hiding this comment

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

LGTM except the blob version in default.rb. This will be fixed after the PR being merged.

@vinjiang vinjiang merged commit 252e3f0 into Azure:dev Mar 9, 2020
@c-w c-w deleted the user-delegation-key branch March 9, 2020 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants