Support the OPT pseudo-record

[exarkun]

RFC 2671 defines a new record type, OPT (value 41). This record doesn't carry information about a domain name. Instead, it carries extended information about the format of the request or response message.

We should be able to decode these into a structured object and serialize them back into a message.

See EDNS0 for some notes on the big picture.

[exarkun]

(In [34367]) Branching to 'opt-record-5668'

[exarkun]

(In [34368]) Apply just the parts of the #5454 patch which add the OPT record type

refs #5668

[exarkun]

Next thing to do, I think, is figure out why OPTHeader exists, and why RRHeader is changed to subclass it.

[BobNovas]

Jean-Paul - In the 5454 patch, OPTHeader exists because Record_OPT uses or overloads some of the fields of what would be RRHeader. So OPTHeader is just the header fields of a Record_OPT and so OPTHeader is a subset of RRHeader in terms of fields (and OPTHeader is also a subclass of RRHeader).

OPTHeader._headerFactory decodes just enough of a stream to decide if it should create an OPTHeader or an RRHeader and then either creates an OPTHeader or rewinds the stream and creates an RRHeader and returns one or the other. So, Message.parseRecords calls OPTHeader._headerFactory and gets the right Header (OPTHeader or RRHeader, which is a subclass of OPT), and then decodes the rest of the record from the type of the record, which is in OPTHeader, hence also in RRHeader.

Bad idea? Sorry.

[exarkun]

Thanks for the explanation, it helps a lot. On a closer reading of the EDNS RFC, now I wonder if it wouldn't be better to just have OPTHeader and forget about Record_OPT. Not sure yet, just the direction my thoughts are going right now.

[rwall]

(In [38420]) Branching to 'opt-record-5668-2'

[rwall]

Ready for review in log:branches/opt-record-5668-2

• This isn't finished but I'd like some feedback on what I've done and how much further to go in this ticket.
• I've been concentrating on OPTHeader
• So far I've implemented all the fixed fields of the OPT RR described here:  http://tools.ietf.org/html/rfc6891#section-6.1.2
• I need to decide whether to bother supporting the EDNS options tuples
• There don't seem to be (m)any real world uses for it yet - only proposals ( http://www.iana.org/assignments/dns-parameters/dns-parameters.xml#dns-parameters-11)
• I've left BobNovas original Record_OPT class in place for now, but it could become the container and parser for the Option tuples if we choose to support them.
• I've made some initial modifications to dns.Message to allow decoding OPT RRs but this part could be moved to a separate ticket where we introduce a client API for issuing EDNS queries.

Build Results:

•  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/opt-record-5668-2

-RichardW.

[tom.prince]

So, a question I have: are the data structures meant to encode messages syntactically or semantically? I think that the answer to this whether we have something like OPTHeader or Record_OPT. If we view things syntactically, then OPT is a record that can appear in messages (theoretically anywhere, but only in the additional data in a conforming implementation). If we view things semantically, then OPT is just additional data of the message, that just happens to look like a record. If I understand EDNS? correctly, it seems this ticket is about the former representation.

1. If this is about implementing a syntactic view, then does it make sense to disallow OPT in other sections?
2. After further perusal of the RFCs, I realize that OPT interprets the RR header differently. I wonder if it would make sense to
   1. Probe for the type before decoding.
   2. or extract the data for OPTHeader from the existing instance of RRHeader.

Some (incomplete) notes about the code itself:

