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_storage_account: Support managed HSMs for CMKs #25088

Merged
merged 7 commits into from
May 3, 2024

Conversation

Botje
Copy link
Contributor

@Botje Botje commented Feb 28, 2024

This PR adds separate arguments to the customer_managed_key block to supply HSM-backed CMKs.
For now it just adds a counterpart to the key vault uri / key ID, which does mean that the storage_account_customer_managed_key resource accepts a managed hsm uri + key name + version whereas storage_account accepts a HSM key ID.
Future work could add the HSM key ID to the storage_account_customer_managed_key resource as well.

It does not address the problem raised in this comment by @tombuildsstuff but tries to use the suffixes as published in the environment.

resolves #23321, resolves #14389

@katbyte
Copy link
Collaborator

katbyte commented Feb 28, 2024

I think this may be related to this pr? #25069

@Botje
Copy link
Contributor Author

Botje commented Feb 29, 2024

Thanks for that pointer. Yes, the ManagedHSMKeyID struct and associated parse/validate methods would be very helpful here. Do you suggest I wait until that gets merged and adapt to the new types and methods, or can this be merged separately and can I submit a cleanup PR later? Or I could cherry-pick these changes and hope there are no revisions needed...

@Botje
Copy link
Contributor Author

Botje commented Mar 1, 2024

I rebased my changes on top of (a part of) the mentioned PR.

@riemers
Copy link

riemers commented Mar 6, 2024

Would this not also apply to say keyvault and disk encryption sets as you experience the same issue there too?

@Botje
Copy link
Contributor Author

Botje commented Mar 6, 2024

Absolutely! There is a comment in the source code to the effect that the various implementations of customer_managed_key blocks ought to be centralized so the same code only has to be written once. I have an approach in mind but would prefer to do it after HSM support is added.

@riemers
Copy link

riemers commented Mar 12, 2024

From what i heard, once the #25069 is merged, it should be based of that. As if you now plan/apply on a resource that already has a hsm key it will fail to read the key out properly because of the nestedid's being used.

@Botje
Copy link
Contributor Author

Botje commented Mar 12, 2024

@riemers I am not sure I follow. This PR is partially based on the mentioned PR, see "wuxu92 and others added 5 commits" in the commit list.

As my PR stands currently, the read path for storage_account_customer_managed_key just copies the URI to the managed_hsm_uri attribute and the validation for that is (much too lax, on review) "is it a HTTPS URL".

The read path for storage_account uses an HSM-specific "nested item ID", which really should be renamed to "HSM key id".

If you have spotted something I missed do please tell me via code comment or a simple test case, so I can fix it.

@riemers
Copy link

riemers commented Mar 12, 2024

I overlooked the part where you said "redid the work based on the PR" Just wanted to know if that other PR is merged, if that would fix the Error: internal-error: Managed HSM IDs are not supported as Key Vault Nested Item if the resource gets read out. Since you can still use the azapi provider until the other items are properly supported.

@Botje
Copy link
Contributor Author

Botje commented Mar 12, 2024

#25069 will not fix the issue you mention by itself, because it does not change the function used to process the IDs in the various customer_managed_key blocks that my PR touches.

@riemers
Copy link

riemers commented Mar 13, 2024

Thanks for the info, suppose we just have to wait a bit more until parts are moved forward.

@tombuildsstuff
Copy link
Contributor

Just to aid internal triage (since this otherwise appears ready for review) - since this PR's dependent on a couple of other PRs I'm going to mark this PR as Blocked temporarily until the dependent PR's are addressed - this isn't indicative of an issue in this PR, it's purely to help from an ordering/triage perspective for the moment.

@Botje
Copy link
Contributor Author

Botje commented Mar 22, 2024

Understood, thanks. I will keep track of #25069 and #25324 and rework when needed.

@riemers
Copy link

riemers commented Apr 24, 2024

FYI, there is a internal ticket under IPL-6341 with Hashicorp to look at it. All we can do now is wait i suppose ;)

@katbyte
Copy link
Collaborator

katbyte commented May 2, 2024

#25601 has been merged which has the updated id parsers and validators and unblocks this pr

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM 🏗️

@katbyte katbyte merged commit 982d373 into hashicorp:main May 3, 2024
34 checks passed
@github-actions github-actions bot added this to the v3.102.0 milestone May 3, 2024
katbyte added a commit that referenced this pull request May 3, 2024
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request May 3, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.101.0&#34; to &#34;3.102.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.102.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.102.0&#xA;FEATURES:&#xA;&#xA;*
New Resource: `azurerm_storage_sync_server_endpoint`
([#25831](hashicorp/terraform-provider-azurerm#25831
New Resource: `azurerm_storage_container_immutability_policy`
([#25804](https://github.com/hashicorp/terraform-provider-azurerm/issues/25804))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
`azurerm_load_test` - add support for `encryption`
([#25759](hashicorp/terraform-provider-azurerm#25759
`azurerm_network_connection_monitor` - update validation for
`target_resource_type` and `target_resource_id`
([#25745](hashicorp/terraform-provider-azurerm#25745
`azurerm_mssql_managed_database` - support for a Restorable Database ID
to be used as the `source_database_id` for point in time restore
([#25568](hashicorp/terraform-provider-azurerm#25568
`azurerm_storage_account` - support for the `managed_hsm_key_id`
property
([#25088](hashicorp/terraform-provider-azurerm#25088
`azurerm_storage_account_customer_managed_key` - support for the
`managed_hsm_key_id` property
([#25088](https://github.com/hashicorp/terraform-provider-azurerm/issues/25088))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_linux_function_app` - now sets docker
registry url in `linux_fx_version` by default
([#23911](hashicorp/terraform-provider-azurerm#23911
`azurerm_resource_group` - work around sporadic eventual consistency
errors
([#25758](https://github.com/hashicorp/terraform-provider-azurerm/issues/25758))&#xA;&#xA;DEPRECATIONS:&#xA;&#xA;*
`azurerm_key_vault_managed_hardware_security_module_role_assignment` -
the `vault_base_url` property has been deprecated in favour of the
`managed_hsm_id` property
([#25601](https://github.com/hashicorp/terraform-provider-azurerm/issues/25601))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/148/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
Copy link

github-actions bot commented Jun 2, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants