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.

Sign up for GitHub

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

Jump to bottom

Install Azure Key Vault gMSA plugin if configured #835

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Install Azure Key Vault gMSA plugin if configured #835

merged 1 commit into from
Mar 29, 2022

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Mar 17, 2022 •
edited
Loading

What this PR does / why we need it:

This installs the key vault ccg plugin on Azure VMs. This allows for Windows gMSA to be used without domain joining the host. It will allow for CAPZ cluster to pass the gMSA upstream windows tests.

I put this into its own role because eventually there could be other gMSA providers and although at first glance this seems like it could only be used by Azure, other providers could potentially interact and store things in Azure keyvault and this would enable that scenario. Some of the registration scripts could eventually be shared as well.

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #

Additional context
Add any other context for the reviewers

/sig windows
/assign @marosset @knabben

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Mar 17, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsturtevant

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2022
@stuartpreston
Copy link

@jsturtevant There definitely will be other CCG plugins ;) - for example we have one prototyped which is a little bit more generic that we will also want to deploy on Azure (not AKS) at some point - unless the CCG/Keyvault plugin is built in the open and we could contribute to that instead? Anyway with that in mind I wonder if that changes any of the naming choices especially around the role naming?

@jsturtevant
Copy link
Contributor Author

looks like the goss commands failed which is interesting since the VM (that i built with this) has the setting correct. Looking into it

@jsturtevant
Copy link
Contributor Author

@jsturtevant There definitely will be other CCG plugins ;) - for example we have one prototyped which is a little bit more generic that we will also want to deploy on Azure (not AKS) at some point - unless the CCG/Keyvault plugin is built in the open and we could contribute to that instead?

Great to hear! It is not opensource right now but will pass on the feedback.

Anyway with that in mind I wonder if that changes any of the naming choices especially around the role naming?

Good point, this plugin works as long as you have connection to Keyvault not only in AKS, so the file name shouldn't be gmsa_aks.yml but instead gmsa_keyvault. I left it generic everywhere else but it makes more sense to make it generic key-vault.

@jsturtevant
Copy link
Contributor Author

@stuartpreston good news! the gmsa plugin is available at https://github.com/microsoft/Azure-Key-Vault-Plugin-gMSA 🥳

@jsturtevant jsturtevant force-pushed the install-gmsa-plugin branch from fae7863 to 2cb4bb0 Compare March 18, 2022 21:39
@jsturtevant
Copy link
Contributor Author

looks like the goss commands failed which is interesting since the VM (that i built with this) has the setting correct. Looking into it

I had a few typos in the powershell commands. Fixed

marosset
marosset reviewed Mar 18, 2022
View reviewed changes
images/capi/ansible/windows/roles/gmsa/files/install-keyvault-gmsa.ps1 Outdated
# limitations under the License.

# script modified from https://github.com/Azure/AgentBaker/blob/8d5323f3b1a622d558e624e5a6b0963229f80b2a/staging/cse/windows/configfunc.ps1 under MIT
function Enable-Privilege {
Copy link
Contributor

Choose a reason for hiding this comment

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

yuck... It would be nice use an MSI or something to install this so we don't need to do any of this privilege escalation in powershell.
I'm not sure how feasible that is tho...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

left a note here: microsoft/Azure-Key-Vault-Plugin-gMSA#2 (comment)

@jsturtevant jsturtevant mentioned this pull request Mar 18, 2022
Open
@jsturtevant jsturtevant force-pushed the install-gmsa-plugin branch 4 times, most recently from f205b4a to bb9cbd7 Compare March 23, 2022 20:33
@marosset
Copy link
Contributor

thanks for the updates.

Powershell code looks good to me (except it would be nice if we didn't need to have any of it!)

@jsturtevant
Copy link
Contributor Author

sigs failure was #766
/retest

knabben
knabben reviewed Mar 23, 2022
View reviewed changes
images/capi/packer/azure/packer-windows.json Outdated
@@ -194,6 +196,7 @@
"cloudbase_plugins_unattend": "cloudbaseinit.plugins.common.mtu.MTUPlugin",
"containerd_url": "",
"containerd_version": null,
"gmsa_keyvault_url": "https://acs-mirror.azureedge.net/ccgakvplugin/v1.1.4/binaries/windows-gmsa-ccgakvplugin-v1.1.4.zip",
Copy link
Member

@knabben knabben Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

more a question - will this value be the default?
the plugin is going to always be installed until the user pass gmsa_keyvault_url="" ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this time, it is only configured to install by default for Azure VM images.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should tihs URL be https://kubernetesartifacts.azureedge.net/ccgakvplugin/v1.1.4/binaries/windows-gmsa-ccgakvplugin-v1.1.4.zip
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@knabben
Copy link
Member

knabben commented Mar 23, 2022

lgtm

will defer the last approve for @marosset

@jsturtevant jsturtevant force-pushed the install-gmsa-plugin branch 5 times, most recently from 7d77474 to cc26db5 Compare March 28, 2022 23:11
@jsturtevant jsturtevant force-pushed the install-gmsa-plugin branch from cc26db5 to 624f89e Compare March 29, 2022 18:56
@jsturtevant jsturtevant force-pushed the install-gmsa-plugin branch from 624f89e to 1310a49 Compare March 29, 2022 18:58
@marosset
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2022
@jsturtevant
Copy link
Contributor Author

/retest

@jsturtevant
Copy link
Contributor Author

looks like a different flake than previous on OVA
/retest

@k8s-ci-robot k8s-ci-robot merged commit 52d9604 into kubernetes-sigs:master Mar 29, 2022
@jsturtevant jsturtevant mentioned this pull request Mar 30, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marosset marosset marosset left review comments

@knabben knabben knabben left review comments

@detiber detiber Awaiting requested review from detiber

@nader-ziada nader-ziada Awaiting requested review from nader-ziada

Assignees

@knabben knabben

@marosset marosset

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jsturtevant @k8s-ci-robot @stuartpreston @marosset @knabben