Feature Request - Update Windows Domain Join policy

QBY-MarkusMaring commented 12 months ago

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

The current policy definition to automatically enable the DomainJoin extension on windows VMs is missing the 2022 SKUs and needs access to the Microsoft.KeyVault/vaults/deploy/action permission to retrieve secret values from a key vault.

Is your feature request related to a problem?

Right now it seems users would have to deploy their own variation of this policy using archetype extensions or their own archetype configuration.

Describe the solution you'd like

I've prepared a solution in this fork which:

Adds all 2022 windows server SKUs to the image filter (commit)
Specifies the Key Vault Contributor role definition in the policy definition (commit)

Key Vault Contributor is the least access provided by any Azure built-in role aside from Desktop Virtualization Virtual Machine Contributor. This is not optimal since this policy will likely be assigned by users on a very high level that includes multiple management groups and key vaults. So an alterative solution might be to not include it and have the user manually assign this permission to the system assigned identity instead.

Additionally this fork adds a policy assignment (here) which was not included previously. However the default parameters make not much sense and would need to be replaced while assigning anyways (like the KeyVaultResourceID pointing to a key vault which won't be deployed in this module). So again an alterative solution might be to not include this in the feature and have users do it manually outside of this module.

Additional context

Let me know if this fork is a suitable solution and can be submitted as a PR. If so:

Should the role definition be added here, or done manually by users?
Should the policy assignment be included if the default parameters are unusable as-is?

matt-FFFFFF commented 11 months ago

Moving upstream

Springstone commented 11 months ago

@QBY-MarkusMaring thank you for raising this fair issue. We'll address it asap, but will likely only land next quarter due to time of year resource constraints. This policy is not assigned by default, so not super high priority, but we'll make necessary changes as your observations are valid. Many thanks for doing the heavy lifting and sharing your work. I would encourage you to do this directly against this repo, as this is open source and only works with great contributions like yours!

QBY-MarkusMaring commented 11 months ago

Perfect. No problem at all. This issue currently isn't time critical on our end either.

In that case I will check the linked repository in the coming days/weeks to prepare a fork and PR. I will also raise my concerns mentioned above regarding least privilege there for discussion.

But I think my second question regarding the policy assignment mentioned above is still a valid issue for this repository. Maybe @matt-FFFFFF you have some input on this? Should the assignment be included here globally once the policy definition is updated upstream or should it be left to the users?

QBY-MarkusMaring commented 11 months ago

Whups. Closing this issue was a miss click :)

Springstone commented 10 months ago

@QBY-MarkusMaring many thanks for raising the issue, and to @matt-FFFFFF for forwarding upstream. Please note that all policy related changes are managed through this repo and are then replicated to the upstream repositories (like Terraform, Bicep).

Since you've already done the hard work, I'd really encourage you to submit a pull request here with your recommended changes, considering that we will not assign this policy by default.

As an additional consideration, have a look at how the OS version support has been generalized in this policy (using wildcards): https://www.azadvertizer.net/azpolicyadvertizer/637125fd-7c39-4b94-bb0a-d331faf333a9.html Makes sense to avoid constant policy updates to support the continued functioning of the policy.

Looking forward to your contribution!

Springstone commented 1 month ago

This has fallen off our list. Added backlog item to have this resolved this quarter (AB#37719).

Azure / Enterprise-Scale