Add-Secret's Secret parameter should not be Object, but SecureString

CJHarmath commented 3 years ago

When adding a secret as below without the Secret parameter, the secret will be visible.

>Add-Secret -Name test
cmdlet Add-Secret at command pipeline position 1
Supply values for the following parameters:
Secret: Sup3rS3cr3t

As you can see if the type is not SecureString, a pasted Secret will be printed which is not ideal for a secret management module.

>Get-Help Add-Secret -Parameter Secret | Select -ExpandProperty Type
name 
----
Object

>Get-Command Add-Secret | Select Source, Version
Source                                                                      Version
------                                                                        ------
Microsoft.PowerShell.SecretsManagement                  0.2.0

CJHarmath commented 3 years ago

Seems like this was already fixed in the master branch, just wasn't published? https://github.com/PowerShell/SecretManagement/blob/master/src/code/SecretManagement.cs#L1366

JustinGrote commented 3 years ago

@CJHarmath It's an object parameter because a secret can be multiple types: (securestring, byte[], PSCredential). It's not just strings.

In RC2 there is a SecureStringSecret parameter for this purpose to avoid the logging/output issue, it's just not surfaced well because the help isn't updated correctly.

Also there is no Add-Secret, it's only Set-Secret (Equivalent of Add-Secret would be Set-Secret -NoClobber if you dont want to accidentally overwrite something already there)

CJHarmath commented 3 years ago

@JustinGrote

Also there is no Add-Secret

Well, here is how I stumbled upon this

google microsoft secrets management
land in here https://devblogs.microsoft.com/powershell/secrets-management-development-release/
link to this package https://www.powershellgallery.com/packages/Microsoft.PowerShell.SecretsManagement/0.1.0-alpha1
install via Install-Module -Name Microsoft.PowerShell.SecretsManagement -RequiredVersion 0.1.0-alpha1 -AllowPrerelease

Which still has Add-Secret.

It's an object parameter because a secret can be multiple types

It's weird for me to say this on the main PowerShell org, but this approach is not the PowerShell way.

There is a difference between supporting multiple parameters with different types and trying to shoehorn an object into a single parameter so that it can support multiple types, then trying to workaround it's security implications by introducing another parameter.

All you need to do is to configure the default parameterset to be the one with SecureString and there is no password printing.

for credential it should be Set-Secret -Credential $cred - please don't reinvent this, it's a done deal for years
for SecureString Set-Secret -Secret $secret
for plaintext Set-Secret -Secret $secret -AsPlainText -Force which should do the same as ConvertTo-SecureString does for protecting privacy
for bytes Set-Secret -Bytes $bytes

https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.security/convertto-securestring?view=powershell-7.1#parameters

Thanks

JustinGrote commented 3 years ago

@CJHarmath I am not part of the powershell team FYI, just providing context :)

There has been a discussion around Add-Secret in the past and you'll have to convince @PaulHigin otherwise why Add-Secret is important over Set-Secret. I agree with your assertion in that regard, there should be more cmdlets that implement the verbs correctly.

As far as parametersets, unfortunately supporting pipelining is what is requiring the Object input usually, you see this often with other cmdlets.

@PaulHigin can provide input but at this point it's past the RFC stage and into the release candidate production-supported stage and no longer alpha, so design changes are likely not going to happen at this point in terms of the parameters because they would constitute breaking changes,, but Add-Secret could be done without being a breaking change.

CJHarmath commented 3 years ago

There has been a discussion around Add-Secret in the past and you'll have to convince @PaulHigin otherwise why Add-Secret is important over Set-Secret.

Actually, I don't mind Set-Secret. That's pretty much a standard. Not having Get-Secret -List bothers me more and printing out password bothers me even more especially when it comes from a big tech company like Microsoft which has already solved this problem a while ago.

As far as parametersets, unfortunately supporting pipelining is what is requiring the Object input usually, you see this often with other cmdlets.

You can surely have multiple types piped in which then gets mapped to the right parameter set based on the object type. Here is a dummy example:

