-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
implement wildcard DNS #1536
base: master
Are you sure you want to change the base?
implement wildcard DNS #1536
Conversation
If we are touching this code, it should be done in a RFC compliant matter. This current PR is a step in the right direction, but should take it all the way IMO |
I was hoping to get this out until you managed to find some time to pull the complexity from CoreDNS. |
339daa6
to
d6c70c7
Compare
func (mux *ServeMux) matchWildcard(q string) Handler { | ||
|
||
wildcards := "*." | ||
// replace the labels of q with wildcard labels until we get a match | ||
for off, end := 0, false; !end; off, end = NextLabel(q, off) { | ||
// skip to removing the first label | ||
if off == 0 { | ||
continue | ||
} | ||
if h, ok := mux.z[wildcards+q[off:]]; ok { | ||
return h | ||
} |
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 common case of www.example.com
matching *.example.com
should be fast as it is always the first iteration of the for
loop after the continue
@miekg I have tried to implement full compliance. Please see the tests for examples |
The |
Thanks @janik-cloudflare - Looking at your profile, there's a chance you understand DNS better than I do! I gave the RFC another read, and realised that On the other hand, I am not sure that "A single wildcard should be enough to match any number of labels". See this section of the RFC. I will paste below the bits I deemed relevant (emphasis also mine):
Prompted by your comment, I also set up a Please let me know if this sounds right to you too, and thanks again! |
Wildcards are, unfortunately, very messy and complicated. (Continue reading at your own risk; it may make you never want to touch DNS again.) "A single wildcard should be enough to match any number of labels" is generally true, but perhaps a bit of an oversimplification. Let's say you have a wildcard record
I recommend https://www.ietf.org/slides/slides-edu-dns-00.pdf slides 14-16 as a good overview of these behaviors. Implementing all this efficiently gets quite challenging, especially when empty non-terminals are involved, and it's something we've been having lots of fun with too. For empty non-terminals you'd also need to make sure to return the correct response code (i.e., if This works for me, by the way:
|
Have we ever had any updates yet? |
Hi, sorry for the inactivity. Quite frankly, Janik's post was perscient
I just own a project that makes use of this library and would love to see #1534 fixed. But I am not a DNS expert (and to be honest I do not have the energy to try to become one) so it is hard for me to fix this myself. Ideally I would love @miekg to implement this properly, as he is much more knowledgeable than me. I appreciate he might also not have the bandwidth (and that is fair enough) but in that case I think it would be reasonable to remove RFC-4592 (DNS wildcards) from the README's 'supported RFCs'. |
Implement simple wildcard DNS matching. For a server with
You will now get
1.1.1.1
when querying forwww.example.com
as per https://datatracker.ietf.org/doc/html/rfc4592.Note that after the PR, this lib is still not rfc4592 compliant because for:
foo.bar.example.com
would still matchexample.com
rather than the wildcards.But still, this PR is small and has no backwards-incompatible changes (assuming you were not using wildcards, which were not compliant anyway).
So I do think it's worth getting closer to compliance until we get the full implementation