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

Redis API is masking the account key being used #3037

Open
tombuildsstuff opened this issue May 8, 2018 · 30 comments
Open

Redis API is masking the account key being used #3037

tombuildsstuff opened this issue May 8, 2018 · 30 comments
Assignees
Labels
customer-response-expected Redis Cache Service Attention Workflow: This issue is responsible by Azure service team. Service-team

Comments

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented May 8, 2018

👋

We're seeing an issue with the Redis API where the RDB Storage Connection string sent to Azure is different to the one returned from the API, for instance we see the response:

DefaultEndpointsProtocol=https;BlobEndpoint=https://unlikely23exst2acct4u0j.blob.core.windows.net/;AccountName=unlikely23exst2acct4u0j;AccountKey=[key hidden]

rather than the original value posted

DefaultEndpointsProtocol=https;BlobEndpoint=https://unlikely23exst2acct4u0j.blob.core.windows.net/;AccountName=unlikely23exst2acct4u0j;AccountKey=Fv5Ien72PEKr++K8tp5L/fR3qlPghHUiRqHZZS25dR9LsnD//tz8lXLGRORCkBnwrXxltCrx2qjEOZAmGIEgGA==

which in our case is leading to spurious diff's; whilst we could look into ignoring the returned "[key hidden]" value, this makes it impossible for us to detect changes.

Would it be possible to fix the API to return the key that's being used?

Thanks!

@TimLovellSmith
Copy link
Member

@tombuildsstuff BTW don't forget to rotate that storage account key!

@tombuildsstuff
Copy link
Contributor Author

@TimLovellSmith should have mentioned this was an acceptance test, so the Redis Cache in question's already been deleted :)

@TimLovellSmith
Copy link
Member

@tombuildsstuff
We are intentionally masking it out in responses to help you avoid unintended information disclosure, since azure permissions to view related storage account credentials should not be implied by azure permissions to get general information about a redis cache.

@tombuildsstuff
Copy link
Contributor Author

@TimLovellSmith so how are users expected to know which access key is being used when rotating secrets for example? Whilst this is potentially useful for scenarios in the Azure Portal; this approach has the potential to cause production incidents for example where an operator is unsure which key is currently in use, and cycles the wrong one. As such can we look into adding an override/flag to return this information?

Thanks!

@TimLovellSmith
Copy link
Member

@tombuildsstuff
Assuming you have enough permissions and you don't know which key is being used, and you want to rotate specifically one of the keys, then you can just reconfigure your redis cache to use the key you are not going to rotate, before you rotate.

@tombuildsstuff
Copy link
Contributor Author

tombuildsstuff commented May 22, 2018

@TimLovellSmith whilst that approach could work, that's not a suitable workaround in all situations unfortunately - other API's within Azure expose a sensitive endpoint which returns sensitive information; could we get a similar endpoint added to the Redis API?

cc @joshgav @marstr @jhendrixMSFT - this is the issue I was referring too in our meeting the other day

@marstr
Copy link
Member

marstr commented May 22, 2018

Adding labels to help with our tracking systems.

@TimLovellSmith
Copy link
Member

@tombuildsstuff Got some examples of those APIs? I don't know what exactly you are thinking of and it would probably be useful to be able to make the same comparison.

@tombuildsstuff
Copy link
Contributor Author

@TimLovellSmith off the top of my head - AKS returns an access profile which includes all the sensitive K8S information; App Service returns the connection strings etc.

@joshgav
Copy link

joshgav commented May 23, 2018

@tombuildsstuff what API(s) are you calling that return this obfuscated response? Thanks!

@joshgav
Copy link

joshgav commented Jun 7, 2018

Hi @tombuildsstuff is this still an issue or did you work around it in the end? Thanks!

@tombuildsstuff
Copy link
Contributor Author

@joshgav sorry I've been AFK until today.

what API(s) are you calling that return this obfuscated response? Thanks!

In this case this is the Get API's redisConfiguration dictionary, which is masking the output.

is this still an issue or did you work around it in the end? Thanks!

This is still an issue for us at this time - since we're unable to know which Storage Access Key is being used by the Redis Cache.

Thanks!

@TimLovellSmith
Copy link
Member

