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..134e2c43ddd4 --- /dev/null +++ b/changelog/unreleased/kong/fix-dns-ignore-records-with-non-matching-types.yml @@ -0,0 +1,3 @@ +message: "**DNS Client**: Fixed an issue where the Kong DNS client stored records with non-matching domain and type when parsing answers. It now ignores records when the RR type differs from that of the query when parsing answers and disables the `ADDITIONAL SECTION` in DNS responses." +type: bugfix +scope: Core diff --git a/kong/resty/dns/client.lua b/kong/resty/dns/client.lua index e5fce8b97ff0..075808cd4205 100644 --- a/kong/resty/dns/client.lua +++ b/kong/resty/dns/client.lua @@ -52,7 +52,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 @@ -672,15 +671,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 @@ -697,25 +694,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 @@ -1149,7 +1131,6 @@ end -- @param qname Name to resolve -- @param r_opts Options table, see remark about the `qtype` field above and -- [OpenResty docs](https://github.com/openresty/lua-resty-dns) for more options. --- The field `additional_section` will default to `true` instead of `false`. -- @param dnsCacheOnly Only check the cache, won't do server lookups -- @param try_list (optional) list of tries to add to -- @param force_no_sync force noSynchronisation @@ -1164,10 +1145,8 @@ local function resolve(qname, r_opts, dnsCacheOnly, try_list, force_no_sync) for k,v in pairs(r_opts) do opts[k] = v end -- copy the options table end - -- default the ADDITIONAL SECTION to TRUE - if opts.additional_section == nil then - opts.additional_section = true - end + -- avoid parsing ip addresses that do not match the qname + opts.additional_section = nil -- first check for shortname in the cache -- we do this only to prevent iterating over the SEARCH directive and 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 9051580d9646..a90ed18669f5 100644 --- a/spec/01-unit/21-dns-client/02-client_spec.lua +++ b/spec/01-unit/21-dns-client/02-client_spec.lua @@ -790,29 +790,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.is_nil(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.is_nil(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() @@ -845,17 +845,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.is_nil(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() @@ -1936,4 +1937,107 @@ describe("[DNS client]", function() assert.equal(call_count, 10) end) + it("disable additional section when querying", function() + + local function build_dns_reply(id, name, ip, ns_ip1, ns_ip2) + local function dns_encode_name(name) + local parts = {} + for part in string.gmatch(name, "[^.]+") do + table.insert(parts, string.char(#part) .. part) + end + table.insert(parts, "\0") + return table.concat(parts) + end + + local function ip_to_bytes(ip) + local bytes = { "\x00\x04" } -- RDLENGTH:4bytes (ipv4) + for octet in string.gmatch(ip, "%d+") do + table.insert(bytes, string.char(tonumber(octet))) + end + return table.concat(bytes) + end + + local package = {} + + -- Header + package[#package+1] = id + package[#package+1] = "\x85\x00" -- QR, AA, RD + package[#package+1] = "\x00\x01\x00\x01\x00\x00\x00\x02" -- QD:1 AN:1 NS:0 AR:2 + + -- Question + package[#package+1] = dns_encode_name(name) + package[#package+1] = "\x00\x01\x00\x01" -- QTYPE A; QCLASS IN + + -- Answer + package[#package+1] = dns_encode_name(name) + package[#package+1] = "\x00\x01\x00\x01\x00\x00\x00\x30" -- QTYPE:A; QCLASS:IN TTL:48 + package[#package+1] = ip_to_bytes(ip) + + -- Additional + local function add_additional(name, ip) + package[#package+1] = dns_encode_name(name) + package[#package+1] = "\x00\x01\x00\x01\x00\x00\x00\x30" -- QTYPE:A; QCLASS:IN TTL:48 + package[#package+1] = ip_to_bytes(ip) + end + + add_additional("ns1." .. name, ns_ip1) + add_additional("ns2." .. name, ns_ip2) + + return table.concat(package) + end + + local force_enable_additional_section = false + + -- dns client will ignore additional section + resolver.query_func = function(self, original_query_func, name, options) + if options.qtype ~= client.TYPE_A then + return { errcode = 5, errstr = "refused" } + end + + if force_enable_additional_section then + options.additional_section = true + end + + self.tcp_sock = nil -- disable TCP query + + local id + local sock = assert(self.socks[1]) + -- hook send to get id + local orig_sock_send = sock.send + sock.send = function (self, query) + id = query[1] .. query[2] + return orig_sock_send(self, query) + end + -- hook receive to reply raw data + sock.receive = function (self, size) + return build_dns_reply(id, name, "1.1.1.1", "2.2.2.2", "3.3.3.3") + end + + return original_query_func(self, name, options) + end + + local name = "additional-section.test" + + -- no additional_section by default + assert(client.init({ search = {}, })) + local answers = client.resolve(name) + assert.equal(#answers, 1) + assert.same(answers[1].address, "1.1.1.1") + + -- disable the additional_section option in r_opt + assert(client.init({ search = {}, })) + local answers = client.resolve(name, { additional_section = true }) + assert.equal(#answers, 1) + assert.same(answers[1].address, "1.1.1.1") + + -- test the buggy scenario + force_enable_additional_section = true + assert(client.init({ search = {}, })) + local answers = client.resolve(name) + assert.equal(#answers, 3) + assert.same(answers[1].address, "1.1.1.1") + assert.same(answers[2].address, "2.2.2.2") + assert.same(answers[3].address, "3.3.3.3") + end) + end) 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 2857cb7f9401..e7d3e976ceb6 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 @@ -409,13 +409,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", @@ -424,8 +426,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 @@ -436,11 +438,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() + + resolver.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) @@ -526,9 +632,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()