new resource "azurerm_key_vault_key_encrypt" and data source "azurerm_key_vault_key_decrypt" #8895

njuCZ · 2020-10-15T07:34:58Z

resource "azurerm_key_vault_key_encrypt" is similar with "aws_kms_ciphertext"
data source "azurerm_key_vault_key_decrypt" is similar with "aws_kms_secret"

to keep a stable encrypted value, make "azurerm_key_vault_key_encrypt" be a resource but not a data source.
there is no need to support "import" operation for resource "azurerm_key_vault_key_encrypt"

tombuildsstuff · 2020-10-19T05:58:18Z

dependent on #7856 (comment)

IanMoroney · 2021-03-02T10:25:03Z

azurerm/internal/services/keyvault/key_vault_key_encrypt_resource.go

@@ -0,0 +1,94 @@
+package keyvault


If the intention is to encrypt secrets, might i suggest that the resource instead be called:
azurerm_key_vault_secret_encrypt

becasue key vault key and key vault secret are different resource, and the rest api is using key vault key to encrypt and decrypt data. So personally I think azurerm_key_vault_secret_encrypt maybe a little misleading?

IanMoroney · 2021-03-02T10:25:31Z

azurerm/internal/services/keyvault/key_vault_key_decrypt_data_source.go

@@ -0,0 +1,83 @@
+package keyvault


If the intention is to decrypt secrets, might i suggest that the resource instead be called:
azurerm_key_vault_secret_decrypt

IanMoroney · 2021-03-02T10:26:45Z

azurerm/internal/services/keyvault/key_vault_key_encrypt_resource.go

@@ -0,0 +1,94 @@
+package keyvault
+


I can see there being a value in having a resource block for encrypting secrets with a static encrypted blob, and a data source for encrypting secrets with a value that changes on every apply, but I only see an encrypt resource.

Could you create an equivalent encrypt data source, as if we're modelling it after aws_kms_ciphertext, we should have the ability for users to choose whether to change the encrypted blob frequently or infrequently.

is there any scenario that users want to change the encrypted data every time? In the resource, we could also change the encrypted data by changing the property name

katbyte

Thanks @njuCZ - left some comments inline

azurerm/internal/services/keyvault/key_vault_key_encrypt_resource.go

katbyte · 2021-05-03T19:09:55Z

azurerm/internal/services/keyvault/key_vault_key_decrypt_data_source.go

