-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: add support for wildcard on SNIs for SSL #12668
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: master
Are you sure you want to change the base?
feat: add support for wildcard on SNIs for SSL #12668
Conversation
apisix/ssl/router/radixtree_sni.lua
Outdated
if s ~= "*" then | ||
j = j + 1 | ||
sni[j] = s:reverse() | ||
end |
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.

It is reasonable not to perform additional processing, as it does not cause any issues.
This is not necessary, as ("*"):reverse()
functions correctly. Even if this extra check were added, it would only apply in extremely rare cases and would require explanation for maintainability. I do not believe it differs from normal sni case.
apisix/ssl/router/radixtree_sni.lua
Outdated
if ssl.value.sni ~= "*" then | ||
sni = ssl.value.sni:reverse() | ||
end |
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.
ditto
apisix/ssl/router/radixtree_sni.lua
Outdated
handler = function (api_ctx) | ||
if not api_ctx then | ||
return | ||
if sni and (type(sni) == "table" and #sni > 0 or type(sni) == "string") then |
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.
This seems to cover all existing scenarios. Is this necessary?
apisix/ssl/router/radixtree_sni.lua
Outdated
local sni_rev = sni:reverse() | ||
local ok = radixtree_router:dispatch(sni_rev, nil, api_ctx) | ||
|
||
-- if no SSL matched, try to find a wildcard SSL |
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.
Can't radix trees perform wildcard matching directly? Why do wildcard matches work fine in our HTTP routes (/test/*
) and SSL SNI (*.example.com
)?
In my view, there's no fundamental difference between *.example.com
and *
. If the former works correctly after reverse sort, the latter should also function out of the box. We shouldn't need to add any special logic for this.
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.
Please refer #12668 (comment)
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.
I saw your reply at #12668 (comment), but it doesn't resolve this concern. We should investigate the root cause.
apisix/ssl/router/radixtree_sni.lua
Outdated
for _, s in ipairs(ssl.value.snis) do | ||
j = j + 1 | ||
sni[j] = s:reverse() | ||
if s ~= "*" then |
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.
to me, I think we can keep old code, it seems work fine too? all right?
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.
The main point of contention lies in #12668 (comment), which determines whether we must handle the * case separately.
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.
Okay i was wrong. Due to an initial mistake in my code I got that failure and falsely assumed it was because radixtree couldn't handle it. I have modified the PR. Apologies for mistake
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.
LGTM
The current APISIX SSL SNI only allows exact domain name matching "abc.test.com" and partial wildcard ".test.com". It does not support the complete wildcard value of "" for SNI, which does not meet the core consistency test requirements of HTTPSListener in the Gateway API. This situation needs to be supported.
Checklist