Rectify zones via the API #5779

pieterlexis · 2017-10-06T16:22:55Z

Short description

This PR adds zone rectification to the API. Work done:

Move the pdnsutil rectification code to the DNSSECKeeper
Generate DNSSEC keys for a zone when "dnssec" is true in an API POST/PATCH for zones
Rectify DNSSEC zones after POST/PATCH when API-RECTIFY metadata is 1
Allow setting this metadata via the "api-rectify" param in a Zone object
Show "nsec3param" and "nsec3narrow" in Zone API responses
Add an "rrsets" request parameter for a zone to skip sending RRSets in the response (Closes API zone endpoint should support an 'rrset' parameter #5712)
Add checkNSEC3PARAM function
Add rectify endpoint in the API

This PR takes a lot of ideas and code from #3417 and subsequent development.

Closes #3417

Many thanks to Nils Wisiol (@nils-wisiol) for the initial implementation.

Checklist

I have:

read the CONTRIBUTING.md document
compiled and tested this code
included documentation (including possible behaviour changes)
documented the code
added or modified regression test(s)
added or modified unit test(s)

nils-wisiol · 2017-10-07T10:35:09Z

@pieterlexis, thank you for completing this! 👍

peterthomassen · 2017-10-07T11:06:48Z

@pieterlexis Does this also allow setting NSEC3 settings?

pieterlexis · 2017-10-07T11:12:53Z

@pieterlexis Does this also allow setting NSEC3 settings?

yes

zeha · 2017-10-07T20:10:42Z

pdns/dbdnsseckeeper.cc

+    return false;
+  }
+
+  UeberBackend B("default");


DNSSECKeeper is supposed to have an UeberBackend instance already, so can you reuse that?

Interestingly, re-using the UeberBackend in the DNSSECKeeper makes the bind-dnssec-both test fail because getSOAUncached() returns false for secure-delegated.dnssec-parent.com. Investigating

confirmed, reverting 1d7630f makes all the tests pass.

That sounds reaaaally bad. Like someone keeping a list() operation open or sth.

ah, I think I got it. In pdnsutil.cc:1983 we create a DNSSECKeeper object without handing it an UeberBackend. So when rectifyZone (in pdnsutil) is called we indeed can not get a SOA from that dk.

so this is a bug(ish) in pdnsutil, but it does mean that calling DNSSECKeeper::rectifyZone means we need the Ueber backend. Maybe it indeed makes more sense to move rectifyZone to the UeberBackend (but that has other pain points)

Alright, I have made a solution that uses the existing UeberBackend if it works and allocates a new one when needed. C++ wizards are welcome to make this better......

zeha · 2017-10-07T20:13:48Z

docs/http-api/zone.rst

-
-  ``dnssec``, ``nsec3narrow``, ``nsec3param``, ``presigned`` are not yet implemented.
+  ``secure-zone`` and ``rectify-zone`` (if ``api_rectify`` is set to "1").
+  This also applies to newly created zones. If ``presigned`` is ``true``,


the mismatch of 1 and true is quite ugly here. I think API consumers should not carry the burden of dealing with different truthiness values

I think I do make a bool out of, but it is wrongly documented

