-
Notifications
You must be signed in to change notification settings - Fork 15
Decouple SSH configuration from password provisioning #270
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.
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
Decouple SSH configuration from password provisioning #270
Conversation
Password provisioning operations (set_user_password, lock_user) now have zero SSH side effects. SSH configuration updates are controlled by a new config flag and can be managed independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR decouples SSH configuration management from password provisioning, allowing independent control of SSH settings without triggering password logic. Previously, SSH configuration updates were automatically performed when password operations occurred.
Key Changes:
- Added
ssh.update_sshd_configconfiguration flag (defaults totrue) to control SSH configuration updates - Exposed
update_sshd_config()andget_sshd_config_path()as public APIs for explicit SSH management - Refactored
Provision::provision()into separate logical steps and addedprovision_without_ssh_config()method
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libazureinit/src/provision/ssh.rs | Made SSH functions public and updated documentation to reflect decoupling |
| libazureinit/src/provision/mod.rs | Split provisioning into provision_core(), update_ssh_config(), and provision_ssh_keys() methods; changed SSH update logic from password backend detection to config flag |
| libazureinit/src/lib.rs | Re-exported public SSH functions for external use |
| libazureinit/src/config.rs | Added update_sshd_config field to Ssh config struct with default value true |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
libazureinit/src/provision/ssh.rs
Outdated
| // If the "/etc/ssh/sshd_config.d" directory exists, it returns the path for a drop-in configuration file. | ||
| // Otherwise, it defaults to the main SSH configuration file at "/etc/ssh/sshd_config". | ||
| pub(crate) fn get_sshd_config_path() -> &'static str { | ||
| pub fn get_sshd_config_path() -> &'static str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your example this only gets passed as input to update_sshd_config(), and I don't think we necessarily need users to know or care about this detail. What's the use-case for users to need to get ahold of the path outside of the provision API?
Is the overall desire to let users provision without touching the sshd config, or is there a use case where they don't want to provision at all and only touch sshd config? My guess is it's the first, in which case it might be nicer to keep both these private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed these back to private and removed any ability to only touch sshd config without provisioning!
…assword_authentication; making all SSH methods internal and removing ability to alter sshd_config file outside of provisioning
libazureinit/src/provision/mod.rs
Outdated
| /// Performs hostname, user, password, and SSH key provisioning, but skips | ||
| /// SSH configuration updates. | ||
| #[instrument(skip_all)] | ||
| pub fn provision_without_ssh_config(self) -> Result<(), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like something that could/should be handled by configuration or custom calls, would suggest removing it unless needed
Problem
Currently, SSH configuration updates (
PasswordAuthenticationinsshd_config) are tightly coupled with password provisioning. Whenset_user_password()orlock_user()is called during provisioning, SSH configuration is automatically modified based on password provisioner presence. This prevents consumers from independently controlling SSH settings without triggering password logic.Solution
This PR decouples SSH configuration from password provisioning by:
ssh.configure_sshd_password_authentication(default:true)set_user_password()andlock_user()never modify SSH configurationProvision::provision_without_ssh_config()skips SSH config updates entirelyChanges
Configuration
ssh.configure_sshd_password_authentication: boolfield (defaults totruefor backward compatibility)Public API
Provision::provision_without_ssh_config()method for skipping SSH configuration updatesupdate_sshd_config(),get_sshd_config_path()) remain internal (pub(crate))Refactoring
Provision::provision()into logical steps:provision_core()- hostname, user, password (no SSH impact)update_ssh_config()- SSH configuration (separate, gated step)provision_ssh_keys()- authorized_keys provisioningssh.update_sshd_configinstead of password backend typeDocumentation
Performance Improvements
&Pathinstead ofPathBufin SSH functionsprovision_ssh_keys()to consumeself, avoiding string clonesUsage Examples
For consumers wanting to skip SSH configuration:
Option 1: Disable via config
Option 2: Use
provision_without_ssh_config()Backward Compatibility
No changes required. Default behavior is unchanged -
Provision::provision()still updates SSH configuration whenssh.update_sshd_config = true(default).Resolves #265