Secure and Rectify Zones When they are Created or Modified Through API Calls (POST/PATCH) #3417

nils-wisiol · 2016-02-20T16:42:03Z

This PR extends existing API functionality of POST and PATCH zones by optionally sign and rectify them. To do so, the patch honors the dnssec, nsec3param and nsec3narrow parameters of the API request on POST and stores them. If anything goes wrong with signing or rectifying, the API throws an APIException and aborts the transaction -- the zone will not be inserted. On a PATCH, the zone is rectified again.

Technical note: the code for signing and rectifying was taken from pdnsutil. Due to the amount of code needed for rectifying, it was moved to the DNSSECKeeper to use the same code in pdnsutil and in the API.

The PR does not cause any more gmysql test cases to fail than before. Failing test cases are:

0dyndns-prereq-all
0dyndns-prereq-nxrrset-full
1dyndns-big-package
1dyndns-check-soa-update
1dyndns-update-add-delete
1dyndns-update-add-delete-casesensative
1dyndns-update-add-delete-cname
1dyndns-update-add-delete-ds
1dyndns-update-add-delete-mx
1dyndns-update-add-delete-wildcard
1dyndns-update-add-invalid-record
1dyndns-update-add-soa
1dyndns-update-deep-add-delete
1dyndns-update-deep-delegate
1dyndns-update-delegate
1dyndns-update-delegate-in-between
1dyndns-update-delete-add-host
1dyndns-update-delete-multi-add-host
1dyndns-update-delete-mx-prio
1dyndns-update-delete-ns
1dyndns-update-delete-parent-delegate
1dyndns-update-delete-soa
1dyndns-update-in-between
1dyndns-update-nsec3params
1dyndns-update-nsec3params-with-others
1dyndns-update-replace-a-host
1dyndns-update-replace-cname
1dyndns-update-replace-mx
1dyndns-update-srv
1dyndns-update-update-ttl
2dyndns-update-replace-soa
tsig-axfr
tsig-axfr-algorithm-mismatch
tsig-axfr-key-mismatch
tsig-ixfr
tsig-ixfr-algorithm-mismatch
tsig-ixfr-key-mismatch

I can edit the commit messages if desired.

Please let me know if any more editing is needed in order to pass this PR.

Habbie · 2016-02-20T18:15:01Z

pdns/pdnsutil.cc

-      cerr<<"Adding NSEC ordering information "<<endl;
-    else if(!narrow) {
-      if(!isOptOut)
-        cerr<<"Adding NSEC3 hashed ordering information for '"<<zone.toString()<<"'"<<endl;


It looks like these messages are completely gone after your pull?

zeha · 2016-02-20T18:39:39Z

Thank you for submitting this PR. It's a long-standing TODO and indeed we should be doing this sooner than later. As @Habbie noted elsewhere, we've been holding off doing this for various reasons, but we can work on them later after it works as imagined :-)

I have some comments on your changes; if you want/need clarifications or help feel free to drop by on IRC for a more real-time interaction.

550dd18 and 7a9dd50 should be a single commit, because those two just move code around.
Creating slave zones via API appears to fail now (see travis)
Please add at least one test in regression-tests.api with, say, dnssec=true and ensuring that the zone is indeed set up correctly afterwards. More tests, probably with different nsec3param, nsec3narrow settings would be good as well.
Calling stringFromJson with an empty string as the default is equivalent to just calling document["nsec3param"].string_value().
In apiServerZones all checking and throwing needs to go before B.createDomain, as otherwise a broken domain is left over in the DB.
The error messages for dk.addKey and dk.isSecuredZone failing could be more specific. With pdnsutil there is/was probably more output, and an admin might just look into config files or the DB for clues, where with the API the user might only get that single error message.
Error checking on the rectifyZone call in patchZone is missing.

zeha · 2016-02-20T18:40:03Z

Documentation also needs an update saying what's supported now. (Incl. a warning that turning off dnssec or back on is not supported yet?)

Habbie · 2016-02-20T19:04:47Z