@tombuildsstuff you mentioned way back at the start of the thread that you could look into ignoring the returned "[key hidden]" value, could you still do that? I think this is still doings its intended behavior, since these storage connection strings if exposed may effectively leak the data which is stored in the cache.

@tombuildsstuff
Copy link
Contributor Author

you mentioned way back at the start of the thread that you could look into ignoring the returned "[key hidden]" value, could you still do that? I think this is still doings its intended behavior, since these storage connection strings if exposed may effectively leak the data which is stored in the cache.

@TimLovellSmith taking that approach that means we'd be unable to detect changes to the Storage Account Key, which means that Terraform's Diff would work inconsistently across the Redis Coiguration; as such I don't think that's an appropriate workaround in this instance unfortunately.

Since the Redis Configuration can contain sensitive values, is there a reason this couldn't be moved out to a sensitive endpoint ala GetAccessProfile in AKS or ListKeys in the Storage Accounts?

Thanks!

@TimLovellSmith
Copy link
Member

@tombuildsstuff I don't know Terraform's Diff - what is it supposed to do?

@bsiegel bsiegel added the Service Attention Workflow: This issue is responsible by Azure service team. label Sep 26, 2018
@tombuildsstuff
Copy link
Contributor Author

@TimLovellSmith sorry missed this.

@tombuildsstuff I don't know Terraform's Diff - what is it supposed to do?

Terraform's Diff (or plan) functionality shows you the difference between the current state of the resource (for example, the Connection Strings used in Azure) and the state defined locally in code - and shows you a proposed diff. The App Service API as an example returns the full (unmasked) connection string, so it's possible for users to see which parts of the connection string have changed in the diff.

Hope that helps :)

@TimLovellSmith
Copy link
Member

@tombuildsstuff
So basically this behavior matters only to people who are storing their connection strings in their code?

@TimLovellSmith
Copy link
Member

TimLovellSmith commented Oct 31, 2019

@bsiegel Can we close this? This behavior is by design, to avoid accidentally leaking sensitive connection string data.

@tombuildsstuff
Copy link
Contributor Author

@TimLovellSmith

Can we close this? This behavior is by design, to avoid accidentally leaking sensitive connection string data.

More modern API's (Storage, AKS, Media Services etc) follow a different convention where these keys are exposed on a sensitive privileged endpoint - as such there's precedent for the Redis API to follow here IMO - as such I believe this bug report/feature request is still valid?

@TimLovellSmith
Copy link
Member

@tombuildsstuff What pattern are you referring to - keyvault scenarios? Or something else?

@tombuildsstuff
Copy link
Contributor Author

cc @vladimirjoanovic

@iitsDelbruegger
Copy link

I am deeply disappointed that this bug is not fixed yet. We are setting up all our infrastructure in Terraform and automatically store the secrets in a key vault so that no developer or admin has to fiddle around with them - but Redis cannot output the connection string because of this bug. Do you not want to support modern Infrastructure as Code solutions?

@TimLovellSmith
Copy link
Member

@iitsDelbruegger We can't really solve this problem in the ways suggested without also having security impact. I think the ideal solution here is a non-key-based approach for configuring redis authentication to storage, such as System Assigned Identity, which could both eliminate problems with storage key rotation breaking persistence access, and mitigate the possible problems with exposing credentials, and terraform integration.

@tombuildsstuff
Copy link
Contributor Author

@TimLovellSmith as mentioned above, the AKS API provides one example for doing just that: https://docs.microsoft.com/en-us/rest/api/aks/managedclusters/listclusteradmincredentials - which gives you full access to an AKS Cluster including (via provisioning pods etc) any resources it has access too, which I'd argue is more sensitive.

That said, I'd agree it could make more sense for the Redis RP to use a Managed Identity to look this information up going forward - but that's a whole separate solution to the problem.

