From 7f69142aa01260bca812250c96a6a566a80e557a Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Fri, 1 Mar 2024 16:52:29 +0000 Subject: [PATCH 1/3] resolv.conf comments have '#' or ';' in the first column When a '#' or ';' appears anywhere else, it's not a comment marker. Signed-off-by: Rob Murray --- libnetwork/internal/resolvconf/resolvconf.go | 14 ++------ libnetwork/resolvconf/resolvconf_unix_test.go | 35 +++++++++++-------- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/libnetwork/internal/resolvconf/resolvconf.go b/libnetwork/internal/resolvconf/resolvconf.go index 6e49c37927..5e9278c48a 100644 --- a/libnetwork/internal/resolvconf/resolvconf.go +++ b/libnetwork/internal/resolvconf/resolvconf.go @@ -452,18 +452,8 @@ func UserModified(rcPath, rcHashPath string) (bool, error) { func (rc *ResolvConf) processLine(line string) { fields := strings.Fields(line) - // Strip comments. - // TODO(robmry) - ignore comment chars except in column 0. - // This preserves old behaviour, but it's wrong. For example, resolvers - // will honour the option in line "options # ndots:0" (and ignore the - // "#" as an unknown option). - for i, s := range fields { - if s[0] == '#' || s[0] == ';' { - fields = fields[:i] - break - } - } - if len(fields) == 0 { + // Strip blank lines and comments. + if len(fields) == 0 || fields[0][0] == '#' || fields[0][0] == ';' { return } diff --git a/libnetwork/resolvconf/resolvconf_unix_test.go b/libnetwork/resolvconf/resolvconf_unix_test.go index 6fcbe34381..86ccbf4106 100644 --- a/libnetwork/resolvconf/resolvconf_unix_test.go +++ b/libnetwork/resolvconf/resolvconf_unix_test.go @@ -149,16 +149,16 @@ func TestGetSearchDomains(t *testing.T) { result: []string{"example.com"}, }, { - input: `search example.com # ignored`, - result: []string{"example.com"}, + input: `search example.com # notignored`, + result: []string{"example.com", "#", "notignored"}, }, { input: ` search example.com `, result: []string{"example.com"}, }, { - input: ` search example.com # ignored`, - result: []string{"example.com"}, + input: ` search example.com # notignored`, + result: []string{"example.com", "#", "notignored"}, }, { input: `search foo.example.com example.com`, @@ -169,8 +169,8 @@ func TestGetSearchDomains(t *testing.T) { result: []string{"foo.example.com", "example.com"}, }, { - input: ` search foo.example.com example.com # ignored`, - result: []string{"foo.example.com", "example.com"}, + input: ` search foo.example.com example.com # notignored`, + result: []string{"foo.example.com", "example.com", "#", "notignored"}, }, { input: `nameserver 1.2.3.4 @@ -212,6 +212,9 @@ func TestGetOptions(t *testing.T) { { input: `# ignored`, }, + { + input: `; ignored`, + }, { input: `nameserver 1.2.3.4`, }, @@ -220,32 +223,36 @@ func TestGetOptions(t *testing.T) { result: []string{"opt1"}, }, { - input: `options opt1 # ignored`, - result: []string{"opt1"}, + input: `options opt1 # notignored`, + result: []string{"opt1", "#", "notignored"}, + }, + { + input: `options opt1 ; notignored`, + result: []string{"opt1", ";", "notignored"}, }, { input: ` options opt1 `, result: []string{"opt1"}, }, { - input: ` options opt1 # ignored`, - result: []string{"opt1"}, + input: ` options opt1 # notignored`, + result: []string{"opt1", "#", "notignored"}, }, { input: `options opt1 opt2 opt3`, result: []string{"opt1", "opt2", "opt3"}, }, { - input: `options opt1 opt2 opt3 # ignored`, - result: []string{"opt1", "opt2", "opt3"}, + input: `options opt1 opt2 opt3 # notignored`, + result: []string{"opt1", "opt2", "opt3", "#", "notignored"}, }, { input: ` options opt1 opt2 opt3 `, result: []string{"opt1", "opt2", "opt3"}, }, { - input: ` options opt1 opt2 opt3 # ignored`, - result: []string{"opt1", "opt2", "opt3"}, + input: ` options opt1 opt2 opt3 # notignored`, + result: []string{"opt1", "opt2", "opt3", "#", "notignored"}, }, { input: `nameserver 1.2.3.4 From f04f69e366bf9759988e23339cd11734ff8046fe Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Fri, 1 Mar 2024 16:59:28 +0000 Subject: [PATCH 2/3] Accumulate resolv.conf options If there are multiple "options" lines, keep the options from all of them. Signed-off-by: Rob Murray --- libnetwork/internal/resolvconf/resolvconf.go | 6 ++---- libnetwork/resolvconf/resolvconf_unix_test.go | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/libnetwork/internal/resolvconf/resolvconf.go b/libnetwork/internal/resolvconf/resolvconf.go index 5e9278c48a..c418492324 100644 --- a/libnetwork/internal/resolvconf/resolvconf.go +++ b/libnetwork/internal/resolvconf/resolvconf.go @@ -480,10 +480,8 @@ func (rc *ResolvConf) processLine(line string) { if len(fields) < 2 { return } - // Replace options from earlier directives. - // TODO(robmry) - preserving incorrect behaviour, options should accumulate. - // rc.options = append(rc.options, fields[1:]...) - rc.options = fields[1:] + // Accumulate options. + rc.options = append(rc.options, fields[1:]...) default: // Copy anything that's not a recognised directive. rc.other = append(rc.other, line) diff --git a/libnetwork/resolvconf/resolvconf_unix_test.go b/libnetwork/resolvconf/resolvconf_unix_test.go index 86ccbf4106..6d3492d10b 100644 --- a/libnetwork/resolvconf/resolvconf_unix_test.go +++ b/libnetwork/resolvconf/resolvconf_unix_test.go @@ -263,7 +263,7 @@ options opt1 opt2 opt3`, input: `nameserver 1.2.3.4 options opt1 opt2 options opt3 opt4`, - result: []string{"opt3", "opt4"}, + result: []string{"opt1", "opt2", "opt3", "opt4"}, }, } { test := GetOptions([]byte(tc.input)) From 8921897e3b941202d7d46143fa565008ca242205 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Fri, 1 Mar 2024 19:28:20 +0000 Subject: [PATCH 3/3] Ignore bad ndots in host resolv.conf Rather than error out if the host's resolv.conf has a bad ndots option, just ignore it. Still validate ndots supplied via '--dns-option' and treat failure as an error. Signed-off-by: Rob Murray --- libnetwork/internal/resolvconf/resolvconf.go | 41 +++++++++----- .../internal/resolvconf/resolvconf_test.go | 54 +++++++++++++++++++ libnetwork/sandbox_dns_unix_test.go | 12 ++++- 3 files changed, 92 insertions(+), 15 deletions(-) diff --git a/libnetwork/internal/resolvconf/resolvconf.go b/libnetwork/internal/resolvconf/resolvconf.go index c418492324..634b9df7ad 100644 --- a/libnetwork/internal/resolvconf/resolvconf.go +++ b/libnetwork/internal/resolvconf/resolvconf.go @@ -293,21 +293,17 @@ func (rc *ResolvConf) TransformForIntNS( rc.md.UsedDefaultNS = true } - // Validate the ndots option from host config or overrides, if present. - // TODO(robmry) - pre-existing behaviour, but ... - // Validating ndots from an override is good, but not-liking something in the - // host's resolv.conf isn't a reason to fail - just remove? (And it'll be - // replaced by the value in reqdOptions, if given.) - if ndots, exists := rc.Option("ndots"); exists { - if n, err := strconv.Atoi(ndots); err != nil || n < 0 { - return nil, errdefs.InvalidParameter( - fmt.Errorf("invalid number for ndots option: %v", ndots)) - } - } - // For each option required by the nameserver, add it if not already - // present (if the option already has a value don't change it). + // For each option required by the nameserver, add it if not already present. If + // the option is already present, don't override it. Apart from ndots - if the + // ndots value is invalid and an ndots option is required, replace the existing + // value. for _, opt := range reqdOptions { optName, _, _ := strings.Cut(opt, ":") + if optName == "ndots" { + rc.options = removeInvalidNDots(rc.options) + // No need to update rc.md.NDotsFrom, if there is no ndots option remaining, + // it'll be set to "internal" when the required value is added. + } if _, exists := rc.Option(optName); !exists { rc.AddOption(opt) } @@ -496,3 +492,22 @@ func defaultNSAddrs(ipv6 bool) []netip.Addr { } return addrs } + +// removeInvalidNDots filters ill-formed "ndots" settings from options. +// The backing array of the options slice is reused. +func removeInvalidNDots(options []string) []string { + n := 0 + for _, opt := range options { + k, v, _ := strings.Cut(opt, ":") + if k == "ndots" { + ndots, err := strconv.Atoi(v) + if err != nil || ndots < 0 { + continue + } + } + options[n] = opt + n++ + } + clear(options[n:]) // Zero out the obsolete elements, for GC. + return options[:n] +} diff --git a/libnetwork/internal/resolvconf/resolvconf_test.go b/libnetwork/internal/resolvconf/resolvconf_test.go index 21605178df..6b7e480066 100644 --- a/libnetwork/internal/resolvconf/resolvconf_test.go +++ b/libnetwork/internal/resolvconf/resolvconf_test.go @@ -500,6 +500,60 @@ func TestRCTransformForIntNS(t *testing.T) { } } +// Check that invalid ndots options in the host's file are ignored, unless +// starting the internal resolver (which requires an ndots option), in which +// case invalid ndots should be replaced. +func TestRCTransformForIntNSInvalidNdots(t *testing.T) { + testcases := []struct { + name string + options string + reqdOptions []string + expVal string + expOptions []string + expNDotsFrom string + }{ + { + name: "Negative value", + options: "options ndots:-1", + expOptions: []string{"ndots:-1"}, + expVal: "-1", + expNDotsFrom: "host", + }, + { + name: "Invalid values with reqd ndots", + options: "options ndots:-1 foo:bar ndots ndots:", + reqdOptions: []string{"ndots:2"}, + expVal: "2", + expNDotsFrom: "internal", + expOptions: []string{"foo:bar", "ndots:2"}, + }, + { + name: "Valid value with reqd ndots", + options: "options ndots:1 foo:bar ndots ndots:", + reqdOptions: []string{"ndots:2"}, + expVal: "1", + expNDotsFrom: "host", + expOptions: []string{"ndots:1", "foo:bar"}, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + content := "nameserver 8.8.8.8\n" + tc.options + rc, err := Parse(bytes.NewBuffer([]byte(content)), "/etc/resolv.conf") + assert.NilError(t, err) + _, err = rc.TransformForIntNS(false, netip.MustParseAddr("127.0.0.11"), tc.reqdOptions) + assert.NilError(t, err) + + val, found := rc.Option("ndots") + assert.Check(t, is.Equal(found, true)) + assert.Check(t, is.Equal(val, tc.expVal)) + assert.Check(t, is.Equal(rc.md.NDotsFrom, tc.expNDotsFrom)) + assert.Check(t, is.DeepEqual(rc.options, tc.expOptions)) + }) + } +} + func TestRCRead(t *testing.T) { d := t.TempDir() path := filepath.Join(d, "resolv.conf") diff --git a/libnetwork/sandbox_dns_unix_test.go b/libnetwork/sandbox_dns_unix_test.go index d092ab53fd..ef14aeb15d 100644 --- a/libnetwork/sandbox_dns_unix_test.go +++ b/libnetwork/sandbox_dns_unix_test.go @@ -77,11 +77,19 @@ func TestDNSOptions(t *testing.T) { err = sb2.setupDNS() assert.NilError(t, err) err = sb2.rebuildDNS() - assert.Error(t, err, "invalid number for ndots option: foobar") + assert.NilError(t, err) + currRC, err = resolvconf.GetSpecific(sb2.config.resolvConfPath) + assert.NilError(t, err) + dnsOptionsList = resolvconf.GetOptions(currRC.Content) + assert.Check(t, is.DeepEqual([]string{"ndots:0"}, dnsOptionsList)) sb2.config.dnsOptionsList = []string{"ndots:-1"} err = sb2.setupDNS() assert.NilError(t, err) err = sb2.rebuildDNS() - assert.Error(t, err, "invalid number for ndots option: -1") + assert.NilError(t, err) + currRC, err = resolvconf.GetSpecific(sb2.config.resolvConfPath) + assert.NilError(t, err) + dnsOptionsList = resolvconf.GetOptions(currRC.Content) + assert.Check(t, is.DeepEqual([]string{"ndots:0"}, dnsOptionsList)) }