Single ZSK fixes #3124

Merged
merged 4 commits into from Feb 4, 2016

Projects

None yet

3 participants

@pieterlexis
Member

This PR fixes CDS/CDNSKEY publishing when using only ZSKs (the new default) and it fixes pdnsutil not showing the DS records for these keys.

@zeha
Collaborator
zeha commented Dec 28, 2015

Failed tests:

  • verify-dnssec-zone
  • publishing-cds-cdnskey
publishing-cds-cdnskey
+cat ./tests/publishing-cds-cdnskey/diff
--- ./tests/publishing-cds-cdnskey/expected_result  2015-12-28 11:09:34.305263776 +0000
+++ ./tests/publishing-cds-cdnskey/real_result  2015-12-28 11:23:00.219164521 +0000
@@ -14,19 +14,17 @@
 0  secure-delegated.dnssec-parent.com. IN  CDS 86400   54319 8 2 a0b9c38cd324182af0ef66830d0a0e85a1d58979c9834e18c871779e040857b7
 0  secure-delegated.dnssec-parent.com. IN  RRSIG   86400   CDNSKEY 8 3 86400 [expiry] [inception] [keytag] secure-delegated.dnssec-parent.com. ...
 0  secure-delegated.dnssec-parent.com. IN  RRSIG   86400   CDS 8 3 86400 [expiry] [inception] [keytag] secure-delegated.dnssec-parent.com. ...
-0  cdnskey-cds-test.com.   IN  CDS 86400
-0  cdnskey-cds-test.com.   IN  CDS 86400
-0  cdnskey-cds-test.com.   IN  RRSIG   86400
+1  cdnskey-cds-test.com.   IN  NSEC    86400
+1  cdnskey-cds-test.com.   IN  RRSIG   3600    SOA 13 2 3600 [expiry] [inception] [keytag] cdnskey-cds-test.com. ...
+1  cdnskey-cds-test.com.   IN  RRSIG   86400
+1  cdnskey-cds-test.com.   IN  SOA 3600    ns1.cdnskey-cds-test. ahu.example.com. 2005092501 28800 7200 604800 86400
 2  .   IN  OPT 32768   
 Rcode: 0 (No Error), RD: 0, QR: 1, TC: 0, AA: 1, opcode: 0
 Reply to question for qname='cdnskey-cds-test.com.', qtype=CDS
-0  cdnskey-cds-test.com.   IN  CDNSKEY 86400
-0  cdnskey-cds-test.com.   IN  RRSIG   86400
+1  cdnskey-cds-test.com.   IN  NSEC    86400
+1  cdnskey-cds-test.com.   IN  RRSIG   3600    SOA 13 2 3600 [expiry] [inception] [keytag] cdnskey-cds-test.com. ...
+1  cdnskey-cds-test.com.   IN  RRSIG   86400
+1  cdnskey-cds-test.com.   IN  SOA 3600    ns1.cdnskey-cds-test. ahu.example.com. 2005092501 28800 7200 604800 86400
 2  .   IN  OPT 32768   
 Rcode: 0 (No Error), RD: 0, QR: 1, TC: 0, AA: 1, opcode: 0
 Reply to question for qname='cdnskey-cds-test.com.', qtype=CDNSKEY
-0  cdnskey-cds-test.com.   IN  CDNSKEY 86400
-0  cdnskey-cds-test.com.   IN  CDS 86400
-0  cdnskey-cds-test.com.   IN  CDS 86400
-0  cdnskey-cds-test.com.   IN  RRSIG   86400
-0  cdnskey-cds-test.com.   IN  RRSIG   86400
@Habbie Habbie added this to the auth-4-alpha2 milestone Dec 30, 2015
@pieterlexis
Member

ok, finally green

