dnsjava
patch to support mDNS and multicast to servers
changes so that multicast IP addresses can be used for the DNS "server", so that mDNS servers can be queried to support DNS-SD (Service Discovery).
Thanks for creating a PR! I need to read the mDNS spec to see if this is correct, which will take me a while.
In the meantime, this doesn't even compile and from a first look you seem to have done a lot of code duplication when parsing the answer(s). Please simplify this, e.g. by extracting the validation and parsing into a method that you can call multiple times.
Also, the non-generic response from transaction/multitransaction and later casting is something I'd like to avoid. sendrecv could just return a Future<List<byte[]>> where the single transaction always returns a list with a single entry. Distinguishing if answers should be combined or not can still be done based on the server's destination address.
OK. I was trying to minimize changes to existing (unicast) logic so I wouldn't break anything. But I can rework it to reduce code redundancy.
Andrew
From: Ingo Bauersachs [email protected] Sent: Wednesday, January 6, 2021 5:19 PM To: dnsjava/dnsjava Cc: Andrew Pavlin; Author Subject: Re: [dnsjava/dnsjava] patch to support mDNS and multicast to servers (#148)
Thanks for creating a PR! I need to read the mDNS spec to see if this is correct, which will take me a while.
In the meantime, this doesn't even compile and from a first look you seem to have done a lot of code duplication when parsing the answer(s). Please simplify this, e.g. by extracting the validation and parsing into a method that you can call multiple times.
Also, the non-generic response from transaction/multitransaction and later casting is something I'd like to avoid. sendrecv could just return a Future<List<byte[]>> where the single transaction always returns a list with a single entry. Distinguishing if answers should be combined or not can still be done based on the server's destination address.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/dnsjava/dnsjava/pull/148#issuecomment-755752217, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AESTBTJNLGDIR2OPIHKUNF3SYTOYZANCNFSM4VYCJ7VA.
This is bizarre. The code compiles, but something about a format check in the source code in your maven script caused it to be rejected. And I copied the format that was already in those source files.
So I looked at the error from the pull request build, so I am making a third change to my branch of the code base to keep the format-checking Maven plugin from griping.
From: Ingo Bauersachs [email protected] Sent: Wednesday, January 6, 2021 5:19 PM To: dnsjava/dnsjava Cc: Andrew Pavlin; Author Subject: Re: [dnsjava/dnsjava] patch to support mDNS and multicast to servers (#148)
Thanks for creating a PR! I need to read the mDNS spec to see if this is correct, which will take me a while.
In the meantime, this doesn't even compile and from a first look you seem to have done a lot of code duplication when parsing the answer(s). Please simplify this, e.g. by extracting the validation and parsing into a method that you can call multiple times.
Also, the non-generic response from transaction/multitransaction and later casting is something I'd like to avoid. sendrecv could just return a Future<List<byte[]>> where the single transaction always returns a list with a single entry. Distinguishing if answers should be combined or not can still be done based on the server's destination address.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/dnsjava/dnsjava/pull/148#issuecomment-755752217, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AESTBTJNLGDIR2OPIHKUNF3SYTOYZANCNFSM4VYCJ7VA.
This is bizarre. The code compiles, but something about a format check in the source code in your maven script caused it to be rejected. And I copied the format that was already in those source files. So I looked at the error from the pull request build, so I am making a third change to my branch of the code base to keep the format-checking Maven plugin from griping.
You can auto-format the code by running the Maven target fmt:format. This applies most of the Google Java Format code style. If you're using IntelliJ, you can setup a plugin to format with Ctrl+Alt+L: https://github.com/google/google-java-format#intellij-android-studio-and-other-jetbrains-ides
Re: reformatting: I didn't want to reformat any lines I didn't change, and cause spurious non-changing source code changes. Such behavior of IDEs (that "always know better than you do how your code should be formatted", especially when multiple developers' IDEs are configured differently) makes it a pain to review code changes, so I am always very careful not to change the styling of other lines of code (not even switching spaces versus tabs).
Have you looked at some details of rfc6762? Specifically:
I sure did. That's how I came up with the idea of deliberately timing out the resolver so I could wait for multiple answers to come in.
port 5353 must not be used as the local port as we're not fully compliant
My proposed changes don't specify the port number. The caller is currently required to be smart enough to specify port 5353 when they also specify a multicast address as the "address" of the remote DNS server, since the SimpleResolver constructor I tested with takes InetSocketAddress objects that include the port number. I suppose the constructors taking a String or an InetAddress should check for multicast addresses and switch from DEFAULT_PORT (= 53) to 5353 automatically.
Or are you saying I need to add a safety check for when the local port is selected to ensure it doesn't accidentally get 5353 as the port number?
Section 3 mandates that any query to .local. goes to an mDNS server. Not sure how to properly address that, enabling it by default is not an option because .local has been abused by non-mdns setups too much. But I could imagine that the ExtendedResolver sends .local to the multicast address if the multicast address is one of the resolver addresses. A multicast resolver address could be automatically added if a (new) property is set.
That would have been a larger-impact change that I didn't need to do for my limited use-case, which knew that I was making a ".local" query. Would you like me to make the additional changes to automatically switch to multicast for such queries? Problem is, can I safely assume only IPv6 or only IPv4 for the default multicast address?
Setting the unicast-response bit should be easier to set/strip out (DClass). Not quite sure how repsonses look like. If the bit is still set in the response, it needs to be stripped after validation (or the answers won't be recognized as class IN), otherwise the validation needs to accept responses without the bit set as valid
I didn't catch that, because my tests all worked correctly with multicast responses. I never received such a response when I did a multicast query in my tests. But I can make such a change to clear that bit in the received responses. Would you like me to do so?
What's an easy way to live test this code? Should having some Windows/Apple devices in the network suffice?
Have a Linux system on the LAN with the avahi-daemon running, then use the avahi-publish command to register some mDNS entries for testing. I suppose Apple devices on the LAN would do the same thing, but you would have less control over what answers you could get.
Or are you saying I need to add a safety check for when the local port is selected to ensure it doesn't accidentally get 5353 as the port number?
Yes, that's what I meant.
Section 3 mandates that any query to .local. goes to an mDNS server. Not sure how to properly address that, enabling it by default is not an option because .local has been abused by non-mdns setups too much. But I could imagine that the ExtendedResolver sends .local to the multicast address if the multicast address is one of the resolver addresses. A multicast resolver address could be automatically added if a (new) property is set.
That would have been a larger-impact change that I didn't need to do for my limited use-case, which knew that I was making a ".local" query. Would you like me to make the additional changes to automatically switch to multicast for such queries?
I'm not sure how useful it is. I guess we could always add it later if it seems worthwhile.
Problem is, can I safely assume only IPv6 or only IPv4 for the default multicast address?
It depends :-) If would only activate mDNS if the Resolver contains a broadcast address, and that address would already dictate which IP version to use. If both an IPv4 and an IPv6 address is present, then I guess both should be queried.
Setting the unicast-response bit should be easier to set/strip out (DClass). Not quite sure how repsonses look like. If the bit is still set in the response, it needs to be stripped after validation (or the answers won't be recognized as class IN), otherwise the validation needs to accept responses without the bit set as valid
I didn't catch that, because my tests all worked correctly with multicast responses. I never received such a response when I did a multicast query in my tests. But I can make such a change to clear that bit in the received responses. Would you like me to do so?
I'm not sure it's required, and I couldn't conclude from the RFC without looking at packets.
What's an easy way to live test this code? Should having some Windows/Apple devices in the network suffice?
Have a Linux system on the LAN with the avahi-daemon running, then use the avahi-publish command to register some mDNS entries for testing. I suppose Apple devices on the LAN would do the same thing, but you would have less control over what answers you could get.
Okay, I'll look at some of those packets, then I'll know if that bit mentioned above needs attention or not. Thanks.
Okay, so I sent some sample queries on my LAN with the unicode flag masked in, like this:
public class Test {
public static void main(String[] args) throws IOException {
SimpleResolver sr = new SimpleResolver(new InetSocketAddress("224.0.0.251", 5353));
Record q = Record.newRecord(Name.fromConstantString("macmini.local."), Type.A, DClass.IN | 0x8000);
Message mq = Message.newQuery(q);
System.out.println(sr.send(mq));
}
}
I get a unicast reply from the Mac Mini, but also two multicast responses from avahi-daemons I seem to have running. The answers from the avahi-daemons are thrown away because the validation in verify1response doesn't take the unicast flag into account (or rather: doesn't ignore it).
The relevant part from the RFC is this sentence (emphasis mine):
When this bit is set in a question, it indicates that the querier is willing to accept unicast replies in response to this specific query, as well as the usual multicast responses. These questions requesting unicast responses are referred to as "QU" questions, to distinguish them from the more usual questions requesting multicast responses ("QM" questions).
Setting the unicast flag in the first hand with masking 0x8000 into DClass.IN is unnatural. There needs to be a better way to set this flag. Parsing and removing needs to be better as well because a printed messages will look like this:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 56018
;; flags: qr aa ; qd: 1 an: 1 au: 0 ad: 0
;; QUESTIONS:
;; macmini.local., type = A, class = CLASS32769
;; ANSWERS:
macmini.local. 10 IN A 192.168.10.103
;; AUTHORITY RECORDS:
;; ADDITIONAL RECORDS:
;; Message size: 47 bytes
Note CLASS32769 instead of IN, and maybe a unicast indication. Wireshark for example shows such a query as class IN, "QU" question or as class IN, "QM" question.
Answer records also have a special meaning of the highest bit in the class, meaning "Cache flush" if it is set (sections 10.3 and 10.4).
I'm not sure how to best handle this flag (in questions and answers). Maybe @nresare could chime in here and has an idea?
and again making the formatter happy (since it only makes one complaint at a time).
Hello.
Sorry I'm behind on this patch. I hope to get back to it next week.
Andrew
From: cary-xian @.***> Sent: Monday, July 26, 2021 8:26 PM To: dnsjava/dnsjava Cc: Andrew Pavlin; Author Subject: Re: [dnsjava/dnsjava] patch to support mDNS and multicast to servers (#148)
dns
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/dnsjava/dnsjava/pull/148#issuecomment-887118267, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AESTBTKTWMZGUGRIXCGUBK3TZX4JVANCNFSM4VYCJ7VA.
Is this still a WIP? And are you still working on this @ka2ddo? I wouldn't mind giving it a look.
