PowerShell / Modules

MIT License
112 stars 25 forks source link

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

Closed stknohg closed 4 years ago

stknohg commented 4 years ago

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.

JustinGrote commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

"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 commented 4 years ago

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 commented 4 years ago

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

PaulHigin commented 4 years ago

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.