PowerShell / SecretStore

MIT License
156 stars 23 forks source link

Set-SecretStoreConfiguration PasswordRequired unintuitive #18

Closed ThomasNieto closed 3 years ago

ThomasNieto commented 4 years ago

In order to set PasswordRequired to false requires using the unintuitive syntax for switch parameters of -PasswordRequired:$false. Can this parameter be updated to use two different switches as described in #20? Most beginner/immediate PowerShell users won't know that the -PasswordRequired:$false syntax exists. The other issue comes if NoPasswordRequired is used then a double negative has to be use to get the default value -NoPasswordRequired:$false causing user confusion.

PS C:\> Set-SecretStoreConfiguration -PasswordRequired

Confirm
Are you sure you want to perform this action?
Performing the operation "Changes local store configuration" on target "SecretStore module local store".
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"): y

      Scope PasswordRequired PasswordTimeout DoNotPrompt
      ----- ---------------- --------------- -----------
CurrentUser             True             900       False

PS C:\> Set-SecretStoreConfiguration -PasswordRequired:$false

Confirm
Are you sure you want to perform this action?
Performing the operation "Changes local store configuration" on target "SecretStore module local store".
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"): y
Vault Microsoft.PowerShell.SecretStore requires a password.
Enter password:
****
A password is no longer required for the local store configuration.
To complete the change please provide the current password.
Enter password:
****

      Scope PasswordRequired PasswordTimeout DoNotPrompt
      ----- ---------------- --------------- -----------
CurrentUser            False             900       False
PaulHigin commented 3 years ago

This is done on purpose. Removing password required lowers security, so we want to make it a little bit difficult. Default is to always require a password.

ThomasNieto commented 3 years ago

Making it difficult through a bad syntax is not more secure, its just bad user experience...

PaulHigin commented 3 years ago

Hmm, good point. Using boolean parameters is against PowerShell best practices. So I think the switch should be '-NoPasswordRequired'. Default is to require a password, so the switch is only used to turn it off. That seems like a better user experience.

ThomasNieto commented 3 years ago

In #20 @rkeithhill had the suggestion of having two switch parameters, one to enable and the other to disable. There should be a way to turn it on and off without having to set all the settings back to default.

PaulHigin commented 3 years ago

Having two switches seems like an overkill to me. I think -NoPasswordRequired works since default behavior is always password required.

ThomasNieto commented 3 years ago

If a user disables passwords and then decides that's a bad idea. How do they turn it back on without resetting the timeout or other settings?

PaulHigin commented 3 years ago

-NoPasswordRequired:$false This feels like nit picking to me. But I'll mark this for committee review so it can get resolved.

PaulHigin commented 3 years ago

Also '-NoPasswordRequired' feels consistent with the existing '-DoNotPrompt'. Default is password and prompting, and we have switches to change default behavior. But will let committee decided.

ThomasNieto commented 3 years ago

My intention is not to nitpick but to give my perspective. I'll expand my reasoning for yourself and the committee so they can better understand where I'm coming from. I also cc'd some other community members to hear their opinion since this is a brand new repo and they probably haven't seen it yet.

In my experience, with built-in cmdlets and community modules, a switch parameter is an opt-in way to change a specific default behavior on a cmdlet for a single execution. The switch parameter behaves the same way if not passed or set to false. This is because the switch parameter is designed to only change the one default behavior in the opposite way. The proposed design has the switch parameter behave differently when passed/true or false. Please correct me if I'm wrong, but I'm not aware of any other built-in cmdlets that behave this way other than with preference variables. But that is more of a side effect of an issue rather than a standard use case: https://github.com/PowerShell/PowerShell/issues/4568.

Switch parameters usually only modify the current execution of the cmdlet with no impact to future executions. Since this switch parameter sets a persistent configuration item, it changes the syntax and difficulty of reverting the behavior. For example, Set-SecretStoreConfiguration -NoPasswordRequired is the intended way to use the switch parameter, but if the user wants to make passwords required the syntax is more complex Set-SecretStoreConfiguration -NoPasswordRequired:$false not to mention a double negative is confusing. To enable or disable the feature, the user experience should be the same level of syntax difficulty. Since setting false to a switch parameter is such an uncommon practice, most beginner and intermediate PowerShell users may not even know it exists. The help system command syntax also does not indicate that passing false to a switch is a valid option further reducing visibility.

So that's my $0.02 😄 I welcome any thoughts or ideas you may have.

cc: @vexx32, @mklement0, @iSazonov, @rkeithhill

vexx32 commented 3 years ago

Some thoughts:

