Trusted-signers Add has inconsistent arguments

[heng-liu]

Thanks for @loic-sharma 's question in #10628 (comment)

In NuGet.exe command, the package path could be wildcard, so there might be multiple paths after parsing.
But there is a checking on "Name" to reject any existing "Name" at https://github.com/NuGet/NuGet.Client/blob/ac97d923e633f4ee65eec5ec690c4d99d7a922f3/src/NuGet.Core/NuGet.Commands/TrustedSignersCommand/TrustedSignerActionsProvider.cs#L243
So when there are multiple packages, the first one will be added successfully, but the rest will fail for A trusted signer 'Test' already exists.
Apparently this is a bug. We should make the two consistent.
That is, do not accept multiple paths, or, accept multiple 'Name'.

[aortiz-msft]

@erdembayar - Could we address this in the dotnet.exe implementation as first step?

[erdembayar]

But there is a checking on "Name" to reject any existing "Name" at https://github.com/NuGet/NuGet.Client/blob/ac97d923e633f4ee65eec5ec690c4d99d7a922f3/src/NuGet.Core/NuGet.Commands/TrustedSignersCommand/TrustedSignerActionsProvider.cs#L243
So when there are multiple packages, the first one will be added successfully, but the reset will fail for A trusted signer 'Test' already exists.
Apparently this is a bug. We should make the two consistent.
That is, do not accept multiple paths, or, accept multiple 'Name'.
What is reset here?
Can you give actual full command here for below cases and with expected result for reach?

1. do not accept multiple paths
2. accept multiple 'Name'

[heng-liu]

Hi @erdembayar , the command is nuget trusted-signers add <package(s)> -Name <name> [options]
we could do 1 or 2. I personally feel 1 is safer, but would love to learn more thoughts from others.

1. do not accept multiple paths, means when we pass package path to command, we verify if it's only 1 nupkg. If it's wildcard and there are multiple nupkgs resolved, it will fail and say like " there are multiple nupkgs found"
2. accept multiple 'Name', means if we decide to accept multiple nupkgs for package(s) , then we also need to accept a list of 'Name' of trusted-signers, so that each nupkg will be added as the according 'Name' in the list.

[erdembayar]

@zkat @dtivel @joelverhagen @loic-sharma @nkolev92 @zivkan @JonDouglas
Please give your opinion. *1 or *2? I prefer number *1 too, same as @heng-liu. If there're many it's hard to match visually and might lead to unwanted event.

[zkat]

I see from the code that, yes, we'll throw that error.

