PowerShell / Modules

MIT License
112 stars 25 forks source link

[SecretsManagement][Design] Set-Secret should support -Force #50

Closed Jaykul closed 4 years ago

Jaykul commented 4 years ago

Summary of the new feature/enhancement

As a developer of vault extensions In order to support overwriting My Set-Secret implementation should be able to SupportsShouldProcess

Specifically, as a PowerShell developer, I am not comfortable shipping a function named Set-Secret that automatically overwrites secrets by default -- even if it is "hidden" in a specially named folder. Someone, somewhere is going to use that command without using SecretsManagement. So although SecretsManagement may implement logic to prevent acidental overwriting, I cannot rely on that particular implementation being the only one that calls this code.

As a C# developer, I'm not totally comfortable with that model for critical secrets either, and would prefer to write explicit Add and Update functions. I can live with Set, but would prefer it to have a bool overwrite = false parameter so I can be sure that all callers understands what is expected.

Proposed technical implementation details (optional)

PaulHigin commented 4 years ago

Hmm, when we discussed this internally, we thought '-Force' would be annoying and that the default behavior should be overwrite (with optional '-NoClobber'). I am fine either way, but to maintain consistency across all vaults I feel it should be implemented in the 'Set-Secret' cmdlet and not each individual vault implementation. The internal vault implementation should assume 'force' and just write or overwrite the secret, returning an error only if the operation fails.

JustinGrote commented 4 years ago

@PaulHigin as long as it has SupportShouldProcess so I can do set-secret -whatif (and maybe make the ConfirmImpact High since it has the potential to "delete" a secret), I'm cool with "Force" as the default and -noclobber as the alternate behavior.

Jaykul commented 4 years ago

I guess part of my problem is that (as I said in my comment on #42), I would rather these commands be (able to be) part of the public interface to my module.

If I was going to expose the command at all, then it's interface needs to make sense. The way these are written right now, they have to be hidden because they're not "usable" by human standards (we need the wrapper with -Force that checks before overwriting).

I guess it's fine if we're forcing people to hide them anyway, but it forces me to write a Set-MyVaultNameSecret wrapper that copies the logic of SecretManagement\Set-Secret ...

JustinGrote commented 4 years ago

@jaykul looks like Paul already commited the change for now, so we can probably close this issue as a quick fix, and have a more involved conversation about the extensions interface in #42. What do you think?

SydneyhSmith commented 4 years ago

Since we already made the change to be Set-Secret, we agree that -NoClobber makes the most sense as a switch..in terms of extension interface design, agree that we should continue to use #42 for that discussion