If we have to have a unique parameter for every configurable option that inevitably leads to parameter bloat. IMO this is not a good pattern to continue.

An alternative would be to have the cmdlet take a -Name value to target a specific setting and a -Value parameter to set that (a completer can be added for convenience for names and/or values). If one wants to set multiple configuration values at once, the cmdlet should take a hashtable of names and values and apply each named setting, writing errors for those that don't exist.

The corresponding Get-* cmdlet can just take a -Name as well to retrieve specific named setting(s) if needed.


Additionally, if this is a sufficiently distinct option that can in itself be considered a bit of a security hazard to enable, I would move this particular configuration option to a completely separate cmdlet, and explicitly prevent setting it with the standard cmdlet. Write an error if they try.

So, create two small cmdlets for Enable/Disable-SecretStoreRequirePassword that by default prompt via ShouldProcess() (ConfirmImpact set to High). This lets users know "hey you DO need to think twice about this" and also allows them to override it with -Confirm:$false or -Force (that's if you choose to implement a separate -Force switch; I have other opinions about this and think this is an inconsistent and fundamentally unclear pattern, but that's another topic).

SydneyhSmith commented 3 years ago

Thanks for everyone's thoughts after more consideration @ThomasNieto you make a great point that since this is a Set cmdlet it makes more sense to have as a boolean parameter

PaulHigin commented 3 years ago

I agree. It makes sense for this 'set' cmdlet for individual properties. I'll leave this marked for committee review in case they want to veto, but otherwise will move forward with the changes.

vexx32 commented 3 years ago

Boolean parameters are the one thing that if they're ever mentioned, the design needs a rethink.

Please don't.

iSazonov commented 3 years ago

See Enabled switch in Set-ADUser I mean the UX is popular. My thoughts:

vexx32 commented 3 years ago

"In use" is not the same as popular. They're universally confusing at best.

PaulHigin commented 3 years ago

This has been discussed internally with just as many differing opinions. There were some ideas that I feel were pretty good and am inclined to go with:

  1. Set-SecretStoreConfiguration should be changed to Update-SecretStoreConfiguration since it really updates existing individual properties.

  2. Since this cmdlet updates existing configuration values, it makes sense to remove the existing (-DoNotPrompt, -PasswordRequired) switch parameters and make them enum Parameters:

-Authentication (Passsword | None) -Interaction (Prompt | NoPrompt)

What do you think?

vexx32 commented 3 years ago

Those sound good, and leave room in the future for other auth types (2FA / smart cards making a comeback at some point? 😉 ).

Unsure about -Interaction as a name, but I have nothing better to offer myself, so I think it'll do the job well enough, unless some others have ideas on that one. 🙂

mklement0 commented 3 years ago

Using enumeration values makes sense, and avoids both the awkwardness of a Boolean parameter / having to use a switch parameter with an invariably optional, :-attached argument.

As for the name: Update-SecretStoreConfiguration:

Among the built-in cmdlets we have no *-*Configuration cmdlets, but we have two *-*Option cmdlets (Get-Command -Type Cmdlet *Option), Set-MarkdownOption and Set-PSReadLineOption

Therefore, my vote is for Set-SecretStoreOption.

iSazonov commented 3 years ago

-Authentication (Passsword | None)

Since intention is "to protect": -Protection (Passsword | None)

SydneyhSmith commented 3 years ago

Really appreciate all the input on this issue! Agree that enumeration values are the right call and leave the design adaptable to changes in auth practices. Set makes the most sense to me, similar to Set-PSRepository but I also recognize this may be a bias of spending so much time on the PowerShellGet project recently. Looking at more examples of Set vs Update now--but welcome more thoughts on this 😄

mklement0 commented 3 years ago

We can consult the official definition of the Update verb:

Brings a resource up-to-date to maintain its state, accuracy, conformance, or compliance

We are not talking about bringing something up-to-date here, so Update isn't appropriate.

Contrast with Set:

Replaces data on an existing resource or creates a resource that contains some data

This is appropriate: we're replacing or initially setting one or more configuration values, aka options.

By switching from *Configuration to *Option - i.e., Set-SecretStoreOption:

ThomasNieto commented 3 years ago

As for @iSazonov's suggestion to change -Authentication to -Protection. The parameter name should be left as -Authentication. The setting is to determine what type of authentication is required to access the vault. Also, all the built-in cmdlets use Authentication.

PaulHigin commented 3 years ago

Changed for preview2. Note that this changes the config file schema and so preview2 is incompatible to preview1. Be sure and save/copy your secrets before installing.