Opened 8 years ago
Closed 7 years ago
#6645 defect closed fixed (fixed)
twisted.names.server.DNSServerFactory sends the clients OPT records back in NXDOMAIN responses
Reported by: | Richard Wall | Owned by: | Richard Wall |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | names | Keywords: | |
Cc: | Branch: |
branches/clean-response-message-6645-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | rwall |
Description
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
Attachments (1)
Change History (10)
comment:1 Changed 8 years ago by
Owner: | set to Richard Wall |
---|---|
Status: | new → assigned |
comment:2 Changed 8 years ago by
Author: | → rwall |
---|---|
Branch: | → branches/clean-response-message-6645 |
(In [39686]) Branching to 'clean-response-message-6645'
comment:4 Changed 7 years ago by
Branch: | branches/clean-response-message-6645 → branches/clean-response-message-6645-2 |
---|
(In [41558]) Branching to 'clean-response-message-6645-2'
comment:5 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Status: | assigned → new |
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:
Changed 7 years ago by
Attachment: | use-edns-6839-2.patch added |
---|
A patch showing what I think will be the simplest way to introduce _EDNSMessage. For discussion.
comment:6 Changed 7 years ago by
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.
comment:7 follow-up: 8 Changed 7 years ago by
Keywords: | review removed |
---|
Thanks!
twisted/names/test/test_dns.py
- * Can you split
test_responseFromMessageResponseType
into two tests? - Also maybe the assertions will fit on one line
- * 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 ;) - * 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. MessageContstructorTests
- nice :)
- * Can you split
twisted/names/test/test_server.py
- 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 bothMessage
and_EDNSMessage
. - 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. - 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.
- Can we avoid the need for monkey-patching to test this new API? Maybe the message factory should be an optional initializer argument to
twisted/names/dns.py
- * The documentation for
_responseFromMessage
'scls
parameter saysL{Message}
but the convention for the documentation is that this meanscls
must be an instance ofMessage
which isn't what's meant here. Unfortunately I think the type here istypes.ClassType
which isn't a very helpful thing to write in the documentation (I think). Maybecls
doesn't need to be documented at all here: it's parallel toself
in an instance method and we never document that. - * Do both
Message
and_EDNSMessage
need a_responseFromMessage
implementation? It looks like they're identical (apart from the value ofcls
) to me. CanDNSServerFactory
just usedns._responseFromMessage
directly? Am I overlooking some future plan where putting this functionality onto_messageFactory
is necessary?
- * The documentation for
twisted/names/server.py
- * The new
_messageFactory
attribute should be documented. - * Please file a ticket for fixing
timeReceived
- Nice to see how this cleans up
gotResolverResponse
andmessageReceived
.
- * The new
- Overall / Throughout
- 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. - * It might be nice to avoid single letter variable names (even in tests; eg response instead of r).
- * 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?
- There doesn't seem to be much consistency in how the code uses a
- 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). - 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
- 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!
comment:8 Changed 7 years ago by
Owner: | set to Richard Wall |
---|---|
Status: | new → assigned |
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!
twisted/names/test/test_dns.py
- * 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
- Also maybe the assertions will fit on one line
- * 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
- * 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
MessageContstructorTests
- nice :)
twisted/names/test/test_server.py
- 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 bothMessage
and_EDNSMessage
.- 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.
- 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.
twisted/names/dns.py
- * The documentation for
_responseFromMessage
'scls
parameter saysL{Message}
but the convention for the documentation is that this meanscls
must be an instance ofMessage
which isn't what's meant here. Unfortunately I think the type here istypes.ClassType
which isn't a very helpful thing to write in the documentation (I think). Maybecls
doesn't need to be documented at all here: it's parallel toself
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.
- * Do both
Message
and_EDNSMessage
need a_responseFromMessage
implementation? It looks like they're identical (apart from the value ofcls
) to me. CanDNSServerFactory
just usedns._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.
twisted/names/server.py
- * The new
_messageFactory
attribute should be documented.
- * Please file a ticket for fixing
timeReceived
#6957 for deprecating the verbose logging entirely.
- Nice to see how this cleans up
gotResolverResponse
andmessageReceived
.
- Overall / Throughout
- 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.
- * 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.
- * 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?
- 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).
- 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
- 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.
comment:9 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(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.
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.