Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Extensible "Description" property on [SecretInformation] #46

Closed
JustinGrote opened this issue Sep 22, 2020 · 27 comments
Closed

Comments

@JustinGrote
Copy link

JustinGrote commented Sep 22, 2020

Carried over from PowerShell/Modules#31

Summary of the new feature/enhancement

As a powershell user, I sometimes want some more information about what a secret is for, and a provider may be able to provide that additional information in a way that I can programatically query/filter that information.

For example:

  • Azure Key Vault could have the description of a byte[] secret as an X509 certificate, when it was modified
  • Keepass may provide a folder structure for where the secret resides, or a flag that a string is an SSH key for use in an SSH agent.

Proposed technical implementation details (optional)

Add a [Object] property to Get-SecretInfo output

Some name possibilities: Notes, Description, Metadata

The provider can populate this property with additional information about the secret. This information would be provider specific and the provider can supply any type of object as long as it has a clean ToString() method to convert it. Most providers may just do a simple string description, others may submit a nested hashtable with properties like created, modified, etc.

Future:
Some natural commonalities like "LastModified","Comment" may become common and should be added to the [SecretInformation] class designation to standardize them, however this property allows providers to deliver their own extensible information that's important to their provider.

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.99. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@SydneyhSmith SydneyhSmith added this to the Preview 6 milestone Nov 10, 2020
@PaulHigin
Copy link
Contributor

Implemented 'Description' registry field in preview6.

@JustinGrote
Copy link
Author

@PaulHigin Unless I'm missing something your commit is about adding a description field to the Vault. I'm talking about adding an extensible property to SecretInformation so additional metadata about the secret can be provided and users can consume/filter/sort based on those properties rather than just the name, so I think this should be reopened.

@PaulHigin
Copy link
Contributor

Yes, thanks. Not sure what I was thinking except it seemed to make sense for vault info as well. What I had in mind was to add a string 'Description' field to the secret meta data. I don't want to add an 'object' type field as that makes it difficult to store. Another option is to add a 'Tags' field that takes a string array. Either would help sorting. Right now I am leaning toward the Description field as 'Tags' just feels like alternate names.

@PaulHigin PaulHigin reopened this Nov 12, 2020
@JustinGrote
Copy link
Author

JustinGrote commented Nov 12, 2020

@PaulHigin That's fine, my only concern is that my immediate want as a consumer will be to abuse this field and store JSON in it so as a secret consumer I can filter on more metadata detail

Name-Value tags as a dictionary[string,string] would be a good compromise, and again the storage would be left up to the extension, it would just be an optional supplied parameter to the set-secret and get-secret implemented commands

Ideal solution in my opinion
Have a Metadata property on SecretInfo that is an Object (or more strongly typed e.g. Dictionary[String,Object]) property on the secretInfo object. Then description is a derived get string field as just the ToString() of that. If ToString() fails, just set description to null.

You could also do Dictionary[String,String], but the only reason I could think of would be to avoid arbitrary code execution from a maliciously stored secret, but that security should be the role of the extension author I think.

This allows extension authors to supply any kind of metadata relevant to the secret info for that extension that consumers who are aware of that metadata can consume, so that they can choose to more efficiently filter (or make cmdlets that use Get-Secret on the backend). The author would just optionally supply the additional information to the Metadata property when returning the secret object when Get-SecretInfo is called, and allow arbitrary settings to be specified with Set-SecretInfo that are secret specific instead of vault specific.

For instance, I may want to filter by created/modified/deleted date, I may want to filter by some sort of folder structure the extension provides, I may want to filter by a special kind of tag that 1Password provides, etc. and I shouldn't be hamstrung into the standard properties of a secretInfo for that.

Without it, it will just lead to people (like me) abusing description by putting a JSON string in there that a wrapper cmdlet just decodes to then be able to do the filtering.

@PaulHigin
Copy link
Contributor

No, I don't want to force extension vault writers to have to deal with an arbitrary 'object' type. If anything this would be a well defined field type. But we really want to avoid breaking changes so I will look to see if it can be implemented without breaking existing extension vaults.

@JustinGrote
Copy link
Author

JustinGrote commented Nov 12, 2020

@PaulHigin I agree minimizing breaking changes at this point now that vaults are out there would be good, that's why I said it would be an optional new field on the SecretInformation object, and either have a new constructor to include the information, or just make it a read/write field e.g.

