Support PEM for CertificatesClient.DownloadCertificate

[heaths]

Resolves #16897
Resolves #18945

[heaths]

@pakrym @schaabs if you're good with the PemReader class I added, I can move it to shared and even update ClientCertificateCredential to use it if you like. It's faster than using the Regex classes (which aren't explicitly compiled, though they may be defined in such a way it happens automatically - I forget the heuristics off hand), and handles a few possible resource leaks. I wrote it and the tests so they could easily be drop-ins.

It's not as robust as net50's PemEncoding but it probably doesn't need to be. It's certainly no less robust than the current ClientCertifricateCredential code.

[heaths]

Well, this is surprising:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.103
  [Host]     : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
  DefaultJob : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CurrentClientCertificateCredential	6.936 ms	0.1197 ms	0.1061 ms	7.8125	-	-	39.88 KB
NewPemReader	7.277 ms	0.1079 ms	0.0842 ms	-	-	-	17.95 KB

The regex must get compiled as used. I also took out the file reading from ClientCertificateCredential for benchmarking purposes since that would be a wash anyway. Still, it allocates less and doesn't rely on regex.

[heaths]

A subsequent run against net461 (though, in the end this did not produce results), netcoreapp3.1, and netcoreapp5.0 resulted in more plausible numbers:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.103
  [Host]     : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
  Job-SUXRZX : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
  Job-YJQFCR : .NET Core 5.0.3 (CoreCLR 5.0.321.7212, CoreFX 5.0.321.7212), X64 RyuJIT

Method	Job	Runtime	Toolchain	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
CurrentClientCertificateCredential	Job-RFILUE	.NET 4.6.1	net461	NA	NA	NA	-	-	-	-
CurrentClientCertificateCredential	Job-SUXRZX	.NET Core 3.1	netcoreapp31	6.527 ms	0.0694 ms	0.0542 ms	7.8125	-	-	40816 B
CurrentClientCertificateCredential	Job-YJQFCR	.NET Core 5.0	netcoreapp50	6.441 ms	0.0779 ms	0.0691 ms	7.8125	-	-	35777 B
NewPemReader	Job-RFILUE	.NET 4.6.1	net461	NA	NA	NA	-	-	-	-
NewPemReader	Job-SUXRZX	.NET Core 3.1	netcoreapp31	6.245 ms	0.1198 ms	0.1600 ms	-	-	-	18371 B
NewPemReader	Job-YJQFCR	.NET Core 5.0	netcoreapp50	6.325 ms	0.1263 ms	0.1181 ms	-	-	-	18067 B

PemReader is showing slightly faster. I made sure my laptop didn't sleep, though I don't believe it did before test results were finished last time.

[pakrym]

cert-password-protected.pfx seems to be empty

[@heaths] it's a binary file of 4133 bytes.

[heaths]

Sorry for the force push, but the build break was caused by a change in master after I branched that the PR build was picking up. Mainly just moved PemReader and updated Azure.Identity to use it. Found an issue or two with PemReader and added test case/s for it/them, e.g. if a "CERTIFICATE" is listed in a PEM file before "PRIVATE KEY". Valid, but unusual (openssl writes the "PRIVATE KEY" first).

[schaabs]

It seems PemReader only supports RSA certificates at the moment. This wasn't a concern with ClientCertificateCredentials because ATM AAD only supports RSA certificates for authentication, but the certificate here could also be an EC certificate no? Perhaps we should guard against this and file an issue to support EC certificates as well?

[heaths]

This - and the old implementation - only support labels like "BEGIN PRIVATE KEY", not "BEGIN RSA PRIVATE KEY" or "BEGIN EC PRIVATE KEY", so I don't think there's anything to guard as it just won't parse into a valid private key, which will throw an InvalidDataException as the old impl did before.

Still, I can open a bug to add support for "RSA PRIVATE KEY" since it could work alternatively.

[heaths]

#19046

[heaths]

I also opened #19047 to track support EC private keys for Key Vault.

[schaabs]

I was thinking that since you're calling get certificate you might have information on what the key type of the current certificate is (although it might just be on the policy which could be different from the actual cert). If this is a case you could get use it to give a better error message.

[heaths]

Yeah, it's only in the key_props of the policy, so it wouldn't help.

That said, investigations show that Key Vault and openssl pkcs12 both produce PEM files with only CERTIFICATE and PRIVATE KEY (see #19047 (comment)). We should probably add some tests, but I'm not sure there's really any support to add right now - not for Key Vault, at least. Might if you want to support loading full chains (up to and/or including the root) for ClientCertificateCredential.