1. OPTHeader:
   1. Is there any reason for OPTHeader.fmt to be public? I realize that this is public on a bunch of thins in t.n.dns, but it seems like an implementation detail.
   2. rdlen? Then name is unclear, and I'm not sure it is useful, in any case.
   3. Is there a reason this doesn't use FancyStrMixin?
   4. self.name.decode is modifying a class attribute. :(
      • test_optHeaderName and test_optHeaderType say that the attributes are readonly, which isn't true. This is misleading.
      • Malformed packets might contain different values for name.
2. Record_OPT
   1. Don't use assert (see #6560).
   2. encode doesn't use self.fmt for some reason.

This isn't a complete review, but the branch seems in progress, and I think there are some questions about direction and intent that need to be answered before a more thorough review.

As an aside, it seems like it would have made more sense if .decode was a classmethod.

[rwall]

(In [38749]) Branching to 'opt-record-5668-3'

[rwall]

Ready for another review in log:branches/opt-record-5668-3

1. The individual changes since the last review can be seen in log:branches/opt-record-5668-2
2. Build Results:
   1.  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/opt-record-5668-3
   2. I don't understand why there are so many new pydoctor errors, but they look unrelated to my branch.
3. I removed the Record_OPT and associated tests
4. Reverted all changes to dns.Message - a new EDNSMessage class will be introduced in #5675.
5. Added code to parse OPT variable options and a class to represent variable options.
6. More comments and responses below...

Replying to tom.prince:

So, a question I have: are the data structures meant to encode messages syntactically or semantically? I think that the answer to this whether we have something like OPTHeader or Record_OPT. If we view things syntactically, then OPT is a record that can appear in messages (theoretically anywhere, but only in the additional data in a conforming implementation). If we view things semantically, then OPT is just additional data of the message, that just happens to look like a record. If I understand EDNS? correctly, it seems this ticket is about the former representation. 1. If this is about implementing a syntactic view, then does it make sense to disallow OPT in other sections? 2. After further perusal of the RFCs, I realize that OPT interprets the RR header differently. I wonder if it would make sense to 1. Probe for the type before decoding. 2. or extract the data for OPTHeader from the existing instance of RRHeader.

OPTHeader is meant as a low level 'syntactic' structure (if I understand you correctly).

My plan is to add new EDNSMessage class (in #5675) whose decode method will first parse bytes with RRHeader. If type 41 (OPT) is found in the additional section, it will rewind the strio and parse the bytes again with OPTHeader (as in r38438).

Then the OPTHeader attributes will be used to calculate certain EDNSMessage attributes eg EDNSMessage.rcode = EDNSMessage._rcode & OPTHeader.rcode.

Setting EDNSMessage.rcode > 16 will cause EDNSMessage.encode to append a suitable OPTHeader with the extended part of the rcode.

If type 41 is found in the answer, auth, or query section, a DNS error will be returned (somehow)

 https://tools.ietf.org/html/rfc6891#section-6.1.3

Some (incomplete) notes about the code itself: 1. OPTHeader: 1. Is there any reason for OPTHeader.fmt to be public? I realize that this is public on a bunch of thins in t.n.dns, but it seems like an implementation detail.

Only for consistency with the RRHeader.fmt but I guess that's not necessary. Now private.

2. rdlen? Then name is unclear, and I'm not sure it is useful, in any case.

I just copied the RFC terminology.

In RRHeader it's called rdlength (because thats what its called in rcf1035.)

In RRHeader it's public so that dns.Message can read the correct number of bytes for constructing the RRHeader.payload. (rdata)

In OPTHeader I guess it won't be necessary because I've made it responsible for decoding its own rdata payload (the variable options).

3. Is there a reason this doesn't use FancyStrMixin?

Done.

4. self.name.decode is modifying a class attribute. :(

Fixed.

- test_optHeaderName and test_optHeaderType say that the attributes are readonly, which isn't true. This is misleading.

Fixed.

- Malformed packets might contain different values for name.

There was a short discussion on IRC and I decided to discard the name for OPT records (ignoring possible non-root names).

20:24 < rwall> tomprince: Hi. In https://twistedmatrix.com/trac/ticket/5668 you said "Malformed packets might contain different values for name"
20:24 < rwall> I don't think I should raise an exception in that case, should I log a warning? What do you think?
20:26 < exarkun> no one wants arbitrary network peers to be able to fill their log file with junk
20:28 < rwall> So just ignore it you think?
20:29 < rwall> Other names objects log decoding errors
20:29 -!- Irssi: Pasting 5 lines to #twisted-dev. Press Ctrl-K if you wish to do this or Ctrl-C to cancel.
20:29 < exarkun> is the comment about `Name().decode(strio)`?
20:29 < rwall> eg             log.msg(
20:29 < rwall>                 "Decoded %d bytes in %s record, but rdlength is %d" % (
20:29 < rwall>                     soFar, self.fancybasename, length
20:29 < rwall>                 )
20:29 < rwall>             )
20:29 < rwall> yes
20:29 < exarkun> yes
20:29 < exarkun> Twisted Names is 10 years old though
20:29 < exarkun> So it may contain code that no longer represents what we think is a good idea
20:29 < rwall> understood.
20:30 < exarkun> It seems like ignoring it is probably fine.  If a peer sends something other than "" there, they're broken and who knows what
                 they're expecting to happen.
20:31 < rwall> we could return SRVERROR
20:31 < exarkun> We could
20:31 < rwall> So that they know they're broken.
20:31 < exarkun> But in this case it's not very hard to ignore the fact that they're broken
20:31 < rwall> ok
20:31 < tomprince> If the edns version isn't 0, we should probably error, though.
20:31 < exarkun> Returning an error seems nice to me, but inevitably someone will complain that it breaks interop with their favorite dns tool
20:32 < rwall> tomprince: Yes I can do that.
20:33 < rwall> Return SRVERROR I mean....but let me reread what the rfc says about version.
20:34 < exarkun> The RFC does have something to say about getting an EDNS version you don't understand
20:34 < exarkun> (but I forget what)
20:37 < rwall> "If a responder does not implement the VERSION level of the request, then it MUST respond with RCODE=BADVERS.I think I can take care
               of the
20:37 < rwall> oops
20:38 < rwall> Anyway I think I can defer that to #5675 or a later ticket.

2. Record_OPT 1. Don't use assert (see #6560). 2. encode doesn't use self.fmt for some reason.

I've removed all that code now, it was a legacy of BobNovas original patch. (#5454)

This isn't a complete review, but the branch seems in progress, and I think there are some questions about direction and intent that need to be answered before a more thorough review.

Understood.

As an aside, it seems like it would have made more sense if .decode was a classmethod.

I've converted decode to a classmethod.

Thanks for your review and I look forward to more feedback now that I've tidied up the branch.

-RichardW.

[tom.prince]

As an aside, it seems like it would have made more sense if .decode was a classmethod.

I've converted decode to a classmethod

Sorry I was unclear. I haven't looked at the code, but I'm not sure making .decode a class method is compatible with implementing IEncodable. While I think making .decode a class method across the board would be an improvement, I don't see how to do that compatibly, and certainly don't expect you to do it in this ticket.

[tom.prince]

1. So, looking close, I don't see any need for OPTHeader and OPTVariableOptions to implement IEncodable, other than consistency. But I do think consistency is serious consideration. Perhaps get glyph or exarun or someone to weigh in here.
2. OPTVariableOption: I'm not entirely happy with this name, but have no suggetions.
3. I notice you only test options of lengh '2', and that all the data is very similiar. It would be better if the code and length and data could easily be distinguished in the encoded form.

Otherwise this looks good. Please merge after addressing 3 and either properly implementing IEncodable or after getting other input, stop implementing IEncodable.

[rwall]

Ready for review in log:branches/opt-record-5668-3

Thanks for the review.

1. Build Results
   1.  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/opt-record-5668-3
2. I fixed some Python3 build failures (bytes problems)
3. Removed some unnecessary class attributes.
4. More comments and responses below...

Replying to tom.prince:

1. So, looking close, I don't see any need for OPTHeader and OPTVariableOptions to implement IEncodable, other than consistency. But I do think consistency is serious consideration. Perhaps get glyph or exarun or someone to weigh in here.

Having studied the t.n.dns in more detail it seems to me that IEncodable is mainly there to support DNS name compression, and it's possible that there may be a need to have names in OPTHeader options in the future, so it's probably a good idea to support the interface.

The main problem is IEncodable.decode which would be much better if it *returned* an IEncodable and could then be provided by a classmethod.

I haven't heard from exarkun yet, but in #5675 he did suggest using classmethods for a new EDNSMessage class.

It's easier there, because dns.Message doesn't implement IEncodable. (I guess because it initialises the compDict which is passed down to various IEncoder.encode implementors.)

So anyway, I've changed _OPTHeader and _OPTVariableOption to properly implement IEncodable for now, but I've also made them private so that I can change them freely in a later branch.

_OPTHeader will be an implementation detail of EDNSMessage so it may remain private.

2. OPTVariableOption: I'm not entirely happy with this name, but have no suggetions.

I'm not happy with it either.

How about _OPTVariable?

3. I notice you only test options of lengh '2', and that all the data is very similiar. It would be better if the code and length and data could easily be distinguished in the encoded form.

I added some more varied / longer bytes to those tests.

Otherwise this looks good. Please merge after addressing 3 and either properly implementing IEncodable or after getting other input, stop implementing IEncodable.

Please take another look.

I've started writing some code for #5675 EDNSMessage and to be honest I'm having doubts about the _OPTHeader class.

In particular I wonder if it might be better for it to be a wrapper / adaptor around an RRHeader.

Instead of _OPTHeader.decode, it could read the attributes from an RRHeader (eg RRHeader.ttl) and convert them to corresponding attributes in EDNSMessage.

Similarly, during encode it could generate an RRHeader suitable for adding to the additional section.

That way there would only be one encode and decode implementation.

I'll try and attach some prototype code as a patch.

-RichardW.

[rwall]

Replying to rwall: <snip>

I've started writing some code for #5675 EDNSMessage and to be honest I'm having doubts about the _OPTHeader class. In particular I wonder if it might be better for it to be a wrapper / adaptor around an RRHeader. Instead of _OPTHeader.decode, it could read the attributes from an RRHeader (eg RRHeader.ttl) and convert them to corresponding attributes in EDNSMessage.

So that's what I've done, and I think it's more elegant.

• See log:branches/opt-record-5668-3

I've added a fromRRHeader method which extracts all the standard and EDNS values and options from the RRHeader and it's UnknownRecord.

I've also been working on #5675 to illustrate how I intend _OPTHeader to be used.

I've done as much as I can in a branch

• log:branches/edns-message-5675-2

And added the parts which rely on _OPTHeader to attachment:ticket/5675/edns-message-5675.OPTHeader-integration.patch

That patch illustrates how the ednsVersion attribute is decoded via _OPTHeader via RRHeader.

And how an _OPTHeader can be added directly to the additional list of Message during encoding.

I think I'll stop now as its becoming difficult to work on EDNSMessage until this branch is merged.

Looking forward to some feedback.

-RichardW.