Opened 16 months ago

Closed 16 months ago

Last modified 13 months ago

#6680 enhancement closed fixed (fixed)

Add Authentic Data (AD) and Checking Disabled (CD) flags to twisted.names.dns.Message

Reported by: rwall Owned by: rwall
Priority: normal Milestone: EDNS0
Component: names Keywords: edns
Cc: Branch: branches/ad-cd-6680
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

https://tools.ietf.org/html/rfc2535#section-6.1

Two previously unused bits are allocated out of the DNS
query/response format header. The AD (authentic data) bit indicates
in a response that all the data included in the answer and authority
portion of the response has been authenticated by the server
according to the policies of that server. The CD (checking disabled)
bit indicates in a query that Pending (non-authenticated) data is
acceptable to the resolver sending the query.

I'd originally planned to add these to the new EDNSMessage class in #5675, and not touch the original Message class.

However Message doesn't provide any way of altering the unused Z bits in byte4.

See also:

Change History (9)

comment:1 Changed 16 months ago by rwall

  • Author set to rwall
  • Branch set to branches/ad-cd-6680

(In [39625]) Branching to 'ad-cd-6680'

comment:2 Changed 16 months ago by rwall

  • Keywords review added

Ready for review in log:branches/ad-cd-6680

  1. Added authenticData and checkingDisabled flags to dns.Message.
    1. The flags occupy a previously unused part of byte4 and default to 0 so there should be no backwards compatibility issues.
    2. I chose to use L{int} instead of L{bool} for consistency with the existing arguments.
    3. encode and decode are not documented so that pydoctor will use the docstrings from dns.IEncodable instead.
  2. Build Results
    1. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/ad-cd-6680

comment:3 follow-up: Changed 16 months ago by exarkun

  • Keywords review removed
  • Owner set to rwall

Thanks! I'm quite happy with the majority of this branch as it stands. A few mostly trivial comments, though:

  1. twisted/names/dns.py
    1. The second sentence of the documentation for the id parameter to in the Message.__init__ docstring is missing a word, I think.
    2. In various places in that same docstring the pattern "..., and ..." appears. The "," is extraneous and should be removed.
    3. The documentation for trunc says TrunCation - what does C indicate?
    4. authenticData and checkingDisabled should use L{int} (instead of C{int}) like all the other new docs in this docstring. Also, links should use epytext U{} markup rather than RFC-style [...].
  2. twisted/names/test/test_dns.py
    1. Several new tests use assertIdentical. This is an assertion that x is y. I don't think it is ever safe to use is one integers. They are ... immutable ... so identity is irrelevant (ellipses indicate mild uncertainty ;). I believe that these assertions are unreliable on PyPy, for example - depending on how the JIT is feeling one the particular day the code runs. I expect the desire here is to assert that the attribute is equal to one *and* an instance of int? I think it is mostly safe to assume the latter will be true if the former is (that is, we don't need to assert that). If you did want to, then I guess assertEqual((1, int), (x, type(x))) might be a way to spell it (maybe not the best way, but I've been writing a lot of similar assertions recently and it seems to be okay). But again, I don't think these are the most important assertions in the world to make in the tests.
    2. If test_checkingDisabledEncode and test_checkingDisabledDecode shared their big blobby opaque string, rather than each having a copy of it, I think they'd be a better pair of tests. Same for test_authenticDataDecode and test_authenticDataEncode.
    3. I'm a bit unsure what to think about test_checkingDisabledPosition and test_authenticDataPosition.
  3. Is there any concern about using AD and CD with non-EDNS messages? I suppose they'll just be ignored if someone sets them, so it won't hurt anything? Or are AD/CD and EDNS orthogonal?

Thanks again!

comment:4 in reply to: ↑ 3 Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Thanks for the review exarkun,

IOU a review.

Replying to exarkun:

Thanks! I'm quite happy with the majority of this branch as it stands. A few mostly trivial comments, though:

  1. twisted/names/dns.py
    1. The second sentence of the documentation for the id parameter to in the Message.__init__ docstring is missing a word, I think.

Done. r39673

  1. In various places in that same docstring the pattern "..., and ..." appears. The "," is extraneous and should be removed.

Done. r39674

  1. The documentation for trunc says TrunCation - what does C indicate?

All this documentation is pasted straight from RFC1035 where the
truncation bit is abbreviated as TC. Now fixed. r39675

  1. authenticData and checkingDisabled should use L{int} (instead of C{int}) like all the other new docs in this docstring. Also, links should use epytext U{} markup rather than RFC-style [...].

Done. r39676

  1. twisted/names/test/test_dns.py
    1. Several new tests use assertIdentical. This is an assertion that x is y. I don't think it is ever safe to use is one integers. They are ... immutable ... so identity is irrelevant (ellipses indicate mild uncertainty ;). I believe that these assertions are unreliable on PyPy, for example - depending on how the JIT is feeling one the particular day the code runs. I expect the desire here is to assert that the attribute is equal to one *and* an instance of int? I think it is mostly safe to assume the latter will be true if the former is (that is, we don't need to assert that). If you did want to, then I guess assertEqual((1, int), (x, type(x))) might be a way to spell it (maybe not the best way, but I've been writing a lot of similar assertions recently and it seems to be okay). But again, I don't think these are the most important assertions in the world to make in the tests.

Done. r39677

You're right that I was trying to be clear about the value and type of
the attributes.

But also I was trying to highlight that this is not using or expecting
bool.

Does the pypy JIT issue also make it unsafe to assume that True == 1 /
False == 0?

I'm interested because in EDNSMessage I've made changed the constructor to accept bool and
added assertions to ensure that only bool or 1/0 is assigned to those attributes.
I also use the bool values directly in the encoding and decoding methods.

One more thought, maybe there should be an assertStrictEqual
method. Somewhat equivalent to the JavaScript === operator
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Expressions_and_Operators

  1. If test_checkingDisabledEncode and test_checkingDisabledDecode shared their big blobby opaque string, rather than each having a copy of it, I think they'd be a better pair of tests. Same for test_authenticDataDecode and test_authenticDataEncode.

I don't mind doing what I did for _OPTHeader and moving the message
bytes and corresponding instance to a separate MessageTestData class.

In that case I think it made sense because the tests were encoding
multiple different non-default header values.

In this case the bytes are all null apart from the particular flag
whose encoding is being tested.

So I think that doing that is just going to make the tests
unnecessarily difficult to read.

  1. I'm a bit unsure what to think about test_checkingDisabledPosition and test_authenticDataPosition.

Removed in r39678. I'd originally added the new constructor arguments
inbetween the existing arguments to correspond with their position in
the encoded message bytestring.

I was assuming that Message would always be constructed using keyword
arguments. But I suppose there's a chance that other people may by
supplying positional arguments to Message.

So I added those tests when I moved the arguments to the end of the
list.

Ideally I think classes like Message and EDNSMessage should only
accept keyword arguments.

  1. Is there any concern about using AD and CD with non-EDNS messages? I suppose they'll just be ignored if someone sets them, so it won't hurt anything? Or are AD/CD and EDNS orthogonal?

I do have some concerns.

t.n.server.DNSServerFactory currently takes the incoming query
message, modifies it in place and uses it to generate a
response. Which means that:

  1. Client sends query with CD=1 and/or AD=1
  2. Twisted server responds with the CD=1 / AD=1

...even though it knows nothing about either flag. (It's similar
to #6645)

This shouldn't really matter because neither flag has any meaning
unless the client also sets the EDNS dnssecOK flag.

And I did some tests with dig and wireshark and this actually seems to
be the behaviour of Google DNS too.

Some options in my order of preference:

  1. Open a quick followup ticket for DNSServerFactory to reset the CD and AD bits in responses. Or better still, to construct a new Message for the response and explicitly copy only the appropriate parts of the query to the response. And that can also fix #6645)
  1. Add a flag which forces Message to always set CD and AD to zero. Eg

Message.enableDNSSEC=False

Then in EDNSMessage I can set that to True to allow encoding and
decoding of those two flags.

  1. Refactor Message encoding and decoding into separate functions which I can re-use in EDNSMessage instead of using Message directly. Then Message can force the two new bits to always be zero.

Perhaps all of those options make some sense.

New build results:

-RichardW.

comment:5 follow-up: Changed 16 months ago by exarkun

  • Keywords review removed
  • Owner set to rwall

Does the pypy JIT issue also make it unsafe to assume that True == 1 / False == 0?

Nope, that's okay. By definition, True does equal 1 and False does equal 0. PyPy's JIT isn't allowed to change the results of those comparisons - any more than it's allowed to change the result of 1 + 1 to equal anything other than 2.

What's not really specified by Python is whether 1 is the same object as 1 - or, more interestingly, whether 1 is the same object as 2 - 1 - or, even more interestingly, whether 1 is the same object after it gets unboxed into a native CPU register (in order to make passing it to a function call more efficient at runtime) and then re-boxed when it comes back to Python land. I'm not actually up on the specific current behaviors implemented by the PyPy JIT, but unless python-dev makes an official decree that it is part of the Python language specification that there will ever only be a single object to represent the integer value 1, it's probably not safe to rely on that being the case.

I'm interested because in EDNSMessage I've made changed the constructor to accept bool and added assertions to ensure that only bool or 1/0 is assigned to those attributes. I also use the bool values directly in the encoding and decoding methods.

These seem like safe (and desirable) changes to me.

One more thought, maybe there should be an assertStrictEqual method.

Possibly so. It probably wouldn't hurt but I haven't seen many problems that make me think accidentally confusing True for 1 or vice versa is a big issue in practice.

I don't mind doing what I did for _OPTHeader and moving the message bytes and corresponding instance to a separate MessageTestData class.

I'm primarily interested in avoiding repetition. This means avoiding the possibility for slightly incorrect repetition which allows two parts of the implementation which should be synchronized to get out of sync.

In that case I think it made sense because the tests were encoding multiple different non-default header values.

I'm not sure I follow this. I think each test only encodes a single non-default header value and the test for the encoding and decoding behavior for each particular header uses an identical byte string literal. I'm most interested in having these identical byte string literals turned into a single byte string literal that is shared. Is that how you understood my original review comment? If not, maybe I did a better job of explaining this time. If so, maybe we have more to discuss. :)

Or better still, to construct a new Message for the response and explicitly copy only the appropriate parts of the query to the response. And that can also fix #6645)

This sounds pretty good to me.

Other fixes look good except there are still a couple assertIdentical(..., 1) calls. Please fix that and the repetition issue (or let's talk about it) and then merge. Thanks again for your great work in this area.

comment:6 in reply to: ↑ 5 Changed 16 months ago by rwall

  • Status changed from new to assigned

Replying to exarkun:

Does the pypy JIT issue also make it unsafe to assume that True == 1 / False == 0?

Nope, that's okay. By definition, True does equal 1 and
False does equal 0. PyPy's JIT isn't allowed to change the
results of those comparisons - any more than it's allowed to change
the result of 1 + 1 to equal anything other than 2.

What's not really specified by Python is whether 1 is the same
object as 1 - or, more interestingly, whether 1 is the same
object as 2 - 1 - or, even more interestingly, whether 1 is
the same object after it gets unboxed into a native CPU register (in
order to make passing it to a function call more efficient at
runtime) and then re-boxed when it comes back to Python land. I'm
not actually up on the specific current behaviors implemented by the
PyPy JIT, but unless python-dev makes an official decree that it is
part of the Python language specification that there will ever only
be a single object to represent the integer value 1, it's probably
not safe to rely on that being the case.

Ok understood.

In that case I think it made sense because the tests were encoding
multiple different non-default header values.

I'm not sure I follow this. I think each test only encodes a single
non-default header value and the test for the encoding and decoding
behavior for each particular header uses an identical byte string
literal. I'm most interested in having these identical byte string
literals turned into a single byte string literal that is shared.
Is that how you understood my original review comment? If not,
maybe I did a better job of explaining this time. If so, maybe we
have more to discuss. :)

Yeah I was over complicating. I've moved the bytes to global
variables.

Or better still, to construct a new Message for the response and
explicitly copy only the appropriate parts of the query to the
response. And that can also fix #6645)

This sounds pretty good to me.

Ok. I'll do that next.

Other fixes look good except there are still a couple
assertIdentical(..., 1) calls. Please fix that and the repetition

Done.

issue (or let's talk about it) and then merge. Thanks again for
your great work in this area.

Thanks again for the review. I'll wait for some clean build results
and merge.

comment:7 Changed 16 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [39685]) Merge ad-cd-6680

Author: rwall
Reviewer: exarkun
Fixes: #6680

twisted.names.dns.Message now allows encoding and decoding of the
Authentic Data (AD) and Checking Disabled (CD) flags described in
RFC2535.

comment:8 Changed 16 months ago by rwall

(In [39733]) Merge forward to pick up #6680. Refs: #6680, #5675

comment:9 Changed 13 months ago by rwall

  • Milestone set to EDNS0
Note: See TracTickets for help on using tickets.