Skip to content

Conversation

@arontsang
Copy link

No description provided.

@arontsang arontsang force-pushed the feature/fast-wildcard-certificate-lookup branch 3 times, most recently from 5540ac3 to d979058 Compare April 5, 2025 10:16
Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: there are quite a few blank lines that could be removed too.

Comment on lines 107 to 108
var charA = x[^i] & 0x5F;
var charB = y[^i] & 0x5F;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You compare it backwards?
Is it better then, to write the loop backwards and have normal index access?

Comment on lines 103 to 105
var length = Math.Min(x.Length, y.Length);

for (var i = 1; i <= length; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the JIT can't elide the bound-check for indexing into the spans.
I think it's better to slice the spans to the common (= min) length and use one span in the loop, in order to elide at least one bound check (the JIT should handle the downward loop (see other comment) in a recent version).

Roughly:

Suggested change
var length = Math.Min(x.Length, y.Length);
for (var i = 1; i <= length; i++)
if (x.Length < y.Length)
{
y = y.Slice(0, x.Length);
}
else if (y.Length < x.Length)
{
x = x.Slice(0, y.Length);
}
for (var i = x.Length; i >= 1; --i)

_hasBeenUpdated = true;
}

public X509Certificate2 GetCertificate(ConnectionContext connectionContext, string domainName)
Copy link

@KLuuKer KLuuKer Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well poop, forgot about SNI. I would expect that everything supports SNI by now. But yes, get your point...sigh...

Copy link

@KLuuKer KLuuKer Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would suggest skipping certificate lookups and immediately fallback to the default cert when string.IsNullOrEmpty
you WILL get this traffic anytime someone runs ssllabs server test, or some other kind of auditing tool
or for the handful of people running this on their own internal network and connecting with some kind of weird legacy system

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to handle non SNI TLS hand shake.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KLuuKer
Copy link

KLuuKer commented Nov 11, 2025

If this pull also needs to work for the Kubernetes.Controller the following needs to be modified also

https://github.com/dotnet/yarp/blob/785e5fc80540baf3f34db17686d90cad595000a3/src/Kubernetes.Controller/Caching/IngressCache.cs#L100C9-L103C10

note: i don't work for MS but i have this code running right now with those lines patched out.
and from my limited testing (cloudflare strict certificate check) it seems to work.

@arontsang arontsang force-pushed the feature/fast-wildcard-certificate-lookup branch from e56abc3 to 0d20eac Compare November 11, 2025 06:37
@arontsang
Copy link
Author

If this pull also needs to work for the Kubernetes.Controller the following needs to be modified also

https://github.com/dotnet/yarp/blob/785e5fc80540baf3f34db17686d90cad595000a3/src/Kubernetes.Controller/Caching/IngressCache.cs#L100C9-L103C10

note: i don't work for MS but i have this code running right now with those lines patched out. and from my limited testing (cloudflare strict certificate check) it seems to work.

Thanks for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants