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

Provider authentication to blob using Azure AD #3651

Closed
omerlh opened this issue Jun 13, 2019 · 11 comments · Fixed by #5614
Closed

Provider authentication to blob using Azure AD #3651

omerlh opened this issue Jun 13, 2019 · 11 comments · Fixed by #5614

Comments

@omerlh
Copy link
Contributor

omerlh commented Jun 13, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Azure Storage now supports authentication using Azure AD, in addition to authentication with a SAS token or access keys. The provider also supports authentication with Azure AD service principal, but look like it's using the credentials to get access keys, and then use them to access the storage. I noticed that when I tried to use SP credentials and encountered the following error:

Error retrieving keys for Storage Account "<>": storage.AccountsClient#ListKeys: Failure responding to request: StatusCode=403

It happened because I granted the SP contributor role to the blob, not to the storage (least privilege!). It was solved once I granted it a contributor role to the entire storage account.

Please consider support authentication with AD credentials.

@tombuildsstuff
Copy link
Contributor

hey @omerlh

Thanks for opening this issue :)

I'm currently working on replacing/writing our own Storage SDK we use to communicate with the Storage API's - so that we can move forward on some other issues; a side-effect of this is that this SDK now supports authenticating using Azure AD instead; which we'll be using in future versions of the Provider. Whilst I don't have a timeline for this, this should form a part of the 2.0 release for the Storage Services which support authenticating using Azure AD; as the other storage services switch to supporting this in the future I expect we'll also migrate them over to using AzureAD for authentication too

Thanks!

@omerlh
Copy link
Contributor Author

omerlh commented Jun 16, 2019

Looking forward to try it out!

@mud5150
Copy link

mud5150 commented Aug 21, 2019

@tombuildsstuff Our developers use a read only service principal when running plan locally. We are also running into this issue since these SPs don't have access to the storage account keys. This is intentional as I wouldn't want to grant these accounts access to retrieve credentials for every storage account. Is there a use case that requires listkeys during the plan phase?

@tombuildsstuff
Copy link
Contributor

@mud5150 running terraform plan means that Terraform is going to refresh the state of the resource; which since this resource exposes the keys means the refresh will load those (hence this error).

In addition Azure counts the List Account Keys operation as a Write operation, which means these fail when users only have Read permissions (upstream issue)

Whilst Terraform /could/ return empty strings if the user doesn't have permission to the Access Keys - it currently doesn't; but it may make sense to do this - if you're looking for this functionality would you mind opening a new issue specifically for that?

Thanks!

@tombuildsstuff
Copy link
Contributor

👋

After spending some time with Azure AD authentication for Storage - chatting internally we believe this would make sense as an optional toggle rather than something the provider defaults to (instead continuing to default to using a SharedKey for authentication).

This is primarily since it requires non-standard permissions (e.g. users with the Owner/Contributor roles are unable to access this data, which is confusing) - as such we believe an optional toggle of some sort (either in the config, or an environment variable) is the best way to allow both mechanisms to co-exist.

Thanks!

@mud5150
Copy link

mud5150 commented Aug 22, 2019

@tombuildsstuff Thanks. I created #4138 to address the access issue.

@drdamour
Copy link
Contributor

drdamour commented Jan 6, 2020

couldn't this be fall thru behaviour similar to the portal.. try keys first, ad next?

@tombuildsstuff
Copy link
Contributor

@drdamour unfortunately that's harder than it may first appear due to the way the API/SDK works - given these permissions haven't been granted retroactively, I believe a toggle is the best way to approach this in the interim (at least until these permissions are more widespread). In addition attempting to obtain the keys prior to AAD would show as a failed event in the Azure (Security) Events system which isn't ideal when we could instead try AAD/Key auth directly

@tombuildsstuff tombuildsstuff self-assigned this Feb 4, 2020
tombuildsstuff added a commit that referenced this issue Feb 4, 2020
This adds support for authenticating to the Blob & Queue Storage API's
using AzureAD authentication via a Feature Toggle on the Provider block.

Fixes #3651
@tombuildsstuff tombuildsstuff modified the milestones: v2.0.0, v1.44.0 Feb 6, 2020
tombuildsstuff added a commit that referenced this issue Feb 10, 2020
This adds support for authenticating to the Blob & Queue Storage API's
using AzureAD authentication via a Feature Toggle on the Provider block.

Fixes #3651
@ghost
Copy link

ghost commented Feb 12, 2020

This has been released in version 1.44.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.44.0"
}
# ... other configuration ...

@drdamour
Copy link
Contributor

@tombuildsstuff should this work with remote state too?

@ghost
Copy link

ghost commented Mar 13, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants