Opened 3 years ago

Last modified 9 months ago

#5454 enhancement new

Add EDNS0 and DNSSEC behavior

Reported by: BobNovas Owned by:
Priority: normal Milestone:
Component: names Keywords:
Cc: thijs Branch:
Author: Bob Novas Launchpad Bug:

Description (last modified by thijs)

This patch, applied to twisted 11.1.0 in addition to but AFTER the patch in #5453, will add EDNS0 and DNSSEC behavior. EDNS0 behavior includes the ability to specify EDNS0 version (currently only version 0 is defined), the ability to set the DNSSEC OK flag which requests a security aware resolver to respond with DNSSEC records, and the ability to specify a maximum UDP Packet length that the path between this stub resolver and the recursive resolver can handle. This value can be as large as 65535, though smaller values, such as 1492 for WAN or 4096 for LAN or 8192 for local (e.g., 127.0.0.1) are more relevant. DNSSEC behavior includes the ability to receive and decode all the DNSSEC record types, and the ability to decode the AD (Authentic Data) flag. This means that with this patch, twisted.names client resolver can function as a security-aware non-validating stub resolver. In conjunction with a validating recursive resolver such as provided locally (e.g., 127.0.0.1) by dnssec-trigger (http://nlnetlabs.nl/projects/dnssec-trigger/) or by any comcast resolver, this allows a python client to determine if a name is secure.

Attachments (5)

add edns0-dnssec-behavior-02.patch (11.3 KB) - added by BobNovas 3 years ago.
adds EDNS0 and DNSSEC behavior to twisted (requires 5453)
AddEDNS0andDNSSEC5454.patch (79.3 KB) - added by BobNovas 3 years ago.
patch for ticket 5454
addEdsn0AndDnssec5454V02.patch (95.3 KB) - added by BobNovas 3 years ago.
patch for ticket 5454
addEdsn0AndDnssec5454V03.patch (101.7 KB) - added by BobNovas 3 years ago.
replaces V02 file of similar name
addEdsn0AndDnssec5454V04.patch (105.1 KB) - added by BobNovas 3 years ago.
addresses tjijs' comments

Download all attachments as: .zip

Change History (26)

Changed 3 years ago by BobNovas

adds EDNS0 and DNSSEC behavior to twisted (requires 5453)

comment:1 Changed 3 years ago by exarkun

  • Keywords review added; REVIEW removed

comment:2 Changed 3 years ago by thijs

  • Cc thijs added
  • Description modified (diff)
  • Keywords review removed
  • Owner set to BobNovas

Thanks for the patch BobNovas. Unfortunately it seems something went wrong during the upload of the patch, possibly due to the filename (space between add and edns0). Could you upload a new version of the patch (with a better filename)?

Changed 3 years ago by BobNovas

patch for ticket 5454

comment:3 Changed 3 years ago by BobNovas

  • Keywords review added
  • Owner BobNovas deleted

Here's a new version of the patch. I reviewed the patch, combined 5453 into this patch and added some tests and in general cleaned it up. This patch should sufficient to add EDNS0 behavior and DNSSEC classes and records for DNSKEY, DS, NSEC, NSEC3, NSEC3PARAM and RRSIG DNS records and EDNS0 behavior.

comment:4 Changed 3 years ago by thijs

  • Owner set to BobNovas

Thanks for the patch. Could you also update the howto with the new features you introduced (in 12.1)? Also check out the coding standard document. This also needs a NEWS file.

comment:5 Changed 3 years ago by thijs

  • Keywords review removed

Changed 3 years ago by BobNovas

patch for ticket 5454

comment:6 Changed 3 years ago by BobNovas

  • Keywords review added
  • Owner BobNovas deleted

Added News, howto, example and tweaked for standards.

comment:7 Changed 3 years ago by habnabit

  • Keywords review removed
  • Owner set to BobNovas

This patch still does not conform to the coding standards. There's http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html as well as http://www.python.org/dev/peps/pep-0008/; please note that the twisted standard takes precedence over PEP 8. You can ask (on trac or on IRC) if you have questions.

An incomplete list of specific complaints from the standard (please read the docs linked above):

  1. This wasn't explained thoroughly, but NEWS is automatically generated. http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles explains about these files. Patches modifying NEWS directly won't be accepted.
  2. All trailing whitespace must be removed.
  3. All spaces immediately proceding ( and preceding ) must be removed. i.e. ( foo ) becomes (foo).
  4. Operators must have exactly one space immediately preceding and proceding them. i.e. foo |bar becomes foo | bar.
  5. Methods must be separated with two blank lines; top-level things (i.e. classes and functions) must be separated with three blank lines.
  6. #ivar dnssecConfig should be @ivar dnssecConfig:.

Some other issues with the code unrelated to the coding standards:

  1. What is the point of t.names.ser_num_arith.SNA having both le and __le__ methods for all the comparators?
  2. assert is not for validating user input or isinstance checks; it is for invariants. Most places you have assert, it's either not necessary or should be replaced with an if ...: raise TypeError(...) or raise ValueError.
  3. Every usage of staticmethod is incorrect.
    1. OPTHeader.factory (which is a bad name for the method anyway) should be a classmethod.
    2. SNA.max should be a function in the module.
    3. DateSNA.from{Int,SNA} should be classmethods.
  4. You still have some lines over 79 characters long. Strings can be wrapped using parentheses like so:
    foo = ("some long string which gets"
           "wrapped over multiple lines")
    

Thanks for your contribution, though!

Changed 3 years ago by BobNovas

