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.

Sign up for GitHub

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

Jump to bottom

initial draft of dotnet nuget trusted-signers commands spec #10628

Merged
merged 9 commits into from
Mar 15, 2021
Merged

initial draft of dotnet nuget trusted-signers commands spec #10628

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4c36a0a
initial draft of dotnet nuget trusted-signers commands spec
zkat Mar 5, 2021
5263069
missed a spot, oops
zkat Mar 5, 2021
24f4135
initial review pass
zkat Mar 9, 2021
c4df89a
another big revision
zkat Mar 11, 2021
55d10d3
note about globbing
zkat Mar 11, 2021
8fb9a92
Update proposed/2021/DotnetNugetTrustedSigners.md
zkat Mar 11, 2021
f31d8ad
more review
zkat Mar 11, 2021
ae8aa92
more revisions
zkat Mar 12, 2021
b30a8ff
reword
zkat Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
239 changes: 239 additions & 0 deletions proposed/2021/DotnetNugetTrustedSigners.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,239 @@
# dotnet nuget trust command

zkat marked this conversation as resolved.
Show resolved Hide resolved
- Author Name [Kat Marchan (@zkat)](https://github.com/zkat)
- Start Date 2021-03-04
- GitHub Issue: https://github.com/NuGet/Home/issues/10634
- GitHub PR: https://github.com/NuGet/Home/pull/10628

## Summary

This specifies the syntax and behavior for the `dotnet nuget trust` command.

## Motivation

This is part of a larger effort to reach full parity between `nuget` and
`dotnet` for NuGet-related operations.

## Explanation

### Functional explanation

#### `> dotnet nuget trust`
zkat marked this conversation as resolved.
Show resolved Hide resolved

```
Provides the ability to manage the list of trusted signers.
zkat marked this conversation as resolved.
Show resolved Hide resolved

USAGE:
dotnet nuget trust [COMMAND] [OPTIONS]

COMMANDS:
list
author <name> <package>...
repository <name> <package>...
source <name> [source-url]
certificate <name> <fingerprint>
remove <name>
sync <name>
```

#### `> dotnet nuget trust list`

```
Lists all the trusted signers in the configuration. This option will include all the certificates (with fingerprint and fingerprint algorithm) each signer has. If a certificate has a preceding [U], it means that certificate entry has allowUntrustedRoot set as true.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this?

Suggested change
Lists all the trusted signers in the configuration. This option will include all the certificates (with fingerprint and fingerprint algorithm) each signer has. If a certificate has a preceding [U], it means that certificate entry has allowUntrustedRoot set as true.
Lists all the trusted signers in the configuration. This option will include all the certificates (with fingerprint and fingerprint algorithm) each signer has. If a certificate has a preceding [U], NuGet will not reject the certificate if it chains to an untrusted root.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I think it's good to make it clear that the [U] corresponds to an actual option in the config file, you know? What do you think, @heng-liu ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... IMHO ideally no one ever uses allowUntrustedRoot. If so, maybe we shouldn't bring up such an advanced topic in the command description? It is difficult to balance between useful information and verbosity. What do you think of only explaning [U] in command output if the user has allowUntrustedRoot enabled somewhere as per #10628 (comment)? If so, we can detail both the allowUntrustedRoot config plus give a quick human description of what that entails.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pointing out... you can use links to docs on more advanced topics. We anticipate doing this when we clean up all the CLI help near the end of .NET 6 or start of 7.


USAGE:
dotnet nuget trust list [OPTIONS]

EXAMPLE:
$ dotnet nuget trust list
Registered trusted signers:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like the example being included here. The user will see this output as soon as they run the command and it will push the usage off the screen if you do not maximize your terminal window.

Also, will the list be long? Consider putting in your backlog doing a partial match, but talk to me about making your format consistent with the rest of the CLI direction if you do that (so maybe wait and see what people want).


1. nuget.org [repository]
Service Index: https://api.nuget.org/v3/index.json
Owners: microsoft, aspnet, nuget
Certificate fingerprint(s):
loic-sharma marked this conversation as resolved.
Show resolved Hide resolved
zkat marked this conversation as resolved.
Show resolved Hide resolved
SHA256 - 0E5F38F57DC1BCC806D8494F4F90FBCEDD988B46760709CBEEC6F4219AA6157D

2. microsoft [author]
Certificate fingerprint(s):
SHA256 - 3F9001EA83C560D712C24CF213C3D312CB3BFF51EE89435D3430BD06B5D0EECE
SHA256 - AA12DA22A49BCE7D5C1AE64CC1F3D892F150DA76140F210ABD2CBFFCA2C18A27

3. myUntrustedAuthorSignature [author]
Certificate fingerprint(s):
[U] SHA256 - 518F9CF082C0872025EFB2587B6A6AB198208F63EA58DD54D2B9FF6735CA4434

OPTIONS:
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
--interactive Allows command to stop and wait for user input or action.
--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dotnet list package describes the markers like [U] at the bottom of the command output. For example (LINK):

Project 'SentimentAnalysis' has the following package references
   ...
   > Microsoft.NETCore.App   (A)   [2.1.0, )   2.1.0

(A) : Auto-referenced package.

Personally I find that describing the markers at the bottom of the command output grabs my attention more than the end of the command's help description. What do you think of adding the markers at the bottom of the output too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like if we use markers to describe the output, then we haven't done a good enough job explaining the output in the first place. I especially think describing allowUntrustedRoot is going to be difficult to explain in a single line at the bottom of the command.

Copy link
Contributor

@loic-sharma loic-sharma Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we either:

  1. Remove the [U] marker altogether, or...
  2. Replace the marker with some text next to the SHA-256 fingerprint, or...
  3. Define the [U] marker somewhere in this output. For the single line explanation, I suggest: [U]: NuGet will not reject the certificate if it chains to an untrusted root.. We can skip definition if no certs allow an untrusted root.

I'd vote for option 2 or 3 personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm impartial. tbh just tell me which one y'all like best and I'll do that :)


#### `> dotnet nuget trust sync`

```
Requests the latest list of certificates used in a currently trusted repository to update the the existing certificate list in the trusted signer.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As someone not familiar with this space, I found it difficult to understand this sentence. Is the most important part first? If not try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a generalized concern that this feature is very complex and hard to explain in general, and I'm having trouble figuring out how to write these docs in a way that creates clarity. :(


Note: This gesture will delete the current list of certificates and replace them with an up-to-date list from the repository.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait. If this is what it does, why not just use this note as the description?


USAGE:
dotnet nuget trust sync [OPTIONS] <name>

OPTIONS:
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this in all commands please.

These are now the standard CLI/.NET Tools verbosities and MSBuild is just to be specific among ourselves.

Suggested change
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
-v, --verbosity <LEVEL> Set the verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].

