Add default shell support to sshdconfig #907
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for configuring a default shell in the sshdconfig tool by wiring up new get/set commands to Windows registry keys, updating the DSC resource manifest, and providing Pester tests to verify behavior.
- Implement
get/setforDefaultShellin Rust (src/get.rs,src/set.rs) with Windows registry integration - Extend CLI with clap (
src/args.rs,src/main.rs), update export behavior, and add JSON schema - Add Pester tests (
sshdconfig/tests/defaultshell.tests.ps1) and update DSC manifest (sshdconfig/sshd.dsc.resource.json) and packaging (build.ps1)
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sshdconfig/tests/defaultshell.tests.ps1 | New Pester tests for DefaultShell get/set on Windows and errors on non-Windows |
| sshdconfig/sshd.dsc.resource.json | DSC resource manifest exposing get/set commands for default shell |
| sshdconfig/src/set.rs | Windows registry write/remove logic for DefaultShell |
| sshdconfig/src/get.rs | Windows registry read logic for DefaultShell |
| sshdconfig/src/main.rs | Switch to clap for CLI parsing, wire up get/set/schema |
| sshdconfig/src/metadata.rs | Added RepeatableKeyword enum for future keywords |
| sshdconfig/src/error.rs | Added InvalidInput and RegistryError variants to error enum |
| sshdconfig/src/args.rs | Defined DefaultShell struct and Set subcommand schema |
| sshdconfig/src/export.rs | Changed export to print JSON directly |
| sshdconfig/Cargo.toml | Added clap and conditional registry_lib dependencies |
| build.ps1 | Updated packaging entries for new resource manifest files |
Comments suppressed due to low confidence (1)
build.ps1:64
- Packaging script references 'sshd_config.dsc.resource.json' but this file is not present in the repo. Ensure the file is committed or remove this entry from the build manifest.
'sshd_config.dsc.resource.json',
| Command::Export => invoke_export(), | ||
| Command::Get => invoke_get(), | ||
| Command::Set { input } => invoke_set(input), | ||
| Command::Schema { as_global } => { |
There was a problem hiding this comment.
I think it would be better to have this as an enum given we're likely to add more resources for sshd in the future
| Export, | ||
| /// Get default shell, eventually to be used for `sshd_config` and repeatable keywords | ||
| Get, | ||
| Schema { |
There was a problem hiding this comment.
Would prefer to have it in the order of Get, Set, Test, Export, Schema
| @@ -7,6 +7,8 @@ use thiserror::Error; | |||
| pub enum SshdConfigError { | |||
| #[error("Command: {0}")] | |||
| CommandError(String), | |||
| #[error("IO: {0}")] | |||
There was a problem hiding this comment.
Should probably do the i18n work now before it becomes more of a chore later
| /// | ||
| /// This function will return an error if the desired settings cannot be retrieved. | ||
| pub fn invoke_get() -> Result<(), SshdConfigError> { | ||
| // TODO: distinguish between get commands for default shell, repeatable keywords, and sshd_config |
There was a problem hiding this comment.
Probably make sense for get to take an enum and then use a match to determine which helper function to call
| if let Some(value) = default_shell.value_data { | ||
| match value { | ||
| RegistryValueData::String(s) => { | ||
| let parts: Vec<&str> = s.split_whitespace().collect(); |
There was a problem hiding this comment.
I think it would help future engineers to have a comment indicating that the DefaultShell value is a command-line with the exe and args as a single string.
|
|
||
| #[cfg(not(windows))] | ||
| pub fn get_default_shell() -> Result<(), SshdConfigError> { | ||
| Err(SshdConfigError::InvalidInput("Windows registry operations not applicable to this platform".to_string())) |
There was a problem hiding this comment.
For the user error, it should be "DefaultShell is only applicable to Windows"
| } else { | ||
| schema_for!(SshdConfigParser) | ||
| }; | ||
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); |
There was a problem hiding this comment.
Same here, shouldn't be pretty
|
|
||
| #[cfg(not(windows))] | ||
| pub fn set_default_shell(_shell: Option<String>, _cmd_option: Option<String>, _escape_arguments: Option<bool>, _shell_arguments: Option<Vec<String>>) -> Result<(), SshdConfigError> { | ||
| Err(SshdConfigError::InvalidInput("Windows registry operations not applicable to this platform".to_string())) |
There was a problem hiding this comment.
Message should be: Setting `DefaultShell` is only applicable to Windows
|
|
||
| let mut shell_data = shell.clone(); | ||
| if let Some(shell_args) = shell_arguments { | ||
| let args_str = shell_args.join(" "); |
There was a problem hiding this comment.
shell_args content might need to be enclosed in quotes if it has whitespace or have quote escaped. See https://github.com/dotnet/runtime/blob/ee293c309a9a51f310f29e368e74edb23170ba70/src/libraries/System.Private.CoreLib/src/System/PasteArguments.cs#L10 for an example how to do this properly
| shell_data = format!("{shell} {args_str}"); | ||
| } | ||
|
|
||
| set_registry("DefaultShell", RegistryValueData::String(shell_data))?; |
There was a problem hiding this comment.
DefaultShell should be a const defined somewhere, same with the other reg value names
PR Summary
PR Context