Skip to content
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

Add hostname lookup support #43

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hadess
Copy link
Contributor

@hadess hadess commented Mar 17, 2022

Example output from the test tool (replaced the IPv6 address though):

$ ~/.cache/jhbuild/build/microdns/examples/host-lookup diskstation.local
diskstation.local resolves to IPv4 address 192.168.1.68
diskstation.local resolves to IPv6 address ffff:fff:ffff:ffff:fff:ffff:fffff:fffff

Please check carefully.

@jbkempf
Copy link
Contributor

jbkempf commented Mar 17, 2022

Looks good.

@hadess hadess force-pushed the wip/hadess/hostname-lookup branch from 9ae8294 to 370f270 Compare March 17, 2022 22:39
@hadess
Copy link
Contributor Author

hadess commented Mar 20, 2022

I found a couple of things while looking at using this implementation:

  • in mdns_listen(), we should check whether the names end in either .local or .local. and reject them if not
  • we should also check whether they have have a "label count" of 2 or less (eg. foo.local, not foo.bar.local)
  • and in mdns_listen_probe_network(), we should check make sure that .local and .local. suffixes both match the hostnames

@hadess hadess force-pushed the wip/hadess/hostname-lookup branch 2 times, most recently from 0d81181 to 97cca21 Compare March 20, 2022 23:58
@jbkempf
Copy link
Contributor

jbkempf commented Mar 21, 2022

I found a couple of things while looking at using this implementation:
* in mdns_listen(), we should check whether the names end in either .local or .local. and reject them if not

Why being so strict here?

* we should also check whether they have have a "label count" of 2 or less (eg. `foo.local`, not `foo.bar.local`)

Same?

@hadess
Copy link
Contributor Author

hadess commented Mar 21, 2022

I found a couple of things while looking at using this implementation:

  • in mdns_listen(), we should check whether the names end in either .local or .local. and reject them if not

Why being so strict here?

This is what nss-mdns does:
https://github.com/lathiat/nss-mdns#etcmdnsallow

* we should also check whether they have have a "label count" of 2 or less (eg. `foo.local`, not `foo.bar.local`)

Same?

Because that's not a valid .local hostname, as per draft-cheshire-dnsext-multicastdns.txt, so that must mean that a DNS has authority over the .local domain.

@hadess
Copy link
Contributor Author

hadess commented Mar 21, 2022

Note that I'm fine with removing those checks, that just means they would need to be done by clients.

@jbkempf
Copy link
Contributor

jbkempf commented Mar 28, 2022

Note that I'm fine with removing those checks, that just means they would need to be done by clients.

Good point. I would add a flag then inside the entries that are incorrect then. No? WDYT?

@hadess
Copy link
Contributor Author

hadess commented Mar 28, 2022

Good point. I would add a flag then inside the entries that are incorrect then. No? WDYT?

I'll give it a try.

@hadess
Copy link
Contributor Author

hadess commented Mar 29, 2022

I'll give it a try.

I answered this because I thought I would be able to make sense of it in the morning, but I couldn't ;)

Apart from checking that the hostname we're looking up matches one of the entries we received, we're not doing any checks once we've started listening to answers from the network.

All of the checks are done ahead of time, when calling _listen(), and we would return early on invalid inputs. We don't run any checks on network answers, but if we did, that's definitely when we would be flagging those entries. Did you want me to add that?

@jbkempf
Copy link
Contributor

jbkempf commented Mar 29, 2022

All of the checks are done ahead of time, when calling _listen(), and we would return early on invalid inputs. We don't run any checks on network answers, but if we did, that's definitely when we would be flagging those entries. Did you want me to add that?

Yeah, I thought you were going to add a flag on entries to see if they do or do not respect the spec

@hadess
Copy link
Contributor Author

hadess commented Mar 29, 2022

Yeah, I thought you were going to add a flag on entries to see if they do or do not respect the spec

I've looked through this, and I don't think it's strictly necessary for this particular feature, but I can implement it as a separate PR.

It would protect against dodgy answers from other network devices on the network, but that's already a problem with mdns_listen_probe_network() which sends all the answers if just one entry matches a service we requested.

To implement this, I would need to pass the rr_type from the request to check that the answer type is something we want (a service search shouldn't match a hostname lookup). Do we want to mark those answers of another type as invalid, or drop them altogether? Are answers for a hostname or service we didn't ask for valid or invalid? Should the callback be called for all the answers, or only if there are valid answers in the list of entries?

hadess added 7 commits July 1, 2023 14:52
Don't skip over authoritative entries when receiving entries, otherwise
we'll never send A or AAAA records.
As per https://www.rfc-editor.org/rfc/rfc952, "[n]o distinction is
made between upper and lower case" in hostnames, so check whether the
requested name matches the records we receive in a case insensitive way.
Allow foo.local not foo.bar.local, as per
draft-cheshire-dnsext-multicastdns.txt

This is the two-label limit heuristic, which is also implemented in the
default avahi confifuration:
https://github.com/lathiat/nss-mdns#etcmdnsallow
As implemented in the default avahi configuration:
https://github.com/lathiat/nss-mdns#etcmdnsallow

"
If the request does not end with .local or .local., it is rejected.
Example: example.test is rejected.
"
Prints the IPv4, followed by the IPv6 address for the requested
hostname, and exits.

Closes: videolabs#41
Our network listening code might receive packets that were for a
different request than the one we made. Make sure that we only call the
callback when valid entries were received, and make it possible for
clients to skip over invalid entries.
@hadess hadess force-pushed the wip/hadess/hostname-lookup branch from 97cca21 to b1dbacf Compare July 1, 2023 13:00
@hadess
Copy link
Contributor Author

hadess commented Jan 10, 2024

Was there anything else that would be needed to get this merged?

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.

2 participants