By the way, all gmysql tests pass for us on master. If they don't pass for you, perhaps you are missing some tools (like nsupdate).

Habbie · 2016-02-20T19:06:25Z

What @zeha is referring to that I said elsewhere is: we've always felt that the right approach is to do a surgically small rectify around the actual update (and I still feel that way). However, because of that idea, we basically forgot to consider your approach, i.e. a full rectify after each update. We have given this some thought today and indeed we want to go forward with your approach. For small zones it will be just fine, for big zones it will make updates slower, but most people have small zones and thus a full rectify is much better than no rectify. Great work!

nils-wisiol · 2016-02-23T11:57:49Z

Thanks for the feedback, it is highly appreciated. I'll start working on the mentioned points as soon as possible and then get back to you.

nils-wisiol · 2016-02-28T11:02:16Z

@zeha, I would like to move error checking in ApiServerZones before B.createDomain. Error checking though is partially done by DNSSECKeeper::setNSEC3PARAM, which in turn returns false if some database query fails. The query requires the nsec3data to be in the database, which I believe requires the domain to exist.

Why is B.createDomain called before the transaction is started? Can't we fix this by enclosing the creation of the domain in the transaction as well?

DNSSECKeeper::setNSEC3PARAM btw sometimes throws, sometimes returns false on failure. Do we need to handle them differently?

nils-wisiol · 2016-02-28T11:18:55Z

@zeha, I did some research on the dk.addKey and dk.isSecuredZone errors. Essentially addKey fails, if DNSSEC is not supported by any backend (this is what the current error message says). isSecuredZone fails, if the zone was presigned (which we check before) or the zone has too many non-empty terminals (which I now included into the error msg).

I'd leave the addKey message unchanged, but I'm also open for concrete suggestions.

zeha · 2016-02-28T17:12:22Z

@zeha, I would like to move error checking in ApiServerZones before B.createDomain. Error checking though is partially done by DNSSECKeeper::setNSEC3PARAM, which in turn returns false if some database query fails. The query requires the nsec3data to be in the database, which I believe requires the domain to exist.
Okay, that's a bit unfortunate. I think the only part that we can and should care about is the max-nsec3-iterations check. Maybe we can split setNSEC3PARAM into one function for checking and one for setting.

Why is B.createDomain called before the transaction is started? Can't we fix this by enclosing the creation of the domain in the transaction as well?
It's my understanding that beginTransaction already needs the zone Id, and we get that by calling createDomain.

DNSSECKeeper::setNSEC3PARAM btw sometimes throws, sometimes returns false on failure. Do we need to handle them differently?

It appears to return false on catastrophic failure/misconfiguration. That's odd. Maybe it's behaviour will be a lot clearer if the function is split as suggested above.

Thanks!

Habbie · 2016-04-08T10:49:56Z

This branch has conflicts

Habbie · 2017-08-16T19:33:03Z

This branch still has conflicts.

If a POST request contains the "dnssec" parameter set to true, we sign the zone with DNSSECKeeper.addKey and algorithms and parameters given in the - default-ksk-algorithms - default-zsk-algorithms - default-ksk-size - default-zsk-size config paratmers. If the configured algorithms cannot be used, we throw an ApiException. The zone will not yet be automatically rectified.

Copied code from pdnsutil to DNSSECKeeper in order to reuse it.

This previously resulted into segfault.

cf. 82cc076

cf. 96e63d7

@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.

@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.

@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.

@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.

Habbie reviewed Feb 20, 2016
View reviewed changes

nils-wisiol force-pushed the api-sign-rectify branch from 1f97ce1 to 9cb4f6d Compare February 24, 2016 08:45

pieterlexis added auth enhancement rest-api labels Feb 25, 2016

pieterlexis added this to the auth-4.0.0 milestone Feb 25, 2016

nils-wisiol force-pushed the api-sign-rectify branch 4 times, most recently from 581a0e8 to 2a105a3 Compare February 28, 2016 11:51

nils-wisiol mentioned this pull request Jun 13, 2016

