From e0bdff7ecd1040c5eb74fbfdd648c22e5902f297 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 3 Mar 2020 06:58:45 +0100 Subject: [PATCH 1/2] Fix race condition dnssec-policy with views When configuring the same dnssec-policy for two zones with the same name but in different views, there is a race condition for who will run the keymgr first. If running sequential only one set of keys will be created, if running parallel two set of keys will be created. Lock the kasp when running looking for keys and running the key manager. This way, for the same zone in different views only one keyset will be created. The dnssec-policy does not implement sharing keys between different zones. --- bin/tests/system/kasp/ns4/example1.db.in | 22 +++++++++++++++ bin/tests/system/kasp/ns4/example2.db.in | 22 +++++++++++++++ bin/tests/system/kasp/ns4/named.conf.in | 28 ++++++++++++++++++ bin/tests/system/kasp/ns4/setup.sh | 3 ++ bin/tests/system/kasp/tests.sh | 36 ++++++++++++++++++++++++ lib/dns/kasp.c | 1 + lib/dns/zone.c | 14 +++++++-- 7 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 bin/tests/system/kasp/ns4/example1.db.in create mode 100644 bin/tests/system/kasp/ns4/example2.db.in diff --git a/bin/tests/system/kasp/ns4/example1.db.in b/bin/tests/system/kasp/ns4/example1.db.in new file mode 100644 index 0000000000..ea4a911f07 --- /dev/null +++ b/bin/tests/system/kasp/ns4/example1.db.in @@ -0,0 +1,22 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; This Source Code Form is subject to the terms of the Mozilla Public +; License, v. 2.0. If a copy of the MPL was not distributed with this +; file, You can obtain one at http://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +$TTL 300 +@ IN SOA mname1. . ( + 1 ; serial + 20 ; refresh (20 seconds) + 20 ; retry (20 seconds) + 1814400 ; expire (3 weeks) + 3600 ; minimum (1 hour) + ) + + NS ns4 +ns4 A 10.53.0.4 + +view TXT "view1" diff --git a/bin/tests/system/kasp/ns4/example2.db.in b/bin/tests/system/kasp/ns4/example2.db.in new file mode 100644 index 0000000000..90004d3391 --- /dev/null +++ b/bin/tests/system/kasp/ns4/example2.db.in @@ -0,0 +1,22 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; This Source Code Form is subject to the terms of the Mozilla Public +; License, v. 2.0. If a copy of the MPL was not distributed with this +; file, You can obtain one at http://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +$TTL 300 +@ IN SOA mname1. . ( + 1 ; serial + 20 ; refresh (20 seconds) + 20 ; retry (20 seconds) + 1814400 ; expire (3 weeks) + 3600 ; minimum (1 hour) + ) + + NS ns4 +ns4 A 10.53.0.4 + +view TXT "view2" diff --git a/bin/tests/system/kasp/ns4/named.conf.in b/bin/tests/system/kasp/ns4/named.conf.in index c8d4094f85..8d209aef63 100644 --- a/bin/tests/system/kasp/ns4/named.conf.in +++ b/bin/tests/system/kasp/ns4/named.conf.in @@ -26,6 +26,16 @@ key "sha256" { secret "R16NojROxtxH/xbDl//ehDsHm5DjWTQ2YXV+hGC2iBY="; }; +key "keyforview1" { + algorithm "hmac-sha1"; + secret "YPfMoAk6h+3iN8MDRQC004iSNHY="; +}; + +key "keyforview2" { + algorithm "hmac-sha1"; + secret "4xILSZQnuO1UKubXHkYUsvBRPu8="; +}; + dnssec-policy "test" { keys { csk key-directory lifetime 0 algorithm 14; @@ -115,3 +125,21 @@ view "none" { file "none.none.signed.db"; }; }; + +view "example1" { + match-clients { key "keyforview1"; }; + + zone "example.net" { + type master; + file "example1.db"; + }; +}; + +view "example2" { + match-clients { key "keyforview2"; }; + + zone "example.net" { + type master; + file "example2.db"; + }; +}; diff --git a/bin/tests/system/kasp/ns4/setup.sh b/bin/tests/system/kasp/ns4/setup.sh index ca830dd028..4b02fd3fa0 100644 --- a/bin/tests/system/kasp/ns4/setup.sh +++ b/bin/tests/system/kasp/ns4/setup.sh @@ -26,3 +26,6 @@ do zonefile="${zone}.db" cp template.db.in $zonefile done + +cp example1.db.in example1.db +cp example2.db.in example2.db diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh index 30b82679a3..24bd5f21ef 100644 --- a/bin/tests/system/kasp/tests.sh +++ b/bin/tests/system/kasp/tests.sh @@ -28,6 +28,8 @@ TSIG="" SHA1="FrSt77yPTFx6hTs4i2tKLB9LmE0=" SHA224="hXfwwwiag2QGqblopofai9NuW28q/1rH4CaTnA==" SHA256="R16NojROxtxH/xbDl//ehDsHm5DjWTQ2YXV+hGC2iBY=" +VIEW1="YPfMoAk6h+3iN8MDRQC004iSNHY=" +VIEW2="4xILSZQnuO1UKubXHkYUsvBRPu8=" ############################################################################### # Key properties # @@ -1799,6 +1801,7 @@ dnssec_verify # ns4/override.none.signed # ns5/override.override.unsigned # ns5/override.none.unsigned +# ns4/example.net (both views) set_keyrole "KEY1" "csk" set_keylifetime "KEY1" "0" set_keyalgorithm "KEY1" "14" "ECDSAP384SHA384" "384" @@ -1850,6 +1853,39 @@ check_apex check_subdomain dnssec_verify +set_keydir "ns4" +set_zone "example.net" +set_server "ns4" "10.53.0.4" +TSIG="hmac-sha1:keyforview1:$VIEW1" +check_keys +check_apex +dnssec_verify +n=$((n+1)) +# check subdomain +echo_i "check TXT example.net (view example1) rrset is signed correctly ($n)" +ret=0 +dig_with_opts "view.${ZONE}" "@${SERVER}" TXT > "dig.out.$DIR.test$n.txt" || log_error "dig view.${ZONE} TXT failed" +grep "status: NOERROR" "dig.out.$DIR.test$n.txt" > /dev/null || log_error "mismatch status in DNS response" +grep "view.${ZONE}\..*${DEFAULT_TTL}.*IN.*TXT.*view1" "dig.out.$DIR.test$n.txt" > /dev/null || log_error "missing view.${ZONE} TXT record in response" +check_signatures TXT "dig.out.$DIR.test$n.txt" "ZSK" +test "$ret" -eq 0 || echo_i "failed" +status=$((status+ret)) + +TSIG="hmac-sha1:keyforview2:$VIEW2" +check_keys +check_apex +dnssec_verify +n=$((n+1)) +# check subdomain +echo_i "check TXT example.net (view example2) rrset is signed correctly ($n)" +ret=0 +dig_with_opts "view.${ZONE}" "@${SERVER}" TXT > "dig.out.$DIR.test$n.txt" || log_error "dig view.${ZONE} TXT failed" +grep "status: NOERROR" "dig.out.$DIR.test$n.txt" > /dev/null || log_error "mismatch status in DNS response" +grep "view.${ZONE}\..*${DEFAULT_TTL}.*IN.*TXT.*view2" "dig.out.$DIR.test$n.txt" > /dev/null || log_error "missing view.${ZONE} TXT record in response" +check_signatures TXT "dig.out.$DIR.test$n.txt" "ZSK" +test "$ret" -eq 0 || echo_i "failed" +status=$((status+ret)) + # Clear TSIG. TSIG="" diff --git a/lib/dns/kasp.c b/lib/dns/kasp.c index 1d9bbb27f4..69c069e5f7 100644 --- a/lib/dns/kasp.c +++ b/lib/dns/kasp.c @@ -90,6 +90,7 @@ destroy(dns_kasp_t *kasp) { } INSIST(ISC_LIST_EMPTY(kasp->keys)); + isc_mutex_destroy(&kasp->lock); isc_mem_free(kasp->mctx, kasp->name); isc_mem_putanddetach(&kasp->mctx, kasp, sizeof(*kasp)); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index ff02549c78..04f4b8e74b 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -7279,6 +7279,7 @@ signed_with_good_key(dns_zone_t *zone, dns_db_t *db, dns_dbnode_t *node, int zsk_count = 0; bool approved; + LOCK(&kasp->lock); for (kkey = ISC_LIST_HEAD(dns_kasp_keys(kasp)); kkey != NULL; kkey = ISC_LIST_NEXT(kkey, link)) { @@ -7289,6 +7290,7 @@ signed_with_good_key(dns_zone_t *zone, dns_db_t *db, dns_dbnode_t *node, zsk_count++; } } + UNLOCK(&kasp->lock); if (type == dns_rdatatype_dnskey || type == dns_rdatatype_cdnskey || type == dns_rdatatype_cds) @@ -19423,6 +19425,10 @@ zone_rekey(dns_zone_t *zone) { kasp = dns_zone_getkasp(zone); fullsign = DNS_ZONEKEY_OPTION(zone, DNS_ZONEKEY_FULLSIGN); + if (kasp != NULL) { + LOCK(&kasp->lock); + } + result = dns_dnssec_findmatchingkeys(&zone->origin, dir, now, mctx, &keys); if (result != ISC_R_SUCCESS) { @@ -19431,9 +19437,9 @@ zone_rekey(dns_zone_t *zone) { isc_result_totext(result)); } - if (kasp && (result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND)) { + if (kasp != NULL && + (result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND)) { ttl = dns_kasp_dnskeyttl(kasp); - result = dns_keymgr_run(&zone->origin, zone->rdclass, dir, mctx, &keys, kasp, now, &nexttime); if (result != ISC_R_SUCCESS) { @@ -19444,6 +19450,10 @@ zone_rekey(dns_zone_t *zone) { } } + if (kasp != NULL) { + UNLOCK(&kasp->lock); + } + if (result == ISC_R_SUCCESS) { result = dns_dnssec_updatekeys(&dnskeys, &keys, &rmkeys, &zone->origin, ttl, &diff, mctx, From 47e42d575027ce62f7e814d6703b45f113d66340 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 3 Mar 2020 07:52:23 +0100 Subject: [PATCH 2/2] Update changes, documentation --- CHANGES | 4 ++++ doc/arm/Bv9ARM-book.xml | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/CHANGES b/CHANGES index fac1565025..a17ba33d5c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5366. [bug] Fix a race condition with the keymgr when the same + zone plus dnssec-policy is configured in multiple + views. [GL #1653] + 5365. [bug] Algorithm rollover was stuck on submitting DS because keymgr thought it would move to an invalid state. Fixed by when checking the current key, diff --git a/doc/arm/Bv9ARM-book.xml b/doc/arm/Bv9ARM-book.xml index a006816a29..108732fb27 100644 --- a/doc/arm/Bv9ARM-book.xml +++ b/doc/arm/Bv9ARM-book.xml @@ -11132,6 +11132,13 @@ example.com CNAME rpz-tcp-only. roll, which cryptographic algorithms to use, and how often RRSIG records need to be refreshed. + + Keys are not shared among zones, which means that one set of keys + per zone will be generated even if they have the same policy. + If multiple views are configured with different versions of the + same zone, each separate version will use the same set of signing + keys. + Multiple key and signing policies can be configured. To attach a policy to a zone, add a dnssec-policy