Disable provisioning with password #57

dongsupark · 2024-03-13T09:49:33Z

Password authentication by itself not as secure as ssh key. For better security, we should disable password authentication. So if user enabled password authentication in Azure, azure-init now simply fails to provision.

Clean up unnecessary functions in libazureinit, like mount_media, unmount_media, allow_password_authentication.

Fixes #52.

Testing done

Manual test is done.

src/main.rs

jeremycline

Just a few style nits which you should feel free to ignore if you like 😄

It would be nice to have a functional test that attempts to provision a host with a password and demonstrate it fails, as well, but that might need a bit more thought on how testing should be organized and whatnot.

src/main.rs

dongsupark · 2024-03-14T16:05:53Z

Tested this PR manually in Azure, with help of the existing script demo/image_creation.sh.
(Though some tweak was needed for running that script.)

A provisioning with a ssh key seems to work well, at least no regression. On that machine, a systemd unit azure-init.service runs well, with an empty password given.

I did not yet manage to actually test the case of failing when a non-empty password.

jeremycline

It looks good to me.

jeremycline · 2024-03-14T20:13:53Z

src/main.rs

+        match imds::get_username(imds_body.clone()) {
+            Ok(username) => Ok(username),
+            Err(_err) => Err("Failed to get username".into()),
+        }


This was a case I missed that could also be a map_err, but it's up to you if you want to adjust it. I'm touching all the errors in #59 anyway and need to rebase it after this is merged so I can adjust it there.

It looks a little tricky touch that in this PR, because main does not return error in this context.
It might be probably better to touch in your PR.

Password authentication by itself not as secure as ssh key. For better security, we should disable password authentication. So if user enabled password authentication in Azure, azure-init now simply fails to provision. Clean up unnecessary functions in libazureinit, like mount_media, unmount_media, allow_password_authentication.

…bled Rename function name from get_provision_with_password to is_password_authentication_disabled, because that function actually returns true if the password authentication is disabled. The original name is rather the opposite of the actual meaning.

dongsupark · 2024-03-15T11:14:31Z

I did not yet manage to actually test the case of failing when a non-empty password.

Also tested the scenario, by passing --admin-password **** to az vm create command.
It works as expected.

anhvoms · 2024-03-15T11:19:52Z

Tested this PR manually in Azure, with help of the existing script demo/image_creation.sh. (Though some tweak was needed for running that script.)

A provisioning with a ssh key seems to work well, at least no regression. On that machine, a systemd unit azure-init.service runs well, with an empty password given.

I did not yet manage to actually test the case of failing when a non-empty password.

You can try to deploy a VM with boot diagnostic enabled, using password. The deployment will fail in about 20 minutes due to OS Provisioning timeout (because the agent returned an error). Assuming the output from azure-init makes it to serial console (if not we should make any err/warn/into output go to console to aid with debugging) we should see the error about non-empty pasword

dongsupark · 2024-03-15T15:22:42Z

You can try to deploy a VM with boot diagnostic enabled, using password. The deployment will fail in about 20 minutes due to OS Provisioning timeout (because the agent returned an error). Assuming the output from azure-init makes it to serial console (if not we should make any err/warn/into output go to console to aid with debugging) we should see the error about non-empty pasword

Thanks for the tip.
It turns out azure-init simply returned success even on failure, so azure-init systemd unit looked ok.
With the latest commit I pushed, now azure-init systemd unit fails when it should fail.

However, provisioning itself seems to run even when the systemd unit failed.
That's why I could not see such a scenario of timeout with 20 min upon provisioning failure.
I assume it has something to do with the issue you created a few days ago, right?
I think that's all I can do for now with this PR.

Currently azure-init does not fail, even when it should fail with error. That's why azure-init systemd service simply keeps running even on failure. Make it exit with exit code, so systemd unit azure-init fails in case of failure.

anhvoms · 2024-03-15T16:06:37Z

You can try to deploy a VM with boot diagnostic enabled, using password. The deployment will fail in about 20 minutes due to OS Provisioning timeout (because the agent returned an error). Assuming the output from azure-init makes it to serial console (if not we should make any err/warn/into output go to console to aid with debugging) we should see the error about non-empty pasword

Thanks for the tip. It turns out azure-init simply returned success even on failure, so azure-init systemd unit looked ok. With the latest commit I pushed, now azure-init systemd unit fails when it should fail.

However, provisioning itself seems to run even when the systemd unit failed. That's why I could not see such a scenario of timeout with 20 min upon provisioning failure. I assume it has something to do with the issue you created a few days ago, right? I think that's all I can do for now with this PR.

