Opened 6 years ago
Last modified 6 years ago
#6797 enhancement new
Implement dns.Record_CERT record class
Reported by: | Aaron Spike | Owned by: | Aaron Spike |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | names | Keywords: | |
Cc: | Branch: |
branches/cert-record-6797-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | rwall |
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.
Attachments (3)
Change History (11)
Changed 6 years ago by
Attachment: | twisted_names_trunk_CERT.diff added |
---|
Changed 6 years ago by
Attachment: | twisted_names_trunk_CERT_tests.diff added |
---|
version of patch with tests
Changed 6 years ago by
Attachment: | twisted_names_trunk_CERT.2.diff added |
---|
Version of patch with tests. Sorry about the last file (which was a copy of the dnskey tests).
comment:1 Changed 6 years ago by
Keywords: | review added |
---|
comment:2 Changed 6 years ago by
Author: | → rwall |
---|---|
Branch: | → branches/cert-record-6797 |
(In [40424]) Branching to 'cert-record-6797'
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
Branch: | branches/cert-record-6797 → branches/cert-record-6797-2 |
---|
(In [40426]) Branching to 'cert-record-6797-2'
comment:5 Changed 6 years ago by
comment:6 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Aaron Spike |
Thanks acspike!
The patch looks very good. Here are some notes and points.
Notes:
- I applied your patch to a new branch started some builds.
- log:branches/cert-record-6797-2 (NB: I messed up the first branch, so we're using "-2" in the branch name)
- https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/cert-record-6797-2
- The coverage seems complete
Points:
- twistedchecker warnings (https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1439/steps/run-twistedchecker/logs/new%20twistedchecker%20errors)
- Most of the warnings are spurious, but there are one or two that need fixing
- "W9010:1607,0: Trailing whitespace found in the end of line"
- "W9012:1905,0: Expected 2 blank lines, found 3"
- "W9013:2676,0: Expected 3 blank lines, found 2"
- branches/cert-record-6797-2/twisted/names/dns.py
- _base64Format
- Missing fullstop on first docstring line.
- Record_CERT
- Typo "recommende"
- hash
- Typo "A has allowing"
- _base64Format
- branches/cert-record-6797-2/twisted/names/test/test_names.py
- testCERT
- New test methods should use the "test_CERT" naming convention.
- testCERT
- branches/cert-record-6797-2/twisted/names/test/test_dns.py
- test_cert
- docstring should be wrapped at 80 columns
- test_cert
- 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.
- I haven't properly read RFC4398 so treat this as a cosmetic review.
- 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:
- 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
- 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==
- 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.
- Missing news file
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 6 years ago by
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 6 years ago by
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
Patch implementing DNS CERT resource record