Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
initial draft of dotnet nuget trusted-signers commands spec #10628
initial draft of dotnet nuget trusted-signers commands spec #10628
Changes from 2 commits
4c36a0a
5263069
24f4135
c4df89a
55d10d3
8fb9a92
f31d8ad
ae8aa92
b30a8ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?There was a problem hiding this comment.
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 hasallowUntrustedRoot
enabled somewhere as per #10628 (comment)? If so, we can detail both theallowUntrustedRoot
config plus give a quick human description of what that entails.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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):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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we either:
[U]
marker altogether, or...[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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only updates the trusted source, not trusted author, right? Maybe
trusted-source
is better thantrusted-signers
, if it doen't also sync to the latest author certificates.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the code, yes you're right, it just sync certificate of repository signature.
I think trusted-signers means both author signers and repository signers.
An example is: we add certificate of repository signature by running trusted-signers add command.
So the question is, it good enough to only call it out explicitly in the document, that author certs will not be synced?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :(
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'?
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 thoughnuget 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say I've already trusted 15 package owners for my mid-sized project. But now I want to add one more package that is owned by someone I did not previously trust. What would be my flow to append one more trusted owner? Do I need to provide all 16 package owners to the
trust-source
command? What would my flow be if I have a large project with many more owners?The reason I ask is that I recently tried to set up trusted signers for the NuGetGallery codebase. This proved to be difficult using
nuget.exe
, so I resorted to manually editing thenuget.config
file instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea may be to add a
trust-owner
command that appends one or more owners to a trusted repository. See @zivkan comment here: https://github.com/NuGet/Home/pull/10628/files/5263069456da2276d0f3143fd5bf616e933b7809#r589669891There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a similar question about a missing function in NuGet.exe:
After repo certificate rotation, we need to change the trustedSigners section to have two certs :
But seems there is no command could do that. We have to edit config file manually.
Since we don't have that for NuGet.exe either, perhaps we can address that afterwards.
There was a problem hiding this comment.
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 phrasing?
This sentence is really hard! Maybe pull in a copywriter 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this command doesn't have a
--repository
option. Consider:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, copy-paste fail. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think the add command would be better suited for being multi-purpose with different parameters. The syntax of
dotnet nuget add trusted-signers trust-*
seems a bit verbose compared todotnet nuget add trusted-signers <package(s)>
ordotnet nuget add trusted-signers <service url>
.In other words, it would be cool to just use one command and it sets up the trusted signers regardless of the parameter I pass in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you differentiate packages from service URLs? I guess we're going to start detecting whether something looks like a URL and hope the regex does the right thing? What about for relative paths like
myprivatehost
? Is that a service index, or a local file?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why's that? In another comment I mentioned there are already multiple
dotnet add <noun>
anddotnet nuget add <noun>
. Therefore, this spec is not following the convention of other commands in the dotnet cliThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I remembered from last discussion is that dotnet want to change (they do not want to keep the same with dotnet add source any more). But I'm not 100% sure if that is the final decision.
Perhaps we can discuss with them when reviewing this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #10628 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the biggest challenge is consistency throughout the dotnet CLI. I think decisions were made in the past that we have to adhere to in this spec as it might seem odd to users when using
nuget
contexts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't change how the settings are saved in the nuget.config file, but otherwise can we use this as an opportunity to rethink how to make all the commands easier to use? Why do we have to have a 1:1 mapping between nuget.exe and dotnet cli commands? If we can learn from what we did in nuget.exe to make the dotnet cli commands easier to use, then that will be extra motivation for customers to use the better cli experience.
Or maybe the nuget.config schema is what you were referring to, not the commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly meant in the sense of the CLI guidelines under the
nuget
context umbrella. I agree we should make things easier right now if there are opportunities to do so.