pdnsutil and REST API implement redundant functionallity #3969

Open

Habbie modified the milestones: auth-4.1.0, auth-4.0.0 Jul 7, 2016

This was referenced Jul 6, 2017

API: dnssec support (Auth) #1433

Closed

REST API docs: several issues which need clarification #2064

Open

Habbie modified the milestones: auth-4.2.0, former-auth-4.1.0 Aug 16, 2017

nils-wisiol added 3 commits August 16, 2017 21:57

feat(dnsseckeeper): move recitfyZone() to DNSSECKeeper

15ea6e3

Copied code from pdnsutil to DNSSECKeeper in order to reuse it.

feat(api): rectify zone on POST and PATCH

28fbda3

nils-wisiol added 5 commits August 16, 2017 22:23

docs(api): DNSSEC support for POST and PATCH zones

1799b80

feat(dnsseckeeper): added check method for NSEC3Params

e99d0a1

fix(api): check NSEC3Params before creating domain

42dc353

fix(api): only rectify secured zones

1956419

fix(dnsseckeeper): don't rectify unknown zones

4ebc9d1

This previously resulted into segfault.

nils-wisiol force-pushed the api-sign-rectify branch 3 times, most recently from 9b96283 to ddeffb0 Compare August 19, 2017 06:13

fix(ws-auth): adjust addKey parameters

85e721d

cf. 82cc076

nils-wisiol force-pushed the api-sign-rectify branch from ddeffb0 to 85e721d Compare August 19, 2017 06:33

fix(rectify): added missing variable to moved code

4c795ab

nils-wisiol force-pushed the api-sign-rectify branch from 94cd603 to 0df0818 Compare August 19, 2017 07:25

nils-wisiol added 2 commits August 19, 2017 10:24

fix(SOAData): updated updateDNSSECOrderNameAndAuth signature

9b469c8

cf. 96e63d7

fix(addKey): set seperator bit to false for zsk

92698a2

nils-wisiol force-pushed the api-sign-rectify branch from 0df0818 to 92698a2 Compare August 19, 2017 08:25

aerique mentioned this pull request Sep 11, 2017

Rectify #5680

Closed

aerique modified the milestones: auth-4.1.0, auth-4.2.0 Sep 11, 2017

pieterlexis self-assigned this Sep 11, 2017

aerique added the auth41MUSTHAVE label Sep 26, 2017

pieterlexis mentioned this pull request Oct 6, 2017

Rectify zones via the API #5779

Merged

6 tasks

aerique closed this in #5779 Oct 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure and Rectify Zones When they are Created or Modified Through API Calls (POST/PATCH) #3417

Secure and Rectify Zones When they are Created or Modified Through API Calls (POST/PATCH) #3417

nils-wisiol commented Feb 20, 2016

Habbie Feb 20, 2016

zeha commented Feb 20, 2016

zeha commented Feb 20, 2016

Habbie commented Feb 20, 2016

Habbie commented Feb 20, 2016

nils-wisiol commented Feb 23, 2016

nils-wisiol commented Feb 28, 2016

nils-wisiol commented Feb 28, 2016

zeha commented Feb 28, 2016

Habbie commented Apr 8, 2016

Habbie commented Aug 16, 2017

Secure and Rectify Zones When they are Created or Modified Through API Calls (POST/PATCH) #3417

Secure and Rectify Zones When they are Created or Modified Through API Calls (POST/PATCH) #3417

Conversation

nils-wisiol commented Feb 20, 2016

Habbie Feb 20, 2016

Choose a reason for hiding this comment

zeha commented Feb 20, 2016

zeha commented Feb 20, 2016

Habbie commented Feb 20, 2016

Habbie commented Feb 20, 2016

nils-wisiol commented Feb 23, 2016

nils-wisiol commented Feb 28, 2016

nils-wisiol commented Feb 28, 2016

zeha commented Feb 28, 2016

Habbie commented Apr 8, 2016

Habbie commented Aug 16, 2017