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

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

Open
JustinGrote opened this issue Jan 16, 2021 · 15 comments · May be fixed by #219

Comments

@JustinGrote
Copy link

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
Copy link
Collaborator

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
Copy link
Author

  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
Copy link

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
Copy link

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
Copy link

DonPwrShellHunt commented Oct 18, 2023

@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
Copy link
Collaborator

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

@DonPwrShellHunt
Copy link

@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
Copy link

@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
Copy link
Member

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:

private void WriteResults(SecretInformation[] results)
{
if (results == null) { return; }
// Ensure each vaults results are sorted by secret name.
var sortedList = new SortedDictionary<string, SecretInformation>(StringComparer.OrdinalIgnoreCase);
foreach (var item in results)
{
sortedList.Add(
key: item.Name,
value: item);
}
foreach (var item in sortedList.Values)
{
WriteObject(item);
}
}

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.

andyleejordan added a commit that referenced this issue Feb 16, 2024
Instead of a `SortedDictionary` so that keys don't have to be unique.
Fixes #95.
@andyleejordan andyleejordan linked a pull request Feb 16, 2024 that will close this issue
@Callidus2000
Copy link

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
Copy link
Author

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

andyleejordan added a commit that referenced this issue Feb 20, 2024
Instead of using a `SortedDictionary` which introduced a bug because it required
the keys (secret names) to be unique, we just sort the array directly using a
comparer that sorts by name.

Fixes #95.
@andyleejordan
Copy link
Member

@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
Copy link

Yes, perfect.

@andyleejordan
Copy link
Member

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.

andyleejordan added a commit that referenced this issue Sep 27, 2024
Instead of using a `SortedDictionary` which introduced a bug because it required
the keys (secret names) to be unique, we just sort the array directly using a
comparer that sorts by name.

Fixes #95.
@MariusStorhaug
Copy link

MariusStorhaug commented Nov 24, 2024

Just wanted to mention that I also see this when i run two consecutive Set-Secret with characters I suppose was not intended to be supported?

Using:

  • pwsh 7.4.6
  • SecretStore 1.0.6
  • SecretManagement 1.1.2

Repro steps:

Set-Secret -Name 'test[case]' -Secret '123' -Vault SecretStore
VERBOSE: Secret test[case] was successfully added to vault SecretStore.
Set-Secret -Name 'test[case]' -Secret '123' -Vault SecretStore
Set-Secret: Exception calling "WriteObject" with "4" argument(s): "An item with the same key has already been added. Key: test[case]"
Set-Secret: Exception calling "WriteObject" with "4" argument(s): "An item with the same key has already been added. Key: test[case]"

Removing the special characters [] I get no errors:

Set-Secret -Name 'testcase' -Secret '123' -Vault SecretStore  
VERBOSE: Secret testcase was successfully added to vault SecretStore.
➜ Set-Secret -Name 'testcase' -Secret '123' -Vault SecretStore
VERBOSE: Secret testcase was successfully added to vault SecretStore.

Resorted to store the name inside the secret I am creating (json structure) and just using a base64 encoding of the string and use that as the name of the secret.

Projects:

  • PSModule/Context -> Creating a context store based on SecretManagement and SecretStore.
  • PSModule/GitHub -> Using the Context module to store credentials and settings in Context structures, keyed on (module identifier)/(hostname)/(github login) -> One of the type of logins are GitHub App installations, they are bots and I end up with names like: 'Context:PSModule.GitHub/github.com/github-actions[bot]' for the secret.

MariusStorhaug added a commit to PSModule/Context that referenced this issue Nov 24, 2024
## Description

- Convert all secret names to and from base64, breaks with all current
secrets that was managed by `Context`.
- The main issue that caused this was the contexts that are made when
running as a GitHub App Installation. The context created for this
contains `[]` which causes all sorts of issues.
- This is to get around
PowerShell/SecretManagement#95, which also
applies to Set-Secret.

## Type of change

<!-- Use the check-boxes [x] on the options that are relevant. -->

- [ ] 📖 [Docs]
- [ ] 🪲 [Fix]
- [ ] 🩹 [Patch]
- [ ] ⚠️ [Security fix]
- [ ] 🚀 [Feature]
- [x] 🌟 [Breaking change]

## Checklist

<!-- Use the check-boxes [x] on the options that are relevant. -->

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants