On 13.06.2016 14:42, Oleg Fayans wrote:
Hi guys,

Here is a test for dnssec key rotation mechanism.
The full set of commands works perfectly when run manually (even in the
mode of a full copy-pasting from the test). However, when run
automatically, the test always fails as `dig +rrcomments test.here DS`
does not display zone keytag. I tried to decrease default key TTL values
with no success. Could anyone take a look into this (after 4.4 is
released, of course)?




Patch 0043: NACK

1)
+    DNSSEC_KSK_LIFETIME = 'P2Y'
+    DNSSEC_ZSK_LIFETIME = 'P90D'

Why this is needed? This is no used in code so you have no guarantee that if we change default values this will still work. If you want use this, you need to use this values in installers too otherwise it can be in patch 44

2)
+    DNSSEC_KASP_TEMPLATE = '/usr/share/ipa/opendnssec_kasp.template'
This is precedent, for templates other parts of IPA just use os.patch.join(USR_SHARE_IPA_DIR, 'opendnssec_kasp.template'), But if you want we may keep this.


Patch 0044: NACK

1)
You should raise error if values haven't been replaces (when I tried sed, it returned 0 even if nothing was replaced). Without this check it will be lot of fun to detect why tests failed in case we change default config of DNSSEC (what is possible in distant future)

Maybe this http://stackoverflow.com/questions/15965073/return-code-of-sed-for-no-match or do grep before

2)
I'm not sure if just changing of this two values are enough, there are multiple timeouts, and I'm afraid it may delay key rotation and you will not be able to test it. This should be discussed.

Patch 0045: NACK

1) PEP8 2 lines required
         ), ("Zone %s is not signed (master)"
             % example3_test_zone)
+class TestDNSSECRotation(IntegrationTest):
+    num_replicas = 0

2) the rest of testuite/code use 'absolute' or 'relative', not 'reduced'. I prefer to use absolute
+    testzone = "myexample.test."
+    testzone_reduced = "myexample.test"

3)
Why is sleep here?
+    def test_dnssec_rotation(self):
+        time.sleep(850)
+        self.master.run_command(['ipa', 'dnszone-add',
+                                 '--dnssec=true', self.testzone])


4)
I really prefer to parse lines using regexp (as extra method), and get particular parts in list of dicts [{domain: 'example_com', key_type:'KSK', next_trans: '2015-06-08 12:33:00 (retire)', state:'active'},]
+        assert(text.count('ready') == 1), (
+            "Zone must have 1 ready key, found %i" % text.count('ready'))
+        ksk_keytag = ods_output.stdout_text.split('\n')[2].split(' ')[-1]

5)
+ self.master.run_command("dig %s DNSKEY > dnskey.txt" % self.testzone)
+        result0 = self.master.run_command(['dnssec-dsfromkey', '-f',
+ 'dnskey.txt', '-2', self.testzone])

+        dsrec = " ".join(result0.stdout_text.split(' ')[3:]).strip()
+        self.master.run_command(
+            "ipa dnsrecord-add test. %s --ns-rec=%s. --ds-rec='%s'" % (
+                self.testzone, self.master.hostname, dsrec
+            )
+        )
This is done using python in test_chain_of_trust, please extract that part into function and use in both tests

6)
+        self.master.run_command(['ipa', 'dnszone-add', '--dnssec=true',
+                                 'test.'])
Use variable for 'test.' please

7)
+        result1 = self.master.run_command(['dig', '+rrcomments',
+                                           self.testzone, 'DS'])
+        assert(ksk_keytag in result1.stdout_text), (
+            "Failed to find zone's ksk keytag in the command output")

Use function
ans = resolve_with_dnssec(nameserver, self.testzone , log, rtype="DS")
ans.rrset contains all DS records, you can iterate over it

ans.rrset.items[0].key_tag will return keytag for DS record (just example count with multiple DS records)

8)
+        assert(text2.count('ready') == 2), (
+            "Zone must have 2 keys in ready state, found %i" %
+            text2.count(self.testzone_reduced))

explicitly test which key has which state, because you will have multiple keys there (continue reading :-) )

9)
+        self.master.run_command(['ipa', 'dnszone-add', '--dnssec=true',
+                                 'test.'])
+        restart_named([self.master])
+        dsrec = " ".join(result0.stdout_text.split(' ')[3:]).strip()
+        self.master.run_command(
+            "ipa dnsrecord-add test. %s --ns-rec=%s. --ds-rec='%s'" % (
+                self.testzone, self.master.hostname, dsrec
+            )
+        )
+
+        result1 = self.master.run_command(['dig', '+rrcomments',
+                                           self.testzone, 'DS'])

Give time to named service to process newly added zone and records, maybe wait_until_signed function may help

10)
+        # activate rotation
+        result2 = self.master.run_command(
+ "export SOFTHSM2_CONF=%s ; %s key ds-seen --zone %s --keytag %s" %
+            (paths.DNSSEC_SOFTHSM2_CONF, paths.ODS_KSMUTIL,
+             self.testzone, ksk_keytag))

Nope this is not a key rotation, you just enabled KSK key (current bind-dyndb-ldap implementation uses key even without activation)

10)
I miss the part where key rotation is tested for ZSK key

I expected some sleep for '15M' and then new check if new key is in opendnssec database and if new key is used in RRSIG records, and if DNSKEY was updated

11)
I miss the part where key rotation is tested for KSK key

I expected to see 3 rotation of ZSK key, and then one KSK key rotation, with new ds-seen procedure

Martin^2


-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to