Opened 14 months ago

Last modified 12 months ago

#6797 enhancement new

Implement dns.Record_CERT record class

Reported by: acspike Owned by: acspike
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/cert-record-6797-2
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

Implement the DNS CERT resource record following cues in #6664 by rwall.

http://tools.ietf.org/html/rfc4398

The goal of this effort is to provide a DNS server implementation that is compatible with the needs of the Direct Project.

http://directproject.org/

Attachments (3)

twisted_names_trunk_CERT.diff (7.0 KB) - added by acspike 14 months ago.
Patch implementing DNS CERT resource record
twisted_names_trunk_CERT_tests.diff (2.9 KB) - added by acspike 14 months ago.
version of patch with tests
twisted_names_trunk_CERT.2.diff (16.8 KB) - added by acspike 14 months ago.
Version of patch with tests. Sorry about the last file (which was a copy of the dnskey tests).

Download all attachments as: .zip

Change History (11)

Changed 14 months ago by acspike

Patch implementing DNS CERT resource record

Changed 14 months ago by acspike

version of patch with tests

Changed 14 months ago by acspike

Version of patch with tests. Sorry about the last file (which was a copy of the dnskey tests).

comment:1 Changed 14 months ago by itamar

  • Keywords review added

comment:2 Changed 14 months ago by rwall

  • Author set to rwall
  • Branch set to branches/cert-record-6797

(In [40424]) Branching to 'cert-record-6797'

comment:3 Changed 14 months ago by rwall

(In [40425]) apply twisted_names_trunk_CERT.2.diff from acspike. Refs #6797.

comment:4 Changed 14 months ago by rwall

  • Branch changed from branches/cert-record-6797 to branches/cert-record-6797-2

(In [40426]) Branching to 'cert-record-6797-2'

comment:5 Changed 14 months ago by rwall

(In [40427]) Really apply twisted_names_trunk_CERT.2.diff from acspike. Refs #6797.

comment:6 Changed 14 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to acspike

Thanks acspike!

The patch looks very good. Here are some notes and points.

Notes:

  • The coverage seems complete

Points:

  1. twistedchecker warnings (https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1439/steps/run-twistedchecker/logs/new%20twistedchecker%20errors)
    1. Most of the warnings are spurious, but there are one or two that need fixing
    2. "W9010:1607,0: Trailing whitespace found in the end of line"
    3. "W9012:1905,0: Expected 2 blank lines, found 3"
    4. "W9013:2676,0: Expected 3 blank lines, found 2"
  1. branches/cert-record-6797-2/twisted/names/dns.py
    1. _base64Format
      1. Missing fullstop on first docstring line.
    2. Record_CERT
      1. Typo "recommende"
    3. hash
      1. Typo "A has allowing"
  1. branches/cert-record-6797-2/twisted/names/test/test_names.py
    1. testCERT
      1. New test methods should use the "test_CERT" naming convention.
  1. branches/cert-record-6797-2/twisted/names/test/test_dns.py
    1. test_cert
      1. docstring should be wrapped at 80 columns
  1. I guess that some of these points will also apply to my DNSKEY branch (#6664) so I should probably go back and fix them there too.
  1. I haven't properly read RFC4398 so treat this as a cosmetic review.
  1. I don't yet understand how this fits in with DANE (http://tools.ietf.org/html/rfc6698) and TLSA -- have you got any good comparisons / explanations. I found this summary:
  1. https://bugzilla.mozilla.org/show_bug.cgi?id=589537#c16
  1. I'm interested to know how you intend to create the CERT records. Do you think Record_CERT is usable as is, or do we need to create some sort of factory method which accepts a PEM certificate file or an OpenSSL.crypto.X509 instance. eg
    1. https://twistedmatrix.com/trac/browser/branches/ssl-endpoint-description-docs-6744/doc/core/howto/listings/ssl/make_certificate.py?rev=39996
  1. I found this page from your project http://wiki.directproject.org/DNS+Configuration+Guide but couldn't find any live examples of CERT records. I wanted to see how dig formats CERT records and eventually found this:
    $ dig @8.8.8.8 danm.prime.gushi.org CERT +tcp +short
    PGP 0 0 mQGiBDnY2vERBAD3cOxqoAYHYzS+xttvuyN9wZS8CrgwLIlT8Ewo/CCF I11PEO+gJyNPvWPRQsyt1SE60reaIsie2bQTg3DYIg0PmH+ZOlNkpKes PULzdlw4Rx3dD/M3Lkrm977h4Y70ZKC+tbvoYKCCOIkUVevny1PVZ+mB 94rb0mMgawSTrct03QCg/w6aHNJFQV7O9ZQ1Fir85M3RS8cEAOo4/1AS Vudz3qKZQEhU2Z9O2ydXqpEanHfGirjWYi5RelVsQ9IfBSPFaPAWzQ24 nvQ18NU7TgdDQhP4meZXiVXcLBR5Mee2kByf2KAnBUF9aah5s8wZbSrC 6u8xEZLuiauvWmCUIWe0Ylc1/L37XeDjrBI2pT+k183X119d6Fr1BACG fZVGsot5rxBUEFPPSrBqYXG/0hRYv9Eq8a4rJAHK2IUWYfivZgL4DtrJ nHlha+H5EPQVYkIAN3nGjXoHmosY+J3Sk+GyR+dCBHEwCkoHMKph3igc zCEfxAWgqKeYd5mf+QQq2JKrkn2jceiIO7s3CrepeEFAjDSGuxhZjPJV m7QoRGFuaWVsIFAuIE1haG9uZXkgPGRhbm1AcHJpbWUuZ3VzaGkub3Jn PohiBBARAgAiAhkBBQJMGHtPBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIX gAAKCRD7vlowYkuySf9RAKD2qK8LjgLZHzMsVZgJPhtS1+JMfACgmJgQ x0bd6kMaMmipvzLzF6gMPaO0MkRhbmllbCBNYWhvbmV5IChTZWNvbmRh cnkgRW1haWwpIDxndXNoaUBndXNoaS5vcmc+iGIEExECACICGyMCHgEC F4AFAkwYe1sGCwkIBwMCBhUIAgkKCwQWAgMBAAoJEPu+WjBiS7JJcFgA nR2PHl/bXj8uinpYufdKCd28ANzPAJwIXJF8osm+EhuGNPq2Ugbk94jq SrkCDQQ52Nr0EAgA9kJXtwh/CBdyorrWqULzBej5UxE5T7bxbrlLOCDa AadWoxTpj0BV89AHxstDqZSt90xkhkn4DIO9ZekX1KHTUPj1WV/cdlJP PT2N286Z4VeSWc39uK50T8X8dryDxUcwYc58yWb/Ffm7/ZFexwGq01ue jaClcjrUGvC/RgBYK+X0iP1YTknbzSC0neSRBzZrM2w4DUUdD3yIsxx8 Wy2O9vPJI8BD8KVbGI2Ou1WMuF040zT9fBdXQ6MdGGzeMyEstSr/POGx KUAYEY18hKcKctaGxAMZyAcpesqVDNmWn6vQClCbAkbTCD1mpF1Bn5x8 vYlLIhkmuquiXsNV6TILOwACAgf/Xho8xdoA4elOyO9sf+m0nZRMcdi7 yBfdjmSnNEueSDkuC4M7Ox235tWji+KCbe8AYPeMZBeHHq8c+8vEfSfp NxjZdeCjo22GjAIda3cXQM4pGDB9adYUu/BjLcMZMuoxOXp/OwRhjJp2 wvOCZccDfjA+3YrvA9BpII4/6cTqd9g+YxHtNsAT1YxU6RSyY6RZ4i1G OgKIUQxHUrmcFj7qClVoaXlpGrDZ+aoMBsg0RG16cj7FNNgZMBOCYhrP iTDHTp/SjIeXcYrsLDDPYB4kGUt5kjQQSj1iOWV7HUrVRb2qY39hVBQ1 y1G00Tj79V4an9Lu2GDkRZ1nlbb8yiMVWohGBBgRAgAGBQI52Nr0AAoJ EPu+WjBiS7JJQVoAnje8/cZOdsv2qGgrheoWG9Hft5PfAKDEcbx7lyNT XNhV2P8euT8B4lG2mA==
    
    1. I note that dig presents the "Mnemonic" certificate type. We could do something similar in our repr if we defined and used twisted.python.constants for the various certificate types and algorithms. I might do the same in #6664 for DNSKEY.
  1. Missing news file
    1. https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

Anyway, that's all for now. Please address or answer the numbered points above
and submit another patch against the new branch.

I'm waiting for the DNSKEY branch #6664 to be reviewed and I expect that the
feedback I get there will also be applicable here.

Also note that twisted branches are mirrored on github at:

...which might be useful if you prefer git.

Thanks again.

-RichardW.

comment:7 Changed 14 months ago by acspike

Thanks for the thorough review. I will address the all of the needed code changes in the branch.

#7 I'm not familiar with DANE and so I cannot comment on the relationship.

#8 I wondered if generating the record from a PEM would be the best solution. (Or at the very least generating the keyTag.) I decided that I'm not familiar enough with the standards to avoid limiting the usefulness of the implementation by accident. (Sysadmins have to generate this configuration for bind in some way too.) So I figured the most compatible and flexible implementation would allow all of the properties to be directly specified. Here is a tool (implemented using dnspython) which generates the bind configuration: http://www.videntity.com/2013/08/how-to-serve-public-certificates-in-bind-for-the-direct-project/

#9 I'm just a concerned citizen, not a representative of the Direct Project. (This applies to my answer to #7 equally.) So I can't offer any explanation on why this particular record type was chosen, but it is being used in the wild. Here are a few examples:
dig -t CERT healthvault.direct.healthvault.com
dig -t CERT sharelinkexchange.com

#9.1 I wondered what about best practices for implementing those enumerations in twisted.

comment:8 Changed 12 months ago by acspike

Sorry for the wait. I have addressed the cosmetic issues in the following change set:
https://github.com/acspike/twisted/compare/cert-record-6797-2

Note: See TracTickets for help on using tickets.