Http error: Error for non-https resource endpoints

[Nigusu-Allehu]

Bug

Fixes: NuGet/Home#13364

Description

Here is a discussion on the implementation: https://github.com/NuGet/Client.Engineering/pull/2966
An https NuGet source could have a non https service endpoint.
for example a v3 source with the following index would end up using a non https service endpoint.

{
    "version": "3.0.0",
    "resources": [
        {
            "@id": "http://source",
            "@type": "SearchQueryService",
            "comment": "Query endpoint of NuGet Search service (primary)"
        }]
}

This PR makes sure we throw an error for such endpoints unless the source is set to AllowInsecureConnections in the Nuget config file. In addition, it also takes SdkAnalysisLevel into consideration. I created a static class that allows restore to communicate with NuGet.Protocol whether it should throw the http error or not

How?

• When any NuGet command requests for service endpoints, we use GetServiceEntries to get a list of service endpoints that are required for the specific command.
• Before adding an entry to the list of entries, check whether it is an insecure http endpoint.
  • If it is insecure throw an exception.
• Look at the following test Http error: Error for non-https resource endpoints #5915 (comment) to understand the logic

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

[Nigusu-Allehu]

This test shows where the Exceptions are thrown.

[jgonz120]

I don't see any obvious problems but I'm new to the space.

[donnie-msft]

Messaged you offline as well about how sure we are about the runtime error experience here. Client does not error when adding a package source with an insecure service entry, so it may be jarring to receive this error at runtime.

Checking for HTTP service entries when adding a package source would introduce overhead, though.

Curious if you can share how this change came about, the tradeoffs discussed, etc.

[zivkan]

I haven't yet made up my mind if I think the API design should be changed, or if it's acceptable given the constraints of the existing public APIs. So, I'm not approving, but I'm also not blocking. Maybe I'll form a stronger opinion with more time, or after reading more feedback on the PR

[zivkan]

I don't know if the deep link will work, since the comment is resolved in the other PR: #5760 (comment)

But there I suggested that rather than checking every single time the same resource is requested over and over, we can check once during initialization. But the trade off is that it means any service index entry that is unused, but pointing to an insecure HTTP URL, will prevent NuGet from working.

[Nigusu-Allehu]

After reviewing the issue, I see no major problem to performing the check each time a resource is requested. The current implementation includes a loop that goes through each service entry to locate the desired version. The http check is integrated within this loop, thus the performance impact is minimal, maintaining an O(n) complexity both before and after the change. In addition, this approach doesn't restrict users from accessing a secure resource just because another listed resource may be insecure. It's possible that the source provider is in the process of upgrading their service entries to HTTPS. As long as the insecure resource isn't being utilized, I don't believe we should block

[Nigusu-Allehu]

Messaged you offline as well about how sure we are about the runtime error experience here. Client does not error when adding a package source with an insecure service entry, so it may be jarring to receive this error at runtime.

Checking for HTTP service entries when adding a package source would introduce overhead, though.

Curious if you can share how this change came about, the tradeoffs discussed, etc.

You are right, checking for HTTP service entries when adding a package source would have additional overhead. My implementation was designed to have a central validation point, allowing us to confirm that no HTTP resources are being used without customer consent. This approach avoids the need to implement checks on a command-by-command basis, which could lead to oversight and errors as new features are introduced. The current implementation is centralized, meaning every command that needs to make an API request will require a service entry check. Both NuGet and the dotnet CLI will log the exception as an error, providing visibility to the customer. This error logging offers a clear explanation of what went wrong. However, I am not certain about the best way to communicate these errors within Visual Studio.

[jeffkl]

Using static singletons makes our tests unreliable since one test sets it and another changes it, then something fails randomly. Is there any way to avoid this here?

[jeffkl]

Suggested change

	List<ServiceIndexEntry> matchingEntries = new List<ServiceIndexEntry>();
	List<ServiceIndexEntry> matchingEntries = new List<ServiceIndexEntry>(entries.Count);

[jeffkl]

Suggested change

<value>There is a non-HTTPS resource: '{0}', defined by your package source: '{1}'. NuGet requires HTTPS endpoints. To allow unencrypted HTTP, you must explicitly set 'allowInsecureConnections' to true in your NuGet.Config file, or talk to the administrator of your source and ask them to migrate to a secure HTTPS URL. Please refer to https://aka.ms/nuget-https-everywhere for more information.</value>

<value>There is a non-HTTPS resource, '{0}', defined by your package source: '{1}'. NuGet requires HTTPS endpoints. Update your package source to only use HTTPS resources. To allow unencrypted HTTP connections, you must explicitly set 'allowInsecureConnections' to true in your NuGet.Config file. Please refer to https://aka.ms/nuget-https-everywhere for more information.</value>

[nkolev92]

I thought these changes were gonna wait on the telemetry first.
In particular: #6125.

[microsoft-github-policy-service]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.