NuGet / Home

Repo for NuGet Client issues
Other
1.49k stars 250 forks source link

In NuGet trusted-signers command, owner option should not be case sensitive #9720

Open heng-liu opened 4 years ago

heng-liu commented 4 years ago

According to @anangaur , the owner option should not be case sensitive. (However, I didn't find it in document)

But now, it's case sensitive:

nuget trusted-signers add -name NuGet.org -serviceindex https://api.nuget.org/v3/index.json -owners microsoft
nuget config -set signatureValidationMode=require

When I tried to install Microsoft.Extensions.Logging, it failed, NU3034: Package 'Microsoft.Extensions.Logging 5.0.0-preview.5.20278.1' from source 'https://api.nuget.org/v3/index.json': This package is signed but not by a trusted signer.

If I change the microsoft to Microsoft, then the installation will be successful.

joelverhagen commented 4 years ago

We changed the case of microsoft org name to Microsoft to accommodate this. I agree it does not align with NuGet.org implementation (owners are case insensitive there) but IIRC there were reasons. /cc @loic-sharma @shishirx34 @dtivel

loic-sharma commented 4 years ago

Yes I remember that we discussed this, but I don't remember why we chose to be case sensitive. I will defer to @dtivel or @ridomin

nkolev92 commented 4 years ago

@dtivel Do you recall the reasoning behind this approach?

anangaur commented 4 years ago

As far as I remember, we didn’t want to mess with the free text field that can include Unicode characters as well I think. But we were discussing to make it case insensitive for nuget.org but punted the discussion then to revisit in future. @dtivel do add anything that I missed or miscommunicated

loic-sharma commented 4 years ago

Rido shared the following meeting notes from the Package signature Owners casing verification meeting:

The proposed solutions were:

a. We update the client to ignore casing and change the repository signature spec to be case insensitive a.1 ) We hack it in the client so this only happens for nuget.org b. We resign the 7,152 packages that have “microsoft” as an owner to have “Microsoft” and be consistent with all of the others. b.1) Apart from this, we introduce a new signed attribute in the repository signature that specifies if the owners should be treated case sensitive or case insensitive by the client. c. We do a combination of (a) and (b) d. We resign all the package in nuget.org and set owners in the signatures to be lowercase.

In the discussion we agreed that we would want to reposign as few packages as possible, therefore (d) got discarded.

There was a debate if (a) or (b) should be done.

• The argument against doing (a) was that the client behavior (and the repository signature spec) should not be driven by server policies, therefore the fact that nuget.org is or isn’t case sensitive shouldn’t influence the trust decision in the client. • The argument against doing (b) was that in the future we might face this same issue if we decide to implement a feature to let the users change the casing (or even rename) their username.

Given that we are only facing this issue today because we decided to change the casing on Microsoft packages and said feature for casing changes (or renames) doesn’t exist yet, it was decided to do (b) right away (reposign 7,152 “incorrect” packages) and revisit this conversation later on to decide if we would even want to implement a feature that would require the client to be case insensitive.

anangaur commented 4 years ago

Its also about users experience where users are not used to being case sensitive while dealing with NuGet. The package Ids are not case sensitive. So irrespective or whether we have the "microsoft" or "Microsoft" as author text as part of repo signing, the user may not use the right casing to define the trust policies. We are making it tougher for users to adopt this feature.

nkolev92 commented 4 years ago

A quick reaction from me on the adoption comment is that this feature has been out for 2 years. The repercussions of this issue are a failure at restore time.

How often would people come back to the casign problem once they've resolved it?

It's trivial from an implementation stand point the engineering cost is low from that perspective, but curious if we have a good idea about how many customers have hit this.

anangaur commented 4 years ago

I agree with you @nkolev92. This is one of those features which we "enabled" but haven't evangelized with customers. However, my comment was more about creating additional friction points (with possibly low investment) for already possibly not so well adopted feature. I have seen folks from our team struggle because of the casing while trying it out. Using "M" capital for Microsoft was hard enough without knowing about the limitations. Think about accounts with longer names (say AppInsightsSdk or dotnetfoundation) - do we expect folks to know if the accounts had camel casing or not? They may not be trademarks where there may be a brand recall.

nkolev92 commented 4 years ago

That's a fair argument :)

I'm not against the change, but rather questioning our motivation. Last time, the discussions left off on waiting for customer feedback, and we haven't received that necessarily :)