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
patch to support mDNS and multicast to servers #148
base: master
Are you sure you want to change the base?
Conversation
add conditional logic to support receiving either a byte array of one answer (existing unicast server behavior), or an ArrayList containing one or more byte arrays (multiple responses to multicast query from multiple mDNS servers).
Add support to wait for multiple answers to arrive if and only if the address of the DNS server is a multicast address.
fix misleading text in logging message where the handling has changed.
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. |
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 <notifications@github.com>
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 GitHub<#148 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AESTBTJNLGDIR2OPIHKUNF3SYTOYZANCNFSM4VYCJ7VA>.
|
Refactor the mDNS changes to reduce duplicate code.
refactor most of the duplicated code for error-checking a received query response datagram into a method object.
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 <notifications@github.com>
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 GitHub<#148 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AESTBTJNLGDIR2OPIHKUNF3SYTOYZANCNFSM4VYCJ7VA>.
|
resolve maven com.coveo:format plugin's complaints
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.
Have you looked at some details of rfc6762? Specifically:
- port 5353 must not be used as the local port as we're not fully compliant
- 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 theExtendedResolver
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. - 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 classIN
), otherwise the validation needs to accept responses without the bit set as valid
What's an easy way to live test this code? Should having some Windows/Apple devices in the network suffice?
|
||
// Check that the response is long enough. | ||
if (in.length < Header.LENGTH) { | ||
exc = new WireParseException("invalid DNS header - too short"); |
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.
Why don't you just throw these exceptions? They're unsual enough that I wouldn't consider it to be normal code flow.
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 didn't want to throw them unless there were no successful responses to a mDNS query. mDNS can get garbage packets back which should be silently ignored.And, because there could be multiple responses, I only wanted to throw the exception if no valid responses were returned (i.e., hacker returned garbage, then valid Avahi-daemon returned a legitimate answer [in that order], then the query shouldn't fail).
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.
Yes, I get that, but instead of returning null and then checking the exception instance from a getter, you can throw, catch and still continue in the mDNS case.
You can auto-format the code by running the Maven target |
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). |
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.
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?
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?
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?
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. |
Yes, that's what I meant.
I'm not sure how useful it is. I guess we could always add it later if it seems worthwhile.
It depends :-)
I'm not sure it's required, and I couldn't conclude from the RFC without looking at packets.
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 The relevant part from the RFC is this sentence (emphasis mine):
Setting the unicast flag in the first hand with masking 0x8000 into
Note I'm not sure how to best handle this flag (in questions and answers). Maybe @nresare could chime in here and has an idea? |
of TCP vs. UDP DNS queries and preventing accidental collision of UDP client port with mDNS server.
additional changes you requested submitted to mDNS_patch_ka2ddo branch. |
and fixing the formatting again.... |
first build failure.
and again making the formatter happy (since it only makes one complaint at a time). |
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.
Thanks for the update. I've left a bunch of comments inline.
The bigger question about how to handle the DClass in questions and responses however is still not clear:
- In questions, the highest bit set in the Class means Unicast
- In answers, the highest bit set in the Class means "Cache flush"
For both cases, this needs to be properly handled, i.e. not only in the SimpleResolver
but also in e.g. Message.toString()
, Record.getDClass()
and probably lots of other locations. Maybe a flag boolean isMulticast;
in the Message
and Record
classes is an option.
and again making the formatter happy (since it only makes one complaint at a time).
You can run the checks locally with mvn verify
or make the checks happy by applying the formats to your IDE with the links provided previously.
@@ -31,6 +31,9 @@ | |||
/** Matches any class */ | |||
public static final int ANY = 255; | |||
|
|||
/** Indicates on mDNS that querier will accept unicast replies from a multicast request. */ | |||
public static final int UNICAST_RESPONSE = 0x8000; |
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.
-> UNICAST_RESPONSE_FLAG
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.
OK, in next request.
@@ -169,6 +229,9 @@ private void silentCloseChannel() { | |||
|
|||
addr = new InetSocketAddress(local.getAddress(), port); | |||
} | |||
if (addr.getPort() == SimpleResolver.RESERVED_MDNS_PORT) { |
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.
addr
can be null if the config option dnsjava.udp.ephemeral.use_ephemeral_port
is set. Also, port 5353 is a valid source-port in normal DNS
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 kind of disagree with the statement that "port 5353 is valid in normal DNS". Since mDNS is becoming more prevalent, it would be recommended not to use that port accidentally to avoid receiving requests from other hosts when you're expecting replies. And it doesn't hurt to avoid it, since this part of the code no longer differentiates between receiving DNS and mDNS replies.
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.
In regular DNS the socket is bound, so the only replies accepted are from a DNS server. The case shouldn't occur when using defaults though.
But addr==null
needs to be handled anyway.
@@ -28,6 +29,9 @@ | |||
/** The default port to send queries to */ | |||
public static final int DEFAULT_PORT = 53; | |||
|
|||
/** The port we can't use as a client because it's reserved for mDNS servers. */ | |||
public static final int RESERVED_MDNS_PORT = 5353; |
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.
-> MDNS_PORT
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.
OK
+ Type.string(response.getQuestion().getType()))); | ||
return f; | ||
VerifyOneResponse verifyOneResponse = new VerifyOneResponse(query, qid); | ||
@SuppressWarnings("unchecked") |
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 is no longer necessary
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.
OK
if (tcp) { | ||
result = NioTcpClient.sendrecv(localAddress, address, query, out, timeoutValue); | ||
} else { | ||
result = NioUdpClient.sendrecv(localAddress, address, out, udpSize, timeoutValue); | ||
} | ||
|
||
return result.thenComposeAsync( | ||
in -> { | ||
v -> { |
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.
name this something useful, e.g. answers
(v
is typically used for Void
, which is not the case here)
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.
OK, in next request.
source = channel.receive(buffer); | ||
read = buffer.position(); | ||
if (read <= 0 || source == null) { | ||
return; // ignore this datagram |
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 is a lot of code duplication. The return
here vs. EOFException
could be:
if (remoteSocketAddress.getAddress().isMulticast()) {
return;
}
throw new EOFException();
byte[] data = new byte[read]; | ||
System.arraycopy(buffer.array(), 0, data, 0, read); | ||
verboseLog("UDP read", channel.socket().getLocalSocketAddress(), source, data); | ||
answers.add(data); |
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 is the only other line in this method that differs for mDNS. Instead of duplicating everything, move the processing of the data byte-array into a separate method, e.g.
// in Transaction:
...
System.arraycopy(buffer.array(), 0, data, 0, read);
verboseLog("UDP read", channel.socket().getLocalSocketAddress(), source, data);
processAnswer(data);
}
processAnswer(byte[] data) {
silentCloseChannel();
f.complete(Collections.singletonList(data));
pendingTransactions.remove(this);
}
// in MultiAnswerTransaction:
processAnswer(byte[] data) {
answers.add(data);
}
private ArrayList<byte[]> answers = new ArrayList<>(); | ||
|
||
@Override | ||
void closeTransaction() { |
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.
closeTransaction is only triggered when the timeout has expired. Is this really the only condition on which you want to return answers? What about the case where one is only interested in the first answer, as fast as possible?
The timeout processing currently only runs every second. Is that still sufficient?
@@ -444,4 +426,90 @@ private Message sendAXFR(Message query) throws IOException { | |||
public String toString() { | |||
return "SimpleResolver [" + address + "]"; | |||
} | |||
|
|||
private class VerifyOneResponse { |
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.
Actually, this doesn't need to be a class. As I mentioned before, throwing the exceptions is fine and whole thing can be a method: Message verifyResponse(Message query, byte[] in) throws WireParseException { ... }
return null; | ||
} | ||
|
||
if ((query.getQuestion().getDClass() & ~DClass.UNICAST_RESPONSE) |
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 masking must only be applied for mDNS, not regular DNS
- In the mDNS case, the mask should be 0x7FFF so that 32768 to 65535 is ignored (see the note at IANA or in the RFC)
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 GitHub<#148 (comment)>, or unsubscribe<https://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. |
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).