Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

[SecretsManagement] Get-Secret should return plain text by default. #47

Closed
stknohg opened this issue Feb 17, 2020 · 6 comments
Closed
Labels
Issue-Enhancement Feature request or suggestion Module-SecretsManagement Issues for the Microsoft.PowerShell.SecretsManagement module Resolution-ByDesign

Comments

@stknohg
Copy link

stknohg commented Feb 17, 2020

Summary of the new feature/enhancement

Currently, Get-Secret returns System.Security.SecureString by default, but now System.Security.SecureString is obsolete.

Proposed technical implementation details (optional)

Get-Secret returns plain text by default, and add -AsSecureString parameter instead of -AsPlainText parameter.
-AsSecureString parameter is for backward compatiblity.

@stknohg stknohg added the Issue-Enhancement Feature request or suggestion label Feb 17, 2020
@JustinGrote
Copy link

@stknohg This one is tricky. While SecureString is indeed deprecated, it's still integrally used by Powershell especially with the [PSCredential] object. It does also help prevent exposing secrets to the console or logs in a way, so while it's not the ideal solution, I don't think plaintext default is a better solution due to the exposure risk mentioned.

@Jaykul
Copy link

Jaykul commented Feb 17, 2020

I disagree. The entire premise of DE0001 is flawed:

The general approach of dealing with credentials is to avoid them and instead rely on other means to authenticate, such as certificates or Windows authentication.

Sure. You should totally avoid credentials and api tokens and secrets. Except in the real world, where they are required for roughly ... everything.

Even if the only purpose of a SecureString is that it doesn't spill the secret into the console, it's preferable to a simple string as the default output.

@JustinGrote
Copy link

JustinGrote commented Feb 17, 2020

"I disagree. The entire premise of DE0001 is flawed"

I think the premise of DE0001 is to not assume SecureString will protect/encrypt your credentials in memory especially on .NET core implementations. It's no longer guaranteed to do that, so using securestring is a false/misunderstood sense of security. "But the string was secure! How come someone was able to just mimikatz it out?"

However 100% that the casual obfuscation it provides is worth continuing to use, and DE0001 is kind of crappy to say "don't use it" without offering a better implementation because as you say, real world tokens need to be used and at least temporarily stored somewhere.

@stknohg
Copy link
Author

stknohg commented Feb 18, 2020

I undarstand that the real world needs SecureString, and I really undarstand that "DE0001 is flawed" reading this ISSUE.

I withdraw "using plain text by default".

But I still think it is better not to use SecureString directly.
To prevent spill the secret into the console, we can use custom class containing supported secret types(byte[], string, PSCredential, Hashtable) and SecureString.

@JustinGrote
Copy link

A custom class makes more sense, its ToString() could indicate what kind of secret it is without divulging the secret itself to the console.

@SydneyhSmith SydneyhSmith added Module-SecretsManagement Issues for the Microsoft.PowerShell.SecretsManagement module Resolution-ByDesign labels Feb 21, 2020
@PaulHigin
Copy link
Contributor

I am closing this as 'by design'. We are aware that SecureString has limitations, and isn't implemented with crypto on Linux platforms, but decided to include it anyway since it at least prevents the secret from being displayed as plain text. We view Get-Secret not returning a string as plain text by default a security in depth mitigation, and will continue with this approach.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Issue-Enhancement Feature request or suggestion Module-SecretsManagement Issues for the Microsoft.PowerShell.SecretsManagement module Resolution-ByDesign
Projects
None yet
Development

No branches or pull requests

5 participants