+				ValidateFunc: azure.ValidateKeyVaultChildId,
+			},
+
+			"payload": {


what format are we expecting here? base64?

Suggested change

"payload": {

"encrypted_base64_data": {

katbyte · 2021-05-03T19:11:38Z

azurerm/internal/services/keyvault/key_vault_key_encrypt_resource.go

+				}, false),
+			},
+
+			"cipher_text": {


should this also then be

Suggested change

"cipher_text": {

"encrypted_data_in_base64": {

the result format is base64url, which is slightly different from base64. please refer to RFC: https://tools.ietf.org/html/rfc4648#section-5. I have noted it in the doc. Should I still rename this property?

yes lets rename it to match what is expected so people don't have to look at docs and can tell from the property name. as for url in there is this different then any other base64 property for azure?

lets go with encrypted_data_in_base64 and then add a validation function to ensure its the right base64 format

azurerm/internal/services/keyvault/key_vault_key_encrypt_resource.go

katbyte · 2021-05-03T19:15:06Z

azurerm/internal/services/keyvault/tests/key_vault_key_decrypt_data_source_test.go

+	return fmt.Sprintf(`
+%s
+
+data "azurerm_key_vault_key_decrypt" "test" {


could we use the encrypt resource and decrypt to check that the workflow works?

yes, in this data source, we assign the cipher_text of azurerm_key_vault_key_encrypt to payload, and in the check functions, we checked whether the descrypt value is the inital input plaintext.
In this way, we checked the whole encrypt and decrypt workflow

website/docs/d/key_vault_key_decrypt.html.markdown

website/docs/r/key_vault_key_encrypt.html.markdown

…_key_vault_key_decrypt"

njuCZ · 2021-05-06T08:41:45Z

Thanks @katbyte , I have rebased and refine this PR

katbyte · 2021-05-07T03:55:11Z

azurerm/internal/services/keyvault/key_vault_key_decrypt_data_source.go

+			"payload": {
+				Type:     schema.TypeString,
+				Required: true,
+			},


lets just use base64 without any url in the property name and then validate it?

katbyte · 2021-05-07T03:55:57Z

azurerm/internal/services/keyvault/key_vault_key_encrypt_resource.go

+				}, false),
+			},
+
+			"cipher_text": {


lets go with encrypted_data_in_base64 and then add a validation function to ensure its the right base64 format

njuCZ · 2021-05-07T07:43:07Z

@katbyte Thanks for your opinion, I have renamed the field and added validation

IanMoroney · 2021-05-07T08:16:38Z

Any thoughts on my comments @njuCZ?

njuCZ · 2021-05-07T08:46:30Z

@IanMoroney sorry for late reply. I thought this PR was blocked, but it seems things become better now. I have commented inline. Thanks for your review

gro1m · 2021-07-30T06:23:24Z

@njuCZ @IanMoroney What is the status here, actually?

``` $ TF_ACC=1 go test -v ./internal/services/keyvault -run=TestAccEncryptedValueDataSource_ === RUN TestAccEncryptedValueDataSource_encryptAndDecrypt === PAUSE TestAccEncryptedValueDataSource_encryptAndDecrypt === CONT TestAccEncryptedValueDataSource_encryptAndDecrypt --- PASS: TestAccEncryptedValueDataSource_encryptAndDecrypt (274.42s) PASS ok github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault 276.443s ``` This commit supersedes #8895 by converting this into a Data Source rather than a Resource which handles the bi-directional conversion of encrypted/decrypted values as necessary.

tombuildsstuff · 2022-03-18T06:49:16Z

hi @njuCZ

Apologies for the delayed re-review of this PR - taking a look through here whilst this is functional these both want to be data sources and since the value can be converted bi-directionally we can make these a singular data source for "encrypted value".

Since there's some larger changes to get this PR in, I hope you don't mind but I've opened #15873 which supersedes this PR and will allow this to ship via the azurerm_key_vault_encrypted_value data source, which as mentioned above allows a bi-directional encryption/decryption of a plain-text/encrypted value as necessary - as such whilst I'd like to thank you for this contribution I'm going to close this in favour of #15873

Thanks!

``` $ TF_ACC=1 go test -v ./internal/services/keyvault -run=TestAccEncryptedValueDataSource_ === RUN TestAccEncryptedValueDataSource_encryptAndDecrypt === PAUSE TestAccEncryptedValueDataSource_encryptAndDecrypt === CONT TestAccEncryptedValueDataSource_encryptAndDecrypt --- PASS: TestAccEncryptedValueDataSource_encryptAndDecrypt (274.42s) PASS ok github.com/hashicorp/terraform-provider-azurerm/internal/services/keyvault 276.443s ``` This commit supersedes #8895 by converting this into a Data Source rather than a Resource which handles the bi-directional conversion of encrypted/decrypted values as necessary.

github-actions · 2022-04-18T02:29:58Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ghost added size/XL documentation labels Oct 15, 2020

tombuildsstuff self-assigned this Oct 19, 2020

tombuildsstuff added the waiting-response label Oct 19, 2020

This comment has been minimized.

Sign in to view

ghost removed the waiting-response label Jan 4, 2021

IanMoroney reviewed Mar 2, 2021

View reviewed changes

katbyte added new-resource service/key-vault Key Vault labels Apr 6, 2021

katbyte requested changes May 3, 2021

View reviewed changes

katbyte added the waiting-response label May 3, 2021

njuCZ added 3 commits May 6, 2021 13:21

new resource "azurerm_key_vault_key_encrypt" and data source "azurerm…

fe89a93

…_key_vault_key_decrypt"

update

4b4ae5a

rebase and refactor the codes

ee064ef

njuCZ force-pushed the issue_7856 branch from 743af6f to ee064ef Compare May 6, 2021 08:34

ghost removed the waiting-response label May 6, 2021

njuCZ requested a review from katbyte May 6, 2021 08:41

njuCZ added 2 commits May 6, 2021 17:34

avoid linter error

e8619b9

avoid linter error

a895368

katbyte requested changes May 7, 2021

View reviewed changes

katbyte added the waiting-response label May 7, 2021

update

88695e0

ghost removed the waiting-response label May 7, 2021

tombuildsstuff mentioned this pull request Mar 18, 2022

New Data Source: azurerm_key_vault_encrypted_value #15873

Merged

tombuildsstuff closed this Mar 18, 2022

github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource "azurerm_key_vault_key_encrypt" and data source "azurerm_key_vault_key_decrypt" #8895

new resource "azurerm_key_vault_key_encrypt" and data source "azurerm_key_vault_key_decrypt" #8895

njuCZ commented Oct 15, 2020

tombuildsstuff commented Oct 19, 2020

This comment has been minimized.

IanMoroney Mar 2, 2021

njuCZ May 7, 2021

IanMoroney Mar 2, 2021

njuCZ May 7, 2021

IanMoroney Mar 2, 2021 •

edited

Loading

njuCZ May 7, 2021

katbyte left a comment

katbyte May 3, 2021

njuCZ May 6, 2021

katbyte May 3, 2021

njuCZ May 6, 2021

katbyte May 7, 2021

katbyte May 7, 2021

katbyte May 3, 2021

njuCZ May 6, 2021

njuCZ commented May 6, 2021

katbyte May 7, 2021

katbyte May 7, 2021

njuCZ commented May 7, 2021

IanMoroney commented May 7, 2021 via email

njuCZ commented May 7, 2021

gro1m commented Jul 30, 2021

tombuildsstuff commented Mar 18, 2022

github-actions bot commented Apr 18, 2022

new resource "azurerm_key_vault_key_encrypt" and data source "azurerm_key_vault_key_decrypt" #8895

new resource "azurerm_key_vault_key_encrypt" and data source "azurerm_key_vault_key_decrypt" #8895

Conversation

njuCZ commented Oct 15, 2020

tombuildsstuff commented Oct 19, 2020

This comment has been minimized.

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

IanMoroney Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte 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

njuCZ commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njuCZ commented May 7, 2021

IanMoroney commented May 7, 2021 via email

njuCZ commented May 7, 2021

gro1m commented Jul 30, 2021

tombuildsstuff commented Mar 18, 2022

github-actions bot commented Apr 18, 2022

IanMoroney Mar 2, 2021 •

edited

Loading