-
Notifications
You must be signed in to change notification settings - Fork 43
Add support for secret()
function and secret extensions
#908
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
base: main
Are you sure you want to change the base?
Conversation
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 adds support for the new secret()
function in DSC configurations and introduces “secret” extensions that can be discovered and invoked at runtime. It wires the secret capability through discovery, configuration context, and the function evaluator, updates the CLI to list secret support, and provides test manifests and PowerShell end-to-end tests.
- Implemented
secret()
function indsc_lib/src/functions/secret.rs
- Extended the extension manifest, discovery, and
DscExtension
to handle “Secret” capability and argument processing - Added PowerShell integration tests, updated CLI listing to surface secret support
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
dsc_lib/src/functions/secret.rs | New secret() function with argument validation and lookup |
dsc_lib/src/functions/mod.rs | Registered secret in the function dispatcher |
dsc_lib/src/extensions/secret.rs | Defines SecretMethod , SecretArgKind , and SecretResult |
dsc_lib/src/extensions/extension_manifest.rs | Added optional secret section to the extension manifest |
dsc_lib/src/extensions/dscextension.rs | Implemented DscExtension::secret() and added Capability::Secret |
dsc_lib/src/discovery/command_discovery.rs | Load secret capability and expose extensions via discovery |
dsc_lib/src/discovery/mod.rs | Collect discovered extensions into the overall context |
dsc_lib/src/configure/mod.rs | Populate Context.extensions so secret() sees available extensions |
dsc_lib/src/configure/context.rs | Added extensions field to the context struct |
dsc/src/subcommand.rs | Updated list-extensions to show secret (“s”) flag |
extensions/test/secret/* | Test manifests and PowerShell helper script |
dsc/tests/dsc_functions_secret.tests.ps1 | PowerShell end-to-end tests for the secret() function |
dsc_lib/locales/en-us.toml | Added i18n strings for secret function and extension errors |
Comments suppressed due to low confidence (2)
dsc_lib/src/functions/secret.rs:74
- Only a single test (
not_string
) coverssecret()
, leaving scenarios like no extensions, multiple secrets, or successful retrieval untested. Add unit tests to cover these code paths for better coverage.
#[cfg(test)]
dsc_lib/src/functions/secret.rs:33
- An empty string vault name (
''
) will be treated asSome("")
and passed along, causingprocess_secret_args
to include an empty vault argument. You should treat empty string asNone
so that unspecified vaults trigger global lookup as intended.
let vault_name: Option<String> = if args.len() > 1 {
If I'm reading the spec right, DSC will look through all secret extensions in order to find the secret? If I have a few secret extensions can I specify which extension it should be searching? Like in DSC resources we specify the type name. |
The intent is to follow |
The use case I'm thinking of is during vault migration. Lets say I have an existing vault extension using one technology and migrating to a new vault extension. The vaults have the same name in both extensions. A particular secret could exist in the original or new or both depending on the stage. The secret would also have the same name. Specifying the extension would be helpful to differentiate which vault should be used and also reduce time during secret discovery. |
Can you open an issue for vault migration? Or perhaps add your scenario to #673 which is about exporting secrets. In my mind, the capability you're asking for might be better suited for vault DSC resources that support export and set. However, we would need to standardize the schema so it works across vaults. |
The use case I describe really isn't about having DSC handle vault migration but when in the process of migrating from one vault to another you will a period of time where you need to have multiple vault extensions present which may have duplicate secrets.
In my use case both vault extensions would be present on the device for a long time since each team and script would need to update their references from the existing vault extension to the new one on a secret by secret basis. |
In the case of transition from one vault to another, the current code allows for that. Basically, if you simply request |
- name: Echo | ||
type: Microsoft.DSC.Debug/Echo | ||
properties: | ||
output: "[secret('MySecret')]" |
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.
Not the point of this PR, but should we prevent leaking secrets in this way? Accidentally leaking a secure string (and the output of the secret
function is always secure, right?) to the output seems dangerous.
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.
secureString
internally is just a string (as it's a JSON type), the only control DSC has is that itself it won't write it to console or traces.
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.
I'm specifically wondering whether we want to add an issue to prevent things like this - afaik there's only two ways to get a secret value into a configuration document:
- As a parameter.
- With the
secret()
function.
In both cases, DSC is aware that the string value is a secret and shouldn't be emitted (consider the case where the desired state for a resource requires a credential object) - should we mask the value even in the actually-emitted JSON output?
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.
For example, internally we could keep it as a JSON object with a single secureString
(or secureObject
) property, but resources that receive it could unwrap it and DSC wouldn't know. I think it's best effort at best.
PR Summary
This PR adds support for
secret extensions
that get used in the config via thesecret(<name>, [vault])
function.The
secret()
function has the first parameter as the name of the secret and the second optional parameter as the vault name.secureString
benull
indicating that secret was not foundOne of the challenges is plumbing the layers so that the discovered extensions get bubbled up so that the
secret()
function can call extensions to do their work. Basically: command_discovery -> discovery -> configuration_contextExtension manifest extended for new
secret
operation with it's own custom args for passing the secret name and vault name.An extension supporting
secret
operation returns JSON like:Some fixes to i18n strings.
PR Context
Fix #685