Skip to content
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

Updating the Secrets Management RFC #234

Closed
wants to merge 5 commits into from

Conversation

SydneyhSmith
Copy link
Collaborator

@SydneyhSmith SydneyhSmith commented Nov 14, 2019

An updated version of the RFC originally proposed by @SteveL-MSFT RFC to securely and simply manage secrets with PowerShell.

Install-Module AKVaultExtension

# In this example, we explicitly register this extension
Register-SecretsVault -Name Azure -ModuleName AKVaultExtension -VaultParamters "_SPT_Parameters_AzKeyVault_"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-VaultParamters - FYI - missing an e on the parameter name here


### PowerShell script implementation

For a script implementation, the PowerShell module must include a subdirectory named 'ImplementingModule' in the same directory containing the module manifest file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any consideration for using classes here instead of lose functions? Class based resources for DSC was a much appreciated improvement.

@rkeithhill
Copy link

Any plans to support Hashicorp's Vault?

@ThomasNieto
Copy link

ThomasNieto commented Dec 4, 2019

@SydneyhSmith please add a change log to your comment for what has changed from the original.

@FriedrichWeinmann
Copy link
Contributor

Pretty awesome stuff.
Only problem I have with this design:
All implementing script modules will have conflicting command names. I'm sure the module can handle that, but it's a bad user experience for installing them in parallel (Lots of -AllowClobber will be needed).

How about instead using the module manifest's PrivateData.PSData region to declare which function implements which functionality?

Could do a pattern like this:

PSData = @{
    Secrets = @{
        Set = 'Set-AkvSecret'
        Get = 'Get-AkvSecret'
        Remove = 'Remove-AkvSecret'
        GetInfo = 'Get-AkvSecretInfo'
    }
}

@SydneyhSmith
Copy link
Collaborator Author

Any plans to support Hashicorp's Vault?

@rkeithhill we will not be implementing/supporting any of the extensions (aside from the built in local vaults), however we are hoping to work with many partners (including hashicorp vault) for them to build out the vault ecosystem

All implementing script modules will have conflicting command names. I'm sure the module can handle that, but it's a bad user experience for installing them in parallel (Lots of -AllowClobber will be needed).

@FriedrichWeinmann That's a great point and something we tried to avoid in the design by asking that the actual module containing the script implementation is in a subdirectory of the vault module and named ImplementingModule... this allows extension vault script module implementations to be hidden from command discovery because the files are in a subfolder

@vexx32
Copy link
Contributor

vexx32 commented Dec 5, 2019

@SydneyhSmith seems like that could be very prone to having one bad/misguided vault provider making life difficult for folx. I see that the plan is to have an interface / abstract class to be implemented by vault providers; perhaps there should be a single set of cmdlets that have a -Vault parameter instead, and/or support vaults as PSDrives so that you can target a single vault by doing Set-Location Secrets:\Vault which could then effectively allow Get-Secret (etc) to determine the vault to use automatically thereafter?

Not sure why a vault provider should be expected to implement entirely their own cmdlets; the filesystem provider does not, as an example. It shares cmdlets with several other providers. If we want a central solution, I think we're going to need to work with something similarly.

@awakecoding
Copy link

@SydneyhSmith we spoke at Microsoft Ignite following your presentation on Get-Secret. I would like to contribute to this RFC, and evaluate the current implementation that corresponds to it, before it becomes final. This PR appears to be the most up-to-date RFC draft, but can you point me to the corresponding WIP code branch? I'd like to build it from source and experiment with it. My main interest is to see how extensible Get-Secret is for third-party credential providers, especially from a vendor point of view. I'm pinging @SteveL-MSFT at the same time because he seems to be heavily involved in this RFC.


### Storing Secrets

The `Add-Secret` cmdlet is used to store a secret.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere else you've used Set as the verb, and then here you switched to Add but you're still implementing it as Set, where it overwrites by default. If it's going to be named Add or New it shouldn't clobber by default, it should have a -Force parameter rather than a -NoClobber switch.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important that if the user leaves this mandatory parameter off, they need to get prompted with the SecureString prompt (where the characters you type aren't visible).


The `Add-Secret` cmdlet is used to store a secret.
The `-Name` must be unique within a vault.
The `-Vault` parameter defaults to the local vault.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a $PSDefaultSecretsVault or a Set-DefaultVault -VaultName <String> command that sets the defaults like:

$PSDefaultParameterValues["Set-Secret:Vault"] = $PSDefaultParameterValues["Get-Secret:Vault"] = $VaultName

Or will we handle that purely by documentation?

@hbro
Copy link

hbro commented Feb 7, 2020

Any plans to implement a persistence parameter in Add-Secret, or a 'default persistence' parameter in the BuiltInLocalVault?

I know that CredMan has a persistence setting for each entry, which can either be LOCAL_MACHINE, ENTERPRISE (user) or SESSION. On Windows, I assume that BuiltInLocalVault defaults the entry in CredMan to a persistence setting of ENTERPRISE. I'm not sure if a similar setting exists for other password vaults though, so it might be too confusing to expose the setting for one vault but not another.

The benefit of a persistence parameter would be from a security perspective. I prefer to have certain secrets get deleted automatically when I log off from my session, instead of being cached (though encrypted) indefinitely. Sure, I could just store a secret in a PowerShell variable, but I do really like the comfort of sharing secrets across PowerShell sessions.

A log-off script could also do the trick of course, but built-in would be a bit cleaner and less invasive.

Any thoughts?

@Jaykul
Copy link
Contributor

Jaykul commented Feb 13, 2020

Can we refine the RFC to specify that the main module MUST import the script extension modules once, when they are first used in a given PowerShell session, and then let them persist in the session and keep their script:scope state?

The `Add-Secret` cmdlet is used to store a secret.
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent with the requirement that the -Name argument must be unique. What you're saying is: the name must be unique, but you're going to overwrite it by default? That's not right.

public abstract KeyValuePair<string, string>[] GetSecretInfo(
string filter,
IReadOnlyDictionary<string, object> parameters,
out Exception error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So GetSecretInfo just has to return an array of tuple[string,string]?

The spec here needs to specify that must return the name with the key "Name". Honestly, why not a PSCustomObject or a hashtable -- is it so that you can order it? Why not an IDictionary and I can return an ordered one if I want?

This is weird, and the way it;s is specified currently, we could have some returning Name and others SecretName etc. That will be a mess.

out Exception error);

public abstract KeyValuePair<string, string>[] GetSecretInfo(
string filter,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the method parameter name filter while the PowerShell Get-SecretInfo has a Name parameter?

@joeyaiello
Copy link
Contributor

Given that this modules has released as a 1.0 GA, and given that it's not part of the mainline PowerShell package, we're closing this RFC. Thank you for the feedback that got us to a 1.0 release!

For any future discussion of issues or features proposals in the SecretManagement or the SecretStore module, please use the appropriate repos:

@joeyaiello joeyaiello closed this May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet