From fb0498a7f2f18dbe43917a5c6d08a01c660fb631 Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 11:32:15 -0400 Subject: [PATCH 01/11] canonname -> getnameinfo Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index 8547c11e5..e71835bf1 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -47,7 +47,12 @@ def hex_to_dec_netmask(netmask) # server), and the method should return the hostname and not the IP address. # def canonicalize_hostname(hostname) - Addrinfo.getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME).first.canonname + # canonname does not fully qualify the hostname if on Windows node that + # is not joined to a domain, but getnameinfo does. + Addrinfo. + getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME). + first&. + getnameinfo&.[](0) || hostname end def canonicalize_hostname_with_retries(hostname) From 0a251950861c3cf9580de8cf342e75347c7e957d Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 12:29:36 -0400 Subject: [PATCH 02/11] Leave getaddrinfo canonname in Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index e71835bf1..f51e4f323 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -47,12 +47,15 @@ def hex_to_dec_netmask(netmask) # server), and the method should return the hostname and not the IP address. # def canonicalize_hostname(hostname) + ai = Addrinfo. + getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME). + first + + return ai&.canonname if ai&.canonname != hostname + # canonname does not fully qualify the hostname if on Windows node that # is not joined to a domain, but getnameinfo does. - Addrinfo. - getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME). - first&. - getnameinfo&.[](0) || hostname + ai&.getnameinfo&.[](0) || hostname end def canonicalize_hostname_with_retries(hostname) From 1621300c4cc1785290ddeb133e3f91409ab0a69e Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 12:42:22 -0400 Subject: [PATCH 03/11] Lint for . position Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index f51e4f323..e9a61b64f 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -47,11 +47,12 @@ def hex_to_dec_netmask(netmask) # server), and the method should return the hostname and not the IP address. # def canonicalize_hostname(hostname) - ai = Addrinfo. - getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME). - first + ai = Addrinfo + .getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME) + .first - return ai&.canonname if ai&.canonname != hostname + # use canonname + return ai&.canonname if (ai&.canonname != hostname || !ChefUtils.windows?) # canonname does not fully qualify the hostname if on Windows node that # is not joined to a domain, but getnameinfo does. From a3ce7d8aecbb36d752a7e400d2eddaf311e7f5fa Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 14:43:53 -0400 Subject: [PATCH 04/11] A couple of tweaks for the brittle unit test Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 3 ++- spec/unit/mixin/network_helper_spec.rb | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index e9a61b64f..0527807c3 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -51,8 +51,9 @@ def canonicalize_hostname(hostname) .getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME) .first + canonname = ai&.canonname # use canonname - return ai&.canonname if (ai&.canonname != hostname || !ChefUtils.windows?) + return canonname if (canonname != hostname || !ChefUtils.windows?) # canonname does not fully qualify the hostname if on Windows node that # is not joined to a domain, but getnameinfo does. diff --git a/spec/unit/mixin/network_helper_spec.rb b/spec/unit/mixin/network_helper_spec.rb index 4a2c4cbfc..8a57f52d9 100644 --- a/spec/unit/mixin/network_helper_spec.rb +++ b/spec/unit/mixin/network_helper_spec.rb @@ -36,6 +36,7 @@ addrinfo = instance_double(Addrinfo) expect(Addrinfo).to receive(:getaddrinfo).with(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME).and_return([addrinfo]) expect(addrinfo).to receive(:canonname).and_return(hostname) + expect(addrinfo).to receive(:getnameinfo).and_return([hostname, "0"]) if windows? expect(mixin.canonicalize_hostname(hostname)).to eql(hostname) end end From 110217ddbf543c01e97420a8585b19373189e89b Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 14:47:52 -0400 Subject: [PATCH 05/11] Linter doesn't like my if Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index 0527807c3..8ae307a75 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -53,7 +53,7 @@ def canonicalize_hostname(hostname) canonname = ai&.canonname # use canonname - return canonname if (canonname != hostname || !ChefUtils.windows?) + return canonname if canonname != hostname || !ChefUtils.windows? # canonname does not fully qualify the hostname if on Windows node that # is not joined to a domain, but getnameinfo does. From 6bde4b19708a9e5bd3b4857bffafd0464055f3b5 Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 19:42:38 -0400 Subject: [PATCH 06/11] Refactor and tweak canonicalize_hostname logic Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index 8ae307a75..005952685 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -19,6 +19,7 @@ # require "socket" unless defined?(Socket) +require "resolv" unless defined?(Resolv) module Ohai module Mixin @@ -37,6 +38,11 @@ def hex_to_dec_netmask(netmask) dec end + def ip?(hostname) + !!(canonname =~ Resolv::IPv4::Regex) || !!(canonname =~ Resolv::IPv6::Regex) + + end + # This does a forward and reverse lookup on the hostname to return what should be # the FQDN for the host determined by name lookup (generally DNS). If the forward # lookup fails this will throw. If the reverse lookup fails this will return the @@ -52,12 +58,21 @@ def canonicalize_hostname(hostname) .first canonname = ai&.canonname - # use canonname - return canonname if canonname != hostname || !ChefUtils.windows? + # use canonname if it's an FQDN + # This API is preferred as it never gives us an IP address for broken DNS + # (see https://github.com/chef/ohai/pull/1705) + # However, we have found that Windows hosts that are not joined to a domain + # can return a non-qualified hostname) + return canonname unless ip?(canonname) + + # If we got a non-qualified name, then we do a standard reverse resolve + # which, assuming DNS is working, will work around that windows bug + # (and maybe others) + canonname = ai&.getnameinfo&.first + return canonname unless ip?(canonname) - # canonname does not fully qualify the hostname if on Windows node that - # is not joined to a domain, but getnameinfo does. - ai&.getnameinfo&.[](0) || hostname + # if all else fails, return the name we were given as a safety + hostname end def canonicalize_hostname_with_retries(hostname) From 8ec28c00e140d7f5e8b8e7c584b7f14c4a789c18 Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 20:38:48 -0400 Subject: [PATCH 07/11] Replace ip? for getnameinfo, but look for dot for canonname Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index 005952685..8d5757d08 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -38,9 +38,10 @@ def hex_to_dec_netmask(netmask) dec end + # Addrinfo#ip*? methods return true on AI_CANONNAME Addrinfo records that match + # the ipv* scheme and #ip? always returns true unless a non tcp Addrinfo def ip?(hostname) !!(canonname =~ Resolv::IPv4::Regex) || !!(canonname =~ Resolv::IPv6::Regex) - end # This does a forward and reverse lookup on the hostname to return what should be @@ -62,8 +63,9 @@ def canonicalize_hostname(hostname) # This API is preferred as it never gives us an IP address for broken DNS # (see https://github.com/chef/ohai/pull/1705) # However, we have found that Windows hosts that are not joined to a domain - # can return a non-qualified hostname) - return canonname unless ip?(canonname) + # can return a non-qualified hostname). + # Use a '.' in the canonname as indicator of FQDN + return canonname if canonname =~ /\./ # If we got a non-qualified name, then we do a standard reverse resolve # which, assuming DNS is working, will work around that windows bug From ffa04af78d4ecbb0e14e6e3f07b960cbb3e69d5c Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Tue, 3 Oct 2023 20:43:35 -0400 Subject: [PATCH 08/11] Linter for regex match? Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index 8d5757d08..202e9002b 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -41,7 +41,7 @@ def hex_to_dec_netmask(netmask) # Addrinfo#ip*? methods return true on AI_CANONNAME Addrinfo records that match # the ipv* scheme and #ip? always returns true unless a non tcp Addrinfo def ip?(hostname) - !!(canonname =~ Resolv::IPv4::Regex) || !!(canonname =~ Resolv::IPv6::Regex) + Resolv::IPv4::Regex.match?(hostname) || Resolv::IPv6::Regex.match?(hostname) end # This does a forward and reverse lookup on the hostname to return what should be @@ -65,7 +65,7 @@ def canonicalize_hostname(hostname) # However, we have found that Windows hosts that are not joined to a domain # can return a non-qualified hostname). # Use a '.' in the canonname as indicator of FQDN - return canonname if canonname =~ /\./ + return canonname if /\./.match?(canonname) # If we got a non-qualified name, then we do a standard reverse resolve # which, assuming DNS is working, will work around that windows bug From be77e967275d4afb17006fbee92a2566b4cf3fa6 Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Wed, 4 Oct 2023 08:21:20 -0400 Subject: [PATCH 09/11] Use include for '.' search Signed-off-by: Thomas Powell --- lib/ohai/mixin/network_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ohai/mixin/network_helper.rb b/lib/ohai/mixin/network_helper.rb index 202e9002b..c650674b4 100644 --- a/lib/ohai/mixin/network_helper.rb +++ b/lib/ohai/mixin/network_helper.rb @@ -65,7 +65,7 @@ def canonicalize_hostname(hostname) # However, we have found that Windows hosts that are not joined to a domain # can return a non-qualified hostname). # Use a '.' in the canonname as indicator of FQDN - return canonname if /\./.match?(canonname) + return canonname if canonname.include?(".") # If we got a non-qualified name, then we do a standard reverse resolve # which, assuming DNS is working, will work around that windows bug From dd2a99fd56cf21673d1c779e27328a0d2debcf0f Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Wed, 4 Oct 2023 11:09:44 -0400 Subject: [PATCH 10/11] Added detailed set of tests for canonicalize_hostname Signed-off-by: Thomas Powell --- spec/unit/mixin/network_helper_spec.rb | 56 +++++++++++++++++++++----- 1 file changed, 47 insertions(+), 9 deletions(-) diff --git a/spec/unit/mixin/network_helper_spec.rb b/spec/unit/mixin/network_helper_spec.rb index 8a57f52d9..9fa33681b 100644 --- a/spec/unit/mixin/network_helper_spec.rb +++ b/spec/unit/mixin/network_helper_spec.rb @@ -29,15 +29,53 @@ end describe "canonicalize hostname" do - # this is a brittle test deliberately intended to discourage modifying this API - # (see the node in the code on the necessity of manual testing) - it "uses the correct ruby API" do - hostname = "broken.example.org" - addrinfo = instance_double(Addrinfo) - expect(Addrinfo).to receive(:getaddrinfo).with(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME).and_return([addrinfo]) - expect(addrinfo).to receive(:canonname).and_return(hostname) - expect(addrinfo).to receive(:getnameinfo).and_return([hostname, "0"]) if windows? - expect(mixin.canonicalize_hostname(hostname)).to eql(hostname) + [ + { + desc: "canonname == initial hostname returns those", + initial_hostname: "fullhostname.example.com", + canonname: "fullhostname.example.com", + final_hostname: "fullhostname.example.com", + }, + { + desc: "canonname(hostname) => fqdn returns fqdn", + initial_hostname: "hostnamepart", + canonname: "hostnamepart.example.com", + final_hostname: "hostnamepart.example.com", + }, + { + desc: "hostname only canonname, getnameinfo is tried and succeeds", + initial_hostname: "hostnamepart2", + canonname: "hostnamepart2", + nameinfo: "hostnamepart2.example.com", + final_hostname: "hostnamepart2.example.com", + }, + { + desc: "hostname only canonname, getnameinfo returns ip => original hostname", + initial_hostname: "hostnameip.example.com", + canonname: "hostnameip", # totally contrived + nameinfo: "192.168.1.1", + final_hostname: "hostnameip.example.com", + } + ].each do |example_hash| + # this is a brittle set of tests deliberately intended to discourage modifying + # this API (see the note in the code on the necessity of manual testing) + example example_hash[:desc] do + addrinfo = instance_double(Addrinfo) + expect(Addrinfo).to receive(:getaddrinfo) + .with(example_hash[:initial_hostname], nil, nil, nil, nil, Socket::AI_CANONNAME) + .and_return([addrinfo]) + expect(addrinfo).to receive(:canonname).and_return(example_hash[:canonname]) + + # only expect this call if :nameinfo key is set, otherwise code should not + # fall through to getnameinfo + if example_hash[:nameinfo] + expect(addrinfo).to receive(:getnameinfo).and_return([example_hash[:nameinfo], "0"]) + end + + # the actual input and output for #canonicalize_hostname method + expect(mixin.canonicalize_hostname(example_hash[:initial_hostname])) + .to eql(example_hash[:final_hostname]) + end end end end From bd8d95294321f7cccc34fba7830715132c2e3cab Mon Sep 17 00:00:00 2001 From: Thomas Powell Date: Wed, 4 Oct 2023 11:20:33 -0400 Subject: [PATCH 11/11] Lint likes trailing commas Signed-off-by: Thomas Powell --- spec/unit/mixin/network_helper_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/mixin/network_helper_spec.rb b/spec/unit/mixin/network_helper_spec.rb index 9fa33681b..0a0bd4cb2 100644 --- a/spec/unit/mixin/network_helper_spec.rb +++ b/spec/unit/mixin/network_helper_spec.rb @@ -55,7 +55,7 @@ canonname: "hostnameip", # totally contrived nameinfo: "192.168.1.1", final_hostname: "hostnameip.example.com", - } + }, ].each do |example_hash| # this is a brittle set of tests deliberately intended to discourage modifying # this API (see the note in the code on the necessity of manual testing)