Add HTTPS client certificate support

[BlackGad]

Implements: NuGet/Home#5773
Specification: https://github.com/NuGet/Home/blob/dev/proposed/archive/NuGet-ClientCertificates.md

Added feature implementation accordingly to merged specification

[zivkan]

I didn't look at the code, but there are some things that can be improved before then.

[BlackGad]

Resolved all above issues

[BlackGad]

Because of XPlat templating I reformatted generic command runner to separated ones. The way how command executes now is unified for both command line tools.

I am afraid command runners must be revalidated again.

[BlackGad]

Any progress?

[zivkan]

Sorry, I had a very busy week last week. Some very quick comments:

• I'm concerned about the lack of tests.

  • I'm pretty sure we already have a bunch of tests for nuget config and nuget sources. I expect Rob added tests when he added dotnet nuget [add|remove] sources. You should be able to copy & adapt those tests.
  • I feel confident we can unit test the NuGet.Protocol changes, similar to how I tested multiple certs with HttpClient a few months ago. NuGet doesn't have a lot of unit tests, most are integration/functional tests, so we don't have a good example for you to follow. But the test could set up a HTTP server listening on localhost that needs an HTTPS client cert and always returns some hardcoded string for all URLs if successful, which the test can assert on. The test creates a SourceRepository object, call GetResourceAsync to get a HttpSourceResource, then use that to call the test HTTP server that without needing to implement NuGet's HTTP protocol. Since one of the .NET Core 2.x releases, Kestrel and ASP.NET Core only runs on netcoreapp, no longer on .NET Framework, but NuGet still targets net472. So, it might be necessary to put these tests in a #if NETCOREAPP2_1 block if we can't find something that works with all our test TFMs.

• I'm not a fan of static classes or methods unless they're "pure functions" (no side-effects). In particular static methods with side-effects make testing harder and unreliable, and often prevent parallel testing. One alternative is using NuGet.Protocol's resource & provider pattern to make a certificate resource that the rest of the code can use. I don't know how username & password credentials are passed from config to SourceRepository, but certs could do the same thing. Otherwise another option, which I'd be less happy about, is to have internal constructors used only for testing.

  • this will also make it much easier to write tests that don't need certificates in the certificate store. I don't like changing machine state as part of a test. I don't know what package signing and package validations tests do, but I hope they don't change the machine. Extract an interface to your ClientCertificates class, and create a mock instance for testing that doesn't use X509Store. Just create certs in memory and let them be garbage collected at the end of the test.

[zivkan]

I just thought of another reason why getting rid of statics with side-effects is important. They can cause problems with long-lived processes like Visual Studio, Visual Studio for Mac, and Rider. I only know about Visual Studio (the Windows one), but if the static class is kept, it needs to correctly handle when the user closes the solution and loads a solution with different HTTPS client cert settings. I think it would be less effort, and would "just work" for VS for Mac and Rider, if we got rid of the static class and had the values flow through in a similar way to username/password credentials do.

[BlackGad]

I just thought of another reason why getting rid of statics with side-effects is important. They can cause problems with long-lived processes like Visual Studio, Visual Studio for Mac, and Rider. I only know about Visual Studio (the Windows one), but if the static class is kept, it needs to correctly handle when the user closes the solution and loads a solution with different HTTPS client cert settings. I think it would be less effort, and would "just work" for VS for Mac and Rider, if we got rid of the static class and had the values flow through in a similar way to username/password credentials do.

I will research possibility to apply client certificates without static. I am agree that this is not so handy solution (was copied from TLS functionality)

[BlackGad]

Did a research. I am afraid it is not possible to apply certificates with out static invocation.

All stuff around in HttpHandlerResourceV3Provider are static.

private static HttpHandlerResourceV3 CreateResource(PackageSource packageSource)
{
	var sourceUri = packageSource.SourceUri;
	var proxy = ProxyCache.Instance.GetProxy(sourceUri);

	// replace the handler with the proxy aware handler
	var clientHandler = new HttpClientHandler
	{
		Proxy = proxy,
		AutomaticDecompression = (DecompressionMethods.GZip | DecompressionMethods.Deflate)
	};

	// Setup http client handler client certificates
	ClientCertificates.SetupClientHandler(clientHandler,  packageSource);

	// HTTP handler pipeline can be injected here, around the client handler
	HttpMessageHandler messageHandler = new ServerWarningLogHandler(clientHandler);

	if (proxy != null)
	{
		messageHandler = new ProxyAuthenticationHandler(clientHandler, HttpHandlerResourceV3.CredentialService?.Value, ProxyCache.Instance);
	}

#if !IS_CORECLR
	{
		var innerHandler = messageHandler;

		messageHandler = new StsAuthenticationHandler(packageSource, TokenStore.Instance)
		{
			InnerHandler = messageHandler
		};
	}
#endif
	{
		var innerHandler = messageHandler;

		messageHandler = new HttpSourceAuthenticationHandler(packageSource, clientHandler, HttpHandlerResourceV3.CredentialService?.Value)
		{
			InnerHandler = innerHandler
		};
	}

	var resource = new HttpHandlerResourceV3(clientHandler, messageHandler);

	return resource;
}

Static calls:

• HttpHandlerResourceV3.CredentialService
• ProxyCache.Instance
• TokenStore.Instance

Last chance to patch ClientHandler is CreateHttpClientAsync method in HttpSource.cs file. Theoretically it is possible to encapsulate client certificates into PackageSource object but a lot of code change would be required.

Сorrect me if I'm missed something.

[zivkan]

