From d4b2b7072d3b858a53f47ad5de7c50663d543ad5 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 26 Jan 2021 11:24:40 +0100 Subject: [PATCH 1/3] Update legacy-keys kasp test The 'legacy-keys.kasp' test checks that a zone with key files but not yet state files is signed correctly. This test is expanded to cover the case where old key files still exist in the key directory. This covers bug #2406 where keys with the "Delete" timing metadata are picked up by the keymgr as active keys. Fix the 'legacy-keys.kasp' test, by creating the right key files (for zone 'legacy-keys.kasp', not 'legacy,kasp'). Use a unique policy for this zone, using shorter lifetimes. Create two more keys for the zone, and use 'dnssec-settime' to set the timing metadata in the past, long enough ago so that the keys should not be considered by the keymgr. Update the 'key_unused()' test function, and consider keys with their "Delete" timing metadata in the past as unused. Extend the test to ensure that the keys to be used are not the old predecessor keys (with their "Delete" timing metadata in the past). Update the test so that the checks performed are consistent with the newly configured policy. --- bin/tests/system/kasp/clean.sh | 2 + bin/tests/system/kasp/ns3/named.conf.in | 2 +- .../system/kasp/ns3/policies/kasp.conf.in | 9 ++ bin/tests/system/kasp/ns3/setup.sh | 15 +- bin/tests/system/kasp/tests.sh | 136 ++++++++++++++---- 5 files changed, 136 insertions(+), 28 deletions(-) diff --git a/bin/tests/system/kasp/clean.sh b/bin/tests/system/kasp/clean.sh index ab5890da77..a6b8ee1124 100644 --- a/bin/tests/system/kasp/clean.sh +++ b/bin/tests/system/kasp/clean.sh @@ -25,7 +25,9 @@ rm -f ns*/managed-keys.bind rm -f ns*/*.mkeys rm -f ns*/zones ns*/*.db.infile rm -f ns*/*.zsk1 ns*/*.zsk2 +rm -f ns3/legacy-keys.* rm -f *.created published.test* retired.test* rm -f rndc.dnssec.*.out.* rm -f python.out.* rm -f *-supported.file +rm -f created.key-* unused.key-* diff --git a/bin/tests/system/kasp/ns3/named.conf.in b/bin/tests/system/kasp/ns3/named.conf.in index 3df661b4ae..cf454d6e9a 100644 --- a/bin/tests/system/kasp/ns3/named.conf.in +++ b/bin/tests/system/kasp/ns3/named.conf.in @@ -155,7 +155,7 @@ zone "some-keys.kasp" { zone "legacy-keys.kasp" { type primary; file "legacy-keys.kasp.db"; - dnssec-policy "rsasha1"; + dnssec-policy "migrate-to-dnssec-policy"; }; /* diff --git a/bin/tests/system/kasp/ns3/policies/kasp.conf.in b/bin/tests/system/kasp/ns3/policies/kasp.conf.in index 8230606302..825b21b1fa 100644 --- a/bin/tests/system/kasp/ns3/policies/kasp.conf.in +++ b/bin/tests/system/kasp/ns3/policies/kasp.conf.in @@ -45,6 +45,15 @@ dnssec-policy "rsasha1" { }; }; +dnssec-policy "migrate-to-dnssec-policy" { + dnskey-ttl 1234; + + keys { + ksk key-directory lifetime P6M algorithm 5; + zsk key-directory lifetime P6M algorithm 5; + }; +}; + dnssec-policy "rsasha1-nsec3" { dnskey-ttl 1234; diff --git a/bin/tests/system/kasp/ns3/setup.sh b/bin/tests/system/kasp/ns3/setup.sh index 521c192329..dcfaee4157 100644 --- a/bin/tests/system/kasp/ns3/setup.sh +++ b/bin/tests/system/kasp/ns3/setup.sh @@ -87,9 +87,18 @@ zone="some-keys.kasp" $KEYGEN -G -a RSASHA1 -b 2000 -L 1234 $zone > keygen.out.$zone.1 2>&1 $KEYGEN -G -a RSASHA1 -f KSK -L 1234 $zone > keygen.out.$zone.2 2>&1 -zone="legacy.kasp" -$KEYGEN -a RSASHA1 -b 2000 -L 1234 $zone > keygen.out.$zone.1 2>&1 -$KEYGEN -a RSASHA1 -f KSK -L 1234 $zone > keygen.out.$zone.2 2>&1 +zone="legacy-keys.kasp" +ZSK=$($KEYGEN -a RSASHA1 -b 2048 -L 1234 $zone 2> keygen.out.$zone.1) +KSK=$($KEYGEN -a RSASHA1 -f KSK -L 1234 $zone 2> keygen.out.$zone.2) +echo $ZSK > legacy-keys.kasp.zsk +echo $KSK > legacy-keys.kasp.ksk +# Predecessor keys: +Tact="now-9mo" +Tret="now-3mo" +ZSK=$($KEYGEN -a RSASHA1 -b 2048 -L 1234 $zone 2> keygen.out.$zone.3) +KSK=$($KEYGEN -a RSASHA1 -f KSK -L 1234 $zone 2> keygen.out.$zone.4) +$SETTIME -P $Tact -A $Tact -I $Tret -D $Tret "$ZSK" > settime.out.$zone.1 2>&1 +$SETTIME -P $Tact -A $Tact -I $Tret -D $Tret "$KSK" > settime.out.$zone.2 2>&1 zone="pregenerated.kasp" $KEYGEN -G -k rsasha1 -l policies/kasp.conf $zone > keygen.out.$zone.1 2>&1 diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh index 4519717fa6..8b1fb3bd75 100644 --- a/bin/tests/system/kasp/tests.sh +++ b/bin/tests/system/kasp/tests.sh @@ -587,7 +587,17 @@ key_unused() { [ -s "$STATE_FILE" ] || ret=1 [ "$ret" -eq 0 ] || return - # Check timing metadata. + # Treat keys that have been removed from the zone as unused. + _check_removed=1 + grep "; Created:" "$KEY_FILE" > created.key-${KEY_ID}.test${n} || _check_removed=0 + grep "; Delete:" "$KEY_FILE" > unused.key-${KEY_ID}.test${n} || _check_removed=0 + if [ "$_check_removed" -eq 1 ]; then + _created=$(awk '{print $3}' < created.key-${KEY_ID}.test${n}) + _removed=$(awk '{print $3}' < unused.key-${KEY_ID}.test${n}) + [ "$_removed" -le "$_created" ] && return + fi + + # If no timing metadata is set, this key is unused. grep "; Publish:" "$KEY_FILE" > /dev/null && log_error "unexpected publish comment in $KEY_FILE" grep "; Activate:" "$KEY_FILE" > /dev/null && log_error "unexpected active comment in $KEY_FILE" grep "; Inactive:" "$KEY_FILE" > /dev/null && log_error "unexpected retired comment in $KEY_FILE" @@ -600,13 +610,11 @@ key_unused() { grep "Revoke:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected revoked in $PRIVATE_FILE" grep "Delete:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected removed in $PRIVATE_FILE" - if [ "$_legacy" = "no" ]; then - grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE" - grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE" - grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE" - grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE" - grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE" - fi + grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE" + grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE" + grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE" + grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE" + grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE" } # Test: dnssec-verify zone $1. @@ -1902,22 +1910,6 @@ check_apex check_subdomain dnssec_verify -# -# Zone: legacy-keys.kasp. -# -set_zone "legacy-keys.kasp" -set_policy "rsasha1" "3" "1234" -set_server "ns3" "10.53.0.3" -# Key properties, timings and states same as above. - -check_keys -check_dnssecstatus "$SERVER" "$POLICY" "$ZONE" -set_keytimes_algorithm_policy -check_keytimes -check_apex -check_subdomain -dnssec_verify - # # Zone: pregenerated.kasp. # @@ -2407,6 +2399,102 @@ check_subdomain dnssec_verify check_rrsig_refresh +# +# Zone: legacy-keys.kasp. +# +set_zone "legacy-keys.kasp" +# This zone has two active keys and two old keys left in key directory, so +# expect 4 key files. +set_policy "migrate-to-dnssec-policy" "4" "1234" +set_server "ns3" "10.53.0.3" + +# Key properties. +key_clear "KEY1" +set_keyrole "KEY1" "ksk" +set_keylifetime "KEY1" "16070400" +set_keyalgorithm "KEY1" "5" "RSASHA1" "2048" +set_keysigning "KEY1" "yes" +set_zonesigning "KEY1" "no" + +key_clear "KEY2" +set_keyrole "KEY2" "zsk" +set_keylifetime "KEY2" "16070400" +set_keyalgorithm "KEY2" "5" "RSASHA1" "2048" +set_keysigning "KEY2" "no" +set_zonesigning "KEY2" "yes" +# KSK: DNSKEY, RRSIG (ksk) published. DS needs to wait. +# ZSK: DNSKEY, RRSIG (zsk) published. +set_keystate "KEY1" "GOAL" "omnipresent" +set_keystate "KEY1" "STATE_DNSKEY" "rumoured" +set_keystate "KEY1" "STATE_KRRSIG" "rumoured" +set_keystate "KEY1" "STATE_DS" "hidden" + +set_keystate "KEY2" "GOAL" "omnipresent" +set_keystate "KEY2" "STATE_DNSKEY" "rumoured" +set_keystate "KEY2" "STATE_ZRRSIG" "rumoured" +# Two keys only. +key_clear "KEY3" +key_clear "KEY4" + +check_keys +check_dnssecstatus "$SERVER" "$POLICY" "$ZONE" + +# Make sure the correct legacy keys were used (and not the removed predecessor +# keys). +n=$((n+1)) +echo_i "check correct keys were used when migrating zone ${ZONE} to dnssec-policy ($n)" +ret=0 +kskfile=$(cat ns3/legacy-keys.kasp.ksk) +basefile=$(key_get KEY1 BASEFILE) +echo_i "filename: $basefile (expect $kskfile)" +test "$DIR/$kskfile" = "$basefile" || ret=1 +zskfile=$(cat ns3/legacy-keys.kasp.zsk) +basefile=$(key_get KEY2 BASEFILE) +echo_i "filename: $basefile (expect $zskfile)" +test "$DIR/$zskfile" = "$basefile" || ret=1 +test "$ret" -eq 0 || echo_i "failed" +status=$((status+ret)) + +# KSK times. +created=$(key_get KEY1 CREATED) +keyfile=$(key_get KEY1 BASEFILE) +grep "; Publish:" "${keyfile}.key" > published.test${n}.key1 +published=$(awk '{print $3}' < published.test${n}.key1) +set_keytime "KEY1" "PUBLISHED" "${published}" +set_keytime "KEY1" "ACTIVE" "${published}" +published=$(key_get KEY1 PUBLISHED) +# The DS can be published if the DNSKEY and RRSIG records are OMNIPRESENT. +# This happens after max-zone-ttl (1d) plus publish-safety (1h) plus +# zone-propagation-delay (300s) = 86400 + 3600 + 300 = 90300. +set_addkeytime "KEY1" "SYNCPUBLISH" "${published}" 90300 +# Key lifetime is 6 months, 315360000 seconds. +set_addkeytime "KEY1" "RETIRED" "${published}" 16070400 +# The key is removed after the retire time plus DS TTL (1d), parent +# propagation delay (1h), and retire safety (1h) = 86400 + 3600 + 3600 = 93600. +retired=$(key_get KEY1 RETIRED) +set_addkeytime "KEY1" "REMOVED" "${retired}" 93600 + +# ZSK times. +created=$(key_get KEY2 CREATED) +keyfile=$(key_get KEY2 BASEFILE) +grep "; Publish:" "${keyfile}.key" > published.test${n}.key2 +published=$(awk '{print $3}' < published.test${n}.key2) +set_keytime "KEY2" "PUBLISHED" "${published}" +set_keytime "KEY2" "ACTIVE" "${published}" +published=$(key_get KEY2 PUBLISHED) +# Key lifetime is 6 months, 315360000 seconds. +set_addkeytime "KEY2" "RETIRED" "${published}" 16070400 +# The key is removed after the retire time plus max zone ttl (1d), zone +# propagation delay (300s), retire safety (1h), and sign delay (signature +# validity minus refresh, 9d) = 86400 + 300 + 3600 + 777600 = 867900. +retired=$(key_get KEY2 RETIRED) +set_addkeytime "KEY2" "REMOVED" "${retired}" 867900 + +check_keytimes +check_apex +check_subdomain +dnssec_verify + # # Test dnssec-policy inheritance. # From 76cf72e65a7ff23d3ad10dc917d58d7520e1d44e Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 26 Jan 2021 11:32:24 +0100 Subject: [PATCH 2/3] Correctly initialize old key with state file The 'key_init()' function is used to initialize a state file for keys that don't have one yet. This can happen if you are migrating from a 'auto-dnssec' or 'inline-signing' to a 'dnssec-policy' configuration. It did not look at the "Inactive" and "Delete" timing metadata and so old keys left behind in the key directory would also be considered as a possible active key. This commit fixes this and now explicitly sets the key goal to OMNIPRESENT for keys that have their "Active/Publish" timing metadata in the past, but their "Inactive/Delete" timing metadata in the future. If the "Inactive/Delete" timing metadata is also in the past, the key goal is set to HIDDEN. If the "Inactive/Delete" timing metadata is in the past, also the key states are adjusted to either UNRETENTIVE or HIDDEN, depending on how far in the past the metadata is set. --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 4 ++++ lib/dns/keymgr.c | 37 ++++++++++++++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 344afbca5f..4372665812 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5575. [bug] When migrating to dnssec-policy, BIND considered keys + with the "Inactive" and/or "Delete" timing metadata as + possible active keys. This has been fixed. [GL #2406] + 5574. [func] Incoming zone transfers can now use TLS. Addresses in a "primaries" list take an optional "tls" argument, specifying either a previously diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 82677a00ee..844902dcf4 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -92,3 +92,7 @@ Bug Fixes - Named ``allow-update`` acls where broken in BIND 9.17.9 and BIND 9.16.11 preventing ``named`` starting. [GL #2413] + +- When migrating to ``dnssec-policy``, BIND considered keys with the "Inactive" + and/or "Delete" timing metadata as possible active keys. This has been fixed. + [GL #2406] diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index bfd8009c3a..170eca37f4 100644 --- a/lib/dns/keymgr.c +++ b/lib/dns/keymgr.c @@ -1407,10 +1407,11 @@ static void keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) { bool ksk, zsk; isc_result_t ret; - isc_stdtime_t active = 0, pub = 0, syncpub = 0; + isc_stdtime_t active = 0, pub = 0, syncpub = 0, retire = 0, remove = 0; dst_key_state_t dnskey_state = HIDDEN; dst_key_state_t ds_state = HIDDEN; dst_key_state_t zrrsig_state = HIDDEN; + dst_key_state_t goal_state = HIDDEN; REQUIRE(key != NULL); REQUIRE(key->key != NULL); @@ -1437,6 +1438,7 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) { } else { dnskey_state = RUMOURED; } + goal_state = OMNIPRESENT; } ret = dst_key_gettime(key->key, DST_TIME_PUBLISH, &pub); if (pub <= now && ret == ISC_R_SUCCESS) { @@ -1447,6 +1449,7 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) { } else { zrrsig_state = RUMOURED; } + goal_state = OMNIPRESENT; } ret = dst_key_gettime(key->key, DST_TIME_SYNCPUBLISH, &syncpub); if (syncpub <= now && ret == ISC_R_SUCCESS) { @@ -1457,6 +1460,38 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) { } else { ds_state = RUMOURED; } + goal_state = OMNIPRESENT; + } + ret = dst_key_gettime(key->key, DST_TIME_INACTIVE, &retire); + if (retire <= now && ret == ISC_R_SUCCESS) { + dns_ttl_t zone_ttl = dns_kasp_zonemaxttl(kasp); + zone_ttl += dns_kasp_zonepropagationdelay(kasp); + if ((retire + zone_ttl) <= now) { + zrrsig_state = HIDDEN; + } else { + zrrsig_state = UNRETENTIVE; + } + ds_state = UNRETENTIVE; + goal_state = HIDDEN; + } + ret = dst_key_gettime(key->key, DST_TIME_DELETE, &remove); + if (remove <= now && ret == ISC_R_SUCCESS) { + dns_ttl_t key_ttl = dst_key_getttl(key->key); + key_ttl += dns_kasp_zonepropagationdelay(kasp); + if ((remove + key_ttl) <= now) { + dnskey_state = HIDDEN; + } else { + dnskey_state = UNRETENTIVE; + } + zrrsig_state = HIDDEN; + ds_state = HIDDEN; + goal_state = HIDDEN; + } + + /* Set goal if not already set. */ + if (dst_key_getstate(key->key, DST_KEY_GOAL, &goal_state) != + ISC_R_SUCCESS) { + dst_key_setstate(key->key, DST_KEY_GOAL, goal_state); } /* Set key states for all keys that do not have them. */ From 82632fa6d92ef37508fc77a14255e4d1ff12e09d Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 2 Feb 2021 20:02:54 +0100 Subject: [PATCH 3/3] Remove initialize goal code Since keys now have their goals initialized in 'keymgr_key_init()', remove this redundant piece of code in 'keymgr_key_run()'. --- lib/dns/keymgr.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index 170eca37f4..fccd3c68f4 100644 --- a/lib/dns/keymgr.c +++ b/lib/dns/keymgr.c @@ -1842,19 +1842,6 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, continue; } - /* - * This is possibly an active key created - * outside dnssec-policy. Initialize goal, - * if not set. - */ - dst_key_state_t goal; - if (dst_key_getstate(dkey->key, DST_KEY_GOAL, - &goal) != ISC_R_SUCCESS) { - dst_key_setstate(dkey->key, - DST_KEY_GOAL, - OMNIPRESENT); - } - /* * Save the matched key only if it is active * or desires to be active.