$mysecretInfo.metadata = @{
  Path='/my/special/path';
  IsGroupCredential=$true;
  Tags=@{
    Environment='Production';
    AssociatedServer='Test5'
  }`

As you see you're not "forcing" them to do anything, and it wouldn't be a breaking change, it's entirely up to them if they want to incorporate it into Get-SecretInfo

And then I as a consumer can use that info to filter to find the secret I want.

Get-Secret | where {$_.metadata.tags.environment -eq 'Production'}

@PaulHigin
Copy link
Contributor

Yes, changing SecretInformation type is fine for script functions, but would require binary cmdlets to be recompiled (breaking change). Fortunately, I believe all extension vaults are currently implemented with script functions.

Also the field needs to be passed in so Set-Secret has to be updated with a new parameter. But this should not be a breaking change if implemented as a simple script function, which handles unbounded arguments.

@constantinhager
Copy link

I have the same problem. I build a Pleasant Password Server SecretManagement Extension. At the moment if I call Set-Secret I can only create that secret, but I cannot add additional information to that new secret like tags or the folder where the secret should be stored in. This would be great if you could add that parameter. At the moment I add that secret with Set-Secret and provide a wrapper function where I add the tags and move the newly created secret to the appropriate folder.

@PaulHigin
Copy link
Contributor

Not all vaults need or want to deal with extra secret metadata. So I think it makes sense for individual vault implementations to provide this functionality if needed.

@JustinGrote
Copy link
Author

@PaulHigin The metadata is optional in the same way the description is. How do you propose individual vault extensions provide this functionality? Make the user to call a separate Get-MyExtraMetadata $secret after Get-Secret?

Then how about setting secrets with additional information required (e.g. tags, etc.), do you want us to hack the name with a lot of extra special syntax and then have set-secret parse it? Doesn't seem very user friendly to me.

Lets take the very common example of creating a new secret that needs both a subfolder organization (e.g. a "Path" metadata parameter) and some tags to describe it. Without being able to specify the metadata at creation, then you're asking the user workflow to be something like this:

  1. Set-Secret -Name 'test' -Secret 'test'
  2. Move-MyCustomCommandToMove -Secret $secret -Path '/my/new/path'
  3. Set-SecretTag -Secret $secret -tags 'application','production'

When instead it could simply be:
Set-Secret -Name 'test' -secret 'test' -Metadata @{Path='/my/new/path';Tags=@('application','production')

And extension authors could instead provide a casting object to go to the Metadata.

All secret management is doing is passing through that object, so I don't understand why so much resistance to the idea.

@PaulHigin
Copy link
Contributor

Nothing is ever simple. It is not clear if the metadata should be treated as sensitive data and be protected, and complicates the cmdlets we support. I don't yet see the value for most users and prefer to not add it if it is only rarely needed. At this point it feels to me as something that can be handled as custom vault functionality. But I freely admit I may be wrong, and we can look at adding it if it provides value to most users.

@JustinGrote
Copy link
Author

JustinGrote commented Jan 11, 2021

@PaulHigin I would say per most common example above, most password managers have some sort of hierarchy to organize secrets. When creating a new secret, how would you propose a vault implementer create the secret to a specific path?

If it's for the vault module to provide an additional wrapper cmdlet around set-secret, why even bother using set-secret in the first place since the whole idea is to standardize the getting and setting of secrets?

@JustinGrote
Copy link
Author

JustinGrote commented Jan 11, 2021

Also, per your comment:
#78 (comment)

@PaulHigin So having -VaultParameters as a loose object is OK but having a -Metadata loose object is not (it could be called -SecretParameters for consistency)? Vault parameters is more likely to contain sensitive information than metadata, so I don't see congruent logic here.

@JustinGrote
Copy link
Author

Here's another use case:
For Chromium, passwords are stored in a SQLite database, and the only thing that is unique about them is the ID. Because usernames and URIs can have so many characters, and because those can conflict, without some sort of DisplayName, Description, or Metadata field, I'm forced to present this to the user in order for things to work because I can only identify a secret by its ID string in order to keep pipelining working and whatnot.

So we need at least some sort of separate property to distinguish the identifier of the secret from what we display to the user.

image

@PaulHigin
Copy link
Contributor

We can look into adding an optional -Metadata parameter to Set-Secret. A vault implementation could decide to support it or not. But I would like to get a better idea of how important this is to vault implementers, and understand the scenarios better. Since it is optional and non-breaking, it is something we can add anytime in the future.

Vaults that force users to provide a bunch of extra information for each secret, or to require very long Id strings to identify secrets, feel like a poor user experience. And I wonder if the vault implementation can provide some kind of mapping layer to make the user experience better.

@JustinGrote
Copy link
Author

JustinGrote commented Jan 20, 2021

@PaulHigin I'm more concerned about having it for the [SecretInformation] result per the issue title, so that users can filter for things such as say datemodified, path/folder, user who created it, or any other vault-specific metadata using standard powershell filters like Where-Object

As a workaround today I use the -Filter like an OData query field basically to be able to filter to the correct info, but it's not very discoverable compared to a metadata field. The module then provides basically a Get-MoreSecretInfo to then get the metadata based on the path.

@PaulHigin
Copy link
Contributor

@JustinGrote How do you suggest the metadata be handled? Should it be treated as sensitive information? I feel it should be considered as non-sensitive information, like secret name, stored unencrypted, and included with other SecretInformation data, when returned to the user.

We would have to make this clear to ensure expectations and vault implementations are consistent.

@JustinGrote
Copy link
Author

@PaulHigin I envision it as either a [hashtable] or [dictionary[string,string]] optional field member (if you want more strict data) that effectively act as name/value "tags" about the secret. For instance:

Description: MyTestSecret
DateCreated: date
DateModified: date
Path: path/to/my/secret
Creator: Justin Grote jgrote@mail.com
Environment: Production

None of the tags would be required and implementation is completely optional though some "de-facto" standards may emerge from the community that users can potentially come to reliably expect to exist.

Implementation would be simply adding the field to the [SecretInformation] type and having an overloaded constructor to include it (so as not to break existing implementations) and should not be a breaking change.

@awakecoding
Copy link

+1 on this, the vast majority of password managers would need metadata storage to keep at least minimal path or id information, such as a UUID, etc. A generic [HashTable] or [Dictionary[string,string]] would be sufficient for this, and I don't think it needs to be secured in any way, making it clear that this is meant for metadata only. It doesn't need to be mandatory, extensions that don't use it can simply leave the metadata empty.

@JustinGrote
Copy link
Author

And as mentioned it wouldn't break existing implementations because the existing constructor would still exist, and it's optional to consume.

I already laid out my more detailed recommendation back in November that is just reiterating what I summarized above.
#46 (comment)

@JustinGrote
Copy link
Author

As a real world example, I am already having to abuse the name field of secretmanagement.chromium because it requires unique secrets and in Chromium browsers secrets are only guaranteed unique by their UID. If I had a metadata field where I could store the UID, I could make the display names less onerous and feel safe displaying them as "Secret", "Secret (2)" etc. because when I need to then go back and get that secret I have the UID to reference in the metadata field.

image

@PaulHigin
Copy link
Contributor

We decided this feature is important enough to include in the next RC release. This will be an optional feature for vaults to implement, but SecretStore vault will be updated to support it. This is what I had in mind for the feature design:

Secret metadata is not treated as sensitive and does not have to be stored with encryption.

Secret metadata will be a [hashtable], that should support at minimum [string], [int], [datetime] value types.
But SecretManagement will not enforce type adherence, leaving this to individual vault implementations.

Secret metadata will be included in the [SecretInformation] type in a new Metadata property of type [Dictionary[[string],[object]]

The feature will be exposed to users through a new -Metadata parameter in the Set-Secret cmdlet.

There will also be a new Set-SecretMetadata cmdlet that allows user to add metadata to existing secrets.

There will be a new Set-SecretMetadata command that vault implementations can optionally provide to support metadata.
If a vault implementation does not provide this command, SecretManagement will throw a 'not supported' error.

Let me know what you think.

@awakecoding
Copy link

@PaulHigin this design looks perfectly good to me! thanks a lot

@JustinGrote
Copy link
Author

Sounds directly on par with my request! If the implementation is as you describe that PR can close this issue. I really appreciate you taking the time @PaulHigin to review

@JustinGrote
Copy link
Author

@PaulHigin can you update the milestone to RC2?

@PaulHigin PaulHigin modified the milestones: Preview 6, 1.0-GA Jan 25, 2021
@JustinGrote
Copy link
Author

Implemented in 9de1193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants