-
Notifications
You must be signed in to change notification settings - Fork 129
Secrets Management RFC #208
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
Conversation
initial draft
Are there any blockers to this being provided as a module itself and NOT a native addition to PS? I can see this capability being useful to lower versions of PS. From the RFC, I'm not seeing anything that couldn't be provided via a module. |
Out of curiosity, what's the motivation for deferring certificates to a future release? Is it the complexity of storing certs in other, non-AKV stores? For my use cases, the ability to store pscredential and securestring secrets is only half a solution. |
@thomasrayner intent was to scope this and incrementally improve over time. We may be able to implement cert support within PS7 timeframe, but it wasn't clear to us if certs is still being used heavily. |
We can certainly see about making this module compatible with WinPS 5.1 and newer. |
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.
If this was made, what I’d use is: local vaults, with a credential stored for the current user and other credentials shared between users.
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 should clearly be an external module shipped through the gallery. Other than that, I have only minor nits to pick with this proposal (in-line).
I'm hoping that one of the goals for this RFC is easy use of credentials in memory once they have been pulled from a (remote) credential store so that they don't have to be cached on a local disk at all. This allows for credentials to be managed in a cloud credential store and used in scripts run elsewhere by users who have access to those credentials. But how is this even going to work? Different systems have different definitions of "credentials", and you mentioned You also mentioned that What about AWS or other systems that don't use either of these? In the Vaults used to store secrets should be able to be registered using whatever credentials they require. Vaults should be able to store an encrypted version of whatever object containing the secret is required by the platform where the secret is used, and users should be able to retrieve those decrypted versions of those objects. The current RFC does not properly support either of those. By limiting the definition of 'Credential' to |
I think that the proposed module has over-simplified the complex process of secret management, and as such, limits its usefulness. It's still a good idea though. Regarding the remote stores, I recommend working backwards from some common providers that exist today. Take a look at the existing Cmdlets and API's for AWS Secrets Manager, Thycotic Secret Server, Azure Key Vault etc. That's how people do "Remote" secret management today. Take note that secrets are never returned as a PSCredential; they're always a string or binary value. The plaintext value is also included as a property of an object rather than just a value; it usually also contains the secret ID, the version ID, and the last modified date as a bare minimum. It would make sense to return an object with similar properties, or better yet, define an ISecret interface that providers can implement, with some common properties like the ones listed above. |
@@ -45,8 +46,8 @@ Registering and using remote secrets: | |||
Install-Module Az.KeyVault | |||
|
|||
# In this example, we explicitly register this extension | |||
Register-SecretsVaultExtension -Name Azure -Cmdlet Get-AzKeyVaultSecret ` | |||
-Module Az.KeyVault -Version 1.0.0 | |||
Register-SecretsVault -Name Azure -Cmdlet Get-AzKeyVaultSecret ` |
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.
Should be Register-SecretVault (pluralization in cmdlet names)
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 true. Seems like you missed @SteveL-MSFT's reply about that. SecretsVault
is singular.
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.
While there might only be one "SecretsVault", and it might actually contain multiple "Secrets", the point of the "No Plurals" rule is about how plurals translate. At this point in the ecosystem, it's also about a reasonable expectation.
I do not believe that any PowerShell user will assume that Register-SecretVault will register a vault and then make it secret (it kinda flies in the face of the cmdlet name). I do believe that, especially as we have "Add-Secret", "Register-SecretsVault" would end up being confusing in practice. Your brain would remember one or the other, and you'd be more likely to flub it.
The other rationale, past pure pluralization, is the desire for common noun roots and the easy memorization that comes with it.
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.
One other call out here, now that I see it, is that it shold support a -Parameter @{} ,which will be passed down to the undelying command. The scenario for this is that my cmdlet fits fine, but I want to supply default parameters to map to the underlying store.
Register-SecretVault -Name Azure Cmdlet Get-AzKeyVaultSecret -Module AZ.KeyVault -Version 1.0.0 -Parameter @{VaultName='MyVault;Location="West US"}
As I type out that bunch of code, I believe it would be nice to accept a command via pipeline, or to look up the command / assume defaults given the name. e.g.
Get-Command Get-AzureKeyVaultSecret | Regster-SecretVault # -Name would be AzureKeyVaultSecret, unless specified
Register-SecretVault -Name Azure -Cmdlet Get-AzKeyVaultSecret # finds the command and supplies module / version, maybe complains or warns if there is no module
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.
One more thing is that the -Cmdlet parameter is somewhat nonstandard. In a quick check of PS 5.1, the only cmds with -Cmdlet are:
- Export-ModuleMember
- Import-Module
- New-Module
All of which actually need cmdlet.
Conversely, it's -Commad on:
- Get/Remove Job
- Invoke-Expression
- Trace-Command
- Get-PSBreakpoint /Set-PSBreakpoint
The last pair is especially instructive here, as I recall a discussion when naming that parameter on this topic. We wanted to avoid naming command breakpoints -Cmdlet because they could use commands that were not Cmdelts.
Unless you're saying this secret vault has to be a Cmdlet, the parameter should be -Command
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.
SecretsVault
indicates a single vault containing multiple secrets. SecretVault
indicates a single vault that itself is secret. I am with @KirkMunro on this one, the proposed noun is singular as the noun itself here is Vault
while Secrets
is describing what the vault itself is for.
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 to beat the drum too much, but the Cmdlet Guidelines don't offer with that rationale.
"Plural parameter names should be used only in those cases where the value of the parameter is always a multiple-element value. In these cases, the cmdlet should verify that multiple elements are supplied, and the cmdlet should display a warning to the user if multiple elements are not supplied."
Are we registering N Secret(s)Vault(s) at a time? No. In fact we probably are only ever registering one or two.
Also, note the natural confusion in where this could be pluralized, and realize this is why people should not pluralize cmdlet names. We've got about a decade of establishment of this paradigm. Why start to break good habits now?
@@ -45,8 +46,8 @@ Registering and using remote secrets: | |||
Install-Module Az.KeyVault | |||
|
|||
# In this example, we explicitly register this extension | |||
Register-SecretsVaultExtension -Name Azure -Cmdlet Get-AzKeyVaultSecret ` | |||
-Module Az.KeyVault -Version 1.0.0 | |||
Register-SecretsVault -Name Azure -Cmdlet Get-AzKeyVaultSecret ` |
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.
One other call out here, now that I see it, is that it shold support a -Parameter @{} ,which will be passed down to the undelying command. The scenario for this is that my cmdlet fits fine, but I want to supply default parameters to map to the underlying store.
Register-SecretVault -Name Azure Cmdlet Get-AzKeyVaultSecret -Module AZ.KeyVault -Version 1.0.0 -Parameter @{VaultName='MyVault;Location="West US"}
As I type out that bunch of code, I believe it would be nice to accept a command via pipeline, or to look up the command / assume defaults given the name. e.g.
Get-Command Get-AzureKeyVaultSecret | Regster-SecretVault # -Name would be AzureKeyVaultSecret, unless specified
Register-SecretVault -Name Azure -Cmdlet Get-AzKeyVaultSecret # finds the command and supplies module / version, maybe complains or warns if there is no module
@@ -45,8 +46,8 @@ Registering and using remote secrets: | |||
Install-Module Az.KeyVault | |||
|
|||
# In this example, we explicitly register this extension | |||
Register-SecretsVaultExtension -Name Azure -Cmdlet Get-AzKeyVaultSecret ` | |||
-Module Az.KeyVault -Version 1.0.0 | |||
Register-SecretsVault -Name Azure -Cmdlet Get-AzKeyVaultSecret ` |
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.
One more thing is that the -Cmdlet parameter is somewhat nonstandard. In a quick check of PS 5.1, the only cmds with -Cmdlet are:
- Export-ModuleMember
- Import-Module
- New-Module
All of which actually need cmdlet.
Conversely, it's -Commad on:
- Get/Remove Job
- Invoke-Expression
- Trace-Command
- Get-PSBreakpoint /Set-PSBreakpoint
The last pair is especially instructive here, as I recall a discussion when naming that parameter on this topic. We wanted to avoid naming command breakpoints -Cmdlet because they could use commands that were not Cmdelts.
Unless you're saying this secret vault has to be a Cmdlet, the parameter should be -Command
The items you're quoting @StartAutomating are specific to parameter pluralization though, not pluralization of the noun in the cmdlet name itself. There are no guidelines specified in the linked document, that I can see, regarding ensuring cmdlet names are singular. Regarding your assumption about "only ever registering one or two", you point out the possibility of having multiple secrets in a single vault. The cmdlet itself is scoped per vault, not per secret, so indicating that the vault can store and retrieve multiple secrets within it points at the value of |
The point is that plurals are frowned upon. Not that guidance is perfect. I've embedded a little helper thought experiment for you.
If you look, there are very few cmdlet names that even have something plural-like. The vast majority of those are not PowerShell team cmdlets, but Azure. A number of other ones have a better claim to "not being a plural", like "BIOS". On my box, these unique plurals represent only about 1% of the cmdlet names. If you all for duplication, they only represent around 15% of all cmdlet names. So, instead of arguing so hard to make this one a "Secrets" vault, ask why it should be the exception to the generally established rule. |
The exception to the rule is the same argument: a There aren't too many occurrences in the overall English language where you use a plural adjective to describe a singular noun, so I'm not surprised at the lack of matches that sample code returns. Using the singular form of |
Maybe a better command that satisfies all arguments here would be to drop |
That's too generic, IMHO. |
I think that there's some crosstalking going on here. I cede your point about the English language. The primary point I am making is consistency with the way most things in PowerShell work, and with itself. This point has a few facets:
I think the short name of Get-Vault, Register-Vault, and Unregister-Vault would be fine, though I'd prever Get/Add/Remove (see the bit on tab completion below) By the above rationale:
To Kirk's comment about genericness, a few points:
|
|
Backup and recovery vaults in cloud services. As for which is more appropriate for a generic vault term, that's not a call that I can make. Personal experience says both are used/known, and specific nouns are better. |
…type and validate our design
Register-SecretsVault -Name AzKeyVault { | ||
param($Name) | ||
Get-AzKeyVaultSecret -VaultName (Get-Secret AzureKeyVaultName) -Name $Name | | ||
Select -Expand SecretValue |
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.
The downside to this kind of implementation (as opposed to a C# implementation with standardized implementations) is that there's no consistency as to how things like error handling work.
Can we include a list of standard error messages that providers should use for a few common scenarios such as:
- Access Denied
- Not Found
If providers all implement the exact same error handling for these scenarios, then it will make switching between providers much easier, as error handling code won't have to be rewritten.
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.
As ideal as standardized error codes are, I do not believe this is something where one could guarantee underlying consistency. A web-based SecretVault will have a different return code under the covers compared to a C-api based SecretVault.
We could encourage people to use standard -ErrorIDs ( or at least a standard -ErrorCategory) to attempt to address the problem. Luckily, -ErrorCategory already contains the two we need: ObjectNotFound and PermissionDenied (or AuthenticationError).
As far as the need/want for a C# api, one can always use PowerShell's C# api to treat these cmdlets as a C# API. e.g.
PowerShell.Create().AddScript('Get-Secret MySecret').Invoke<string>();
Thus I believe not only isn't it in scope for this RFC, it shouldn't be for any future RFCs. I believe to do so opens a nasty pandora's box which would encourage other aspects of PowerShell to provide a C# api as well. This in turn de-emphasizes the need or want to ever write scripts.
Thus I believe we should specify standard error categories, but should not be in the habit of making special C# wrappers for PowerShell.
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.
Cool, the important part of my comment above was the standardization of errors. C# makes it easier to enforce standards, but having a section in the RFC stating that providers should implement the existing error categories/ID's to ensure consistency between different providers is the next best thing.
For example, if one had multiple remote providers and wanted to find a secret but wasn't certain which provider contained it (e.g. during a migration from one provider to another) then they could write a script that iterates through each installed remote provider and puts a "continue" in the catch block if it encounters an exception in the "ObjectNotFound" category. It would suck to have to handle each provider's error in a different way.
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.
ErrorCategory
is an enum, so not sure what needs to be explicitly stated here.
I had a little language integration thought that could make this feature more interesting / a better draw to vNext: It would be interesting to add a standard parameter name to look up and use these secrets:, say -SecretParameter @{NameOfParameter=NameOfSecret} The reason I say this is that I have written a lot of commands wrapping the -SecureSetting commands (which do a similar thing). A number of these commands are written with a parameter that can either be the secret value or the name of the secret (for instance -ConnectionStringOrSetting in Select-Sql). Essentially, having a -SecretParameter common parameter would enable this sort of scenario for any command without any authors work, and could act as an effective "hook" to encourage migration to vNext. I personally have seen secure scripting to be a killer feature for a long time, and see this sort of language integration as a great carrot to draw people into vNext. Additionally, since one can hook and override common parameters, this wouldn't break if someone were to implement their own support for -SecretParameter downlevel. |
@StartAutomating That's an interesting idea, however, it's out of scope for this initial version. I'll add it to the future considerations section. |
Just a few thoughts as I finally get around to reading all the comments here:
But yeah, on the whole, I think it's a good scoping for a good start. If we go with the Gallery approach for pre-release, we can also mess with the surface area of the objects if we find certain patterns to be more/less useful in practice. |
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 don't have anything blocking here, other than being sure that we get some solid usage/output examples.
|
||
Secrets supported will be: | ||
|
||
- PSCredential |
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.
network credential?
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.
Is that a secret of any kind? It has a plaintext password property.
```powershell | ||
# For local vault, we can register custom secrets | ||
# In this example, we store a PSCredential object | ||
Add-Secret -Secret $cred -Name MyCreds |
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.
Is it possible to add tags? It seems quite useful to add additional descriptive data (and it looks to be supported by the back end store). Also, I would like to have a description of all the parameters.
I assume this supports ShouldProcess
, but want to be explicit.
Also, what is the nature of the object that is returned by some of these cmdlets?
be splatted to the extension cmdlet to support additional metadata needed | ||
by the extension (such as authentication). | ||
|
||
When using this model, the extension cmdlet would have to match the parameters and |
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 is pretty sketchy - would you please provide a demo.txt, or similar?
The `-Name` must be unique within a vault. | ||
The `-Vault` parameter defaults to the local vault. | ||
A `-NoClobber` parameter will cause this cmdlet to fail if the secret already exists. | ||
A `-Secret` parameter accepts one of the supported types outlined below. |
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 is where I would love to have some sort of -Tag
or similar
|
||
In this release, the following are non-goals that can be addressed in the future: | ||
|
||
- Provision to rotate certs/access tokens |
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, in any event, as additional code based on the ones described herein.
## High Level Design | ||
|
||
This is a new independent module called `Microsoft.PowerShell.SecretsManagement`. | ||
Secrets are stored securely in a local vault. |
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.
Can you please list the cmdlets exposed from the module? I find it hard to infer from rest of the RFC about exposed cmdlets.
It would be great if you can list all exposed cmdlets at the beginning of the specification section.
|
||
# In this example, we explicitly register this extension | ||
Register-SecretsVault -Name Azure -Cmdlet Get-AzKeyVaultSecret ` | ||
-Module Az.KeyVault |
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.
Maybe move this line to the previous line instead of using a line continuation?
|
||
`User Context` --> `Local Vault` --> `SecretsVault` --> `Remote Vault` | ||
|
||
## User Experience |
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 suggest to move this second to where the extension model is discussed. For someone who is not familiar with Az.KeyVault
, he/she don't even know whether Register-SecretsVault
is from that module or Microsoft.PowerShell.SecretsManagement
.
Moving this to extension model
discussion, it can serve as an example for that discussion.
Closing this as @SydneyhSmith has taken ownership of this RFC going forward and has published an updated one here: #234 |
RFC to securely and simply manage secrets with PowerShell