--interactive Allows command to stop and wait for user input or action.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually include -i as abbrev for --interactive, unless you have a legacy conflict. Suggest adding for all commands.

--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at this in the output and see how bad the wrapping is. I do not think the second sentence is helpful and this seems long and verbose for help. This is probably a description you use a lot across NuGet help (and the CLI), so let's see if we can find some nice concise consistent language.

```

#### `> dotnet nuget trust remove`

```
Removes any trusted signers that match the given name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it confusing that when adding a trusted signer you need to specify either source or package, but when you remove you don't specify which 'type'?

Copy link
Contributor

@heng-liu heng-liu Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the code , there is a checking on duplicate names in trusted-signers section of config file when adding, to guarantee that there is no duplicate name, among all trusted-signers (both author and repository signers). So it might be good to just specifying the name, without 'type'.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think this is nice. When you add a signer from a package you are not adding the package, but using it to find the certificate. When you remove the package, it does not matter where you got it.

Not symmetric, but in this case the action is not symmetric


USAGE:
dotnet nuget trust remove [OPTIONS] <name>

OPTIONS:
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
--interactive Allows command to stop and wait for user input or action.
--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.
```

#### `> dotnet nuget trust author`

```
Adds a trusted signer with the given name to the config, based on the author signature(s) of one or more packages. If <name> already exists in the configuration, the signature(s) will be appended.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all of these, what does "to the config" add for a normal use reading this. Suggest dropping it in all these commands.


USAGE:
dotnet nuget trust author [OPTIONS] <name> <package>...

EXAMPLE:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see this example adding much.

Copy link
Contributor

@loic-sharma loic-sharma Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me <name> and <package> are vague terms. From this command alone, it's unclear how <name> will be used. Also, it's also unclear that <package> should be a path to a .nupkg file. The example does help clarify some of these questions.

Is it possible to add descriptions for the command arguments? Maybe that'd be better than an example.

dotnet nuget trust author Contoso ./contoso.library.nupkg

OPTIONS:
--allow-untrusted-root Specifies if the certificate for the trusted signer should be allowed to chain to an untrusted root.
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
--interactive Allows command to stop and wait for user input or action.
--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.
```

##### NOTE

For `trust author` and `trust repository`, `<package>` may be a glob/wildcard.
This must be consistent across platforms, so the Windows version must expand
these wildcards when it receives them as arguments.

#### `> dotnet nuget trust repository`

```
Adds a trusted signer with the given name to the config, based on the repository signature(s) or countersignature(s) of one or more packages. If <name> already exists in the configuration, the signature(s) will be appended.


USAGE:
dotnet nuget trust repository [OPTIONS] <name> <package>...

EXAMPLE:
dotnet nuget trust repository Contoso ./contoso.library.nupkg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently dotnet nuget verify allows to pass more than 1 path even though nuget verify doesn't, how about here do we have same gesture behavior allow pass more than 1 paths or serviceindexes, fingerprints?
dotnet nuget verify C:\Users\xxxx\source\repos\SigningDemo\TestSigned\TestSigned.1.0.0.nupkg C:\Users\xxxx\source\repos\SigningDemo\TestSigned\*.nupkg -v d

Copy link
Contributor