I think we should investigate why provisioning didn't timeout. Likely there's another entity/agent in the VM that was reporting health. It would be good to know what it is and if it is expected. We might want to remove such noise in our testing. It shouldn't block this PR, however. See #61

Functions mount_media and remove_media were removed in #57. To parse a ovf_env file, however, both mount_media and remove_media are necessary. Bring back the functions.

Functions mount_media and remove_media were removed in #57. To parse a ovf_env file, however, both mount_media and remove_media are necessary. Bring back the functions. Adjust return types according to the new LibError interface as well.

Functions mount_media and remove_media were removed in #57. To parse a ovf_env file, however, both mount_media and remove_media are necessary. Bring back the functions. Adjust return types according to the new LibError interface as well. Define const PATH_MOUNT_DEVICE, PATH_MOUNT_POINT for path to mount point for media source and target to be mounted, and replace hard-coded path with the new const. Co-authored-by: Jeremy Cline <jeremycline@microsoft.com>

Functions `mount_media` and `remove_media` were removed in #57. To parse a ovf_env file, however, both mount_media and remove_media are necessary. Bring back the functions. Adjust return types according to the new LibError interface as well. Co-authored-by: Jeremy Cline <jeremycline@microsoft.com>

dongsupark force-pushed the disable-password-auth branch from 5762e37 to 0699dfb Compare March 13, 2024 10:57

anhvoms requested changes Mar 13, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

dongsupark force-pushed the disable-password-auth branch from 2db1745 to e49ea68 Compare March 13, 2024 16:36

jeremycline reviewed Mar 13, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

jeremycline mentioned this pull request Mar 13, 2024

Add errors to the library and remove all unwrap()/expect() calls #59

Merged

dongsupark force-pushed the disable-password-auth branch from e49ea68 to a0a37ed Compare March 14, 2024 15:00

dongsupark marked this pull request as ready for review March 14, 2024 15:58

jeremycline approved these changes Mar 14, 2024

View reviewed changes

dongsupark added 2 commits March 15, 2024 11:51

dongsupark force-pushed the disable-password-auth branch from 2bdfc39 to 000a851 Compare March 15, 2024 11:15

make azure-init fail when it met failure

266fc98

Currently azure-init does not fail, even when it should fail with error. That's why azure-init systemd service simply keeps running even on failure. Make it exit with exit code, so systemd unit azure-init fails in case of failure.

dongsupark force-pushed the disable-password-auth branch from 902dd22 to 266fc98 Compare March 15, 2024 15:58

anhvoms approved these changes Mar 15, 2024

View reviewed changes

anhvoms mentioned this pull request Mar 15, 2024

VM provisioning succeeds even when azure-init returns error #61

Open

dongsupark merged commit b4aaf3e into main Mar 18, 2024
2 checks passed

dongsupark deleted the disable-password-auth branch March 18, 2024 14:52

dongsupark added a commit that referenced this pull request Mar 19, 2024

bring back mount_media, remove_media

77ecfb5

Functions mount_media and remove_media were removed in #57. To parse a ovf_env file, however, both mount_media and remove_media are necessary. Bring back the functions.

dongsupark mentioned this pull request Mar 19, 2024

bring back mount_media, remove_media #62

Merged

dongsupark added a commit that referenced this pull request Mar 25, 2024

bring back mount_media, remove_media

e07ba8e

Functions mount_media and remove_media were removed in #57. To parse a ovf_env file, however, both mount_media and remove_media are necessary. Bring back the functions.

dongsupark mentioned this pull request Jun 14, 2024

add Azure Linux support #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable provisioning with password #57

Disable provisioning with password #57

dongsupark commented Mar 13, 2024 •

edited

Loading

jeremycline left a comment

dongsupark commented Mar 14, 2024

jeremycline left a comment

jeremycline Mar 14, 2024

dongsupark Mar 15, 2024

dongsupark commented Mar 15, 2024

anhvoms commented Mar 15, 2024

dongsupark commented Mar 15, 2024

anhvoms commented Mar 15, 2024

Disable provisioning with password #57

Disable provisioning with password #57

Conversation

dongsupark commented Mar 13, 2024 • edited Loading

Testing done

jeremycline left a comment

Choose a reason for hiding this comment

dongsupark commented Mar 14, 2024

jeremycline left a comment

Choose a reason for hiding this comment

jeremycline Mar 14, 2024

Choose a reason for hiding this comment

dongsupark Mar 15, 2024

Choose a reason for hiding this comment

dongsupark commented Mar 15, 2024

anhvoms commented Mar 15, 2024

dongsupark commented Mar 15, 2024

anhvoms commented Mar 15, 2024

dongsupark commented Mar 13, 2024 •

edited

Loading