From Terraform's side, just knowing if it's "primary" or "secondary" (or "no longer valid", if the keys been rolled, but the redis cache hasn't been updated) could be a sufficient interim fix too rather than returning the entire key otherwise (presuming the API goes through and raises an issue should this connection string no longer work to be able to detect the "no longer valid" state).

WDYT?

@vamshisiram
Copy link

vamshisiram commented May 2, 2022

The document needs to be updated for the ignore_changes block, as it needs to reflect lifecycle the new way of doing it.

https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/redis_cache

if you add what is mentioned in the article above it fails...

image

Right now, it says just add ignore_changes to the resource block, but it is deprecated and no longer supported. so, add a lifecycle block as below to ignore this....

lifecycle {
ignore_changes = [redis_configuration.0.rdb_storage_connection_string]
}

@dmitrytokarev
Copy link
Contributor

dmitrytokarev commented Jan 20, 2023

@TimLovellSmith any update to @tombuildsstuff 's question?
This issue has been causing additional issues even when using lifecycle {ignore_changes = ...}:

  lifecycle {
    ignore_changes = [
      redis_configuration.0.rdb_storage_connection_string,
      redis_configuration.0.aof_storage_connection_string_0,
    ]
  }

Getting this error intermittently:

│ Error: updating Redis Cache "my-redis" (Resource Group "my-redis"): redis.Client#Update: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidRequestBody" Message="The value of the parameter 'properties.redisConfiguration.rdb-storage-connection-string' is invalid.\r\nRequestID=4de173a2-2895-4f85-a13e-74233270ef5d"
│ 
│   with module.redis.azurerm_redis_cache.redis,
│   on .terraform/modules/redis/redis.tf line 1, in resource "azurerm_redis_cache" "redis":
│    1: resource "azurerm_redis_cache" "redis" {

Terraform v1.3.7
on darwin_arm64

  • provider registry.terraform.io/hashicorp/azurerm v3.40.0

@TimLovellSmith
Copy link
Member

@tombuildsstuff
The API you list for AKS cluster credentials is a POST API, so it would never expose credentials by accident, only when they are explicitly requested.

If we were following regular guidance around secret connection string properties, we would probably not return them in the PUT or GET response at all. But because this makes it hard to see if you configured those properties we went for a different approach where only the sensitive part is not returned. Now if we want to make any change to this API we will have to go through a long deprecation cycle.

Unfortunately, there is no way for redis service to know whether the storage key it has been provided with is a secondary one, or a primary one. You could choose to store this information in the redis 'tags' though, if you wanted to record it somewhere.

@mangonc
Copy link

mangonc commented Feb 27, 2024

I'm getting a similar problem on AOF:
│ Redis Name: 'redis-dev-dapla-rb-dev'): unexpected status 400 with error: InvalidRequestBody: The value of the parameter 'properties.redisConfiguration.aof-storage-connection-string-0' is invalid.

Do you confirm that we have the same issue on AOF connection string?

@Xaxetrov
Copy link

Xaxetrov commented Mar 6, 2024

I'm getting a similar problem on AOF: │ Redis Name: 'redis-dev-dapla-rb-dev'): unexpected status 400 with error: InvalidRequestBody: The value of the parameter 'properties.redisConfiguration.aof-storage-connection-string-0' is invalid.

Do you confirm that we have the same issue on AOF connection string?

Having the same issue since v3.93.0 of the azurerm provider, released on 2024/02/26. I am going to create an issue on their side.

Update:
I found a workaround. As the documentation was suggesting, we had an ignore_changes block on rdb_storage_connection_string.

Because of azurerm_redis_cache update on v3.93 (support for data_persistence_authentication_method), some minor changes had to be made but could not. Maybe because of a bug, maybe because the connection string actually changed (I checked and am pretty sure it did not).

By removing rdb_storage_connection_string from ignore_changes, I could apply the pipeline successfully. Then I had to restore the ignore_changes block to avoid the documented problem.

Hope this helps.

@AkshayNarayan777
Copy link

By removing rdb_storage_connection_string from ignore_changes, I could apply the pipeline successfully. Then I had to restore the ignore_changes block to avoid the documented problem.

I tried this but after adding back the 'ignore_changes' block the pipeline fails giving the same error

unexpected status 400 (400 Bad Request) with error: InvalidRequestBody: The value of the parameter 'properties.redisConfiguration.rdb-storage-connection-string' is invalid.
│ RequestID=6434343f5-029d-4af9-981c-3sdfe43454erw0a

I we completely remove the ignore_changes block then on every terraform apply it tries to update this value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-response-expected Redis Cache Service Attention Workflow: This issue is responsible by Azure service team. Service-team
Projects
None yet
Development

No branches or pull requests