-
Notifications
You must be signed in to change notification settings - Fork 904
Implement a fast lookup for wildcard certificates #2815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ac3d38d
9fc3a43
6f7c9a7
0d20eac
d0cdea6
d13a5fb
d5a7a07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -99,10 +99,13 @@ public void Update(WatchEventType eventType, V1Secret secret) | |||||||||||
|
|
||||||||||||
| if (!string.Equals(namespacedName.ToString(), _options.DefaultSslCertificate, StringComparison.OrdinalIgnoreCase)) | ||||||||||||
| { | ||||||||||||
| return; | ||||||||||||
| _logger.LogInformation("Found secret `{NamespacedName}` to use as default certificate for HTTPS traffic", namespacedName); | ||||||||||||
| } | ||||||||||||
| else | ||||||||||||
| { | ||||||||||||
| _logger.LogInformation("Found secret `{NamespacedName}` to use for HTTPS traffic", namespacedName); | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might result in a bunch of misleading logs (including warnings) since we're assuming all secrets are certs yarp/src/Kubernetes.Controller/Certificates/CertificateHelper.cs Lines 33 to 37 in e04fc5a
We should probably check that it's a relevant secret before logging. |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| _logger.LogInformation("Found secret `{NamespacedName}` to use as default certificate for HTTPS traffic", namespacedName); | ||||||||||||
|
|
||||||||||||
| var certificate = _certificateHelper.ConvertCertificate(namespacedName, secret); | ||||||||||||
| if (certificate is null) | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,123 @@ | ||||||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||
|
|
||||||||||||||||||
| using System; | ||||||||||||||||||
| using System.Collections.Generic; | ||||||||||||||||||
| using System.Diagnostics.CodeAnalysis; | ||||||||||||||||||
|
|
||||||||||||||||||
| #nullable enable | ||||||||||||||||||
| namespace Yarp.Kubernetes.Controller.Certificates; | ||||||||||||||||||
|
|
||||||||||||||||||
| public abstract class ImmutableCertificateCache<TCert> where TCert : class | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make this internal. Why does it need to be abstract and generic? We should be able to merge this and |
||||||||||||||||||
| { | ||||||||||||||||||
| private readonly List<WildCardDomain> _wildCardDomains = new(); | ||||||||||||||||||
| private readonly Dictionary<string, TCert> _certificates = new(StringComparer.OrdinalIgnoreCase); | ||||||||||||||||||
|
|
||||||||||||||||||
| public ImmutableCertificateCache(IEnumerable<TCert> certificates, Func<TCert, IEnumerable<string>> getDomains) | ||||||||||||||||||
| { | ||||||||||||||||||
| foreach (var certificate in certificates) | ||||||||||||||||||
| { | ||||||||||||||||||
| foreach (var domain in getDomains(certificate)) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (domain.StartsWith("*.")) | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| { | ||||||||||||||||||
| _wildCardDomains.Add(new (domain[1..], certificate)); | ||||||||||||||||||
| } | ||||||||||||||||||
| else | ||||||||||||||||||
| { | ||||||||||||||||||
| _certificates[domain] = certificate; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| _wildCardDomains.Sort(DomainNameComparer.Instance); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| protected abstract TCert? GetDefaultCertificate(); | ||||||||||||||||||
|
Comment on lines
+34
to
+38
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Please remove all the extra new lines (across all touched files) |
||||||||||||||||||
|
|
||||||||||||||||||
| public TCert? GetCertificate(string domain) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (TryGetCertificateExact(domain, out var certificate)) | ||||||||||||||||||
| { | ||||||||||||||||||
| return certificate; | ||||||||||||||||||
| } | ||||||||||||||||||
| if (TryGetWildcardCertificate(domain, out certificate)) | ||||||||||||||||||
| { | ||||||||||||||||||
| return certificate; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return GetDefaultCertificate(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| protected IReadOnlyList<WildCardDomain> WildcardCertificates => _wildCardDomains; | ||||||||||||||||||
|
|
||||||||||||||||||
| protected IReadOnlyDictionary<string, TCert> Certificates => _certificates; | ||||||||||||||||||
|
|
||||||||||||||||||
| protected record struct WildCardDomain(string Domain, TCert? Certificate); | ||||||||||||||||||
|
|
||||||||||||||||||
| private bool TryGetCertificateExact(string domain, [NotNullWhen(true)] out TCert? certificate) => | ||||||||||||||||||
| _certificates.TryGetValue(domain, out certificate); | ||||||||||||||||||
|
|
||||||||||||||||||
| private bool TryGetWildcardCertificate(string domain, [NotNullWhen(true)] out TCert? certificate) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (_wildCardDomains.BinarySearch(new WildCardDomain(domain, null!), DomainNameComparer.Instance) is { } index and > -1) | ||||||||||||||||||
| { | ||||||||||||||||||
| certificate = _wildCardDomains[index].Certificate!; | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| certificate = null; | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// <summary> | ||||||||||||||||||
| /// Sorts domain names right to left. | ||||||||||||||||||
| /// This allows us to use a Binary Search to achieve a suffix | ||||||||||||||||||
| /// search. | ||||||||||||||||||
| /// </summary> | ||||||||||||||||||
| private class DomainNameComparer : IComparer<WildCardDomain> | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| { | ||||||||||||||||||
| public static readonly DomainNameComparer Instance = new(); | ||||||||||||||||||
|
|
||||||||||||||||||
| public int Compare(WildCardDomain x, WildCardDomain y) | ||||||||||||||||||
| { | ||||||||||||||||||
| var ret = Compare(x.Domain.AsSpan(), y.Domain.AsSpan()); | ||||||||||||||||||
| if (ret != 0) | ||||||||||||||||||
| { | ||||||||||||||||||
| return ret; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return (x.Certificate, y.Certificate) switch | ||||||||||||||||||
| { | ||||||||||||||||||
| (null, not null) when x.Domain.Length > y.Domain.Length => 0, | ||||||||||||||||||
| (not null, null) when x.Domain.Length < y.Domain.Length => 0, | ||||||||||||||||||
| _ => x.Domain.Length - y.Domain.Length | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private static int Compare(ReadOnlySpan<char> x, ReadOnlySpan<char> y) | ||||||||||||||||||
| { | ||||||||||||||||||
|
|
||||||||||||||||||
| var length = Math.Min(x.Length, y.Length); | ||||||||||||||||||
| x = x[^length..]; | ||||||||||||||||||
| y = y[^length..]; | ||||||||||||||||||
|
|
||||||||||||||||||
| for (var index = length - 1; index >= 0; index--) | ||||||||||||||||||
| { | ||||||||||||||||||
| var charA = x[index] & 0x5F; | ||||||||||||||||||
| var charB = y[index] & 0x5F; | ||||||||||||||||||
|
Comment on lines
+109
to
+110
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it might corrupt various characters. If the intention is to normalize the case for only ASCII characters, these should first check that these are ASCII letters. |
||||||||||||||||||
|
|
||||||||||||||||||
| if (charA == charB) | ||||||||||||||||||
| { | ||||||||||||||||||
| continue; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return charB - charA; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return 0; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||||||||||
| #nullable enable | ||||||||||||||||||||||
| using System.Collections.Generic; | ||||||||||||||||||||||
| using System.Formats.Asn1; | ||||||||||||||||||||||
| using System.Linq; | ||||||||||||||||||||||
| using System.Security.Cryptography.X509Certificates; | ||||||||||||||||||||||
| using Microsoft.Extensions.Options; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| namespace Yarp.Kubernetes.Controller.Certificates; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| internal partial class ServerCertificateSelector | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| private class ImmutableX509CertificateCache(X509Certificate2? defaultCertificate, IEnumerable<X509Certificate2> certificates) | ||||||||||||||||||||||
| : ImmutableCertificateCache<X509Certificate2>(certificates, GetDomains) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| protected override X509Certificate2? GetDefaultCertificate() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| if (WildcardCertificates.Count != 0) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return WildcardCertificates[0].Certificate; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return Certificates.Values.FirstOrDefault(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public X509Certificate2? DefaultCertificate() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return defaultCertificate ?? Certificates.Values.FirstOrDefault(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private static IEnumerable<string> GetDomains(X509Certificate2 certificate) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| if (certificate.GetNameInfo(X509NameType.DnsName, false) is { } dnsName) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| yield return dnsName; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const string SAN_OID = "2.5.29.17"; | ||||||||||||||||||||||
| var extension = certificate.Extensions[SAN_OID]; | ||||||||||||||||||||||
| if (extension is null) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| yield break; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var dnsNameTag = new Asn1Tag(TagClass.ContextSpecific, tagValue: 2, isConstructed: false); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var asnReader = new AsnReader(extension.RawData, AsnEncodingRules.BER); | ||||||||||||||||||||||
| var sequenceReader = asnReader.ReadSequence(Asn1Tag.Sequence); | ||||||||||||||||||||||
| while (sequenceReader.HasData) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| var tag = sequenceReader.PeekTag(); | ||||||||||||||||||||||
| if (tag != dnsNameTag) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| sequenceReader.ReadEncodedValue(); | ||||||||||||||||||||||
| continue; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var alternativeName = sequenceReader.ReadCharacterString(UniversalTagNumber.IA5String, dnsNameTag); | ||||||||||||||||||||||
| yield return alternativeName; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+61
to
+67
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,100 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.Security.Cryptography.X509Certificates; | ||
| using System.Text.RegularExpressions; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Connections; | ||
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| #nullable enable | ||
| namespace Yarp.Kubernetes.Controller.Certificates; | ||
|
|
||
| internal class ServerCertificateSelector : IServerCertificateSelector | ||
| internal partial class ServerCertificateSelector | ||
| : BackgroundService | ||
| , IServerCertificateSelector | ||
| { | ||
| private X509Certificate2 _defaultCertificate; | ||
| private readonly ConcurrentDictionary<NamespacedName, X509Certificate2> _certificates = new(); | ||
| private bool _hasBeenUpdated; | ||
| private string? _defaultCertificate; | ||
| private readonly IDisposable? _defaultCertificateSubscription; | ||
|
|
||
| private ImmutableX509CertificateCache _certificateStore = new(null, []); | ||
|
|
||
| public ServerCertificateSelector(IOptionsMonitor<YarpOptions> options) | ||
| { | ||
| _defaultCertificateSubscription = options.OnChange(x => | ||
| { | ||
| if (_defaultCertificate != x.DefaultSslCertificate) | ||
| { | ||
| _defaultCertificate = x.DefaultSslCertificate; | ||
| _hasBeenUpdated = true; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| public override void Dispose() | ||
| { | ||
| if (_defaultCertificateSubscription is {} subscription) | ||
| { | ||
| subscription.Dispose(); | ||
| } | ||
| base.Dispose(); | ||
| } | ||
|
|
||
| public void AddCertificate(NamespacedName certificateName, X509Certificate2 certificate) | ||
| { | ||
| _defaultCertificate = certificate; | ||
| _certificates[certificateName] = certificate; | ||
| _hasBeenUpdated = true; | ||
| } | ||
|
|
||
| public X509Certificate2 GetCertificate(ConnectionContext connectionContext, string domainName) | ||
| public X509Certificate2? GetCertificate(ConnectionContext connectionContext, string? domainName) | ||
| { | ||
| return _defaultCertificate; | ||
| if (string.IsNullOrEmpty(domainName)) | ||
| { | ||
| return _certificateStore.DefaultCertificate(); | ||
| } | ||
| return _certificateStore.GetCertificate(domainName); | ||
| } | ||
|
|
||
| public void RemoveCertificate(NamespacedName certificateName) | ||
| { | ||
| _defaultCertificate = null; | ||
| _ = _certificates.TryRemove(certificateName, out _); | ||
| _hasBeenUpdated = true; | ||
| } | ||
|
|
||
| [GeneratedRegex("(?<namespace>[a-z0-9\\-\\.]*)/(?<certificateName>[a-z0-9\\-\\.]*)", RegexOptions.IgnoreCase, "en-US")] | ||
| private static partial Regex DefaultCertificateNameParser(); | ||
|
|
||
| protected override async Task ExecuteAsync(CancellationToken stoppingToken) | ||
| { | ||
| // Poll every 10 seconds for updates to | ||
| while (!stoppingToken.IsCancellationRequested) | ||
| { | ||
| await Task.Delay(TimeSpan.FromSeconds(10), stoppingToken); | ||
|
Comment on lines
+75
to
+78
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this can use |
||
| if (_hasBeenUpdated) | ||
| { | ||
| _hasBeenUpdated = false; | ||
|
Comment on lines
+79
to
+81
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to handle |
||
|
|
||
| X509Certificate2? defaultCert = null; | ||
| if (_defaultCertificate is { } certificateName | ||
| && DefaultCertificateNameParser().Match(certificateName) is { Success: true } match) | ||
| { | ||
| var namespaceName = match.Groups["namespace"].Value; | ||
| var name = match.Groups["certificateName"].Value; | ||
| var certificateNamespacedName = new NamespacedName(namespaceName, name); | ||
|
|
||
| _ = _certificates.TryGetValue(certificateNamespacedName, out defaultCert); | ||
| } | ||
|
|
||
| _certificateStore = new ImmutableX509CertificateCache(defaultCert, _certificates.Values); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using Xunit; | ||
| #nullable enable | ||
| namespace Yarp.Kubernetes.Controller.Certificates.Tests; | ||
|
|
||
| public class CertificateCacheTests | ||
| { | ||
|
|
||
| private static readonly FakeCertificateCache Cache = new( | ||
| new FakeCertificate("Acme", "mail.acme.com", "www.acme.com"), | ||
| new FakeCertificate("Initech", "*.initech.com", "initech.com"), | ||
| new FakeCertificate("Northwind", "*.northwind.com") | ||
| ); | ||
|
|
||
| [Theory] | ||
| [InlineData("www.acme.com", "Acme")] | ||
| [InlineData("www.ACME.com", "Acme")] | ||
| [InlineData("mail.acme.com", "Acme")] | ||
| [InlineData("acme.com", null)] | ||
| [InlineData("store.acme.com", null)] | ||
| [InlineData("www.northwind.com", "Northwind")] | ||
| [InlineData("mail.northwind.com", "Northwind")] | ||
| [InlineData("northwind.com", null)] | ||
| [InlineData("initech.com", "Initech")] | ||
| [InlineData("www.initech.com", "Initech")] | ||
| [InlineData("www.IniTech.coM", "Initech")] | ||
| public void CertificateConversionFromPem(string requestedDomain, string? expectedCompanyName) | ||
| { | ||
| var certificate = Cache.GetCertificate(requestedDomain); | ||
| if (expectedCompanyName != null) | ||
| { | ||
| Assert.Equal(expectedCompanyName, certificate?.Name); | ||
| } | ||
| else | ||
| { | ||
| Assert.Null(certificate?.Name); | ||
| } | ||
| } | ||
|
|
||
| private record FakeCertificate(string Name, params string[] Domains); | ||
|
|
||
| private class FakeCertificateCache(params IEnumerable<FakeCertificate> certificates) | ||
| : ImmutableCertificateCache<FakeCertificate>(certificates, static cert => cert.Domains) | ||
| { | ||
| protected override FakeCertificate? GetDefaultCertificate() | ||
| { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these if/else logs inverted? (note the
!string.Equals)