replaces V02 file of similar name

comment:8 Changed 3 years ago by BobNovas

  • Keywords review added
  • Owner BobNovas deleted

addressed habnabit's comments.

comment:9 Changed 3 years ago by thijs

  • Keywords review removed
  • Owner set to BobNovas

Thanks for the update.

  • There should be space before a comment:
    # if dnssecOk is enabled,
    
  • all new public methods and classes should have an @since: 12.1 marker
  • twisted.names.common.DnssecConfig
    • use @ivar and @type instead of declaring it with indentation
    • references to code in docstrings should be wrapped in L{ednsEnabled}
  • RFCs should be linked in the docstring, like:
     - U{RFC 4033: DNS Security Introduction and Requirements<http://www.ietf.org/rfc/rfc4033.txt>}
    
  • Use """ instead of ''' in docstrings, also put them on 3 lines:
    """
    Define the less than operator.
    """
    
  • twisted/names/topfiles/5454.feature should be a file that only includes the description of the changes, not the version/date header etc. See ReviewProcess#Newsfiles for details.
  • One of the docstrings contains your emailaddress, but we try to avoid this because it gets outdated/spammed etc, so use @author: Bob Novas instead.

comment:10 Changed 3 years ago by itamar

  • Keywords review added; EDNS0 DNSSEC removed
  • Owner BobNovas deleted

While matching the coding standard is necessary, there's no point spending too much time on that if e.g. it's on a class that will need some major refactoring. So getting the design right first is more important.

Changed 3 years ago by BobNovas

addresses tjijs' comments

comment:11 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

This patch is too large. I'm going to try to learn a bit more about EDNS0 and DNSSEC and then find a way to split this into smaller pieces.

comment:12 Changed 3 years ago by exarkun

I created EDNS0 with some of the results of my exploration. I haven't gotten into DNSSEC yet, but I think I might have a sufficient understanding of EDNS to reason about this code. I also filed several new tickets with narrow, EDNS-related scopes. They're linked from the wiki page, and represent the units in which I think it makes sense to tackle this work. Next on my todo list is to look at the patch on this ticket and find the parts of it that might resolve those narrower tickets (unless anyone else wants to help out and gets to it before me).

comment:13 Changed 17 months ago by philmayers

See also #4610 which I no longer have time or inclination to work on

comment:14 Changed 16 months ago by exarkun

  • Owner exarkun deleted

rwall has been doing some work in this area. I'm going to leave things in his hands for the time being.

comment:15 Changed 16 months ago by rwall

BobNovas, philmayers,

If you have time please take a look at wiki:EDNS0 where I've expanded the EDNS0 + DNSSEC plan and linked to your previous work.

For ease of review I intend to create separate tickets for implementing each of the DNSSEC record container classes, the lookup functions and then separate tickets for implementing DNSKEY signature verification.

I've also been building on BobNovas original EDNS work and I'd be interested in your feedback on that.

I've love some feedback on that plan, your thoughts on Twisted DNSSEC client and server APIs and your thoughts on how to integrate EDNS + DNSSEC while maintaining backwards compatibility with the existing APIs.

-RichardW.

comment:16 follow-up: Changed 16 months ago by BobNovas

I'm not sure if this helps, but I'll point out that http://sourceforge.net/projects/vsresolver/ is an implementation of a DNSSEC validating stub resolver built on top of dnspython. If nothing else, there is a 440 line file, test-data.txt, that contains DNS zones and the expected result of a DNSSEC query to the zone, useful for testing a DNSSEC stub resolver. http://superawesum.novas.us/py/hello.py is a trivial web app that uses vsResolver and presents a simple GUI to play with. If you "enable detailed output" on the GUI, the output greatly resembles the output of http://dnssec-debugger.verisignlabs.com/ for the same zone.

I won't claim that vsResolver is the greatest code, but it does illustrate what's involved in validating a DNSSEC query. And the test data probably would be helpful for any implementation you decide upon.

comment:17 Changed 16 months ago by rwall

(In [39499]) import the BobNovas tests from attachment:ticket:5454:addEdsn0AndDnssec5454V04.patch. Refs: #5454

comment:18 Changed 16 months ago by rwall

(In [39500]) and the SNA implementation from attachment:ticket:5454:addEdsn0AndDnssec5454V04.patch. Refs: #5454

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

Replying to BobNovas:

I'm not sure if this helps, but I'll point out that http://sourceforge.net/projects/vsresolver/ is an implementation of a DNSSEC validating stub resolver built on top of dnspython.

Thanks Bob,

That'll be a useful reference.

I just updated your RFC1982 SNA code and submitted it for review.

I had one or two questions which I added as XXX: comments.

I'd appreciate your thoughts on those and maybe a code review if you have time:

Thanks again.

-RichardW.

comment:20 Changed 16 months ago by rwall

(In [39533]) Merge opt-record-5668-4: A private API for dealing with EDNS OPT records.

Author: rwall, BobNovas
Reviewers: tom.prince, exarkun
Fixes: #5668
Refs: #5454

A private API for dealing with EDNS OPT records and OPT variable
options.

comment:21 Changed 9 months ago by rwall

(In [41756]) Merge serial-number-arithmetic-6672

Author: BobNovas, rwall
Reviewers: exarkun, glyph
Fixes: #6672
Refs: #5454, #6665

A private Serial Number Arithmetic module for manipulating Serial Numbers
defined in RFC1982. This will be used for comparison of DNS zone serial numbers
and RRSIG inception dates.

Note: See TracTickets for help on using tickets.