the code below indeed confused me. please check whats actually there and i think boolifying it is a good idea (:

The POST/PUT and GET for api_rectify were not consistent. It is fully boolified on the API level in the upcoming push.

zeha · 2017-10-07T20:17:40Z

pdns/ws-auth.cc

+        if (k_algo != -1) {
+          int64_t id;
+          if (!dk.addKey(zonename, true, k_algo, id, k_size)) {
+            throw ApiException("No backend was able to secure '" + zonename.toString() + "', most likely because no DNSSEC"


This message is duplicated at least three times.

Also I can't say I like these super-long messages that suggest backend-specific config settings...

zeha · 2017-10-07T20:18:12Z

pdns/ws-auth.cc

+    }
+    if (!dk.setNSEC3PARAM(zonename, ns3pr, boolFromJson(document, "nsec3narrow", false))) {
+      throw ApiException("NSEC3PARAMs provided for zone '" + zonename.toString() +
+          "' passed our basic sanity checks, but cannot be used with the current backend.");


s/current/hosting/?

I believe current is the better word here; "hosting" over-specifies the backend and suggests there is more than one backend at a time.

zeha · 2017-10-07T20:20:52Z

pdns/ws-auth.cc

+    if (!dk.rectifyZone(zonename, error_msg))
+      throw ApiException("Failed to rectify '" + zonename.toString() + "' " + error_msg);
+  }
+
  di.backend->commitTransaction();


dk.rectifyZone calls commitTransaction too, right? Maybe dk.rectifyZone shouldn't deal with backend lookups and transactions at all.

That would mean that all callers are responsible for this? I would prefer dk.rectifyZone() to do some sanity checking and perform adding the ENTs in a safe way

Probably. But it'd be great if the API could modify and rectify the zone in one TX, so no unrectified stuff is ever served by the DNS parts...

that is a sexy idea......

Will be implemented

pieterlexis · 2017-10-16T10:43:50Z

@zeha I think the new commits will satisfy your comments?

@nils-wisiol

This commit takes a lot of ideas and code from PowerDNS#3417 and subsequent development and implements the following things: - Generate DNSSEC keys for a zone when "dnssec" is true in an API POST/PATCH for zones - Rectify DNSSEC zones after POST/PATCH when API-RECTIFY metadata is 1 - Allow setting this metadata via the "api-rectify" param in a Zone object - Shows "nsec3param" and "nsec3narrow" in Zone API responses - Adds an "rrsets" request parameter for a zone to skip sending RRSets in the response (Closes PowerDNS#5712) Closes PowerDNS#3417 Many thanks to Nils Wisiol (@nils-wisiol) for the initial implementation.

But use a full UeberBackend when needed.

This is added so the API can wrap an update to a zone's records *and* DNSSEC info into a single transaction.

pieterlexis added auth auth41MUSTHAVE labels Oct 6, 2017

pieterlexis added this to the auth-4.1.0 milestone Oct 6, 2017

pieterlexis requested review from zeha and Habbie October 6, 2017 16:22

zeha requested changes Oct 7, 2017

View reviewed changes

aerique assigned pieterlexis Oct 13, 2017

pieterlexis force-pushed the api-rectify-version-2 branch from e302b52 to 0448186 Compare October 16, 2017 10:41

zeha approved these changes Oct 17, 2017

View reviewed changes

pieterlexis force-pushed the api-rectify-version-2 branch from 8018aba to b921754 Compare October 17, 2017 11:16

pieterlexis added 8 commits October 17, 2017 16:17

Move rectifyZone from pdnsutil to DNSSECKeeper

25b1ce1

Add checkNSEC3PARAM function

68fd116

API: add rectify endpoint

4bc8379

API: Fully boolify api_rectify

c00908f

Reuse UeberBackend in DNSSECKeeper::rectifyZone()

a2e700d

But use a full UeberBackend when needed.

Add doRectify bool to DNSSECKeeper::rectifyZone()

8d0f207

This is added so the API can wrap an update to a zone's records *and* DNSSEC info into a single transaction.

Deduplicate and shorten API exception message

89a7e70

pieterlexis force-pushed the api-rectify-version-2 branch from b921754 to 89a7e70 Compare October 17, 2017 14:43

aerique merged commit b7abf35 into PowerDNS:master Oct 18, 2017

pieterlexis deleted the api-rectify-version-2 branch October 18, 2017 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rectify zones via the API #5779

Rectify zones via the API #5779

pieterlexis commented Oct 6, 2017

nils-wisiol commented Oct 7, 2017

peterthomassen commented Oct 7, 2017

pieterlexis commented Oct 7, 2017

zeha Oct 7, 2017

pieterlexis Oct 16, 2017

pieterlexis Oct 17, 2017

pieterlexis Oct 17, 2017

zeha Oct 17, 2017 •

edited

Loading

pieterlexis Oct 17, 2017

pieterlexis Oct 17, 2017

pieterlexis Oct 17, 2017

zeha Oct 7, 2017

pieterlexis Oct 9, 2017

zeha Oct 9, 2017

pieterlexis Oct 16, 2017

zeha Oct 7, 2017

zeha Oct 7, 2017

nils-wisiol Oct 8, 2017

zeha Oct 7, 2017

pieterlexis Oct 9, 2017

zeha Oct 9, 2017

pieterlexis Oct 9, 2017

pieterlexis Oct 16, 2017

pieterlexis commented Oct 16, 2017

Rectify zones via the API #5779

Rectify zones via the API #5779

Conversation

pieterlexis commented Oct 6, 2017

Short description

Checklist

nils-wisiol commented Oct 7, 2017

peterthomassen commented Oct 7, 2017

pieterlexis commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeha Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterlexis commented Oct 16, 2017

zeha Oct 17, 2017 •

edited

Loading