twisted.names.server.DNSServerFactory sends the clients OPT records back in NXDOMAIN responses

[rwall]

twisted.names.server.DNSServerFactory uses the original query message in its response.

When there is an answer, it resets the original answers, authority, and additional lists.

But when responding with NXDOMAIN it leaves the original query lists.

If the query contained special OPT records, then these get sent back to the client giving the impression that server supports the same EDNS version as the client....which it doesn't...yet (wiki:EDNS0)

Instead, it should remove the OPT records before replying.

Need to check RFCs to see if the other records should be left in place or not.

$ dig @ns1.twistedmatrix.com. foobar.twistedmatrix.com A  +norecurse

; <<>> DiG 9.9.3-rl.156.01-P1-RedHat-9.9.3-3.P1.fc19 <<>> @ns1.twistedmatrix.com. foobar.twistedmatrix.com A +norecurse
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NXDOMAIN, id: 2260
;; flags: qr; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;foobar.twistedmatrix.com.	IN	A

;; Query time: 153 msec
;; SERVER: 66.35.39.66#53(66.35.39.66)
;; WHEN: Sat Jul 27 14:11:11 BST 2013
;; MSG SIZE  rcvd: 53

[rwall]

We also need to reset the AD and CD bits to 0 in responses.

See ticket:6680#comment:4

It might be simplest to create a new Message instead of re-using the query message. Then copy only the appropriate bits from the query message.

[rwall]

(In [39686]) Branching to 'clean-response-message-6645'

[rwall]

Waiting for #6700 before continuing with this.

[rwall]

(In [41558]) Branching to 'clean-response-message-6645-2'

[rwall]

Ready for review in log:branches/clean-response-message-6645-2

• DNSServerFactory now has a mechanism for creating a new response message from a request message
• The response message uses default values for its flags and fields except...
• queries, id which are copied from the request and
• answer which is always set to True

This means that responses are no longer blindly mirroring the fields and flags of the request and things like OPT records will no longer be sent back to the client.

This branch should make it easier to substitute dns._EDNSMessage (#5675) for dns.Message in dns.DNSProtocol and DNSDatagramProtocol; because we will have a way for the server to set the ednsVersion and maxSize independently of the values received from the client.

Build Results:

• ​https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/clean-response-message-6645-2

[rwall]

A patch showing what I think will be the simplest way to introduce _EDNSMessage. For discussion.

[rwall]

See attachment:use-edns-6839-2.patch​ where I simply replace (almost) all uses of dns.Message with dns._EDNSMessage.

• All tests still pass
• ednsVersion is forced to None, which turns off all OPT record manipulation during encoding
• which means that twistd dns servers should behave exactly as before

Having got rid of all internal references to dns.Message, I'll then be able to deprecate dns.Message in favour of a refactored dns.StandardMessage (or some such) with better constructor argument names, attribute names, better encode / decode interface and fewer public methods. (#6862)

This also has the advantage that I won't have worry about creating new EDNSProtocol and EDNSDatagramProtocol or the alternative of supplying alternative messageFactories to the existing protocols. (if that makes sense) (wiki:EDNS0#a6839dns.EDNSStreamProtocolanddns.EDNSDatagramProtcol)

So is this simple approach acceptable? The use of dns.Message in DNSProtocol and DNSServerFactory is mostly (all) implementation detail and not (ASFAICS) part of the public APIs. And as I said above, it should not affect the behaviour of twistd dns either.

[exarkun]

Thanks!

1. twisted/names/test/test_dns.py
   1. * Can you split test_responseFromMessageResponseType into two tests?
   2. Also maybe the assertions will fit on one line
   3. * The docstring for test_responseFromMessageResponseAttributes might be missing some words? Or perhaps it has some extras. whose queries are those of on the request (And for a truly picky nit, whose applies to people not objects ;)
   4. * It looks like there's some overlap in the test values used in test_responseFromMessageResponseAttributes - 1234 is both the request id and the response code. Using different values will make the test better by demonstrating the two don't accidentally get mixed up inside the API under test.
   5. MessageContstructorTests - nice :)
2. twisted/names/test/test_server.py
   1. Can we avoid the need for monkey-patching to test this new API? Maybe the message factory should be an optional initializer argument to DNSServerFactory? OTOH, see comment below regarding (not) having _responseFromMessage on both Message and _EDNSMessage.
   2. Also, can we avoid the double private? foo._messageFactory._responseFromMessage seems like an encapsulation violation to me (particularly since in this case there are two implementations that might be _messageFactory - sounds like there's an interface here). Oops, see that same comment below.
   3. I'm still not really a fan of the RaisedArguments style of testing. This seems like a weak form of mock-based testing (which I am also not a fan of). The tests that are using this pattern are all codifying an exact signature for a private interface making them fragile and expensive to maintain.
3. twisted/names/dns.py
   1. * The documentation for _responseFromMessage's cls parameter says L{Message} but the convention for the documentation is that this means cls must be an instance of Message which isn't what's meant here. Unfortunately I think the type here is types.ClassType which isn't a very helpful thing to write in the documentation (I think). Maybe cls doesn't need to be documented at all here: it's parallel to self in an instance method and we never document that.
   2. * Do both Message and _EDNSMessage need a _responseFromMessage implementation? It looks like they're identical (apart from the value of cls) to me. Can DNSServerFactory just use dns._responseFromMessage directly? Am I overlooking some future plan where putting this functionality onto _messageFactory is necessary?
4. twisted/names/server.py
   1. * The new _messageFactory attribute should be documented.
   2. * Please file a ticket for fixing timeReceived
   3. Nice to see how this cleans up gotResolverResponse and messageReceived.
5. Overall / Throughout
   1. There doesn't seem to be much consistency in how the code uses a ) on a line by itself or on the line before that. The coding standard doesn't calls for such consistency but it's nice to see it at least within a single patch.
   2. * It might be nice to avoid single letter variable names (even in tests; eg response instead of r).
   3. * Ostensibly this is a fix to prevent OPT records being sent back with NXDOMAIN responses. There isn't a unit test that verifies this behavior, is there? Ditto for the AD and CD flags... I think there should be. Did I miss something?
6. See attachment:use-edns-6839-2.patch​ where I simply replace (almost) all uses of dns.Message with dns._EDNSMessage - I only skimmed this but it looks pretty nice. I am a little worried that it touches a lot of things and we might be overlooking some of its consequences. But only a little: the more I think about it the more I realize that Message just isn't that big of a part of Twisted Names public API (it's certainly in there here and there, but so much of the API is in terms of tuples of rrsets).
7. Having got rid of all internal references to dns.Message, I'll then be able to deprecate dns.Message in favour of a refactored dns.StandardMessage - This sounds great
8. So is this simple approach acceptable? - It seems like it will work to me. :)

As an experiment I have marked mandatory items with an asterisk. Some of these are questions and addressing them may just mean providing an answer. The rest are optional (though obviously I think it would be nice to address them :).

Thanks again!

[rwall]

Thankyou exarkun,

My responses are below.

I'll run a build and merge tomorrow - unless you have any objections in the meantime.

​https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/clean-response-message-6645-2

Replying to exarkun:

Thanks!

1. twisted/names/test/test_dns.py
   1. * Can you split test_responseFromMessageResponseType into two tests?

Yep. But I've actally removed that test and move the request / response identity test to ResponseFromMessageTests at the bottom of test_dns

r41613

1. Also maybe the assertions will fit on one line

r41614

1. * The docstring for test_responseFromMessageResponseAttributes might be missing some words? Or perhaps it has some extras. whose queries are those of on the request (And for a truly picky nit, whose applies to people not objects ;)

I removed that test in r41613. The attributes are tested individually in ResponseFromMessageTests

1. * It looks like there's some overlap in the test values used in test_responseFromMessageResponseAttributes - 1234 is both the request id and the response code. Using different values will make the test better by demonstrating the two don't accidentally get mixed up inside the API under test.

I removed that test in r41613. The attributes are tested individually in ResponseFromMessageTests

1. MessageContstructorTests - nice :)

1. twisted/names/test/test_server.py
   1. Can we avoid the need for monkey-patching to test this new API? Maybe the message factory should be an optional initializer argument to DNSServerFactory? OTOH, see comment below regarding (not) having _responseFromMessage on both Message and _EDNSMessage.
   2. Also, can we avoid the double private? foo._messageFactory._responseFromMessage seems like an encapsulation violation to me (particularly since in this case there are two implementations that might be _messageFactory - sounds like there's an interface here). Oops, see that same comment below.

Well I removed the Message._responseFromMessage classmethod, and use the dns._responseFromMethod instead.

So I'm still monkey patching that, just to establish that that function is called with a particular set of arguments.

I could call dns._responseFromMessage via a DNSServerFactory class attribute, but it would make the code less readable IMHO.

1. I'm still not really a fan of the RaisedArguments style of testing. This seems like a weak form of mock-based testing (which I am also not a fan of). The tests that are using this pattern are all codifying an exact signature for a private interface making them fragile and expensive to maintain.

I use raised arguments to establish that dns._responseFromMessage is called, so that I don't have to duplicate all the _responseFromMessage tests for DNSServerFactory.gotResolverResponse and gotResolverError.

On the other hand, perhaps those tests should be on gotResolverResponse so that if someone comes along in the future and decides to optimise, they don't inadvertantly re-introduce the request / response attribute leakage.

But in that case, I probably wouldn't bother testing dns._responseFromMessage directly.

1. twisted/names/dns.py
   1. * The documentation for _responseFromMessage's cls parameter says L{Message} but the convention for the documentation is that this means cls must be an instance of Message which isn't what's meant here. Unfortunately I think the type here is types.ClassType which isn't a very helpful thing to write in the documentation (I think). Maybe cls doesn't need to be documented at all here: it's parallel to self in an instance method and we never document that.

I've ditched the classmethods (see below). But I've also changed the argument name of dns._responseFromMessage from cls to responseConstructor. Which I think is much clearer.

I've described the type as "like L{dns.Message.init}". This ties in with Tom's idea for introducing an interface for Message constructors. I'm not going to do that here, but it will make more sense when in the follow up work where I introduce better constructor argument names and attribute names.

r41615

1. * Do both Message and _EDNSMessage need a _responseFromMessage implementation? It looks like they're identical (apart from the value of cls) to me. Can DNSServerFactory just use dns._responseFromMessage directly? Am I overlooking some future plan where putting this functionality onto _messageFactory is necessary?

You're right, DNSServerFactory can use dns._responseFromMessage directly, and that's what I originally intended.. I just thought it was neater to have _responseFromMessage as an alternative constructor. And if we ever decide to make it a public function, that's probably where I'd put it.

I've removed the two classmethods here though, and it simplifies things.

r41613.

1. twisted/names/server.py
   1. * The new _messageFactory attribute should be documented.

r41616.

1. * Please file a ticket for fixing timeReceived

#6957 for deprecating the verbose logging entirely.

1. Nice to see how this cleans up gotResolverResponse and messageReceived.

1. Overall / Throughout

1. There doesn't seem to be much consistency in how the code uses a ) on a line by itself or on the line before that. The coding standard doesn't calls for such consistency but it's nice to see it at least within a single patch.

I've rewrapped a few of those, where the assertion will fit on one line. But I like the ) on it's own line in multiline assertions.

1. * It might be nice to avoid single letter variable names (even in tests; eg response instead of r).

I agree that some of those are better expanded, and some can be removed altogether. But I don't see any harm in using e = self.assertRaises. I also don't see any harm in f = DNSServerFactory.

r41619 and r41620.

1. * Ostensibly this is a fix to prevent OPT records being sent back with NXDOMAIN responses. There isn't a unit test that verifies this behavior, is there? Ditto for the AD and CD flags... I think there should be. Did I miss something?

r41621.

1. See attachment:use-edns-6839-2.patch​ where I simply replace (almost) all uses of dns.Message with dns._EDNSMessage - I only skimmed this but it looks pretty nice. I am a little worried that it touches a lot of things and we might be overlooking some of its consequences. But only a little: the more I think about it the more I realize that Message just isn't that big of a part of Twisted Names public API (it's certainly in there here and there, but so much of the API is in terms of tuples of rrsets).

1. Having got rid of all internal references to dns.Message, I'll then be able to deprecate dns.Message in favour of a refactored dns.StandardMessage - This sounds great

1. So is this simple approach acceptable? - It seems like it will work to me. :)

Thanks for those thoughts. I'm encouraged that I may be on the right track.

I'll apply that patch to a branch as soon as this is merged and we can take a closer look.

As an experiment I have marked mandatory items with an asterisk. Some of these are questions and addressing them may just mean providing an answer. The rest are optional (though obviously I think it would be nice to address them :).

Hopefully I addressed most of them.

-RichardW.

[rwall]

(In [41629]) Merge clean-response-message-6645-2

Author: rwall Reviewer: exarkun Fixes: #6645

twisted.names.server.DNSServerFactory now responds with messages whose flags and fields are reset to their default values instead of copying these from the request. This means that AD and CD flags, and EDNS OPT records in the request are no longer mirrored back to the client.