From b3269a5a22503c00d4772bee7e236883b6c4cd21 Mon Sep 17 00:00:00 2001 From: Caleb Kiage <747955+calebkiage@users.noreply.github.com> Date: Thu, 25 Jul 2024 15:19:29 +0300 Subject: [PATCH 1/4] fix: only fetch valid X.509 certificates from the certificate store perf: remove linq code use to improve code generation --- .../ClientCertificateCredentialFactory.cs | 36 +++++++++------ .../Http/LoggingHandler.cs | 45 +++++++++++++------ 2 files changed, 55 insertions(+), 26 deletions(-) diff --git a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs index c2741904..9168752b 100644 --- a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs +++ b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs @@ -1,10 +1,8 @@ using System; -using System.Linq; using System.Security; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using Azure.Identity; -using Microsoft.Graph.Cli.Core.Utils; namespace Microsoft.Graph.Cli.Core.Authentication; @@ -23,7 +21,8 @@ public static class ClientCertificateCredentialFactory /// The entra authentication endpoint (to use with national clouds) /// A ClientCertificateCredential /// When a null url is provided for the authority host. - public static ClientCertificateCredential GetClientCertificateCredential(string? tenantId, string? clientId, string? certificateName, string? certificateThumbPrint, Uri authorityHost) + public static ClientCertificateCredential GetClientCertificateCredential(string? tenantId, string? clientId, + string? certificateName, string? certificateThumbPrint, Uri authorityHost) { if (string.IsNullOrWhiteSpace(certificateName) && string.IsNullOrWhiteSpace(certificateThumbPrint)) { @@ -46,11 +45,13 @@ public static ClientCertificateCredential GetClientCertificateCredential(string? X509Certificate2? certificate; - if (!string.IsNullOrWhiteSpace(certificateName) && TryGetCertificateFromStore(certificateName, isThumbPrint: false, out certificate)) + if (!string.IsNullOrWhiteSpace(certificateName) && + TryGetCertificateFromStore(certificateName, isThumbPrint: false, out certificate)) { return new ClientCertificateCredential(tenantId, clientId, certificate, credOptions); } - else if (!string.IsNullOrWhiteSpace(certificateThumbPrint) && TryGetCertificateFromStore(certificateThumbPrint, isThumbPrint: true, out certificate)) + else if (!string.IsNullOrWhiteSpace(certificateThumbPrint) && + TryGetCertificateFromStore(certificateThumbPrint, isThumbPrint: true, out certificate)) { return new ClientCertificateCredential(tenantId, clientId, certificate, credOptions); } @@ -65,7 +66,8 @@ public static ClientCertificateCredential GetClientCertificateCredential(string? /// If true, try to find the certificate by the thumb print. /// A matching unexpired certificate from the store. /// Returns true if the certificate was fetched successfully. - internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPrint, bool isThumbPrint, out X509Certificate2? certificate) + internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPrint, bool isThumbPrint, + out X509Certificate2? certificate) { bool result = false; certificate = null; @@ -74,11 +76,10 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri try { store.Open(OpenFlags.ReadOnly); - - // If using a certificate with a trusted root you do not need to FindByTimeValid, instead: - // currentCerts.Find(X509FindType.FindBySubjectDistinguishedName, certName, true); - X509Certificate2Collection signingCerts = store.Certificates.Find(X509FindType.FindByTimeValid, DateTime.Now, false) - .Find(isThumbPrint ? X509FindType.FindByThumbprint : X509FindType.FindBySubjectDistinguishedName, certificateNameOrThumbPrint, false); + X509Certificate2Collection signingCerts = store.Certificates + .Find(X509FindType.FindByTimeValid, DateTime.Now, true) + .Find(isThumbPrint ? X509FindType.FindByThumbprint : X509FindType.FindBySubjectDistinguishedName, + certificateNameOrThumbPrint, true); if (signingCerts.Count == 0) { result = false; @@ -86,7 +87,15 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri else { // Return the first certificate in the collection, has the right name and is current. - certificate = signingCerts.OrderByDescending(static c => c.NotBefore).FirstOrDefault(); + foreach (var cert in signingCerts) + { + // Use this certificate if it became valid after the currently selected one. + if (certificate is null || cert.NotBefore > certificate.NotBefore) + { + certificate = cert; + } + } + result = true; } } @@ -98,7 +107,7 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri } catch (SecurityException) { - // Isufficient permissions to read the store + // Insufficient permissions to read the store result = false; } catch (ArgumentException) @@ -113,6 +122,7 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri certificate.Dispose(); certificate = null; } + store.Close(); } diff --git a/src/Microsoft.Graph.Cli.Core/Http/LoggingHandler.cs b/src/Microsoft.Graph.Cli.Core/Http/LoggingHandler.cs index 60b79797..faa30dda 100644 --- a/src/Microsoft.Graph.Cli.Core/Http/LoggingHandler.cs +++ b/src/Microsoft.Graph.Cli.Core/Http/LoggingHandler.cs @@ -1,8 +1,8 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Net.Http; using System.Net.Http.Headers; +using System.Text; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -47,29 +47,48 @@ protected override async Task SendAsync( private static string HeadersToString(in HttpHeaders headers, in HttpContentHeaders? contentHeaders) { - if (!headers.Any() && contentHeaders?.Any() == false) return string.Empty; - static string selector(KeyValuePair> h) + var headersEnumerator = headers.GetEnumerator(); + var contentHeadersEnumerator = contentHeaders?.GetEnumerator(); + if (!headersEnumerator.MoveNext() && contentHeadersEnumerator?.MoveNext() == false) return string.Empty; + headersEnumerator.Dispose(); + contentHeadersEnumerator?.Dispose(); + static void StringifyHeader(string name, IEnumerable values, in StringBuilder sb) { - var value = string.Join(",", h.Value); - if (h.Key.Contains("Authorization", StringComparison.OrdinalIgnoreCase)) + sb.Append(name); + sb.Append(':'); + sb.Append(' '); + if (name.Contains("Authorization", StringComparison.OrdinalIgnoreCase)) { - value = "[PROTECTED]"; + sb.Append("[PROTECTED]"); } - return $"{h.Key}: {value}\n"; - }; + else + { + foreach (var value in values) + { + sb.Append(value); + sb.Append(','); + } - static string aggregator(string a, string b) + sb.Remove(sb.Length - 1, 1); + } + } + static void JoinHeaders(IEnumerable>> headers, in StringBuilder sb) { - return string.Join(string.Empty, a, b); + foreach (var header in headers) + { + StringifyHeader(header.Key, header.Value, sb); + sb.Append('\n'); + } } - var h = headers.Select(selector).Aggregate("", aggregator); + var sb = new StringBuilder(200); + JoinHeaders(headers, sb); if (contentHeaders != null) { - h += contentHeaders.Select(selector).Aggregate("", aggregator); + JoinHeaders(contentHeaders, sb); } - return h; + return sb.ToString(); } /// From 8efba40068eda8044d23e6ebf989e560bf6859af Mon Sep 17 00:00:00 2001 From: Caleb Kiage <747955+calebkiage@users.noreply.github.com> Date: Thu, 25 Jul 2024 15:41:48 +0300 Subject: [PATCH 2/4] fix: check certificate validity period is valid --- .../Authentication/ClientCertificateCredentialFactory.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs index 9168752b..214bc953 100644 --- a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs +++ b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs @@ -77,9 +77,9 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri { store.Open(OpenFlags.ReadOnly); X509Certificate2Collection signingCerts = store.Certificates - .Find(X509FindType.FindByTimeValid, DateTime.Now, true) + .Find(X509FindType.FindByTimeValid, DateTime.Now, false) .Find(isThumbPrint ? X509FindType.FindByThumbprint : X509FindType.FindBySubjectDistinguishedName, - certificateNameOrThumbPrint, true); + certificateNameOrThumbPrint, false); if (signingCerts.Count == 0) { result = false; @@ -89,6 +89,8 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri // Return the first certificate in the collection, has the right name and is current. foreach (var cert in signingCerts) { + // Check validity period. + if (cert.NotAfter < DateTime.Now || cert.NotBefore > DateTime.Now) continue; // Use this certificate if it became valid after the currently selected one. if (certificate is null || cert.NotBefore > certificate.NotBefore) { From 59cb6d5d91b6dbea9caacb0a56612da859424c28 Mon Sep 17 00:00:00 2001 From: Caleb Kiage <747955+calebkiage@users.noreply.github.com> Date: Thu, 25 Jul 2024 16:10:30 +0300 Subject: [PATCH 3/4] chore: add certificate selection tests. chore: remove redundant certificate date checks --- ...ClientCertificateCredentialFactoryTests.cs | 52 +++++++++++++++++++ .../ClientCertificateCredentialFactory.cs | 28 ++++++---- 2 files changed, 69 insertions(+), 11 deletions(-) create mode 100644 src/Microsoft.Graph.Cli.Core.Tests/Authentication/ClientCertificateCredentialFactoryTests.cs diff --git a/src/Microsoft.Graph.Cli.Core.Tests/Authentication/ClientCertificateCredentialFactoryTests.cs b/src/Microsoft.Graph.Cli.Core.Tests/Authentication/ClientCertificateCredentialFactoryTests.cs new file mode 100644 index 00000000..616bc090 --- /dev/null +++ b/src/Microsoft.Graph.Cli.Core.Tests/Authentication/ClientCertificateCredentialFactoryTests.cs @@ -0,0 +1,52 @@ +using System; +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; +using Microsoft.Graph.Cli.Core.Authentication; +using Xunit; + +namespace Microsoft.Graph.Cli.Core.Tests.Authentication; + +public class ClientCertificateCredentialFactoryTests +{ + [Fact] + public void ReturnsNullWhenStoreIsEmpty() + { + var store = new X509Certificate2Collection(); + + var result = ClientCertificateCredentialFactory.FindLatestByValidity(store); + + Assert.Null(result); + } + + [Fact] + public void ReturnsLatestValidCertificate() + { + var store = new X509Certificate2Collection + { + GenerateSelfSignedCertificate("1", new DateTimeOffset(2020, 1, 2, 0, 0, 0, TimeSpan.Zero), + new DateTimeOffset(2027, 1, 1, 0, 0, 0, TimeSpan.Zero)), + GenerateSelfSignedCertificate("2", new DateTimeOffset(2020, 1, 1, 0, 0, 0, TimeSpan.Zero), + new DateTimeOffset(2027, 1, 1, 0, 0, 0, TimeSpan.Zero)) + }; + + var result = ClientCertificateCredentialFactory.FindLatestByValidity(store); + + Assert.NotNull(result); + Assert.Equal("CN=1", result.SubjectName.Name); + } + + private static X509Certificate2 GenerateSelfSignedCertificate(string subjectName, DateTimeOffset notBefore, + DateTimeOffset notAfter) + { + if (notAfter < notBefore) + { + throw new ArgumentException("notAfter must be after notBefore"); + } + + const string secp256R1Oid = "1.2.840.10045.3.1.7"; + var ecdsa = ECDsa.Create(ECCurve.CreateFromValue(secp256R1Oid)); + var certRequest = new CertificateRequest($"CN={subjectName}", ecdsa, HashAlgorithmName.SHA256); + var generatedCert = certRequest.CreateSelfSigned(notBefore, notAfter); + return generatedCert; + } +} diff --git a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs index 214bc953..ad76046e 100644 --- a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs +++ b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs @@ -86,17 +86,7 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri } else { - // Return the first certificate in the collection, has the right name and is current. - foreach (var cert in signingCerts) - { - // Check validity period. - if (cert.NotAfter < DateTime.Now || cert.NotBefore > DateTime.Now) continue; - // Use this certificate if it became valid after the currently selected one. - if (certificate is null || cert.NotBefore > certificate.NotBefore) - { - certificate = cert; - } - } + certificate = FindLatestByValidity(signingCerts); result = true; } @@ -131,6 +121,22 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri return result; } + internal static X509Certificate2? FindLatestByValidity(X509Certificate2Collection signingCerts) + { + X509Certificate2? certificate = null; + // Return the first certificate in the collection, has the right name and is current. + foreach (var cert in signingCerts) + { + // Use this certificate if it became valid after the currently selected one. + if (certificate is null || cert.NotBefore > certificate.NotBefore) + { + certificate = cert; + } + } + + return certificate; + } + /// /// Opens a certificate file. /// From 60f9ea876447c50d964e914fb174a2634584c72a Mon Sep 17 00:00:00 2001 From: Caleb Kiage <747955+calebkiage@users.noreply.github.com> Date: Thu, 25 Jul 2024 16:16:07 +0300 Subject: [PATCH 4/4] Update src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs Co-authored-by: Vincent Biret --- .../Authentication/ClientCertificateCredentialFactory.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs index ad76046e..000ec092 100644 --- a/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs +++ b/src/Microsoft.Graph.Cli.Core/Authentication/ClientCertificateCredentialFactory.cs @@ -130,6 +130,7 @@ internal static bool TryGetCertificateFromStore(string certificateNameOrThumbPri // Use this certificate if it became valid after the currently selected one. if (certificate is null || cert.NotBefore > certificate.NotBefore) { + certificate?.Dispose(); certificate = cert; } }