From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Sat, 19 Jan 2019 13:01:34 +0000 (+0100)
Subject: trust anchors: improve .add()
X-Git-Tag: v4.0.0~46^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c5b665c22a396608eb515757c6d9718c2d91e096;p=thirdparty%2Fknot-resolver.git

trust anchors: improve .add()

These keys will now be more uniformly represented and thus also
shown by .summary().  It's still not perfectly synchronized when
that function fails, but that seems acceptable.
---

diff --git a/NEWS b/NEWS
index f4d195795..e4899599e 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,7 @@ Bugfixes
 - predict module: load stats module if config didn't specify period (!755)
 - trust_anchors: don't do 5011-style updates on anchors from files
   that were loaded as unmanaged trust anchors (!753)
+- trust_anchors.add(): include these TAs in .summary() (!753)
 
 
 Knot Resolver 3.2.1 (2019-01-10)
diff --git a/daemon/lua/trust_anchors.lua.in b/daemon/lua/trust_anchors.lua.in
index 7bc330b7e..e8499a015 100644
--- a/daemon/lua/trust_anchors.lua.in
+++ b/daemon/lua/trust_anchors.lua.in
@@ -437,16 +437,20 @@ local function keyset_read(path, string)
 end
 
 -- Replace current TAs for given owner by the "trusted" ones from passed keyset.
--- Return the number of trusted keys for the owner.
+-- Return true iff no TA errored out and at least one is in VALID state.
 local function keyset_publish(keyset)
 	local store = kres.context().trust_anchors
 	local count = 0
+	local has_error = false
 	C.kr_ta_del(store, keyset.owner)
 	for _, ta in ipairs(keyset) do
 		-- Key MAY be used as a TA only in these two states (RFC5011, 4.2)
 		if ta.state == key_state.Valid or ta.state == key_state.Missing then
 			if C.kr_ta_add(store, ta.owner, ta.type, ta.ttl, ta.rdata, #ta.rdata) == 0 then
 				count = count + 1
+			else
+				ta.state = 'ERROR'
+				has_error = true
 			end
 		end
 	end
@@ -454,7 +458,7 @@ local function keyset_publish(keyset)
 		warn('[ ta ] ERROR: no anchors are trusted for ' ..
 		     kres.dname2str(keyset.owner) .. ' !')
 	end
-	return count
+	return count > 0 and not has_error
 end
 
 
@@ -507,7 +511,7 @@ update = function (keyset, new_keys, is_initial)
 	keyset_write(keyset)
 
 	-- Start using the new TAs.
-	if keyset_publish(keyset) == 0 then
+	if not keyset_publish(keyset) then
 		-- TODO: try to rebootstrap if for root?
 		return false
 	elseif verbose() then
@@ -569,7 +573,7 @@ local add_file = function (path, unmanaged)
 	trust_anchors.keysets[owner] = keyset
 
 	-- Replace the TA store used for validation
-	if keyset_publish(keyset) ~= 0 and verbose() then
+	if keyset_publish(keyset) and verbose() then
 		log('[ ta ] installed trust anchors for domain ' .. owner_str .. ' are:\n'
 		    .. trust_anchors.summary(owner))
 	end
@@ -631,13 +635,37 @@ trust_anchors = {
 	config = add_file,
 
 	-- Add DS/DNSKEY record(s) (unmanaged)
-	-- FIXME: this function won't update the .keysets,
-	-- so it won't e.g. be shown by .summary() - confusing.
 	add = function (keystr)
-		local ret = trustanchor(keystr)
-		if verbose() then log(trust_anchors.summary()) end
-		return ret
+		local keyset, err = keyset_read(nil, keystr)
+		if keyset ~= nil then
+			local owner = keyset.owner
+			local owner_str = kres.dname2str(owner)
+			local keyset_orig = trust_anchors.keysets[owner]
+			-- Set up trust_anchors.keysets[owner]
+			if keyset_orig then
+				warn('[ ta ] warning: extending previously set trust anchors for '
+						.. owner_str)
+				for _, ta in ipairs(keyset) do
+					table.insert(keyset_orig, ta)
+				end
+				-- we might also add more warning if it's managed, i.e. has .filename,
+				-- as the next update would overwrite this additional TA
+			else
+				trust_anchors.keysets[owner] = keyset
+			end
+			-- Replace the TA store used for validation
+			if not keyset_publish(keyset) then
+				err = "when publishing the TA set"
+				-- trust_anchors.keysets[owner] was already updated to the
+				-- (partially) failing state, but I'm not sure how much to improve this
+			end
+		end
+		if verbose() or err then log('New TA state:\n' .. trust_anchors.summary()) end
+		if err then
+			panic('[ ta ] .add() failed: ' .. err)
+		end
 	end,
+
 	-- Negative TA management
 	set_insecure = function (list)
 		assert(type(list) == 'table', 'parameter must be list of domain names (e.g. {"a.test", "b.example"})')
diff --git a/daemon/lua/trust_anchors.test/ta.test.lua b/daemon/lua/trust_anchors.test/ta.test.lua
index bdd00fcb9..97da60da5 100644
--- a/daemon/lua/trust_anchors.test/ta.test.lua
+++ b/daemon/lua/trust_anchors.test/ta.test.lua
@@ -13,7 +13,6 @@ local function test_revoked_key()
 		.. 'vN9dlzEheX7ICJBBtuA6G3LQpzW5hOA2hzCTMjJPJ8LbqF6dsV6DoBQzgul0sGIcGOYl7O'
 		.. 'yQdXfZ57relSQageu+ipAdTTJ25AsRTAoub8ONGcLmqrAmRLKBP1dfwhYB4N7knNnulqQxA+Uk1ihz0='
 	boom(trust_anchors.add, { '. 3600 DNSKEY 385 3 8 ' .. key_crypto }, 'refuse revoked key')
-	same(#trust_anchors.keysets, 0, 'no keysets')
 	same(ffi.C.kr_ta_get(ta_c, '\0') == nil, true, 'no TA for root is used')
 	-- Test that we don't have another problem in the key
 	trust_anchors.add('. 3600 DNSKEY 257 3 8 ' .. key_crypto)