@pieterlexis
Member
======================================================================
ERROR: test suite for <class 'test_Advanced.TestAdvancedSpoof'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/.venv/local/lib/python2.7/site-packages/nose/suite.py", line 208, in run
    self.setUp()
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/.venv/local/lib/python2.7/site-packages/nose/suite.py", line 291, in setUp
    self.setupContext(ancestor)
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/.venv/local/lib/python2.7/site-packages/nose/suite.py", line 314, in setupContext
    try_run(context, names)
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/.venv/local/lib/python2.7/site-packages/nose/util.py", line 469, in try_run
    return func()
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/dnsdisttests.py", line 107, in setUpClass
    cls.startDNSDist()
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/dnsdisttests.py", line 93, in startDNSDist
    cls._dnsdist.terminate()
  File "/usr/lib/python2.7/subprocess.py", line 1551, in terminate
    self.send_signal(signal.SIGTERM)
  File "/usr/lib/python2.7/subprocess.py", line 1546, in send_signal
    os.kill(self.pid, sig)
OSError: [Errno 3] No such process

----------------------------------------------------------------------
Ran 30 tests in 20.242s

Related to the dnsdist tests :(

@pieterlexis
Member

ready for review

@Habbie Habbie and 1 other commented on an outdated diff Jan 19, 2016
pdns/dbdnsseckeeper.cc
@@ -367,7 +367,25 @@ bool DNSSECKeeper::unsetPublishCDNSKEY(const DNSName& zname)
return d_keymetadb->setDomainMetadata(zname, "PUBLISH_CDNSKEY", vector<string>());
}
-DNSSECKeeper::keyset_t DNSSECKeeper::getKeys(const DNSName& zone, boost::tribool allOrKeyOrZone, bool useCache)
+/**
+ * Returns all keys that are used to sign the DNSKEY RRSet in a zone
+ *
+ * @param zname DNSName of the zone
+ * @return a keyset_t with all keys that are used to sign the DNSKEY
+ * RRSet (these are the entrypoint(s) to the zone)
+ */
+DNSSECKeeper::keyset_t DNSSECKeeper::getEntryPoints(const DNSName& zname)
+{
+ DNSSECKeeper::keyset_t ret;
+ DNSSECKeeper::keyset_t keys = getKeys(zname);
+
+ for(auto const &keymeta : keys)
+ if(keymeta.second.active && keymeta.second.isSEP)
@Habbie
Habbie Jan 19, 2016 Member

non-SEP 256 keys cannot sign the DNSKEY RRset?

@pieterlexis
pieterlexis Jan 19, 2016 Member

ah, I though I fixed this by testing for (active && (CSK || KSK))

@Habbie
Habbie Jan 19, 2016 Member

that sounds good

@Habbie Habbie and 1 other commented on an outdated diff Jan 19, 2016
pdns/dbdnsseckeeper.cc
@@ -74,11 +74,11 @@ bool DNSSECKeeper::isPresigned(const DNSName& name)
return meta=="1";
}
-bool DNSSECKeeper::addKey(const DNSName& name, bool keyOrZone, int algorithm, int bits, bool active)
+bool DNSSECKeeper::addKey(const DNSName& name, bool isSEP, int algorithm, int bits, bool active)
@Habbie
Habbie Jan 19, 2016 Member

isSEP is a confusing name - the way we use it, hasSEPbit might be cleaner (even if a bit long)

@pieterlexis
pieterlexis Jan 19, 2016 Member

setSEPbit might be better for the function prototype

@Habbie
Member
Habbie commented Jan 19, 2016
  1. Few nits.
  2. Would like explicit testing of '256 CSK' and '257 CSK' and of course the normal KSK+ZSK stuff, can be a separate PR
  3. Could use another pair of eyes after nits are fixed

Good work!

@pieterlexis pieterlexis Add the 'CSK' (Combined Signing Key) nomenclature
This commit removes the 'keyOrZone' boolean from
DNSSECKeeper::KeyMetaData and adds 'keyType' enum to it that can contain
one of 3 values (KSK, ZSK or CSK). A key is marked as a CSK when there
is no other key with the same algorithm for the zone, and if there is
another key, that it does not have a different SEP-bit set.

By default, we now also set the SEP-bit in `pdnsutil secure-zone` when
only a ZSK is created (this is the default) so we comply with the
recommendation in RFC 6781 §3.2.3.

Closes #3194
b6bd795
@Habbie Habbie merged commit e170fa2 into PowerDNS:master Feb 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pieterlexis pieterlexis deleted the pieterlexis:single-zsk-fixes branch Feb 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment