Skip to content

sources/azure: don't set cfg["password"] for default user pw#1592

Merged
blackboxsw merged 2 commits into
canonical:mainfrom
cjp256:azure-no-set-cfg-password
Jul 21, 2022
Merged

sources/azure: don't set cfg["password"] for default user pw#1592
blackboxsw merged 2 commits into
canonical:mainfrom
cjp256:azure-no-set-cfg-password

Conversation

@cjp256
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 commented Jul 14, 2022

The password is still set for the default (admin) user but isn't
immediately expired as a result of this change:
#1577

Signed-off-by: Chris Patterson cpatterson@microsoft.com

@cjp256
Copy link
Copy Markdown
Contributor Author

cjp256 commented Jul 14, 2022

Actually this may still be a problem if the user already exists in the image based on: #671

The password is still set for the default (admin) user but isn't
immediately expired as a result of this change:
canonical#1577

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@cjp256 cjp256 force-pushed the azure-no-set-cfg-password branch from 58068bb to 6ec3e58 Compare July 14, 2022 18:28
@cjp256 cjp256 closed this Jul 14, 2022
@cjp256 cjp256 reopened this Jul 14, 2022
@TheRealFalcon
Copy link
Copy Markdown
Contributor

TheRealFalcon commented Jul 14, 2022

@cjp256 I believe if you use defuser[plain_text_passwd] defuser[hashed_passwd] rather than defuser[passwd], it will work if the user already exists as well, but it will not expire it.

Looking specifically at https://github.com/canonical/cloud-init/blob/main/cloudinit/distros/__init__.py#L636

passwd gets passed unadulterated to useradd, but hashed_passwd and plain_text_passwd get processed separately by chpasswd, so those two should have the password set regardless of if the user if pre-existing or not.

Comment thread cloudinit/sources/DataSourceAzure.py Outdated
defuser["passwd"] = cfg["password"] = encrypt_pass(
ovf_env.password
)
defuser["passwd"] = encrypt_pass(ovf_env.password)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@TheRealFalcon so is this really just the suggestion then below?

Suggested change
defuser["passwd"] = encrypt_pass(ovf_env.password)
defuser["hashed_passwd"] = encrypt_pass(ovf_env.password)

@TheRealFalcon
Copy link
Copy Markdown
Contributor

I took the liberty of pushing the suggested change to this branch.

@anhvoms , do you happen to have any context for these changes. Particular this comment related to this PR?

We're thinking the changes here should fix the issue mentioned by cjp256. Is there any way you can confirm so we don't continue to break Azure functionality?

@anhvoms
Copy link
Copy Markdown
Contributor

anhvoms commented Jul 15, 2022

I took the liberty of pushing the suggested change to this branch.

@anhvoms , do you happen to have any context for these changes. Particular this comment related to this PR?

We're thinking the changes here should fix the issue mentioned by cjp256. Is there any way you can confirm so we don't continue to break Azure functionality?

I had a discussion last week with Christopher and I am aware of the issue. I will get this tested and update back next week

@anhvoms
Copy link
Copy Markdown
Contributor

anhvoms commented Jul 20, 2022

@TheRealFalcon @blackboxsw I have tested this change and it's working as expected for both scenarios (new user with password, existing user with password)

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Thanks @anhvoms confirmed on our side too this appears to work as expected using hashed_passwd instead of passwd.

@blackboxsw blackboxsw merged commit 1bbbad7 into canonical:main Jul 21, 2022
@cjp256 cjp256 deleted the azure-no-set-cfg-password branch July 25, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants