-
Notifications
You must be signed in to change notification settings - Fork 47
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
Comments
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! |
Ideally a vault doesn't have duplicate names, but it can happen especially if working with existing data. |
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 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 -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"? 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... |
For other devs who want to create a new extension: |
@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.
|
@DonPwrShellHunt we are still having difficulty reproducing this (we are using SecretStore) what vault are you using? |
@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. |
@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. |
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: SecretManagement/src/code/SecretManagement.cs Lines 1059 to 1077 in 47bc3b7
When |
Instead of a `SortedDictionary` so that keys don't have to be unique. Fixes #95.
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. |
@Callidus2000 as per the linked PR, @andyleejordan's fix will use a |
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.
@Callidus2000 with my patch
I believe that's what we're looking for, do you agree it's correct now? |
Yes, perfect. |
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. |
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.
Just wanted to mention that I also see this when i run two consecutive Using:
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 ➜ 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:
|
## 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
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.The text was updated successfully, but these errors were encountered: