"Get-SecretInfo: An item with the same key has already been added" exception should be nonterminating

[JustinGrote]

When the Get-SecretInfo: An item with the same key has already been added exception occurs, SecretManagement should emit the error but rather than returning nothing, it should continue and exclude the duplicate results.

[SydneyhSmith]

Thanks @JustinGrote we dont actually create an error like that in SecretManagement, could you please provide repro steps as to hoe that error gets thrown...thanks!

[JustinGrote]

1. Create a vault
2. At the get-secretinfo step, return two secrets with the same name
3. Error is thrown

Ideally a vault doesn't have duplicate names, but it can happen especially if working with existing data.

[Callidus2000]

I've stumbled upon this issue while developing a new extension. I've created a new dummy extension/repository available under https://github.com/Callidus2000/SecretManagement.HashTableBugs for reproduction of the issue.

Preperation-Steps to reproduce (from within the cloned repo)

# Register two vaults
tests\Register-Vaults.ps1
# Check if the vaults exist
Get-SecretVault Hash*

Name               ModuleName                    IsDefaultVault
----               ----------                    --------------
HashTableBugVault1 SecretManagement.HashTableBug False
HashTableBugVault2 SecretManagement.HashTableBug False

The dummy data the vault pretends to store is (available in both vaults!)

Name Type   VaultName
---- ----   ---------
Foo  String HashTableBugVault1
Foo  String HashTableBugVault1
bar  String HashTableBugVault1

No problem to query the bar entry:

Get-SecretInfo -Vault HashTableBugVault1 -Name bar

Name Type   VaultName
---- ----   ---------
bar  String HashTableBugVault1

The issue appears if Get-SecretInfo returns multiple entries with the same name:

Get-SecretInfo -Vault HashTableBugVault1 -Name foo
Get-SecretInfo: An item with the same key has already been added. Key: [Foo, Microsoft.PowerShell.SecretManagement.SecretInformation]

Internally both entries are found and returned

Name   Type VaultName          Metadata
----   ---- ---------          --------
Foo  String HashTableBugVault1 {[Comment, This is not very secret ;-)]}
Foo  String HashTableBugVault1 {[Comment, This is not very secret ;-)]}

Funny part is that Get-SecretInfo is able to return multiple entries with the same name. This is testable by querying 'bar' from all vaults:

Get-SecretInfo  -Name bar
Name Type   VaultName
---- ----   ---------
bar  String HashTableBugVault1
bar  String HashTableBugVault2

Interesting part: My function returns an array, the type of the returned results is an array, too. Where is the unnecessary conversion to a hashtable/dictionary happening?

Is there a rule in the complete SecretManagement multiverse that says "names within a vault have to be unique"? If "yes": Where are those rules written? If "no": Please fix this issue.

For Get-Secret a unique rule is ok. For those duplicate entries my strategy was to show a warning "Dupes found, please use GUID from the metadata from Get-SecretInfo". If Get-SecretInfo is picky regarding duplicates I've got a huge problem...

[Callidus2000]

For other devs who want to create a new extension: I've included a workaround in my template module and in the corresponding ReadMe.

[DonPwrShellHunt]

@SydneyhSmith @JustinGrote Issue #95 can be reproduced by the SecretManagement module after two secrets are added to an extension vault and the secret names differ ONLY in case.

Notice that Get-SecretInfo works for each of these secrets individually, but fails when processed by the Filter wildcard.

I have created a documentation issue for SecretManagement relating to Clobber behavior and case sensitivity of secret names.

Bug #95 would be a show stopper for vault extensions that were supporting case sensitive secret names.

The following log shows that the WriteResults function starting on line 1059 of SecretManagement.cs is using StringComparer.OrdinalIgnoreCase which triggers the failure on line 1067.

donhunt> Get-SecretInfo -Vault kc2 -Name "M*" -Verbose
# should return two SecretInformation objects where Name differs ONLY in case
# Write-Verbose was added in extension vault to show Filtered secretinfo results being returned

VERBOSE: Filtered> MixedCaseName
VERBOSE: Filtered> MIXEDCaseName

VERBOSE: Secret information was successfully retrieved from vault kc2.
Get-SecretInfo: An item with the same key has already been added. Key: [MIXEDCaseName, Microsoft.PowerShell.SecretManagement.SecretInformation]

donhunt> Get-Error

Exception             : 
    Type       : System.ArgumentException
    Message    : An item with the same key has already been added. Key: [MIXEDCaseName, Microsoft.PowerShell.SecretManagement.SecretInformation]
    TargetSite : 
        Name          : AddIfNotPresent
        DeclaringType : System.Collections.Generic.TreeSet`1[T]
        MemberType    : Method
        Module        : System.Collections.dll
    Source     : System.Collections
    HResult    : -2147024809
    StackTrace : 
   at System.Collections.Generic.TreeSet`1.AddIfNotPresent(T item)
   at System.Collections.Generic.SortedSet`1.Add(T item)
   at System.Collections.Generic.SortedDictionary`2.Add(TKey key, TValue value)
   at Microsoft.PowerShell.SecretManagement.GetSecretInfoCommand.WriteResults(SecretInformation[] results) in D:\a\_work\1\s\src\code\SecretManagement.cs:line 1067
   at Microsoft.PowerShell.SecretManagement.GetSecretInfoCommand.WriteExtensionResults(ExtensionVaultModule extensionModule) in 
D:\a\_work\1\s\src\code\SecretManagement.cs:line 1042
TargetObject          : Microsoft.PowerShell.SecretManagement.GetSecretInfoCommand
CategoryInfo          : InvalidOperation: (Microsoft.PowerShel…etSecretInfoCommand:GetSecretInfoCommand) [Get-SecretInfo], ArgumentException
FullyQualifiedErrorId : GetSecretInfoException,Microsoft.PowerShell.SecretManagement.GetSecretInfoCommand
InvocationInfo        : 
    MyCommand        : Get-SecretInfo
    ScriptLineNumber : 1
    OffsetInLine     : 1
    HistoryId        : 11
    Line             : Get-SecretInfo -Vault kc2 -Name "M*" -Verbose
    PositionMessage  : At line:1 char:1
                       + Get-SecretInfo -Vault kc2 -Name "M*" -Verbose
                       + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    InvocationName   : Get-SecretInfo
    CommandOrigin    : Internal
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
PipelineIterationInfo :

donhunt> Get-SecretInfo -Vault kc2 -Name MixedCaseName

Name          Type         VaultName
----          ----         ---------
MixedCaseName SecureString kc2

donhunt> Get-SecretInfo -Vault kc2 -Name MIXEDCaseName

Name          Type   VaultName
----          ----   ---------
MIXEDCaseName String kc2

donhunt> Get-SecretInfo -Vault kc2                    
Get-SecretInfo: An item with the same key has already been added. Key: [MIXEDCaseName, Microsoft.PowerShell.SecretManagement.SecretInformation]

donhunt> gmo *Secret*

Name                                  Version PreRelease ExportedCommands
----                                  ------- ---------- ----------------
Microsoft.PowerShell.SecretManagement 1.1.2              {Get-Secret, Get-SecretInfo, Get-SecretVault, Register-SecretVault…}

[SydneyhSmith]

@DonPwrShellHunt we are still having difficulty reproducing this (we are using SecretStore) what vault are you using?

[DonPwrShellHunt]

@SydneyhSmith - I was testing a development version of SecretManagement.KeyChain where case sensitive secret names were allowed.

SecretStore treats secret names in a case insensitive manner. It will overwrite an existing secret in the vault if Set-Secret is done with a Name parameter that differs only in case. (eg - MixedCaseName & MIXEDCaseName). It cannot be used to reproduce this issue.

[DonPwrShellHunt]

@SydneyhSmith Please read the Clobber and Case Sensitive Secret Names Note that was generated by my Microsoft Docs issue on this topic.

Looking at the detailed error message content and the C# code references I provided above should be sufficient to describe the inappropriate behavior dealing with secret names, where case sensitivity should be the prerogative of the extension.

Case sensitive secret name support is a special case within the context of multiple secrets with the same name, and I would totally understand if a new issue was created just to work case sensitive name support.

[andyleejordan]

Thanks so much @Callidus2000, I was able to reproduce this with the steps you provided. Looking at the code I would bet the issue lies here:

https://github.com/PowerShell/SecretManagement/blob/47bc3b71a810e0810aa2fc2bff645ffb7e5f90b3/src/code/SecretManagement.cs#L1059-L1077

When Get-SecretInfo is getting ready to write out the found secrets, it goes to sort it using a case-insensitive dictionary. Which to me reads like a bug. I will try to get this fixed.

[Callidus2000]

Hi, a fix would be nice 😁

I think the entities shouldn't be added to a dictionary at all. If I've got two vaults with some entries having identical names (casings irrelevant) both should be listed. Imagine the use case of vault migration where identical names are intentionally for each migrated entry.

[JustinGrote]

@Callidus2000 as per the linked PR, @andyleejordan's fix will use a SortedSet rather than a SortedDictionary to allow for identical names.

[andyleejordan]

@Callidus2000 with my patch Get-SecretInfo returns this with your example:

Name   Type VaultName          Metadata
----   ---- ---------          --------
Foo  String HashTableBugVault1 {[Comment, This is not very secret ;-)]}
Foo  String HashTableBugVault1 {[Comment, This is not very secret ;-)]}
bar  String HashTableBugVault1 {[Comment, This is not very secret ;-)]}
Foo  String HashTableBugVault2 {[Comment, This is not very secret ;-)]}
Foo  String HashTableBugVault2 {[Comment, This is not very secret ;-)]}
bar  String HashTableBugVault2 {[Comment, This is not very secret ;-)]}

I believe that's what we're looking for, do you agree it's correct now?

[Callidus2000]

Yes, perfect.

[andyleejordan]

Ok, awesome. I am working on getting the unit tests running locally so I can add test coverage for this. It might be a bit before we see it in a release, but it will happen.