Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SVCB and HTTPS support #9369

Merged
merged 10 commits into from Sep 25, 2020

Conversation

pieterlexis
Copy link
Contributor

@pieterlexis pieterlexis commented Jul 31, 2020

Short description

PR to add SVCB and HTTPS support.

ToDo:

  • Implement SvcParam class for internal representation of service parameters
  • Add SVB
    • qtype.cc/hh
    • dnsrecords.cc/hh + reportOtherTypes
  • Add HTTPS to QTypes and dnsrecords
    • qtype.cc/hh
    • dnsrecords.cc/hh + reportOtherTypes
  • Implement Reading/Writing SvcParams (xfrSvcParamKeyVals)
    • RecordTextReader
    • RecordTextWriter
    • DNSPacketWriter
    • PacketReader
  • Add pdnsutil tests for SVCB/HTTPS correctness
  • Implement auth additional processing
    • Add tests for auth additional processing
  • Implement rec additional processing
    • Add tests for rec additional processing

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

pdns/Makefile.am Outdated Show resolved Hide resolved
@pieterlexis pieterlexis force-pushed the draft-ietf-dnsop-svcb-https-01 branch from 5f01d68 to 8fac44c Compare July 31, 2020 10:00
@Habbie Habbie mentioned this pull request Aug 17, 2020
7 tasks
@pieterlexis pieterlexis force-pushed the draft-ietf-dnsop-svcb-https-01 branch 3 times, most recently from fb75317 to 3030e92 Compare September 23, 2020 15:10
@pieterlexis pieterlexis marked this pull request as ready for review September 23, 2020 15:11
@pieterlexis
Copy link
Contributor Author

After a discussion with @Habbie, some features will be moved to a next iteration so this initial code can land in auth 4.4.0-alpha1. These features will be:

  • Recursor additional processing
  • CNAME processing for additional records (requires a refactor of packethandler::doQuestion)

@Habbie Habbie added this to the auth-4.4.0-alpha1 milestone Sep 24, 2020
@Habbie
Copy link
Member

Habbie commented Sep 24, 2020

upgrading.rst notes that IPSECKEY is no longer unknown; it should note the same for SVCB and HTTPS.

@pieterlexis pieterlexis force-pushed the draft-ietf-dnsop-svcb-https-01 branch 5 times, most recently from 0755636 to 34c84c5 Compare September 24, 2020 12:26
@@ -212,6 +212,18 @@ attributetype ( 1.3.6.1.4.1.2428.20.1.61 NAME 'openPGPKeyRecord'
SUBSTR caseIgnoreIA5SubstringsMatch
SYNTAX 1.3.6.1.4.1.1466.115.121.1.26 )

attributetype ( 1.3.6.1.4.1.2428.20.1.64 NAME 'sVCBRecord'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, but newer records apparently stopped with the 'first char downcase' rule already, so lets not continue it? (cf EUI64Record)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Howard Chu: don't know why people do that, probably historical. Conformity in a schema is a good thing. Also, they are case-insensitive.

Please uppercase :)

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits but the code looks solid. ❤️ the tests :-)

pdns/dnsrecords.hh Outdated Show resolved Hide resolved
pdns/dnsrecords.hh Outdated Show resolved Hide resolved
pdns/packethandler.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Show resolved Hide resolved
pdns/svc-records.hh Outdated Show resolved Hide resolved
pdns/dnsparser.cc Outdated Show resolved Hide resolved
modules/remotebackend/Makefile.am Outdated Show resolved Hide resolved
pdns/Makefile.am Outdated Show resolved Hide resolved
Copy link
Collaborator

@zeha zeha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some initial nits.

Also: the draft suggests that svcParamKeys need to be in increasing numeric order. Is this something the code should do?

pdns/Makefile.am Show resolved Hide resolved
pdns/dnsrecords.cc Outdated Show resolved Hide resolved
pdns/packethandler.cc Outdated Show resolved Hide resolved
@zeha
Copy link
Collaborator

zeha commented Sep 24, 2020

Also: the draft suggests that svcParamKeys need to be in increasing numeric order. Is this something the code should do?

To answer my own question: within the RR this is already done.

@zeha
Copy link
Collaborator

zeha commented Sep 24, 2020

Might make sense to add an API test (didn't verify this works):

diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py
index e2069d8ef..2a4c62717 100644
--- a/regression-tests.api/test_Zones.py
+++ b/regression-tests.api/test_Zones.py
@@ -1462,6 +1462,25 @@ $ORIGIN %NAME%
                                headers={'content-type': 'application/json'})
         self.assert_success(r)  # user should be able to create DNAME at APEX as per RFC 6672 section 2.3

+    def test_rr_svcb(self):
+        name, payload, zone = self.create_zone()
+        rrset = {
+            'changetype': 'replace',
+            'name': 'svcb.' + name,
+            'type': 'SOA',
+            'ttl': 3600,
+            'records': [
+                {
+                    "content": 'mandatory=alpn alpn=h2,h3 ipv4hint=192.0.2.1,192.0.2.2 echconfig="dG90YWxseSBib2d1cyBlY2hjb25maWcgdmFsdWU="',
+                    "disabled": False
+                },
+            ]
+        }
+        payload = {'rrsets': [rrset]}
+        r = self.session.patch(self.url("/api/v1/servers/localhost/zones/" + name), data=json.dumps(payload),
+                               headers={'content-type': 'application/json'})
+        self.assert_success(r)
+
     def test_rrset_ns_dname_exclude(self):
         name, payload, zone = self.create_zone()
         rrset = {

pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
@pieterlexis pieterlexis force-pushed the draft-ietf-dnsop-svcb-https-01 branch 2 times, most recently from 039aeee to 7391637 Compare September 25, 2020 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants