From 8317e481de3cf19e7ccf849c4df7e1a08d9d7d73 Mon Sep 17 00:00:00 2001 From: Xiaochen Wang Date: Tue, 14 May 2024 18:03:12 +0800 Subject: [PATCH] fix(dns): ignore records with RR types differs from that of the query when parsing answers (#13002) Fix KAG-4446, FTI-5834 --- ...ignore-records-with-non-matching-types.yml | 3 + kong/resty/dns/client.lua | 30 +---- spec/01-unit/21-dns-client/02-client_spec.lua | 37 +++--- .../21-dns-client/03-client_cache_spec.lua | 122 +++++++++++++++++- 4 files changed, 145 insertions(+), 47 deletions(-) create mode 100644 changelog/unreleased/kong/fix-dns-ignore-records-with-non-matching-types.yml diff --git a/changelog/unreleased/kong/fix-dns-ignore-records-with-non-matching-types.yml b/changelog/unreleased/kong/fix-dns-ignore-records-with-non-matching-types.yml new file mode 100644 index 000000000000..1cfb60d3dc64 --- /dev/null +++ b/changelog/unreleased/kong/fix-dns-ignore-records-with-non-matching-types.yml @@ -0,0 +1,3 @@ +message: "**DNS Client**: Ignore records with RR types differs from that of the query when parsing answers." +type: bugfix +scope: Core diff --git a/kong/resty/dns/client.lua b/kong/resty/dns/client.lua index 03625790ee58..874515badeb1 100644 --- a/kong/resty/dns/client.lua +++ b/kong/resty/dns/client.lua @@ -45,7 +45,6 @@ local math_max = math.max local math_fmod = math.fmod local math_random = math.random local table_remove = table.remove -local table_insert = table.insert local table_concat = table.concat local string_lower = string.lower local string_byte = string.byte @@ -666,15 +665,13 @@ _M.init = function(options) end --- Removes non-requested results, updates the cache. +-- Removes records with non-matching types, updates the cache. -- Parameter `answers` is updated in-place. -- @return `true` local function parseAnswer(qname, qtype, answers, try_list) - -- check the answers and store them in the cache + -- check the answers and store records with matching types in the cache -- eg. A, AAAA, SRV records may be accompanied by CNAME records - -- store them all, leaving only the requested type in so we can return that set - local others = {} -- remove last '.' from FQDNs as the answer does not contain it local check_qname do @@ -691,25 +688,10 @@ local function parseAnswer(qname, qtype, answers, try_list) -- normalize casing answer.name = string_lower(answer.name) - if (answer.type ~= qtype) or (answer.name ~= check_qname) then - local key = answer.type..":"..answer.name - add_status_to_try_list(try_list, key .. " removed") - local lst = others[key] - if not lst then - lst = {} - others[key] = lst - end - table_insert(lst, 1, answer) -- pos 1: preserve order - table_remove(answers, i) - end - end - if next(others) then - for _, lst in pairs(others) do - cacheinsert(lst) - -- set success-type, only if not set (this is only a 'by-product') - if not cachegetsuccess(lst[1].name) then - cachesetsuccess(lst[1].name, lst[1].type) - end + if answer.type ~= qtype then + table_remove(answers, i) -- remove records with non-matching types + else + answer.name = check_qname end end diff --git a/spec/01-unit/21-dns-client/02-client_spec.lua b/spec/01-unit/21-dns-client/02-client_spec.lua index 55342d5d0189..622d59f07617 100644 --- a/spec/01-unit/21-dns-client/02-client_spec.lua +++ b/spec/01-unit/21-dns-client/02-client_spec.lua @@ -784,29 +784,29 @@ describe("[DNS client]", function() -- check first CNAME local key1 = client.TYPE_CNAME..":"..host local entry1 = lrucache:get(key1) - assert.are.equal(host, entry1[1].name) -- the 1st record is the original 'smtp.'..TEST_DOMAIN - assert.are.equal(client.TYPE_CNAME, entry1[1].type) -- and that is a CNAME + assert.falsy(entry1) -- check second CNAME - local key2 = client.TYPE_CNAME..":"..entry1[1].cname + local key2 = client.TYPE_CNAME..":thuis.kong-gateway-testing.link" local entry2 = lrucache:get(key2) - assert.are.equal(entry1[1].cname, entry2[1].name) -- the 2nd is the middle 'thuis.'..TEST_DOMAIN - assert.are.equal(client.TYPE_CNAME, entry2[1].type) -- and that is also a CNAME + assert.falsy(entry2) - -- check second target to match final record - assert.are.equal(entry2[1].cname, answers[1].name) - assert.are.not_equal(host, answers[1].name) -- we got final name 'wdnaste.duckdns.org' - assert.are.equal(typ, answers[1].type) -- we got a final A type record + assert.are.equal(host, answers[1].name) -- we got final name same to host + assert.are.equal(typ, answers[1].type) -- we got a final A type record assert.are.equal(#answers, 1) -- check last successful lookup references local lastsuccess3 = lrucache:get(answers[1].name) - local lastsuccess2 = lrucache:get(entry2[1].name) - local lastsuccess1 = lrucache:get(entry1[1].name) + local lastsuccess2 = lrucache:get("thuis.kong-gateway-testing.link") + local lastsuccess1 = lrucache:get("kong-gateway-testing.link") assert.are.equal(client.TYPE_A, lastsuccess3) - assert.are.equal(client.TYPE_CNAME, lastsuccess2) - assert.are.equal(client.TYPE_CNAME, lastsuccess1) + assert.is_nil(lastsuccess2) + assert.is_nil(lastsuccess1) + -- check entries in the intermediate cache against the final output result + local key = client.TYPE_A .. ":" .. host + local entry = lrucache:get(key) + assert.same(answers, entry) end) it("fetching multiple SRV records (un-typed)", function() @@ -839,17 +839,18 @@ describe("[DNS client]", function() -- first check CNAME local key = client.TYPE_CNAME..":"..host local entry = lrucache:get(key) - assert.are.equal(host, entry[1].name) - assert.are.equal(client.TYPE_CNAME, entry[1].type) + assert.falsy(entry) -- check final target - assert.are.equal(entry[1].cname, answers[1].name) assert.are.equal(typ, answers[1].type) - assert.are.equal(entry[1].cname, answers[2].name) assert.are.equal(typ, answers[2].type) - assert.are.equal(entry[1].cname, answers[3].name) assert.are.equal(typ, answers[3].type) assert.are.equal(#answers, 3) + + -- check entries in the intermediate cache against the final output result + key = client.TYPE_SRV .. ":" .. host + entry = lrucache:get(key) + assert.same(answers, entry) end) it("fetching non-type-matching records", function() diff --git a/spec/01-unit/21-dns-client/03-client_cache_spec.lua b/spec/01-unit/21-dns-client/03-client_cache_spec.lua index 2aa034b7113d..159d049a94d6 100644 --- a/spec/01-unit/21-dns-client/03-client_cache_spec.lua +++ b/spec/01-unit/21-dns-client/03-client_cache_spec.lua @@ -402,13 +402,15 @@ describe("[DNS client cache]", function() -- in the cache, as in some cases lookups of certain types (eg. CNAME) are -- blocked, and then we rely on the A record to get them in the AS -- (additional section), but then they must be stored obviously. - local CNAME1 = { + local CNAME_from_A_query = { type = client.TYPE_CNAME, cname = "myotherhost.domain.com", class = 1, name = "myhost9.domain.com", ttl = 0.1, } + -- copy to make it different + local CNAME_from_CNAME_query = utils.cycle_aware_deep_copy(CNAME_from_A_query) local A2 = { type = client.TYPE_A, address = "1.2.3.4", @@ -417,8 +419,8 @@ describe("[DNS client cache]", function() ttl = 60, } mock_records = setmetatable({ - ["myhost9.domain.com:"..client.TYPE_CNAME] = { utils.cycle_aware_deep_copy(CNAME1) }, -- copy to make it different - ["myhost9.domain.com:"..client.TYPE_A] = { CNAME1, A2 }, -- not there, just a reference and target + ["myhost9.domain.com:"..client.TYPE_CNAME] = { CNAME_from_CNAME_query }, + ["myhost9.domain.com:"..client.TYPE_A] = { CNAME_from_A_query, A2 }, -- not there, just a reference and target ["myotherhost.domain.com:"..client.TYPE_A] = { A2 }, }, { -- do not do lookups, return empty on anything else @@ -429,11 +431,115 @@ describe("[DNS client cache]", function() }) assert(client.resolve("myhost9", { qtype = client.TYPE_CNAME })) + local cached = lrucache:get(client.TYPE_CNAME..":myhost9.domain.com") + assert.are.equal(CNAME_from_CNAME_query, cached[1]) + assert.are.not_equal(CNAME_from_A_query, cached[1]) + ngx.sleep(0.2) -- wait for it to become stale + assert(client.toip("myhost9")) + -- CNAME entries are not stored into cache as `intermediates` for A type + -- queries, only A entries are stored. + cached = lrucache:get(client.TYPE_A..":myhost9.domain.com") + assert.are.equal(A2, cached[1]) + + -- The original cached CNAME entry will not be replaced by new CNAME entry. + cached = lrucache:get(client.TYPE_CNAME..":myhost9.domain.com") + assert.are.equal(CNAME_from_CNAME_query, cached[1]) + assert.are.not_equal(CNAME_from_A_query, cached[1]) + end) + + it("No RRs match the queried type", function() + mock_records = { + ["myhost9.domain.com:"..client.TYPE_A] = { + { + type = client.TYPE_CNAME, + cname = "myotherhost.domain.com", + class = 1, + name = "myhost9.domain.com", + ttl = 60, + }, + }, + } + + local res, _, try_list = client.resolve("myhost9", {}) + assert.matches(tostring(try_list), '"myhost9.domain.com:1 - cache-miss/querying/dns client error: 101 empty record received"') + assert.is_nil(res) + end) + end) + + describe("fqdn (simple dns_order)", function() + + local lrucache, mock_records, config + before_each(function() + config = { + nameservers = { "198.51.100.0" }, + ndots = 1, + hosts = {}, + resolvConf = {}, + order = { "LAST", "A" }, + badTtl = 0.5, + staleTtl = 0.5, + enable_ipv6 = false, + } + assert(client.init(config)) + lrucache = client.getcache() + + query_func = function(self, original_query_func, qname, opts) + return mock_records[qname..":"..opts.qtype] or { errcode = 3, errstr = "name error" } + end + end) + + -- FTI-5834 + it("only query A type", function() + local CNAME = { + type = client.TYPE_CNAME, + cname = "myotherhost.domain.com", + class = 1, + name = "myhost9.domain.com", + ttl = 60, + } + local A = { + type = client.TYPE_A, + address = "1.2.3.4", + class = 1, + name = "myotherhost.domain.com", + ttl = 60, + } + mock_records = { + ["myhost9.domain.com:"..client.TYPE_A] = { CNAME, A } + } + + local res = client.resolve("myhost9.domain.com") + -- The record's name has been fixed to the queried name. + assert.equal("myhost9.domain.com", res[1].name) + assert.equal(A, res[1]) + + local success = lrucache:get("myhost9.domain.com") + assert.equal(client.TYPE_A, success) + + -- The CNAME records have been removed due to a mismatch with type A. local cached = lrucache:get(client.TYPE_CNAME..":myhost9.domain.com") - assert.are.equal(CNAME1, cached[1]) + assert.is_nil(cached) + end) + + it("No RRs match the queried type", function() + mock_records = { + ["myhost9.domain.com:"..client.TYPE_A] = { + { + type = client.TYPE_CNAME, + cname = "myotherhost.domain.com", + class = 1, + name = "myhost9.domain.com", + ttl = 60, + }, + }, + } + + local res, err = client.resolve("myhost9.domain.com", {}) + assert.equal('dns client error: 101 empty record received', err) + assert.is_nil(res) end) end) @@ -519,9 +625,15 @@ describe("[DNS client cache]", function() }, } } + client.toip("demo.service.consul") + + -- It only stores SRV target entry into cache, not storing A entry. local success = client.getcache():get("192.168.5.232.node.api_test.consul") - assert.equal(client.TYPE_A, success) + assert.is_nil(success) + + success = client.getcache():get("demo.service.consul") + assert.equal(client.TYPE_SRV, success) end) it("are not overwritten by add. section info", function()