@heng-liu heng-liu Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good question.
My understanding is, the reason why we change the dotnet verify command to support multiple paths, is as following:
NuGet.exe only runs Windows(except on Mono) but dotnet.exe will run cross-platform
When executing the command in powershell, the xx*.nupkg will be passed as one argument to nuget.exe,
but when executing the command in bash, the globbing will expand the xx/*.nupkg into multiple paths before passing to dotnet.exe command. So if the dotnet.exe only accept one path, it will only get the first path but ignore the rest paths, and make it inconsistent with what we have on NuGet.exe.

In one word, we change dotnet.exe to accept multiple paths because it makes dotnet acts more like NuGet.exe.

Previously when we only have NuGet.exe, we use ";" to separate elements in one option, if we have multiple serviceindexes, fingerprints.
When we have dotnet.exe later, the ";" indicates the command terminates in bash.
I agree we need to think it over and have more discussion if we want to change the separator. If we're going to deprecate NuGet.exe, we may have less impact on customers.
FYI @JonDouglas

Copy link
Contributor

@loic-sharma loic-sharma Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is globbing files a scenario customers need when updating their trusted signers? Do you have an example when this would be useful?

The reason I ask is because this scenario scares me a little bit. Ideally customers should thoroughly review each and every author/source they choose to trust.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the good question.
Just checked the code.
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 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'.
Raised issue at #10647

OPTIONS:
--allow-untrusted-root Specifies if the certificate for the trusted signer should be allowed to chain to an untrusted root.
zkat marked this conversation as resolved.
Show resolved Hide resolved
--owners Semi-colon separated list of trusted owners to further restrict the trust of a repository.
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
--interactive Allows command to stop and wait for user input or action.
--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.
```

##### NOTE

For `trust author` and `trust repository`, `<package>` may be a glob/wildcard.
This must be consistent across platforms, so the Windows version must expand
these wildcards when it receives them as arguments.

#### `> dotnet nuget trust certificate`

```
Adds a trusted signer with the given name to the config, based on a certificate fingerprint.

Note: If a trusted signer with the given name already exists, the certificate item will be added to that signer. Otherwise a trusted author will be created with a certificate item from given certificate information.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As someone unfamiliar with this space, I do not know what name means. Is it part of the certificate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name in all of these refers to the identifier the trusted signer is saved under. The name of the entity itself, if you will. It's an arbitrary name that we use to key off of.


USAGE:
dotnet nuget trust certificate [OPTIONS] <name> <fingerprint>

OPTIONS:
--allow-untrusted-root Specifies if the certificate for the trusted signer should be allowed to chain to an untrusted root.
--algorithm Specifies the hash algorithm used to calculate the certificate fingerprint. Defaults to SHA256. Values supported are SHA256, SHA384 and SHA512
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
--interactive Allows command to stop and wait for user input or action.
--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.
```

#### `> dotnet nuget trust source`

```
Adds a trusted signer based on a given package source.

If only `<name>` is provided without `<source-url>`, the package source from your NuGet configuration files with the same name will be added to the trusted list.

If a `<source-url>` is provided, it MUST be a v3 package source URL (like https://api.nuget.org/v3/index.json). Other package source types are not supported.

If <name> already exists in the configuration, the signature(s) will be appended.
loic-sharma marked this conversation as resolved.
Show resolved Hide resolved

USAGE:
dotnet nuget trust source <name> [source-url]

OPTIONS:
--allow-untrusted-root Specifies if the certificate for the trusted signer should be allowed to chain to an untrusted root.
--owners Semi-colon separated list of trusted owners to further restrict the trust of a repository.
-h, --help Prints help information.
-v, --verbosity <LEVEL> Set the MSBuild verbosity level. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic].
--interactive Allows command to stop and wait for user input or action.
--configfile The NuGet configuration file. If specified, only the settings from this file will be used. If not specified, the hierarchy of configuration files from the current directory will be used.

```

### Technical explanation

For the most part, these commands map pretty directly to their nuget
counterparts, and most of their implementations should be reusable (removing
`#if IS_DESKTOP` as needed, from the various TrustedSigners classes).

The exception here is the `add` command, which has been split into separate
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? there is no longer an add command. Appears a couple of times in this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is referring to the nuget trusted-signers add command, which is what has been split up.

commands that otherwise have the same behaviors, since options and semantics
can vary significantly.

I don't know if there's any significant implementation above just remapping
command invocations for dotnet.

Additionally, `trust author` and `trust repository` must both not just accept
multiple `<package>` arguments, but must manually expand globbed arguments on
Windows, since powershell and cmd.exe don't do glob/wildcard expansions at the
shell level like \*nix shells do.

## Drawbacks

This whole feature is very complicated, but it's important for parity.

## Rationale and alternatives

I think the only one that might have a reasonable alternative here would be
the `add` command. I found the command as a whole to be inscrutable, and thus
decided splitting its behavior to be the best way forward. We could have, of
course, mostly just copied the behavior from `nuget trusted-signers add`.

## Prior Art

This spec is based on [`nuget trusted-signers`](https://docs.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-trusted-signers).

## Unresolved Questions

Is this the right UX? Does the naming make sense? This is a fairly complex/complicated feature and we want to make sure we deliver something to customers that makes sense and gives an overall good experience.

## Future Possibilities

N/A