One option, if it helps, is to remove the static keyword. Since CreateResource is private, it won't be a breaking change, otherwise it wouldn't be possible (although you could create overloads in that situation).

A second option is to do the same as how credentials are passed. The HttpSourceAuthenticationHandler gets passed a PackageSource object, in fact it's right there near the end of the code snippet you provided. Now, it's unfortunate that the class is used for both runtime information as well as modelling the configuration file. So, one sub-option is to create a new class that represents a PackageSource with additional information (client certs), and pass that instead of the NuGet.Configuration.PackageSource (if this is done, overloads need to be added and existing public APIs need to be modified to work in a API and ABI compatible way, athough anyone relying on this will not get client certs, making it look like a bug). A different sub-option is to add client certs to the PackageSource class, just don't serialize it during nuget.exe and dotnet nuget commands to modify sources. I think this is my only suggestion that will let client certs to work with apps that use NuGet's packages without changing their own code.

A third option is, as I mentioned previously, use NuGet.Protcol's resource & provider patterm. Add a new ClientCertificateResource. Since the code snippet you used is from a ResourceProvider, you can take inspiration how PackageSearchResourceV3Provider depends on ServiceIndexResourceV3 and RawSearchResourceV3. In HttpHandlerResourceV3Provider 's TryCreate, you can get the client cert resource, and pass it into the CreateResource method. Then, you'll need to add a SourceRepository constructor, so that it takes both a PackageSource and whatever configuration object represents the configured client cert information, and during the constructor, call GetResource to get the ClientCertificateResource in order to set the certificates, so that they're available when the HTTP handler resource provider needs to use it. You'll need to then look at all calls to the constructor, and change as many as you can to the new one that takes the extra client certs configuration object. However, this means that all apps that use NuGet need to change their API usage to be compatible with http client certs.

A fourth option, which I really don't like, is to extend the static ClientCertificates class, and call the clear method in the Visual Studio extension when it detects a solution load/unload. The problem with this is that Visual Studio for Mac, JetBrains Rider, and other apps that use NuGet will have a bug until they learn they need to do this too.

A fifth option, is to add a public method to HttpHandlerResourceV3 which will set the certs. Instead of setting the certs in the HTTP handler provider, have the code that created the SourceRepository immediately call GetResource<HttpHandlerResourceV3>(), then call the method to set the client certs. But again it means that apps that use the NuGet SDK will have to change their code, otherwise depending on what APIs they do use, http client certs won't work for me.

Those are all my ideas at this time.

So, unfortunately for you, you're experiencing one of the things that causes the NuGet team problems. New features are hard to add because we need to try to keep not only API compatibility, but also ABI backwards compatibility, so that other assemblies that are compiled against the NuGet SDK keep working. The current PR has a bug when someone opens and closes solutions in Visual Studio, VS for Mac, JetBrains Rider, and other apps that use the NuGet SDK, without restarting the whole process. Some of the proposed ideas will work for VS, but depending on which APIs other apps use, it might mean that http client certs won't work unless the app changes which NuGet APIs they use. The idea that is most likely to work without needing changes in apps using the NuGet SDK, feels dirty because it's changing a configuration class to add a property that isn't saved to that part of the configuration file.

[BlackGad]

Theoretically it is possible to encapsulate client certificates into PackageSource object but a lot of code change would be required.

Yep I meant exactly that)

[zivkan]

I was looking deeper into how credentials work in the configuration code, and I found out that PackageSource is an abstraction of the configuration file format. The file format is represented by the SourceItem and CredentialItem classes. The factory that knows how to join them is PackageSourceProvider.

So ignore my previous message suggesting there's multiple options on how this could be implemented. I'm now convinced that adding client certs into the PackageSource, via PackageSourceProvider is the right way to go. It should take a similar amount of code as the existing username and password credentials, which at a glance is actually not that much.

On a similar topic, your branch adds 3 new classes in the src/NuGet.Core/NuGet.Configuration/Settings/Items/ folder. There's already a class in there named CertificateItem, for use with signed package trust policies. So, unless you're planning on making it extend your new BaseCertItem, I'd rename your new classes to have ClientCert in the name, to make it more obvious it's unrelated to the existing CertificateItem in there.

[BlackGad]

Already working on packageSource extending with client certs. Hard to test cases from several entry points(command line utils. VS plugin etc) so it will take some time.

Will rename classes later.

[BlackGad]

With some minor changes in upstream List nuget command was able to remove all static calls. Also new approach is more generic and will be automatically applied to all clients who are using PackageSourceProvider.LoadPackageSources() method for package sources discovery.

Btw dotnet nuget push/delete has no -configfile option and it is not consistent with other commands.

[BlackGad]

Client certificate items were renamed

[BlackGad]

@zivkan brunch was rebased.
@rrelyea all tt files were updated. ClientCertArgs processing were reworked as well.

[BlackGad]

This time built even without glitches :)

[BlackGad]

@rrelyea docs.tt and commands.xml files extended and fixed.

[BlackGad]

@zivkan changes should not have affected assembly and tests. But seems you need to rebuild CI again in order to pass checks.

[rrelyea]

docs.tt changes look good. thx.

[BlackGad]

@zivkan is it merge time?)

[zivkan]

@BlackGad thanks you so much for your patience and all your efforts 🥳🎉

[BlackGad]

@zivkan Hooray! So when it will be released? NuGet.exe, VS etc?

[zivkan]

I added a comment in the issue tracking the feature: NuGet/Home#5773 (comment)

[sean-gilliam]

You all rock. Thank you so much for this <3