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

azurerm_key_vault_key: add support for EC curve based keys #1814

Merged
merged 8 commits into from
Jun 18, 2019

Conversation

phekmat
Copy link
Contributor

@phekmat phekmat commented Aug 23, 2018

This PR brings a few improvements to azurerm_key_vault_key:

  • Remove ForceNew on several attributes and create new versions of the key instead of replacing it altogether
  • Fix terraform import for key vault keys, which would show a diff in the key_size after being imported
  • Add support for creating EC keys with curves other than the default (P-256)
  • Add support for creating EC-HSM keys

Fixes #1943

- Remove ForceNew on the `key_size` and `key_type` attributes
- Create a new version of the Key Vault key instead of deleting the key
and replacing it. This matches the behavior of Key Vault secrets when
updating their values
- Get the key size from the public modulus `N` when importing a RSA Key
Vault key
- Add test for importing an RSA key
- Add DiffSuppressFunc for the `key_size` when managing an EC key -- the
field isn't used by Azure when creating/updating EC keys, but creates a
diff since it's a required field
- Add test for importing an EC key
- Add support for ECHSM keys
- Add support for specifying the EC curve
- Make `key_size` optional since it does not apply to EC keys
- Export `x` and `y` attributes for EC keys
- Update docs with new schema details
@ghost ghost added the size/L label Aug 23, 2018
@tombuildsstuff tombuildsstuff added this to the 1.20.0 milestone Oct 25, 2018
@tombuildsstuff tombuildsstuff self-assigned this Oct 25, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 1.20.0, 1.21.0 Dec 6, 2018
@katbyte katbyte modified the milestones: 1.21.0, 2.0.0 Jan 11, 2019
@tombuildsstuff tombuildsstuff modified the milestones: 2.0.0, 1.23.0 Feb 7, 2019
@tombuildsstuff tombuildsstuff modified the milestones: 1.23.0, 1.25.0 Mar 5, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.25.0, v1.24.0 Mar 14, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.24.0, Future Mar 28, 2019
@ghost ghost added the documentation label Jun 18, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @phekmat,

Thank you for the enhancements here, i've taken a look through and left some comments inline.

My main concern was the diffSuppressFunctions as I didn't think they are required & removing force new and conditionally doing so in the Update function.

As such I pulled this down to check and was able to get the tests to pass without the suppresses 🙂 I hope you don't mind but i'm going to push some changes so i can get this merged today.

As I am going to remove the force new (as conditional resource recreation on update is not consistent with the rest of the provider) and i'm not sure what you were trying to accomplish please feel free to open a new PR with further details and just that change.

Thanks again!

azurerm/resource_arm_key_vault_key.go Outdated Show resolved Hide resolved
azurerm/resource_arm_key_vault_key.go Outdated Show resolved Hide resolved
azurerm/resource_arm_key_vault_key.go Outdated Show resolved Hide resolved
azurerm/resource_arm_key_vault_key_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_key_vault_key_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @phekmat,

Thank you for the enhancements here, i've taken a look through and left some comments inline.

My main concern was the diffSuppressFunctions as I didn't think they are required & removing force new and conditionally doing so in the Update function.

As such I pulled this down to check and was able to get the tests to pass without the suppresses 🙂 I hope you don't mind but i'm going to push some changes so i can get this merged today.

As I am going to remove the force new (as conditional resource recreation on update is not consistent with the rest of the provider) and i'm not sure what you were trying to accomplish please feel free to open a new PR with further details and just that change.

Thanks again!

@katbyte katbyte modified the milestones: Future, v1.31.0 Jun 18, 2019
@katbyte katbyte changed the title Fix issues with importing key vault keys, creating EC keys, and creating new versions of keys azurerm_key_vault_key: add support for EC curve based keys Jun 18, 2019
@phekmat
Copy link
Contributor Author

phekmat commented Jun 18, 2019

Thanks for the changes. I definitely don't mind :).

@katbyte katbyte merged commit 5eccd92 into hashicorp:master Jun 18, 2019
katbyte added a commit that referenced this pull request Jun 18, 2019
@ghost
Copy link

ghost commented Jul 19, 2019

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 👉 [email protected]. Thanks!

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

Successfully merging this pull request may close these issues.

Support for Key Vault EC-HSM Keys
4 participants