Updated to not save the service principal passwords as plaintext in the statefile

[monkey-jeff]

When looking into using the relatively new resource of azurerm_azuread_service_principal_password I noticed the the password you set is saved in plain text in the statefile. I thought it would be easy to fix this.

The only issue I see is that it will recreate the password the first time you update to these changes since the diff will show difference, I don't know if this is worth fixing.

Let me know if there is anything else you want me to do here.

I did run the testacc to ensure that it all still works as well with an actual module to ensure we aren't saving the password in plain text.

➜  terraform-provider-azurerm git:(master) ✗ make testacc TESTARGS='-run=TestAccAzureRMActiveDirectoryServicePrincipalPassword*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMActiveDirectoryServicePrincipalPassword* -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMActiveDirectoryServicePrincipalPassword_basic
=== PAUSE TestAccAzureRMActiveDirectoryServicePrincipalPassword_basic
=== RUN   TestAccAzureRMActiveDirectoryServicePrincipalPassword_customKeyId
=== PAUSE TestAccAzureRMActiveDirectoryServicePrincipalPassword_customKeyId
=== CONT  TestAccAzureRMActiveDirectoryServicePrincipalPassword_basic
=== CONT  TestAccAzureRMActiveDirectoryServicePrincipalPassword_customKeyId
--- PASS: TestAccAzureRMActiveDirectoryServicePrincipalPassword_basic (31.43s)
--- PASS: TestAccAzureRMActiveDirectoryServicePrincipalPassword_customKeyId (31.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	31.673s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/kubernetes	(cached) [no tests to run]
?   	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response	[no test files]
?   	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/set	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils	(cached) [no tests to run]
?   	github.com/terraform-providers/terraform-provider-azurerm/version	[no test files]

[monkey-jeff]

Hey, I don't want to be a pain but is there an update on this? Maybe what would be needed to get this accepted? I will of course update to what you guys require.

[tombuildsstuff]

hey @monkey-jeff

Thanks for this PR - apologies for the delayed review here!

Taking a look into this, whilst this approach does encrypt this value in the Statefile - unfortunately this approach breaks use-cases where users are interpolating values between different resources, for example:

resource "azurerm_azuread_service_principal_password" "test" {
  # ...
  value = "abc123"
  # ...
}

resource "azurerm_key_vault_secret" "test" {
  # ...
  value = "${azurerm_azuread_service_principal_password.test.value}"
  # ...
}

Whilst it'd potentially be possible to work around this by placing the value into a variable, this then means you'd need to add a depends_on between the resources to ensure they're created in the correct order; which also isn't ideal.

Instead we've been looking into encrypting the entire Statefile, which (amongst other things) would allow this interpolation function to work as it does today. There's some more information on the Terraform Website on our future plans for the Statefile - and a proposal for implementing this on the Terraform Core repository. Whilst some other Terraform Providers have opted to optionally PGP encrypt some fields (which provides another possible approach to this issue) - unfortunately this also breaks interpolation and as such we've stopped adding this approach to new resources.

As such - whilst I'd like to thank you for this contribution I believe it'd be better to fix this within Terraform Core, and as such I'm going to close this in favour of the upstream issue within Terraform Core: hashicorp/terraform#9556

Thanks!

[monkey-jeff]

Hey @tombuildsstuff,

Thank you for the response and I totally understand your position here. Part of the issue our security team is separation of access/roles. The people that have access to the state files should not have access to the application passwords. To that end I have an additional proposal that I would appreciate your thoughts on. I can do the work and expand this PR that I believe will alleviate your concerns as well as address ours.

What if we implement it like the following

resource "azurerm_azuread_service_principal_password" "unhashed" {
  # ...
  value = "abc123"
  # ...
}

resource "azurerm_key_vault_secret" "unhashed" {
  # ...
  value = "${azurerm_azuread_service_principal_password.unhashed.value}"
  # ...
}


resource "azurerm_azuread_service_principal_password" "hashed" {
  # ...
  value = "abc123"
  hash_password = true
  # ...
}

resource "azurerm_key_vault_secret" "hashed" {
  # ...
  # This is a hashed value of the password and we can't get it from the output.
  value = "${azurerm_azuread_service_principal_password.hashed.value}"
  # ...
}

I believe this would leave all current functionality perfectly intact which giving the tools we need to be able to protect that value. I also see it as providing additional value when you add encryption to terraform core as you stated encryption would need to be bi-directional so you'd have a to the original secrets, but often times you don't want that to be bi-directional as it may introduce additional security holes.

Let me know what you think of this proposal please Tom.

-Jeff

[tombuildsstuff]

hey @monkey-jeff

Thanks for the follow up suggestion here - but I don’t think that’s a viable approach either unfortunately. From Terraform’s perspective internally we’ve no way of currently inferring if the field should have a DiffSuppressFunc/validation etc based off another field (there’s no way of having the schema be dynamic since we need to know the schema definition prior to user input).

That said - we recently added support for authenticating using a Service Principal with a Client Certificate to the AzureRM Provider. Whilst we don’t support creating these via Terraform (yet!) - I believe this would solve the issue you’re seeing since you’d only be storing the public thumbprint of the certificate rather than the private key (which, whilst you could generate via the TLS Provider you could also generate through some other means, like a provisioner / input, so this data isn’t held in the Statefile).

We’re currently working on splitting the AzureAD resources out into their own provider (more details can be found in #2322) - but it should be possible to add a resource for this, from memory (when we last looked into it) I believe this just requires the certificate thumbprint. Would that approach work for you?

Thanks?

[monkey-jeff]

Thanks for the quick response @tombuildsstuff

I totally agree that splitting the directory level resources into their own provider makes a lot of sense and really looking forward to that change.

That approach "may" work for our use case. I might have to change the java library a bit we provide to our teams to be able to authenticate via private key, but it that works I don't see why creating of a service principal with public key portion wouldn't work for us.

If it does work I might go ahead and a PR adding that functionality to Terraform. What is the timeline on 2.0 and is there a cut off where no more Directory level resource changes will be accepted until moving to its own provider?

Thanks!

-Jeff

[tombuildsstuff]

@monkey-jeff

That approach "may" work for our use case. I might have to change the java library a bit we provide to our teams to be able to authenticate via private key, but it that works I don't see why creating of a service principal with public key portion wouldn't work for us.

👍

If it does work I might go ahead and a PR adding that functionality to Terraform. What is the timeline on 2.0 and is there a cut off where no more Directory level resource changes will be accepted until moving to its own provider?

We're still working out the timeframe for 2.0 but it'll be in Q1 next year - about the cut-off I'm unsure, we're planning on doing (at least) one more 1.x release then looking towards what's needed for 2.0. Given the providers will be pretty similar either way I'd suggest it's probably fine - we can always move it across if necessary. Whilst we've closed a couple of other PR's for AzureAD resources of late (until we can move them across to the new provider) - this was mostly because we can't easily test them in an automated fashion, but this should be possible to do for the Client Cert resource (since we may end up re-evaluating the schema design as it's moved over anyway).

Thanks!

[ghost]

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!