That said, I thought it was possible to have, say, multiple Author certificates under a single name (as in the example under "list" here: https://docs.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-trusted-signers#nuget-trusted-signers-list).

Am I misunderstanding how this command works? I would expect this to append new signers.

See also: #10628 (comment)

These are the assumptions I was working under with the spec.

[heng-liu]

I tried with multiple nupkgs in the path, like:
nuget trusted-signers add c:\testpackages\*.nupkg -Name TestTrustedSigner1 -Author
Only the first nupkg's author cert is added into trusted-signers section.
The rest of nupkgs failed to be added. As we have a duplicate checking on argument Name . So no duplicate Name could be added into trusted-signers section, and we don't accept multiple Name for now.

[erdembayar]

I tried with multiple nupkgs in the path, like:
nuget trusted-signers add c:\testpackages\*.nupkg -Name TestTrustedSigner1 -Author
Only the first nupkg's author cert is added into trusted-signers section.
The rest of nupkgs failed to be added. As we have a duplicate checking on argument Name . So no duplicate Name could be added into trusted-signers section, and we don't accept multiple Name for now.

But here you're only passing single Name not multiple. Can we pass comma separated names?

[heng-liu]

I tried, multiple Names doesn't work.
We assume there is only one Name in our code .

[zkat]

I am aware that the code will fail as-is. I'm asking about the configuration itself. The spec says that if something already exists under <name>, the new signature should be appended. Is that something we can do, or does that make no sense?

[erdembayar]

I am aware that the code will fail as-is. I'm asking about the configuration itself. The spec says that if something already exists under <name>, the new signature should be appended. Is that something we can do, or does that make no sense?

Let's assume there were 3 files in that c:\testpackages\*.nupkg path.
Then nuget trusted-signers add c:\testpackages\*.nupkg -Name TrustedAuthor1 -Author command will result in below section in config file. Is below your expected assumption?

<author name="TrustedAuthor1">
  <certificate fingerprint="A3AF7AF11EBB8EF729D2D91548509717E7E0FF55A129ABC3AEAA8A6940267641" hashAlgorithm="SHA256" allowUntrustedRoot="false" />
  <certificate fingerprint="cf6ce6768ef858a3a667be1af8aa524d386c7f59a34542713f5dfb0d79acf3dd" hashAlgorithm="SHA256" allowUntrustedRoot="false" />
  <certificate fingerprint="ba5a630994b2b8f562b307a2a3245998232ef0a94ee80bece5cea0b5ceca61f9" hashAlgorithm="SHA256" allowUntrustedRoot="false" />
</author>

Only concern with this approach is doing wholesale blind adding may result, just adding 1 unwanted (test certificate or remnant from old deployment ...) certificate among many then it might open security hole. #10628 (comment) is more about convenience but security and convenience may not go hand in hand sometimes. I see pain point on your local development but it could be problem for CI/CD pipeline, on really secure environment user better be explicit about what to trust.
Then above can be written like below or create custom powershell script to iterate all files for automation.
nuget trusted-signers add c:\testpackages\file1.nupkg -Name TrustedAuthor1 -Author
nuget trusted-signers add c:\testpackages\file2.nupkg -Name TrustedAuthor1 -Author
nuget trusted-signers add c:\testpackages\file3.nupkg -Name TrustedAuthor1 -Author

[zkat]

I'm not overly concerned about this causing major trip-ups, tbh. Maybe @dtivel has a different opinion on what should be done? I'd rather have the wildcard feature, because it's something folks have asked for before (if I understand correctly something that Heng said)

[dtivel]

I agree that the current behavior is broken. It should be possible to add a new certificate to an existing trusted-signer. We should fix this.

Separately, there is a concern that allowing multiple .nupkgs --- either by wildcard or multiple explicit file paths --- could lead to unintentional grant of trusted signer status to one or more package signers.

Initially, I considered other cases where wildcards could lead to unintended behavior. For example, I could shoot myself in the foot if my wildcard pattern is too broad:

del .\*.dll

If I wanted *.dll, then I got exactly what I wanted. But if I really wanted a*.dll, then I'm kicking myself for my mistake. There is no confirmation, let alone per-file confirmation. There are probably more examples.

@erdembayar's concern is around the persistent trust which may be accidentally conferred by such a mistake. I looked around for precedents and found:

• Windows' certutil -addstore root <file path>, which adds a certificate to a trusted certificate store as a trusted root authority, disallows wildcards in the file path. The file path must resolve to a single file.
• PowerShell's Import-Certificate -CertStoreLocation Cert:\LocalMachine\Root -FilePath <file path>, which adds a certificate to a certificate store as a trusted root authority, disallows wildcards in the file path.

In fact, across its many cmdlets PowerShell has multiple parameter names for file path (-Path, -LiteralPath, -FilePath). It seems that -FilePath must always resolve to a single file. Often -FilePath disallows wildcards; however, in a few cases wildcards are allowed but the wildcard must resolve to a single file to be valid.

Based on @erdembayar's concern and these prominent precedents, I'm inclined to err on the side of caution and disallow wildcards for the file path argument of the trusted-signers add command. Allowing multiple, non-wildcard file paths seems less worrisome, but it may be simplest to just require a single file per command invocation.

[zkat]

Based on @erdembayar's concern and these prominent precedents, I'm inclined to err on the side of caution and disallow wildcards for the file path argument of the trusted-signers add command. Allowing multiple, non-wildcard file paths seems less worrisome, but it may be simplest to just require a single file per command invocation.

There is no cross-platform way to disallow wildcards while allowing multiple arguments. We either go with a single file argument, or we don't.

[erdembayar]

Based on @erdembayar's concern and these prominent precedents, I'm inclined to err on the side of caution and disallow wildcards for the file path argument of the trusted-signers add command. Allowing multiple, non-wildcard file paths seems less worrisome, but it may be simplest to just require a single file per command invocation.

There is no cross-platform way to disallow wildcards while allowing multiple arguments. We either go with a single file argument, or we don't.

If detect multiple files then just throw exception here?
https://github.com/NuGet/NuGet.Client/blob/0158a5946a2e425943ac4f93e50e491a18b6f4e0/src/NuGet.Core/NuGet.Commands/TrustedSignersCommand/TrustedSignersCommandRunner.cs#L93-L96

[zkat]

looks about right, yeah