function Test-ParamSets {
    [CmdletBinding(DefaultParameterSetName = 'SecureString')]
    param(
        [Parameter(Mandatory, Position=0)]
        [string]
        $Name,

        [Parameter(Mandatory = $true, ParameterSetName = 'SecureString',Position=1, ValueFromPipeline)]
        [SecureString]
        $SecureString,

        [Parameter(Mandatory = $true, ParameterSetName = 'PSCredential', Position=1, ValueFromPipeline)]
        [PSCredential]
        $Credential

    )
    $PSBoundParameters.Keys | where { $_ -ne "Name"} | % { $PSBoundParameters[$_].GetType().Name }
}

Then testing it

> $cred = Get-Credential foo

Windows PowerShell credential request
Enter your credentials.
Password for user foo: ******
> $cred | Test-ParamSets -Name foo

PSCredential
> $secret = Read-Host -AsSecureString -Prompt "type your secret here"

type your secret here: ************
> $secret | Test-ParamSets -Name secret

SecureString
>

So as you can see I am OK to pipe in a PSCredential or a SecureString and it correctly gets mapped to the right parameter.

JustinGrote commented 3 years ago

Not having Get-Secret -List bothers me more and printing out password bothers me even more especially when it comes from a big tech company like Microsoft which has already solved this problem a while ago.

This exists as Get-SecretInfo. This was done on purpose to prevent people from dumping passwords accidentally. You have to explicitly call the secret you want with Get-Secret -Name <secret> but you can also pipe the secretinformation objects to Get-Secret for the same effect, this was considered enough of a barrier to indicate you really know what you are doing when you do this.

As far as the parameter types, I think there's some legacy stuff around that especially since it is a C# cmdlet. I'm not sure. @PaulHigin what do you think? What was the justification for using Object and then stricting-down the type later? Since it wouldn't effect the API to the extension modules, typed parameterSets could probably be added as a non-breaking change post 1.0

CJHarmath commented 3 years ago

Ah, what I meant by Get-Secret -List is to just list out the secret names, not dumping anything sensitive - that would be really bad. Looking at Get-SecretInfo seem to have the list at least, but it does not seem to be as useful in terms of searching for a secret based on some metadata for example.

I've wrapped HashiCorp's Vault before with PowerShell and some of it's features would have been nice to be factored in here too, such as metadata and list of key value pairs instead of just a dumb secret value.

$ vault kv put secret/hello foo=world excited=yes
$ vault kv get secret/hello

====== Metadata ======
Key              Value
---              -----
created_time     2020-09-02T21:41:17.568155Z
deletion_time    n/a
destroyed        false
version          2

===== Data =====
Key        Value
---        -----
excited    yes
foo        world

https://learn.hashicorp.com/tutorials/vault/getting-started-first-secret?in=vault/getting-started#writing-a-secret

Wonder how would this look like with a Vault extension?

CJHarmath commented 3 years ago

One more thing. At times you might need to store a user name and password but in other times might want to just get it back as a PSCredential. So having a Get-Secret -Name foo -AsCredential would be nice in those cases when you just want to grab a credential don't want to deal with having to convert it. Little things matter. When you store key value pairs like in Vault, these things are as easy as having a username and a password key.

SydneyhSmith commented 3 years ago

Thanks for all the feedback @CJHarmath in our latest release we do include support for SecretMetadata (https://devblogs.microsoft.com/powershell/secretmanagement-and-secretstore-release-candidate-2/), key-value pairs can be saved using either a PSCredential or hashtable....we do have an -AsPlainText switch for Get-Secret and could consider adding -AsCredential in future releases

JustinGrote commented 3 years ago

@CJHarmath the Get-SecretInfo -Filter parameter is meant to be implemented however the extension author wants, so you can search on all kinds of metadata however you implement that syntax (you could do OData, KQL, whatever you want). For instance the Chromium extension has its own rudimentary filter syntax. https://github.com/justingrote/secretmanagement.chromium#get-secretinfo-filter-usage

PaulHigin commented 3 years ago

@CJHarmath Your original example uses a very old alpha version of SecretManagement. Be sure you are using the latest version in your testing.

https://www.powershellgallery.com/packages/Microsoft.PowerShell.SecretManagement/0.9.1

PaulHigin commented 3 years ago

I am marking this as answered and closing the issue.

PowerShell / SecretManagement

Add-Secret's Secret parameter should not be Object, but SecureString #113