✨Support reset password for machine #5885

ruifaling · 2021-12-24T01:21:46Z

What this PR does / why we need it:

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

k8s-ci-robot · 2021-12-24T01:21:53Z

Welcome @ruifaling!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-12-24T01:21:54Z

Hi @ruifaling. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-12-24T01:22:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign timothysc after the PR has been reviewed.
You can assign the PR to them by writing /assign @timothysc in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

jichenjc · 2021-12-27T01:35:20Z

bootstrap/kubeadm/internal/cloudinit/controlplane_init.go

@@ -62,6 +63,9 @@ func NewInitControlPlane(input *ControlPlaneInput) ([]byte, error) {
 	input.WriteFiles = input.Certificates.AsFiles()
 	input.WriteFiles = append(input.WriteFiles, input.AdditionalFiles...)
 	input.SentinelFileCommand = sentinelFileCommand
+	if input.AdminPass != "" {
+		input.ResetPwd = "- '(echo " + input.AdminPass + ";sleep 1;echo " + input.AdminPass + ") | passwd root' "


I am not expert in this area
not sure this is the right way to input?
do we have any reference of the usage here?

If you mean the script:[(https://linuxhandbook.com/passwd-command/;)]
Of course, there are different ways to implement it through cloudinit, but I think it's enough to execute this script once by runcmd https://github.com/madorn/cloud-init-guide/blob/master/13-run_commands.txt

fabriziopandini · 2021-12-28T11:17:31Z

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

+
+	//adminPass is the admin pass to inject to the machine
+	// +optional
+	AdminPass string `json:"adminPass"`


If I'm not wrong, according to https://cloudinit.readthedocs.io/en/latest/topics/examples.html#including-users-and-groups this is already doable with

cluster-api/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

Line 282 in 895b82f

Passwd *string `json:"passwd,omitempty"`

Have you considered this option?
/hold

Also, I have some concerns about this approach given that it requires to specify the admin password in plain text into the KubeadmConfig object, and then the same password will be repeated without encryption into the generated data secret that gets passed to machines.

@randomvariable opinions?

After testing, we can use user group to update the password .，and we can put the hashed passwd in the kubeadmConfig.If It is the right way ,I will continue to modify this pr

not sure I got your comment. if you can use users, then no changes are required, or am I missing something?

There need some changes, As the cloudinit remind(and verified by my test), we should use plain_text_passwd or hashed_passwd for already users.For simplicity and uniformity in coding, we can use hashed_passwd.

There need some changes, As the cloudinit remind(and verified by my test), we should use plain_text_passwd or hashed_passwd for already users

So If I'm reading this correctly I think I'd expect the KubeadmConfigSpec.users user input to satisfy and behave accordingly to the above docs without capi adding any additional magic, would that cover your use case?

Please note this feature has security caveats as stated in the cloud init docs. I'd expect this OS tuning to be configured as a day two thing out of band beyond capi for most scenarios. Also this might be good to discuss as part of #5294 about how much we want to get into providing automated management for this and the appropriate UX.

Just an additional data point, maybe it helps.

CAPO uses the existing passwd and lockPassword fields to set a password: (for debugging purposes in e2e tests)

- name: "capi" sudo: "ALL=(ALL) NOPASSWD:ALL" # user: capi, passwd: capi passwd: "$6$rounds=4096$yKTFKL6RmN128$a7cGMiNjeTSd091s6QzZcUNrMTgm3HhML5rVmpDFlCfgD7scTW7ZHr0OChcXCaeiO/kbhdn0XzIzWk63nSqRH1" lockPassword: false

https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/test/e2e/data/ci-artifacts-platform-kustomization.yaml#L21-L33

Not sure if I follow the discussion correctly. Is your use case different / do the existing fields don't work in your case?

This only suit for new users not for already users

not sure I got your comment. if you can use users, then no changes are required, or am I missing something?

There are some changes here. Please re-check,Thanks.

Ah got it.

In my opinion it should be fine to add the new HashedPasswd field, but we cannot just drop the Passwd field from the v1beta1 API as it's a breaking change. So I would suggest to modify the PR to keep the Passwd field.

Ah got it.

In my opinion it should be fine to add the new HashedPasswd field, but we cannot just drop the Passwd field from the v1beta1 API as it's a breaking change. So I would suggest to modify the PR to keep the Passwd field.

so you mean we keep both Passwd and HashedPasswd fileds

sbueringer · 2022-01-05T10:39:15Z

/ok-to-test

sbueringer · 2022-01-05T10:40:32Z

/retest

fabriziopandini

I'm not a cloud-init expert, but the proposed change violates API deprecation guidelines; it also introduces a regression when creating users.

As suggested by @sbueringer, HashedPasswd should be a new, additional field; also comments should be updated describing when to use one or the other

fabriziopandini · 2022-01-05T10:34:27Z

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

@@ -279,7 +279,7 @@ type User struct {

 	// Passwd specifies a hashed password for the user
 	// +optional
-	Passwd *string `json:"passwd,omitempty"`
+	HashedPasswd *string `json:"hashedPasswd,omitempty"`


Renaming an existing field is an API-breaking change and it cannot be implemented without following deprecation guidelines.

Also, given my understanding of the cloud-init code, passwd is used for setting the password of new users, which is a use case we want to continue to support.

ok,I will have some modify

yes ,passwd is used only for new users,and hashed_passwd could used for new users and already exist users.If we want to distinguish them，we can keep both passwd and hashed_passwd field

fabriziopandini · 2022-01-05T10:42:26Z

bootstrap/kubeadm/internal/cloudinit/users.go

-    {{- if .Passwd }}
-    passwd: {{ .Passwd }}
+    {{- if .HashedPasswd }}
+    hashed_passwd: {{ .HashedPasswd }}


See comment above about the need of preserving passwd

sbueringer

Thx, looks great. One suggestion for godocs.

sbueringer · 2022-01-07T11:28:23Z

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

@@ -296,6 +296,10 @@ type User struct {
 	// SSHAuthorizedKeys specifies a list of ssh authorized keys for the user
 	// +optional
 	SSHAuthorizedKeys []string `json:"sshAuthorizedKeys,omitempty"`
+
+	// HashedPasswd specifies a hashed password for the already exist user


Just to emphasize it a bit more.

Suggested change

// HashedPasswd specifies a hashed password for the already exist user

// HashedPasswd specifies a hashed password for the user.

// NOTE: HashedPasswd can be used for existing and new users.

// Passwd specifies a hashed password for the user. // NOTE: Passwd can be only used for new users. // +optional Passwd *string `json:"passwd,omitempty"`

sbueringer · 2022-01-07T11:47:00Z

About the failed tests:

pull-cluster-api-verify-main

Looks like you just have to run make generate-manifests again because of changes to the fields / godoc.

pull-cluster-api-test-main

Some context: we have to ensure the API types are roundtrip-able, which means we're able to convert between different versions of a type (via a conversion webhook). This is necessary to avoid losing some fields when e.g. a controller which is implemented with the v1alpha3 types writes an object of that type and we don't want to lose a field introduced in v1beta1.

The conversions are implemented in v1alpha3/conversion.go and v1alpha4/conversion.go. To make the conversion: v1beta1 => v1alpha3 => v1beta1 (hub => spoke => hub) lossless we do the following:

v1beta1 => v1alpha3: store v1beta1 resource in an annotation (e.g. bootstrap/kubeadm/api/v1alpha3/conversion.go KubeadmConfig.ConvertFrom)
v1alpha3 => v1beta1: unmarshal the annotation from the previous step into a v1beta1 object and restore the fields (e.g. bootstrap/kubeadm/api/v1alpha3/conversion.go KubeadmConfig.ConvertTo)

Concrete: To fix sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha3: TestFuzzyConversion/for_KubeadmConfig/hub-spoke-hub you have to add the following to bootstrap/kubeadm/api/v1alpha3/conversion.go KubeadmConfig.ConvertTo (~ l.56):

	for i, _ := range restored.Spec.Users {
		dst.Spec.Users[i].HashedPasswd = restored.Spec.Users[i].HashedPasswd
	}

There might be other cases where we didn't already implement the backup/restore with the annotation, but those cases should be implemented similarly.

Feel free to ask if you have further questions, the conversion is not exactly the most trivial thing in our codebase :)

ruifaling · 2022-01-12T07:09:44Z

/retest

ruifaling · 2022-01-12T07:45:08Z

/retest

ruifaling · 2022-01-12T09:00:51Z

/retest

ruifaling · 2022-01-12T09:45:01Z

Thanks very much for your suggestions.Now I really have some question.Firstly, when I run make generate,the conversion.go will remove the method Convert_v1beta1_User_To_v1alpha4_User and Convert_v1beta1_User_To_v1alpha3_User,and it will bring some errors,so what makes it remove the method ? Secondly,because the removal of this method caused some errors, I added this method back manually, now the failed test about 'pull-cluster-api-verify-main' is that what caused it ?https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/5885/pull-cluster-api-verify-main/1481189478797152256/build-log.txt

sbueringer · 2022-01-12T14:59:35Z

Adding the method manually won't work as the zz_generated files are just overwritten again by the generation.

I'll take a closer look locally and come back to you.

sbueringer · 2022-01-12T15:26:11Z

@ruifaling

You have to use the zz_generated files generated by the conversion target.

Essentially the conversion tells us that it doesn't know if the generated conversion in autoConvert_v1beta1_User_To_v1alpha3_User is what we want, because it doesn't know how to handle the field which doesn't exist in v1alpha3.

That's why manual intervention is necessary. You have to do the following:

Add the following func in bootstrap/kubeadm/api/v1alpha3/conversion.go

func Convert_v1beta1_User_To_v1alpha3_User(in *bootstrapv1.User, out *User, s apiconversion.Scope) error {
	// User.HashedPasswd does not exist in kubeadm v1alpha3 API.
	return autoConvert_v1beta1_User_To_v1alpha3_User(in, out, s)
}

Add the following func in bootstrap/kubeadm/api/v1alpha4/conversion.go

func Convert_v1beta1_User_To_v1alpha4_User(in *bootstrapv1.User, out *User, s apiconversion.Scope) error {
	// User.HashedPasswd does not exist in kubeadm v1alpha4 API.
	return autoConvert_v1beta1_User_To_v1alpha4_User(in, out, s)
}

Our version of these funcs just ignores the HasedPasswd field, which is the only thing we can do in that case.

Then the verify job should be happy :)

ruifaling · 2022-01-13T03:05:18Z

@ruifaling

You have to use the zz_generated files generated by the conversion target.

Essentially the conversion tells us that it doesn't know if the generated conversion in autoConvert_v1beta1_User_To_v1alpha3_User is what we want, because it doesn't know how to handle the field which doesn't exist in v1alpha3.

That's why manual intervention is necessary. You have to do the following:

Add the following func in bootstrap/kubeadm/api/v1alpha3/conversion.go
func Convert_v1beta1_User_To_v1alpha3_User(in *bootstrapv1.User, out *User, s apiconversion.Scope) error {
	// User.HashedPasswd does not exist in kubeadm v1alpha3 API.
	return autoConvert_v1beta1_User_To_v1alpha3_User(in, out, s)
}
Add the following func in bootstrap/kubeadm/api/v1alpha4/conversion.go
func Convert_v1beta1_User_To_v1alpha4_User(in *bootstrapv1.User, out *User, s apiconversion.Scope) error {
	// User.HashedPasswd does not exist in kubeadm v1alpha4 API.
	return autoConvert_v1beta1_User_To_v1alpha4_User(in, out, s)
}
Our version of these funcs just ignores the HasedPasswd field, which is the only thing we can do in that case.

Then the verify job should be happy :)

The problem is really solved! Thank you very much for your help. This time PR I learned a lot. As a newcomer to this project, I benefited a lot from your help. Thanks again！！

sbueringer · 2022-01-13T11:09:38Z

You're welcome and thank you for the feedback, that's always nice to hear :)

Can you please squash the commits? We're running Prow in merge-mode which means the commits are preserved when merging (this is done so we're able to generate release notes correctly).

Otherwise lgtm from my side.

fabriziopandini

Only one nit on naming (naming i hard 😉) otherwise lgtm

fabriziopandini · 2022-01-13T13:42:03Z

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

@@ -296,6 +297,11 @@ type User struct {
 	// SSHAuthorizedKeys specifies a list of ssh authorized keys for the user
 	// +optional
 	SSHAuthorizedKeys []string `json:"sshAuthorizedKeys,omitempty"`
+
+	// HashedPasswd specifies a hashed password for the user.
+	// NOTE: HashedPasswd can be used for existing and new users.


If I got it right from the cloud-init codebase, HashedPasswd must be used only for existing users, or am I word

Also:

could you kindly move this below Passwd, given that they are logically related

should this be changePasswd or something that makes it more explicit its intended usage

@vincepri @sbueringer @enxebre @CecileRobertMichon opinions?

I could only find the following doc:

Most of these configuration options will not be honored if the user already exists. The following options are the exceptions; they are applied to already-existing users: plain_text_passwd, hashed_passwd, lock_passwd, sudo, ssh_authorized_keys, ssh_redirect_user.
https://cloudinit.readthedocs.io/en/latest/topics/modules.html?highlight=hashed_passwd#users-and-groups

Based on that I would guess that they also work on new users, but it's not clear.

Based on this comment #5885 (comment) it works on new and existing comments (I assume this part was tested @ruifaling?)

Regarding naming, I was under the impression that we're trying to keep our fields roughly in sync with the field names of cloud-init, but I agree their name is not great. I assume the format of passwd and hashedpasswd in cloud-init is the same? If yes and if we don't necessarily want to mirror cloud-init names, +1 to ChangePasswd

HashedPasswd

The result of my test is that hashed_passwd could be used for new users and existing users

I could only find the following doc:

Most of these configuration options will not be honored if the user already exists. The following options are the exceptions; they are applied to already-existing users: plain_text_passwd, hashed_passwd, lock_passwd, sudo, ssh_authorized_keys, ssh_redirect_user.
https://cloudinit.readthedocs.io/en/latest/topics/modules.html?highlight=hashed_passwd#users-and-groups

Based on that I would guess that they also work on new users, but it's not clear.

Based on this comment #5885 (comment) it works on new and existing comments (I assume this part was tested @ruifaling?)

Regarding naming, I was under the impression that we're trying to keep our fields roughly in sync with the field names of cloud-init, but I agree their name is not great. I assume the format of passwd and hashedpasswd in cloud-init is the same? If yes and if we don't necessarily want to mirror cloud-init names, +1 to ChangePasswd

Yes ,I have tested it,hashedpasswd could be used for new users and existing users.
And yes, the format of passwd and hashedpasswd in cloud-init is the same, and the representative field of HashedPasswd in cloudinit is hashed_ passwd.
And HashedPasswd may be a little better than ChangePasswd based on the fact that it is suitable for both new users and existing users?

For me there are two options with different underlying assumptions:

CABPK tries to mirror cloud-init APIs => HashedPasswd would be the right name, to make clear that we're setting the cloud-init hashed_passwd field

CABPK provides a user-friendly API with the ideal field names independent of cloud-init => I think we could:

either keep only Passwd, document that the passwd is hashed (which it already is today) and use Passwdto set (only) the cloud-init hashed_passwd

add HashedPasswd and deprecate Passwd, as it doesn't seem especially useful to have both

=> tl;dr I would try to get eventually to only one field, for which the ideal field name might be Passwd or HashedPasswd

hashed_passwd will not work for Windows - so do that only if that's acceptable for that user group (it better be).

hashed_passwd will not work for Windows - so do that only if that's acceptable for that user group (it better be).

Okay, not sure I got that right :). We shouldn't break Windows user and I think that would be not acceptable for them.

Just that I got it right:

we need cloud-init passwd for Windows because hashed_passwd does not work

we want cloud-init hashed_passwd for Linux users to be able to also set pw's of existing users.

Correct?

It should probably be in a secret, especially if there is going to support a non hashed variant? That way it can be encrypted at rest.

Ok, thanks all for the quick feedback.
I will try to reach out on people working on windows next week after MLK day, but
WRT to "there is going to support a non hashed variant", this is not an option being considered in this PR

@jayunit100 if you have some time could you give an opinion from windows land?

k8s-ci-robot · 2022-01-19T05:35:54Z

@ruifaling: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vincepri · 2022-04-01T15:59:37Z

Closing due to inactivity.

Due to the lengthy discussions inline I'd suggest to follow-up in an issue first before a new PR is opened.

/close

k8s-ci-robot · 2022-04-01T15:59:48Z

@vincepri: Closed this PR.

In response to this:

Closing due to inactivity.

Due to the lengthy discussions inline I'd suggest to follow-up in an issue first before a new PR is opened.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 24, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 24, 2021

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 24, 2021

k8s-ci-robot requested review from JoelSpeed and sbueringer December 24, 2021 01:22

ruifaling mentioned this pull request Dec 24, 2021

Support reset password for vm kubernetes-sigs/cluster-api-provider-openstack#1081

Closed

3 tasks

ruifaling changed the title ~~#5883 Support reset password for machine~~ # Support reset password for machine Dec 24, 2021

ruifaling changed the title ~~# Support reset password for machine~~ Support reset password for machine Dec 24, 2021

ruifaling changed the title ~~Support reset password for machine~~ :sparkles:Support reset password for machine Dec 24, 2021

ruifaling changed the title ~~:sparkles:Support reset password for machine~~ ✨Support reset password for machine Dec 24, 2021

ruifaling changed the title ~~✨Support reset password for machine~~ ✨Support reset password for machine Dec 24, 2021

jichenjc reviewed Dec 27, 2021

View reviewed changes

fabriziopandini reviewed Dec 28, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 28, 2021

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 5, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 5, 2022

fabriziopandini reviewed Jan 5, 2022

View reviewed changes

sbueringer reviewed Jan 7, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 12, 2022

fabriziopandini reviewed Jan 13, 2022

View reviewed changes

use cloudinit to support reset password for machine

f6b4851

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2022

k8s-ci-robot closed this Apr 1, 2022

	// HashedPasswd specifies a hashed password for the already exist user
	// HashedPasswd specifies a hashed password for the user.
	// NOTE: HashedPasswd can be used for existing and new users.

✨Support reset password for machine #5885

✨Support reset password for machine #5885

Conversation

ruifaling commented Dec 24, 2021

k8s-ci-robot commented Dec 24, 2021

k8s-ci-robot commented Dec 24, 2021

k8s-ci-robot commented Dec 24, 2021

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

sbueringer Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Jan 5, 2022

sbueringer commented Jan 5, 2022

fabriziopandini left a comment • edited Loading

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

sbueringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Jan 7, 2022 • edited Loading

ruifaling commented Jan 12, 2022

ruifaling commented Jan 12, 2022

ruifaling commented Jan 12, 2022

ruifaling commented Jan 12, 2022

sbueringer commented Jan 12, 2022

sbueringer commented Jan 12, 2022 • edited Loading

ruifaling commented Jan 13, 2022

sbueringer commented Jan 13, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 14, 2022 • edited Loading

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

k8s-ci-robot commented Jan 19, 2022

vincepri commented Apr 1, 2022

k8s-ci-robot commented Apr 1, 2022

sbueringer Jan 4, 2022 •

edited

Loading

sbueringer Jan 5, 2022 •

edited

Loading

fabriziopandini left a comment •

edited

Loading

sbueringer commented Jan 7, 2022 •

edited

Loading

sbueringer commented Jan 12, 2022 •

edited

Loading

sbueringer Jan 13, 2022 •

edited

Loading

sbueringer Jan 14, 2022 •

edited

Loading