PowerShell / SecretManagement

PowerShell module to consistent usage of secrets through different extension vaults
MIT License
317 stars 46 forks source link

Get-Secret is Overzealous in SecureString Generation When Returning Hashtables. #202

Closed BinaryWizard904 closed 1 year ago

BinaryWizard904 commented 1 year ago

Prerequisites

Steps to reproduce

If you create a Extension that returns a Hashtable from Get-Secret (say the one I'm working on at: https://github.com/BinaryWizard904/SecretManagement.BitWarden/tree/BackendRewrite ) and design your extension to handle generation of SecureStrings (or lack thereof when -AsPlainText is passed) so you can return a more complex hashtable, Get-Secret will convert any strings in the hashtable into SecureStrings. Worse, if you specify the -AsPlainText argument, only some of the SecureStrings will be returned as strings.

This behavior is unexpected as help for Get-Secret states that "Secrets that are string or SecureString types are returned as SecureString objects by default. Unless the '-AsPlainText' parameter switch is used, in which case the secret is returned as a String type in plain text." Nothing about Hashtables is mentioned.

Expected behavior

Here's an example of the direct (i.e., unwrapped by the SecretManagement module) return of the Get-Secret extension I mentioned earlier. It works as expected.

. './classes/BitwardenEnum.ps1'
. './classes/BitwardenPasswordHistory.ps1'
. './private/ConvertTo-Hashtable.ps1'
. './private/Invoke-BitwardenCLI.ps1'
. './public/Get-Secret.ps1'
. './public/Unlock-SecretVault.ps1'

PS > $password = Read-Host "Please enter password:" -AsSecureString
PS > Unlock-SecretVault -Name "n/a" -Password $password
PS > Get-Secret "test_user" -AdditionalParameters @{}

Name                           Value
----                           -----
Credential                     System.Management.Automation.PSCredential
username                       test_user
totp
password                       System.Security.SecureString
passwordRevisionDate
uris                           {System.Collections.Hashtable}

PS > Get-Secret "test_user" -AdditionalParameters @{} -AsPlainText

Name                           Value
----                           -----
Credential                     System.Management.Automation.PSCredential
username                       test_user
totp
password                       PasSw0rD
passwordRevisionDate
uris                           {System.Collections.Hashtable}

Actual behavior

And here's the same result when I wrap it with the SecretManagement module by registering a vault.

PS > Register-SecretVault -Name "warden" -ModuleName "C:\Git\SecretManagement.BitWarden"
PS > $password = Read-Host "Please enter password:" -AsSecureString
PS > Unlock-SecretVault -Name "warden" -Password $password
PS > Get-Secret "test_user"

Name                           Value
----                           -----
password                       System.Security.SecureString
Credential                     System.Management.Automation.PSCredential
uris                           {System.Collections.Hashtable}
passwordRevisionDate
username                       System.Security.SecureString
totp

Notice that username is now a SecureString.

PS > Get-Secret "test_user" -AsPlainText

Name                           Value
----                           -----
password                       System.Security.SecureString
Credential                     System.Management.Automation.PSCredential
uris                           {System.Collections.Hashtable}
passwordRevisionDate
username                       test_user
totp

Notice that password is still a SecureString.

Error details

No response

Environment data

PS > $PSVersionTable

Name                           Value
----                           -----
PSVersion                      7.2.5
PSEdition                      Core
GitCommitId                    7.2.5
OS                             Microsoft Windows 10.0.19043
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

PS > Get-InstalledModule Microsoft.PowerShell.SecretManagement

Version              Name                                Repository           Description
-------              ----                                ----------           -----------
1.1.2                Microsoft.PowerShell.SecretManagem… PSGallery            This module provides a convenient way for a user to store and retrieve secrets. The secrets are…

Version

N/A

Visuals

No response

PaulHigin commented 1 year ago

SecretManagement only supports hashtable values of known types (byte[], string, SecureString, PSCredential). Any other types are not supported by design (https://github.com/PowerShell/SecretManagement/blob/master/Docs/ARCHITECTURE.md#:~:text=Secret%20objects%20supported%20by%20this%20module%20are%20currently%20limited%20to%3A). The report does not show how secrets are stored or retrieved so I cannot reproduce the issue, but I suspect your extension vault is running into this limitation.

We didn't include other types to support because we couldn't think of use cases that would justify the effort.

But if you wanted to add support for more complex types, you could conceivably serialize into a byte[] or base64 encoded string, and store it that way.

BinaryWizard904 commented 1 year ago

I'm outputting a Hashtable containing Strings, SecureStrings, PSCredential, and nested HashTables containing the same. There are no unsupported types here.

Secrets are retrieved from the Bitwarden CLI, which outputs JSON. Example:

{
  "object": "item",
  "id": "eebe7e08-23bd-4b51-8928-e27970694347",
  "organizationId": null,
  "folderId": null,
  "type": 1,
  "reprompt": 0,
  "name": "test user",
  "notes": null,
  "favorite": false,
  "login": {
    "uris": [
      {
        "match": null,
        "uri": "test.example.com"
      }
    ],
    "username": "test_user",
    "password": "PasSw0rD",
    "totp": null,
    "passwordRevisionDate": "2022-08-09T19:33:38.963Z"
  },
  "collectionIds": [],
  "revisionDate": "2022-08-09T19:33:39.176Z",
  "deletedDate": null,
  "passwordHistory": [
    {
      "lastUsedDate": "2022-08-09T19:33:38.963Z",
      "password": "BY4EMwbxMyyzvcWkbCxnkDo852fS%6^L%bB2t^SVUHUmSR$3XRuKc$7iyz!bJ455"
    }
  ]
}

My implementation of Get-Secret extracts and returns the login, securenote, identity, or card portion of that JSON object as applicable. login in the above example. That information is converted into a Hashtable and returned to the Get-Secret wrapper. exampleSecret.zip contains a CliXml export of one such hashtable. Import it using Import-CliXml to recreate the object in your PowerShell session. You should be able to craft a function that returns it to the Get-Secret wrapper without much of an issue.


I took a look at the code of the wrapper and think I found where my problems stem from. There is a function, ConvertHashtableElements, that loops through top-level objects in hashtables retrieved by Get-Secret and converts them into SecureStrings. This behavior does not appear to be documented so I did not design for it.

https://github.com/PowerShell/SecretManagement/blob/3d279b1222b73b7564d4441cf3908704c369a357/src/code/SecretManagement.cs#L1351-L1378

Unfortunately, there are a few problems with this implementation.

  1. The -AsPlainText flag only works for hashtables if the SecureStrings are generated by the wrapper. If the hashtable contains SecureStrings to start with, -AsPlainText has no effect on them. This is in stark contrast to Strings and SecureStrings where the conversion from one to the other is much more robust.
  2. ConvertHashtableElements only transforms top-level Strings into SecureStrings. If you have nested hashtables with strings (such as the uris array in the example above) they will be output in plaintext unless SecureString functionality is implemented at the extension level. Of course if you do so -AsPlainText won't work there.

This partially implemented, undocumented feature is a real pain. Optimally I'd like the feature to be altered to descend into hashtables, while also adding an option to pass through the -AsPlainText parameter to an extension. That way the String ↔ SecureString conversion could be implemented there in case of complex use cases.

PaulHigin commented 1 year ago

Hashtable is not a supported Hashtable type in SecretManagement (again the supported Hashtable value types are: byte[], string, SecureString, PSCredential). We did briefly consider supporting recursive Hashtable value types, but that opened up the issue of recursion depth and we didn't really see a good use case for it. But it is this limitation that is causing your behavior.

It seems like a bit of a waste to store a large Hashtable object securely when only a few of the fields have sensitive data. But to do this, instead of creating a new extension vault, just use an existing vault (like SecretStore), and convert the Hashtable to a json string and store that. Then when retrieving the Hashtable, retrieve the secret string and convert back into a Hashtable. ConvertTo-Json, ConvertFrom-Json.

BinaryWizard904 commented 1 year ago

I admit I'm a bit surprised that nested hashtables are not supported given how easy it is to create them with a ConvertFrom-Json -AsHashTable command. I'd have thought this would have been encountered before.

To your second point, It is a bit of a waste, but conceptually I don't see any other means of implementing nested hashtables in a manner consistent with the existing implementation. Right now your hashtable implementation converts all properties into a SecureString. It would be reasonably expected that, should support for nested hashtables be added, the same behavior would apply to them as well.

As for why I need to output as complex data, bitwarden returns are complex and nearly completely lacking in required fields. The only required field is the name of the secret; even username and password are not required for a valid login object. Given that, and the fact there is no means for users to specify output format when calling Get-Secret, I feel that the only way to properly implement an extension is to return the entire complex secret. Otherwise I'd be at risk of producing false-negatives.

For now I guess I'll just design the extension to handle any conversion of nested hashtable properties into SecureString itself. Nested HashTables may not be 'supported', but they're not banned either; SecretManagement will return them. Outputting as JSON is not appealing; it defeats the purpose of writing an extension to retrieve secrets as useful PowerShell objects. I can get JSON direct from the underlying CLI.

PaulHigin commented 1 year ago

Maybe you could write a wrapper for SecretManagement that converts json-hashtable, and hides the fact that you are actually storing a json string.

BinaryWizard904 commented 1 year ago

Wouldn't that make SecretManagement entirely superfluous? No standardized interface, no common implementation. I suppose I could create a Proxy Function to extend Microsoft.PowerShell.SecretManagement\Get-Secret with logic that intercepts JSON output and converts it, but that would impact all use of SecretManagment.

Would it be possible to add a configuration option during Register-SecretVault that delegates responsibility for handling the abscence/presence of -AsPlainText to the implementing extension at a per-vault level? Something similar to the SetSecretSupportsMetadata; maybe GetSecretAssumeSecure?

PaulHigin commented 1 year ago

Would it be possible to add a configuration option during Register-SecretVault that delegates responsibility for handling the abscence/presence of -AsPlainText to the implementing extension at a per-vault level?

No, this is considered a security feature.

Wrapper commands are a common way to extend functionality. Or you can create an Enhancement issue request to make recursive hashtable a supported SecretManagement type. I am not sure if it would be accepted or how quickly it would be implemented. If there doesn't seem to be a large need, it may not be accepted.

BinaryWizard904 commented 1 year ago

Have worked around this issue to my satisfaction. Still not ideal, but it is reliable and functional.