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

Feature Request: support for Shared Access Signatures on Storage Objects [AzureRM] #59

Closed
hashibot opened this issue Jun 13, 2017 · 16 comments · Fixed by #1011
Closed

Comments

@hashibot
Copy link

This issue was originally opened by @amcguign as hashicorp/terraform#13451. It was migrated here as part of the provider split. The original body of the issue is below.


In order to fufill our audit requirements we need to ensure that all Azure Blob storage logs are being ingested into Splunk.

After some investigation we believe SAS tokens are the most suitable way to do this. "Using a shared access signature (SAS) is a powerful way to grant limited access to objects in your storage account to other clients, without having to expose your account key." -- https://docs.microsoft.com/en-us/azure/storage/storage-dotnet-shared-access-signature-part-1

@pixelicous
Copy link

+1... still nothing..?

@dcherniv
Copy link

+1

@jzampieron
Copy link
Contributor

I've needed this as well for a while. It's a huge missing feature.

We used an "external" provider and called into the Azure CLI, which is just kind of half-baked and fragile.

I'm going to try to implement this and PR it in the next few days.

@tombuildsstuff : Some design thoughts for discussion:

  1. Should be a "resource" type.
  2. Name "azurerm_storage_sas" ?
  3. Mock Example:
resource "azurerm_storage_sas" "sas_for_metrics" {
  account_id = "${azurerm_storage_account.account_for_metrics.id}"
  https_only = true
  resource_types = "co"
  services = "b"
  start = "<startdate>"
  expiry = "<expiration date>"
  permissions = "rwac"
}

All the parameters values mirror the expected parameters to the Azure CLI.
e.g. az storage account generate-sas --https --permissions rwac --connection-string "${CONN}" --start ${START} --resource-types co --services b --expiry ${EXPIRY} --output json

@tombuildsstuff
Copy link
Contributor

@jzampieron

I'm going to try to implement this and PR it in the next few days.

awesome :)

one thing to be aware of is if you need to vendor the SDK - we're mid-way through migrating from SDKv12.5 -> SDKv14.5 - it may be simpler to hold off a few days until this is completed (to avoid any merge conflicts), but I'll leave that to you :)

Some design thoughts for discussion:

Should be a "resource" type.

This probably belongs as a Data Source rather than a Resource, since it's lifecycle is managed outside of Terraform (e.g. what happens when it expires)

Name "azurerm_storage_sas" ?

I'd suggest this becomes azurerm_storage_access_token - what do you think?

Mock Example:

I'm unfamiliar with this functionality, so you're probably better at guiding this; my guess would be it may make more sense as:

resource "azurerm_storage_sas" "sas_for_metrics" {
  storage_account_id = "${azurerm_storage_account.account_for_metrics.id}"
  services {
    foo = true
  }
  permissions {
    read = true
    write = false
  }
}

Whilst splitting services and permissions out into separate blocks is different from the API, it makes it clearer for users, with each value in the permissions and services blocks defaulted to false - what do you think?

@jzampieron
Copy link
Contributor

The start of the PR is here (still very much WIP):
#1011

I figured we'd adjust names and type (resource vs data source) as we go.

The important part is that I seem to have the connection string parse, signature computation and query string generation code working.

The rest is just terraform glue code.

There is no real dependence on the Azure SDK at all. It's simply Go string functions and standard libraries, so the SDK update in process shouldn't affect it at all.

@jzampieron
Copy link
Contributor

I did change the name to azurerm_storage_account_sas because apparently there are different types of SAS depending on the resource type they apply to... You can have "container" SAS or "Account" SAS or "Object" SAS and they are generated differently:

See: https://docs.microsoft.com/en-us/rest/api/storageservices/constructing-an-account-sas -> Account SAS
vs
Service SAS: https://docs.microsoft.com/en-us/rest/api/storageservices/constructing-a-service-sas

I agree that maybe a data source is better... since they are completely dependent on the storage account existing and they can expire.

@jzampieron
Copy link
Contributor

@pixelicous @dcherniv I'd encourage you to review the PR #1011 that I've opened to see if the design meets your desired DX.
It will work for my purposes, interested in broader input.

@phekmat
Copy link
Contributor

phekmat commented Mar 21, 2018

I hacked up some Python in an external provider to achieve something similar. It looks possible to use the Azure SDK to do the heavy lifting of generating the SAS (https://github.com/Azure/azure-sdk-for-go/blob/v14.5.0/storage/client.go#L529), though it'd be nice if it wasn't coupled to the Client

@jzampieron
Copy link
Contributor

jzampieron commented Mar 22, 2018

The implementation in the aforementioned PR is straight Go code as part of the terraform provider. There's no use of the Azure SDK.

@phekmat
Copy link
Contributor

phekmat commented Mar 22, 2018

I meant as an alternative to reimplementing it, as in the current PR. I don't have a strong opinion on using the SDK or not since the code is relatively straightforward.

@jzampieron
Copy link
Contributor

@tombuildsstuff Have you had a chance to review the PR? It should be in pretty good shape.

@tombuildsstuff
Copy link
Contributor

@jzampieron apologies for the delayed review here - I'll be taking a look shortly :)

@jzampieron
Copy link
Contributor

@tombuildsstuff Just checking in to see where we are here. Would be good to get this feature in. It's handy to be able to pass SAS into next-level provisioning.

@tombuildsstuff
Copy link
Contributor

@jzampieron I took another look through the PR yesterday: #1011 (review) (apologies on the delay looking at that) - on the whole this looks pretty good, if we can sort out the minor issues identified in the comments that PR should be good to merge :)

@achandmsft achandmsft added this to the Soon milestone Apr 20, 2018
@katbyte katbyte modified the milestones: Soon, 1.6.0 May 25, 2018
@katbyte
Copy link
Collaborator

katbyte commented May 25, 2018

Hey @amcguign,

Just wanted to let you know we have released v1.6.0 of the provider containing the new resource 🙂

@ghost
Copy link

ghost commented Mar 31, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

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

Successfully merging a pull request may close this issue.

8 participants