New Data Source: `azurerm_storage_account_sas` #1011

jzampieron · 2018-03-20T21:17:29Z

This PR is a WIP support for programmatically generating Shared Access Signatures for Storage Accounts.

The signature computation and url query string generation from the connection strings generated by terraform azurerm_storage_account resources seem to be working.

Much additional validation and checking logic and more test cases are necessary.

(fixes #59)

jzampieron · 2018-03-21T21:35:41Z

@tombuildsstuff This is now working all the way through as a data source. I still need to write more/better validation functions, documentation and an acceptance test or two.

I'd appreciate a quick once-over to make sure I'm still on-track. The definition now looks like:

data "azurerm_storage_account_sas" "sas_for_metrics" {
    connection_string = "${azurerm_storage_account.dcos.primary_connection_string}"
    https_only        = true
    resource_types {
        service   = true
        container = false
        object    = false
    }
    services {
        blob  = true
        queue = false
        table = false
        file  = false
    }
    start   = "2018-03-21"
    expiry  = "2020-03-21"
    permissions {
        read    = true
        write   = true
        delete  = false
        list    = false
        add     = true
        create  = true
        update  = false
        process = false
    }
}

And gathering the output is this style:

resource "local_file" "sas_output" {
    content  = "${data.azurerm_storage_account_sas.sas_for_metrics.sas}"
    filename = "${path.cwd}/outputs/sas_output.txt"
}

All the fields are required b/c they can accidentally expose something, so it makes sense to be explicit.
The input connection_string and output sas are marked sensitive b/c they contain access tokens and/or keys of sorts.

…re support.

passes.

passes - 2

jzampieron · 2018-03-22T18:55:49Z

@tombuildsstuff This should be now worth a review. I added better unit tests, an acceptance test and the documentation. LMK next steps. Thanks!

tombuildsstuff · 2018-04-03T06:02:36Z

Fixes #59

tombuildsstuff

hey @jzampieron

Thanks for pushing those updates - apologies for the delayed review here, I'd added comments but not hit "submit" 😞. If we can fix up the (mostly minor) issues then this otherwise LGTM :)

Thanks!

tombuildsstuff · 2018-04-03T06:06:48Z

azurerm/data_source_storage_account_sas.go

+			"expiry": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,


(same here) do we want to add some validation to ensure this is formatted correctly? We've got a method which does this for RFC3339 dates here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/validators.go#L13 - which can be used like so:

ValidateFunc: validateRFC3339Date,

I'd prefer to go the route you specify with the validation function. In my research there are some slight differences between ISO-8601 and RFC3339. MSFT specifies that these are ISO-8601 and since the signatures are based on the actual text characters, the subtle differences may result in breakage/no-working.

Given the fragile nature, by design, of the signatures, I think leaving as-is is preferable rather than constraining the input to possibly broken validation.

makes sense from my side - we can always add it in later based on feedback 👍

tombuildsstuff · 2018-04-03T06:06:54Z

azurerm/data_source_storage_account_sas.go

+			"start": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,


do we want to add some validation to ensure this is formatted correctly? We've got a method which does this for RFC3339 dates here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/validators.go#L13 - which can be used like so:

ValidateFunc: validateRFC3339Date,

Same notes as above.

tombuildsstuff · 2018-04-19T09:48:54Z

azurerm/data_source_storage_account_sas.go

+
+						"file": {
+							Type:     schema.TypeBool,
+							Required: true,


I think each of these properties also needs ForceNew: true on them?`

tombuildsstuff · 2018-04-19T09:49:22Z

azurerm/data_source_storage_account_sas.go

+						"process": {
+							Type:     schema.TypeBool,
+							Required: true,
+						},


I think each of these properties also needs ForceNew: true on them?

This is actually a result of my lack of understanding of the semantics of that flag on the outer permissions list object vs the inner schema. I'll add the flag.

tombuildsstuff · 2018-04-19T09:52:23Z

azurerm/data_source_storage_account_sas.go

+						"object": {
+							Type:     schema.TypeBool,
+							Required: true,
+						},


I think each of these properties also needs ForceNew: true on them?

tombuildsstuff · 2018-04-19T09:59:56Z

website/docs/d/storage_account_sas.html.markdown

+
+## Attributes Reference
+
+* `sas` - (Computed) The computed Account Shared Access Signature (SAS). 


can we remove (Computed) here? that should be implied as it's in the Attributes reference

tombuildsstuff · 2018-04-19T10:00:19Z

website/docs/d/storage_account_sas.html.markdown

+* `services` - (Required) A set of true/false flags which define the storage account services that are granted access by this SAS.
+* `start` - (Required) The starting time and date of validity of this SAS. Must be a valid ISO-8601 format time/date string.
+* `expiry` - (Required) The expiration time and date of this SAS. Must be a valid ISO-8601 format time/date string.
+* `permissions` - (Required) A set of true/false flags which define the granted permissions.


minor can we quote true and false here?

tombuildsstuff · 2018-04-19T10:00:22Z

website/docs/d/storage_account_sas.html.markdown

+
+* `connection_string` - (Required) The connection string for the storage account to which this SAS applies. Typically directly from the `primary_connection_string` attribute of a terraform created `azurerm_storage_account` resource.
+* `https_only` - (Optional) Only permit `https` access. If `false`, both `http` and `https` are permitted. (Default `true`)
+* `resouce_types` - (Required) A set of true/false flags which define the storage account resource types that are granted access by this SAS. 


minor can we quote true and false here?

tombuildsstuff · 2018-04-19T10:00:24Z

website/docs/d/storage_account_sas.html.markdown

+* `connection_string` - (Required) The connection string for the storage account to which this SAS applies. Typically directly from the `primary_connection_string` attribute of a terraform created `azurerm_storage_account` resource.
+* `https_only` - (Optional) Only permit `https` access. If `false`, both `http` and `https` are permitted. (Default `true`)
+* `resouce_types` - (Required) A set of true/false flags which define the storage account resource types that are granted access by this SAS. 
+* `services` - (Required) A set of true/false flags which define the storage account services that are granted access by this SAS.


minor can we quote true and false here?

tombuildsstuff · 2018-04-19T10:00:51Z

website/docs/d/storage_account_sas.html.markdown

+## Argument Reference
+
+* `connection_string` - (Required) The connection string for the storage account to which this SAS applies. Typically directly from the `primary_connection_string` attribute of a terraform created `azurerm_storage_account` resource.
+* `https_only` - (Optional) Only permit `https` access. If `false`, both `http` and `https` are permitted. (Default `true`)


can we update "(Default true)" -> "Defaults to true" for consistency

phekmat · 2018-04-19T15:21:38Z

azurerm/data_source_storage_account_sas.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"service": {


Would it be possible to default all of these (the resource_types, permissions, and services parameters) to false and make them optional?

@tombuildsstuff Agree with all of the above and will incorporate those changes.
@phekmat I debated that myself and came to the conclusion to be explicitly verbose here since there are security vs function issues here.

If we default all to false, then there's an implicit no-access (all broken) case.

IMHO it's better here to be verbose and demand that folks specify exactly what they want...

This avoids an oops by granting too much access and an oops by granting no access both of which, IMHO, are a worse UX than some extra typing.

IHMO it's better here to be verbose and demand that folks specify exactly what they want...

I'd agree with this approach for the moment - we can always change it later 👍

…vior.

jzampieron

The latest commit should incorporate the minor items. A few things still under discussion.

jzampieron · 2018-04-26T13:19:54Z

@tombuildsstuff Any further improvements required here? Thanks.

tombuildsstuff · 2018-05-02T11:15:51Z

@jzampieron sorry, I started a review but didn't hit submit - I'll take another look now!

jzampieron · 2018-05-14T16:14:00Z

@tombuildsstuff Checking in. I don't see any other actions required. Should be good to merge.

tombuildsstuff · 2018-05-17T21:54:12Z

@jzampieron apologies for the delayed response here - I started this review then got distracted, I'll post that shortly and re-review this next week.

Chatting with Microsoft last week I discovered there's a couple of endpoints which provide this information - List Account SAS and List Storage SAS - from what I can see it appears the List Account SAS endpoint would allow us to be consistent by leaning on Azure here instead; what do you think about switching over to use that?

Thanks!

tombuildsstuff

👋 hey @jzampieron

Apologies for the delayed review here - as mentioned below I'd started this review but not hit "submit" 😞

tombuildsstuff · 2018-04-23T10:37:26Z

website/docs/d/storage_account_sas.html.markdown

+* `services` - (Required) A set of `true`/`false` flags which define the storage account services that are granted access by this SAS.
+* `start` - (Required) The starting time and date of validity of this SAS. Must be a valid ISO-8601 format time/date string.
+* `expiry` - (Required) The expiration time and date of this SAS. Must be a valid ISO-8601 format time/date string.
+* `permissions` - (Required) A set of `true`/`false` flags which define the granted permissions.


can we update this to explicitly list the permissions, e.g.:

* `permissions` - (Required) A `permissions` block as defined below. --- A `permissions` block contains: * `read` - (Required) Should Read permissions be enabled for this SAS? * `write` - (Required) Should Write permissions be enabled for this SAS? * `delete` - (Required) Should Delete permissions be enabled for this SAS? * `list` - (Required) Should List permissions be enabled for this SAS? * `add` - (Required) Should Add permissions be enabled for this SAS? * `create` - (Required) Should Create permissions be enabled for this SAS? * `update` - (Required) Should Update permissions be enabled for this SAS? * `process` - (Required) Should Process permissions be enabled for this SAS?

tombuildsstuff · 2018-04-23T11:19:31Z

azurerm/data_source_storage_account_sas.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"service": {


IHMO it's better here to be verbose and demand that folks specify exactly what they want...

I'd agree with this approach for the moment - we can always change it later 👍

tombuildsstuff · 2018-04-23T14:33:26Z

azurerm/data_source_storage_account_sas.go

+	if httpsOnly {
+		signedProtocol = "https"
+	}
+	signedIp := ""


👍 works for me

tombuildsstuff · 2018-04-23T15:45:30Z

azurerm/data_source_storage_account_sas.go

+func validateArmStorageAccountSasResourceTypes(v interface{}, _ string) (ws []string, es []error) {
+	input := v.(string)
+
+	if !regexp.MustCompile(`\A([cos]{1,3})\z`).MatchString(input) {


given we're passing in Booleans above, I don't think this is ever possible with this resource - as such I think we can remove this for the moment?

tombuildsstuff · 2018-04-23T16:24:03Z

website/docs/d/storage_account_sas.html.markdown

+* `connection_string` - (Required) The connection string for the storage account to which this SAS applies. Typically directly from the `primary_connection_string` attribute of a terraform created `azurerm_storage_account` resource.
+* `https_only` - (Optional) Only permit `https` access. If `false`, both `http` and `https` are permitted. Defaults to `true`.
+* `resouce_types` - (Required) A set of `true`/`false` flags which define the storage account resource types that are granted access by this SAS. 
+* `services` - (Required) A set of `true`/`false` flags which define the storage account services that are granted access by this SAS.


Can we define these (example below)

tombuildsstuff · 2018-04-23T16:24:05Z

website/docs/d/storage_account_sas.html.markdown

+
+* `connection_string` - (Required) The connection string for the storage account to which this SAS applies. Typically directly from the `primary_connection_string` attribute of a terraform created `azurerm_storage_account` resource.
+* `https_only` - (Optional) Only permit `https` access. If `false`, both `http` and `https` are permitted. Defaults to `true`.
+* `resouce_types` - (Required) A set of `true`/`false` flags which define the storage account resource types that are granted access by this SAS. 


Can we define these (example below)

tombuildsstuff · 2018-05-03T09:30:35Z

azurerm/data_source_storage_account_sas.go

+				},
+			},
+
+			"sas": {


jzampieron · 2018-05-18T12:05:27Z

I'll work through the review in detail as above. w.r.t. the larger question of generating vs using the API.

Short term I think we should just go with this to have an answer to the immediate feature needs.

Longer term moving to the API will shift some of the burden to MSFT for compatibility, although it shouldn't really matter either way since the generation algorithm really shouldn't change at this point.

Update documentation with more verbose field details.

katbyte

Hi @jzampieron,

Thank you for this new data source. it LGTM and the tests pass:

As discussed.

ghost · 2020-03-31T14:58:25Z

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!

jzampieron mentioned this pull request Mar 20, 2018

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

Closed

tombuildsstuff added new-data-source service/storage labels Mar 20, 2018

jzampieron force-pushed the fix-59 branch from 47534b2 to 07a9de0 Compare March 21, 2018 19:54

jzampieron added 11 commits March 22, 2018 14:47

Initial code and unit tests for Storage Account Shared Access Signatu…

4fa76c4

…re support.

Unit test for SAS computation which matches the Azure Portal

048bb76

passes.

Unit test for SAS computation which matches the Azure Portal

d9d1dc2

passes - 2

Move SAS support from a resource to a data source.

d59dfb1

Add unit tests for the string building functions.

109629b

Fix warnings on unused func params.

27e8d5a

Add unit test for the connection string parse.

c19295d

Make sure to set the id of the data source element.

a1f12ee

Make https_only an optional/defaulted field.

66c8aca

Add documentation for data source azurerm_storage_account_sas.

d16f5df

WIP: Add acceptance test.

6088083

jzampieron force-pushed the fix-59 branch from bf04d1c to 6088083 Compare March 22, 2018 18:50

tombuildsstuff previously requested changes Apr 19, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Apr 19, 2018

phekmat reviewed Apr 19, 2018

View reviewed changes

Incorporate fixes from first gh review feedback. - Set force new beha…

bcbf6fb

…vior.

jzampieron commented Apr 20, 2018

View reviewed changes

tombuildsstuff changed the title ~~Implement #59~~ New Data Source: azurerm_storage_account_sas Apr 23, 2018

Fix tabs vs spaces in the hcl string literal.

0eace11

anoff mentioned this pull request Apr 30, 2018

Support Shared Access Signatures for Azure Files service #90

Closed

tombuildsstuff removed the waiting-response label May 3, 2018

tombuildsstuff reviewed May 17, 2018

View reviewed changes

Remove unnecessary validation function per github review.

5bc7264

Update documentation with more verbose field details.

tombuildsstuff assigned katbyte May 18, 2018

data_source_storage_account_sas_test.go make test names similar

71350a4

katbyte approved these changes May 20, 2018

View reviewed changes

katbyte added this to the 1.6.0 milestone May 20, 2018

katbyte merged commit 20514b2 into hashicorp:master May 20, 2018

katbyte added a commit that referenced this pull request May 20, 2018

Update CHANGELOG.md to include #1011

87e8686

jzampieron deleted the fix-59 branch May 21, 2018 15:17

hashicorp locked and limited conversation to collaborators Mar 31, 2020


		## Attributes Reference

		* `sas` - (Computed) The computed Account Shared Access Signature (SAS).

New Data Source: azurerm_storage_account_sas #1011

New Data Source: azurerm_storage_account_sas #1011

Conversation

jzampieron commented Mar 20, 2018 • edited by katbyte

jzampieron commented Mar 21, 2018 • edited

jzampieron commented Mar 22, 2018

tombuildsstuff commented Apr 3, 2018

tombuildsstuff 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

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

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

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

jzampieron Apr 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzampieron left a comment

Choose a reason for hiding this comment

jzampieron commented Apr 26, 2018

tombuildsstuff commented May 2, 2018

jzampieron commented May 14, 2018

tombuildsstuff commented May 17, 2018

tombuildsstuff 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

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

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

jzampieron commented May 18, 2018

katbyte left a comment • edited

Choose a reason for hiding this comment

ghost commented Mar 31, 2020

New Data Source: `azurerm_storage_account_sas` #1011

New Data Source: `azurerm_storage_account_sas` #1011

jzampieron commented Mar 20, 2018 •

edited by katbyte

jzampieron commented Mar 21, 2018 •

edited

jzampieron Apr 20, 2018 •

edited

katbyte left a comment •

edited