Windows - Secret vault DSC resources#1496
Windows - Secret vault DSC resources#1496mimachniak wants to merge 4 commits intoPowerShell:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new Windows DSC resources for managing PowerShell SecretStore configuration and secrets, and adjusts sshdconfig behavior/tests around missing sshd_config handling.
Changes:
- Introduces
Microsoft.Windows/SecretStoreVaultConfigandMicrosoft.Windows/SecretStoreSecretresources backed by a newsecret_storeRust binary. - Adds Pester tests for get/set/test/export operations for the new SecretStore resources.
- Changes
sshdconfig setbehavior for missing files (notably_purge=falsenow errors instead of seeding/initializing), and updates tests/localization accordingly.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| resources/sshdconfig/tests/sshdconfigRepeatList.tests.ps1 | Removes missing-target file contexts/tests for repeat-list scenarios. |
| resources/sshdconfig/tests/sshdconfigRepeat.tests.ps1 | Removes missing-target file contexts/tests for repeat scenarios. |
| resources/sshdconfig/tests/sshdconfig.set.tests.ps1 | Updates missing-file behavior tests for _purge=false to expect failure and new error text. |
| resources/sshdconfig/tests/sshdconfig.get.tests.ps1 | Tweaks missing-file test naming/assertions. |
| resources/sshdconfig/src/util.rs | Removes ensure_sshd_config_exists seeding logic. |
| resources/sshdconfig/src/set.rs | Updates set flow to error when _purge=false and file missing; treats missing file as empty config in another path. |
| resources/sshdconfig/src/error.rs | Removes ConfigInitRequired error variant. |
| resources/sshdconfig/locales/en-us.toml | Removes init-required strings; adds new _purge=false missing-file message. |
| resources/secret_store/tests/vault_config_test.tests.ps1 | Adds test operation coverage for vault config. |
| resources/secret_store/tests/vault_config_set.tests.ps1 | Adds set operation coverage for vault config. |
| resources/secret_store/tests/vault_config_get.tests.ps1 | Adds get operation coverage for vault config. |
| resources/secret_store/tests/secret_set.tests.ps1 | Adds set operation coverage for secrets (create/update/remove/metadata). |
| resources/secret_store/tests/secret_get.tests.ps1 | Adds get operation coverage for secrets. |
| resources/secret_store/tests/secret_export.tests.ps1 | Adds export operation coverage for secrets. |
| resources/secret_store/src/vault_config.rs | Implements vault config get/set/test via pwsh + SecretStore cmdlets. |
| resources/secret_store/src/types.rs | Defines resource input/output types and unit tests for serde/display. |
| resources/secret_store/src/secret.rs | Implements secret get/set/export via pwsh + SecretManagement cmdlets. |
| resources/secret_store/src/main.rs | Adds CLI dispatcher for vault-config/secret operations and stdin/--input handling. |
| resources/secret_store/secretstorevaultconfig.dsc.resource.json | Adds DSC manifest + schema for vault configuration resource. |
| resources/secret_store/secretstoresecret.dsc.resource.json | Adds DSC manifest + schema for secret resource (get/set/export). |
| resources/secret_store/locales/en-us.toml | Adds localized strings for SecretStore resource errors/messages. |
| resources/secret_store/Cargo.toml | Adds new secret_store crate definition and dependencies. |
| Cargo.toml | Adds resources/secret_store to workspace members/default-members/Windows set. |
| let value = input.value.as_deref().unwrap_or(""); | ||
| let vault_arg = input.vault_name.as_deref() | ||
| .map(|v| format!(" -Vault '{v}'")) | ||
| .unwrap_or_default(); | ||
|
|
||
| // Set the secret value | ||
| let script = format!( | ||
| "Set-Secret -Name '{name}' -Secret '{value}'{vault_arg}" | ||
| ); | ||
| run_pwsh(&script) | ||
| .map_err(|e| t!("secret.setFailed", name = name, error = e).to_string())?; | ||
|
|
||
| // Set metadata if provided | ||
| if let Some(ref metadata) = input.metadata { | ||
| if let Some(obj) = metadata.as_object() { | ||
| let mut hashtable_entries = Vec::new(); | ||
| for (key, val) in obj { | ||
| let val_str = match val { | ||
| serde_json::Value::String(s) => format!("'{s}'"), | ||
| other => other.to_string(), | ||
| }; | ||
| hashtable_entries.push(format!("'{key}' = {val_str}")); | ||
| } | ||
| if !hashtable_entries.is_empty() { | ||
| let ht = hashtable_entries.join("; "); | ||
| let meta_script = format!( | ||
| "Set-SecretInfo -Name '{name}'{vault_arg} -Metadata @{{ {ht} }}" | ||
| ); | ||
| run_pwsh(&meta_script) |
There was a problem hiding this comment.
value, vaultName, and metadata keys/values are interpolated into PowerShell commands without escaping. This is both a command-injection risk and a correctness bug (secrets containing ' will fail to set; metadata keys containing ' will break the hashtable). Prefer passing these as pwsh arguments / JSON input and constructing PowerShell values safely, rather than concatenating them into the command string.
| /// Parse `--input <json>` from command line args, or fall back to stdin. | ||
| fn get_input_json(args: &[String]) -> Option<String> { | ||
| // Check for --input arg | ||
| for i in 0..args.len() { | ||
| if args[i] == "--input" { | ||
| if i + 1 < args.len() { | ||
| return Some(args[i + 1].clone()); | ||
| } |
There was a problem hiding this comment.
get_input_json silently ignores --input when it’s provided without a value, and it doesn’t accept the -i alias used by other resources (e.g. resources/windows_service/src/main.rs:135-147). This can lead to confusing behavior where the command falls back to stdin/defaults instead of failing fast. Consider supporting -i and returning an invalid-args error using the existing main.missingInputValue message when --input/-i is missing a value.
| /// Parse `--input <json>` from command line args, or fall back to stdin. | |
| fn get_input_json(args: &[String]) -> Option<String> { | |
| // Check for --input arg | |
| for i in 0..args.len() { | |
| if args[i] == "--input" { | |
| if i + 1 < args.len() { | |
| return Some(args[i + 1].clone()); | |
| } | |
| /// Parse `--input <json>` or `-i <json>` from command line args, or fall back to stdin. | |
| fn get_input_json(args: &[String]) -> Option<String> { | |
| // Check for --input / -i arg | |
| for i in 0..args.len() { | |
| if args[i] == "--input" || args[i] == "-i" { | |
| if i + 1 < args.len() { | |
| return Some(args[i + 1].clone()); | |
| } | |
| write_error(&t!("main.missingInputValue")); | |
| exit(EXIT_INVALID_ARGS); |
| let mut existing_config = match get_sshd_settings(&get_cmd_info, true) { | ||
| Ok(config) => config, | ||
| Err(SshdConfigError::FileNotFound(_)) => { | ||
| return Err(SshdConfigError::InvalidInput( | ||
| t!("set.purgeFalseRequiresExistingFile").to_string() | ||
| )); | ||
| } | ||
| Err(e) => return Err(e), | ||
| }; |
There was a problem hiding this comment.
The PR description focuses on adding Windows SecretStore/SecretManagement DSC resources, but this change set also alters sshdconfig behavior (removing ensure_sshd_config_exists and deleting Windows/non-Windows missing-target tests). If this behavior change is intentional, it should be called out explicitly in the PR description/release notes to avoid surprises for consumers relying on the previous auto-seeding behavior on Windows.
| "description": "Manages individual secrets in the Microsoft.PowerShell.SecretStore vault on Windows. Supports creating, updating, removing, and exporting secrets with optional metadata.", | ||
| "type": "object", | ||
| "required": [ | ||
| "name" | ||
| ], | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "name": { | ||
| "type": "string", | ||
| "title": "Secret name", | ||
| "description": "The name of the secret. Required for get and set operations. For export, this is optional and wildcards (*) are supported." | ||
| }, |
There was a problem hiding this comment.
The embedded schema marks name as required, but the schema description says name is optional for export (wildcards supported) and the implementation also supports export with no input. Having required: ["name"] can cause schema-based validation to reject valid export scenarios (e.g. an empty filter object). Consider removing the top-level required constraint and enforcing name at runtime only for get/set operations (as the code already does).
| $input = '{}' | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | ||
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | ||
|
|
||
| $result = $input | ConvertFrom-Json |
There was a problem hiding this comment.
In this test, $result is parsed from $input (the JSON you sent) instead of from $json (the command output). That makes the assertion meaningless and will pass/fail incorrectly. Parse $json here so you're validating the DSC test result.
| $input = '{}' | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | |
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | |
| $result = $input | ConvertFrom-Json | |
| $json = '{}' | dsc resource test -r $resourceType -f - 2>$testdrive/error.log | |
| $LASTEXITCODE | Should -Be 0 -Because (Get-Content -Raw $testdrive/error.log) | |
| $result = $json | ConvertFrom-Json |
| $results = $raw | ConvertFrom-Json | ||
| $names = @($results | ForEach-Object { | ||
| if ($_.resources) { $_.resources[0].properties.name } else { $_.name } | ||
| }) | ||
| $names | Should -Contain $testSecretName1 | ||
| $names | Should -Contain $testSecretName2 |
There was a problem hiding this comment.
The export output from dsc resource export is typically a wrapper object with a resources array (see e.g. resources/windows_service/tests/windows_service_export.tests.ps1:17-18). This test treats $results as a sequence and, in the wrapper case, only inspects resources[0], which can miss the secrets created in BeforeAll and make the test flaky. Update the parsing so it enumerates all exported resources (e.g., iterate $results.resources when present).
| $results = @($raw | ConvertFrom-Json) | ||
| $results.Count | Should -BeGreaterOrEqual 1 | ||
| $secret = if ($results[0].resources) { $results[0].resources[0].properties } else { $results[0] } | ||
| $secret.name | Should -BeExactly $testSecretName1 | ||
| $secret._exist | Should -BeTrue | ||
| $secret.PSObject.Properties.Name | Should -Contain 'secretType' | ||
| $secret.PSObject.Properties.Name | Should -Contain 'vaultName' | ||
| } |
There was a problem hiding this comment.
Similar to the previous test: when export returns a wrapper object with a resources array, indexing resources[0] assumes ordering and may not select $testSecretName1. This can cause intermittent failures depending on export ordering. Instead, select the exported resource whose properties.name matches the expected secret name before asserting properties.
| let name = input.name.as_deref() | ||
| .ok_or_else(|| t!("secret.nameRequired").to_string())?; | ||
|
|
||
| let vault_arg = input.vault_name.as_deref() | ||
| .map(|v| format!(" -Vault '{v}'")) | ||
| .unwrap_or_default(); | ||
|
|
||
| let script = format!( | ||
| r#" | ||
| $info = Get-SecretInfo -Name '{name}'{vault_arg} -ErrorAction SilentlyContinue | ||
| if ($null -eq $info) {{ | ||
| @{{ name = '{name}'; _exist = $false }} | ConvertTo-Json -Compress | ||
| }} else {{ |
There was a problem hiding this comment.
User-controlled values (name / vaultName) are embedded directly into a PowerShell script inside single quotes. If these values contain a single-quote (') or PowerShell metacharacters, the script can break or be exploited for command injection. Avoid string interpolation for arguments: pass values as pwsh arguments (so they arrive as $args[...]/param(...)), or escape single-quotes properly before embedding.
PR Summary
Add as configuration Windows secret store as resource for better secuirty context
PR Context