PUP-2289 Fix host type and provider. #3005

lmello · 2014-08-22T22:30:10Z

This commit fix the first bug reported on PUP-2289.
This is done by implementing validation of newlines
on ip, hostname, comments and host_aliases, making
sure we don't have any \n or \r.

Bellow is the problem description:

If the ip address or hostname or host_aliases
use double quoting and have \n, Puppet Will:

1 - Write the wrong entry on /etc/hosts with the linebreak.
This leads to a corrupted /etc/hosts, with some lines
that could not be parsed by the host resource provider
parsed.

This leads to second bug on the second puppet run,
where it will have error after failing to parse the file,
but wipes out the /etc/hosts file, writing only the information
on that file that is managed by puppet.

How to reproduce it:
1- backup /etc/hosts
cp /etc/hosts /etc/hosts.orig
2 - Create the script to reproduce the bug 01
cat < /tmp/hostsbug.pp
host {'hostsbug': ip => "10.10.12.1\n" }
EOF
The same apply if you have newline on hostname, hostaliases or comment.
3 - Run the script the first time to reproduce bug 01.
puppet apply /tmp/hostsbug.pp
4 - Check /etc/hosts
cat /etc/hosts

Leonardo Rodrigues de Mello

lmello · 2014-08-22T22:30:30Z

please do not merge yet. i have to work on spec tests.

puppetcla · 2014-08-23T01:00:23Z

CLA signed by all contributors.

kylog · 2014-08-23T23:37:12Z

@lmello thanks! Ping when you have spec tests, but thanks very much for working on this!!

lmello · 2014-08-24T01:00:27Z

@kylog done the tests. I will squash the commits. after that you can merge, if you like it.

This fix the first bug reported on pup-2289, it will still be needed to fix the second bug that is on host provider (parsed or parsedfile).

Would after we have both bugs fixed, would it be possible to generate one puppet 3.4 rpm package ?
:-)

lmello · 2014-08-24T01:06:32Z

@kylog ready to go. Thanks! :-)

lmello · 2014-08-24T16:29:43Z

I had fixed both bugs. :-) , there was one typo that was fixed, i had rebased the code into two commits, one that fix the resource type, and other that fix the resource provider.

I had based the resource provider solution on mount resource type.

lmello · 2014-08-25T17:36:30Z

@kylog

lmello · 2014-08-28T16:31:19Z

@kylog any chance this could go in 3.7 ?

kylog · 2014-08-28T22:56:05Z

@lmello unfortunately it's just a bit late for 3.7. We locked down the 3.7 list a couple weeks ago and we're hoping to release Tuesday (Sept 2).

Thanks for the contribution though! And once 3.7 is out the door, we'll have more time to review open pull requests, etc, so we should get you feedback on this one before long. Thanks!

lmello · 2014-09-04T20:16:53Z

@kylog
First i would like to apology for the delay to answer you. I am delivering one large project at my work, and everything was on hold because of that.

Thanks for your explanation, sadly I discovered this bug, after the locking of 3.7 release :-/ . Completely understand the priorities, congratulations for the 3.7 release today! 👍

please let me know if you need anything from me to help this fix get merged.

Best
Leo

lmello · 2014-09-19T00:12:18Z

@kylog , I know the things are rushed because the puppetconf, but there is any chance this could go on a 3.7.2 release, if we have one ?

I don't plan updating to puppet 4 this year, so having this fix on puppet 3.7 would be really awesome! :-)

Thanks
Leo

ghoneycutt · 2014-09-29T21:51:22Z

@kylog is this ready to be merged?

CC: @Gusson

hlindberg · 2014-10-15T17:27:24Z

lib/puppet/type/host.rb

@@ -22,9 +22,13 @@ def valid_v6?(addr)

        return false
      end
+      def valid_newline?(addr)
+        return false if addr =~ /\n/ or addr =~ /\r/


Use || instead of or, and && instead of and everywhere

lmello · 2014-10-17T12:31:45Z

Thanks for all the feedback. I will have time for making the changes on monday! :-)

lmello · 2014-10-17T12:35:24Z

just for reference:
http://devblog.avdi.org/2010/08/02/using-and-and-or-in-ruby/

lmello · 2014-10-23T12:58:53Z

lib/puppet/provider/host/parsed.rb

@@ -17,10 +17,10 @@

  text_line :comment, :match => /^#/
  text_line :blank, :match => /^\s*$/
-
+  hosts_pattern = '^([0-9a-f:]\S+)\s+([^#\s+]\S+)\s*(.*?)?(?:\s*#\s*(.*))?$'


improved a little bit the regular expression, without making it to much complex :

this was needed because it was matching hostnames as ip address in the following bogus etc/hosts entry:
hostname hostalias #comment

also it was matching #comment as hostnames in the following bogus entry:
127.0.0.1 #comment

I like the original regex for it simplicity, and think we should not change this one to be specific and complex as the regex to validate ip address on host type.

lmello · 2014-10-23T13:04:15Z

spec/unit/type/host_spec.rb

    end

    it "should not accept empty host_aliases" do
-      proc { @class.new(:name => "foo", :host_aliases => ['alias1','']) }.should raise_error
+      expect { @class.new(:name => "foo", :host_aliases => ['alias1','']) }.to raise_error(Puppet::ResourceError, /Host aliases cannot be an empty string/)


same as above.

lmello · 2014-10-23T13:37:27Z

@kylog @hlindberg @ferventcoder

Sorry for the long delay

I had made all the requested changes and improved the spec tests. This helped finding a flaw in the implementation, that I had already fixed.

I had to improve a little bit the regex for parsing etc/hosts lines, because it was matching wrong things as hostname (#comments) and ipaddress (Text for example, hostnames).

This was tested with rspec and vagrant.

could someone review the changes for merging ?

best
Léo

lmello · 2014-10-23T13:38:23Z

lib/puppet/type/host.rb

@@ -76,6 +84,7 @@ def inclusive?
            raise Puppet::Error, "Invalid host name"
          end
        end
+        raise Puppet::Error, "Hostname cannot include newline" if (value =~ /\n/ || value =~ /\r/)


also validate hostname to not include newline.

lmello · 2014-10-23T13:39:19Z

I had also squash all the changes into a single commit.

ferventcoder · 2014-10-23T16:56:26Z

@lmello one small thing in the commit message, we usually surround the ticket with parentheses like (PUP-2289)

This commit fix the first bug reported on PUP-2289. This is done by implementing validation of newlines on ip, hostname, comments and host_aliases, making sure we don't have any \n or \r. The bug and fix is on host resource type. Bellow is the problem description: If the ip address or hostname or host_aliases use double quoting and have \n, Puppet Will: 1 - Write the wrong entry on /etc/hosts with the linebreak. This leads to a corrupted /etc/hosts, with some lines that could not be parsed by the host resource provider parsed. This leads to second bug on the second puppet run, where it will have error after failing to parse the file, but wipes out the /etc/hosts file, writing only the information on that file that is managed by puppet. How to reproduce it: 1- backup /etc/hosts cp /etc/hosts /etc/hosts.orig 2 - Create the script to reproduce the bug 01 cat <<EOF > /tmp/hostsbug.pp host {'hostsbug': ip => "10.10.12.1\n" } EOF The same apply if you have newline on hostname, hostaliases or comment. 3 - Run the script the first time to reproduce bug 01. puppet apply /tmp/hostsbug.pp 4 - Check /etc/hosts cat /etc/hosts Leonardo Rodrigues de Mello PUP-2289 Fix bug on host resource provider The host resource provider parsed file wipes all entries of etc/hosts file if it gets one entry that could not be parsed. This commits fix this problem, considering that incomplete lines are :text_lines. This is the same solution that was used for mount resource provider. I had checked that etc/hosts ignores invalid entries at least on linux, tested on rhel 5, 6, 7 and debian 5,6,7 and fedora 20. It would be great if someone could test it on other platforms. Thanks Leonardo Rodrigues de Mello Fixes as required on the pull request

lmello · 2014-10-23T20:24:01Z

@ferventcoder done! :-)

kylog · 2014-10-25T01:36:51Z

Merged in cc3c608. This will be release in puppet 4.0. Thanks for the contribution!

(Note this was merged in #3235 which was a whitespace-only change based on this - adding newlines to be consistent with rest of that spec file.)

lmello force-pushed the fix_pup-2289 branch from 1bfc8d9 to e95ad67 Compare August 24, 2014 01:05

lmello force-pushed the fix_pup-2289 branch from d997d60 to f9f4551 Compare August 24, 2014 21:37

lmello changed the title ~~PUP-2289 Fix host type newline validation~~ PUP-2289 Fix host type and provider. Aug 24, 2014

hlindberg reviewed Oct 15, 2014
View reviewed changes

lmello force-pushed the fix_pup-2289 branch from 6b7cc93 to 522fa1c Compare October 23, 2014 12:54

lmello reviewed Oct 23, 2014
View reviewed changes

lmello force-pushed the fix_pup-2289 branch from 522fa1c to 44032d4 Compare October 23, 2014 20:22

kylog closed this Oct 25, 2014

nuclearsandwich mentioned this pull request Feb 5, 2018

/etc/hosts gets clobbered ros-infrastructure/buildfarm_deployment#191

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PUP-2289 Fix host type and provider. #3005

PUP-2289 Fix host type and provider. #3005

lmello commented Aug 22, 2014

lmello commented Aug 22, 2014

puppetcla commented Aug 23, 2014

kylog commented Aug 23, 2014

lmello commented Aug 24, 2014

lmello commented Aug 24, 2014

lmello commented Aug 24, 2014

lmello commented Aug 25, 2014

lmello commented Aug 28, 2014

kylog commented Aug 28, 2014

lmello commented Sep 4, 2014

lmello commented Sep 19, 2014

ghoneycutt commented Sep 29, 2014

hlindberg Oct 15, 2014

lmello commented Oct 17, 2014

lmello commented Oct 17, 2014

lmello Oct 23, 2014

lmello Oct 23, 2014

lmello commented Oct 23, 2014

lmello Oct 23, 2014

lmello commented Oct 23, 2014

ferventcoder commented Oct 23, 2014

lmello commented Oct 23, 2014

kylog commented Oct 25, 2014

PUP-2289 Fix host type and provider. #3005

PUP-2289 Fix host type and provider. #3005

Conversation

lmello commented Aug 22, 2014

lmello commented Aug 22, 2014

puppetcla commented Aug 23, 2014

kylog commented Aug 23, 2014

lmello commented Aug 24, 2014

lmello commented Aug 24, 2014

lmello commented Aug 24, 2014

lmello commented Aug 25, 2014

lmello commented Aug 28, 2014

kylog commented Aug 28, 2014

lmello commented Sep 4, 2014

lmello commented Sep 19, 2014

ghoneycutt commented Sep 29, 2014

hlindberg Oct 15, 2014

Choose a reason for hiding this comment

lmello commented Oct 17, 2014

lmello commented Oct 17, 2014

lmello Oct 23, 2014

Choose a reason for hiding this comment

lmello Oct 23, 2014

Choose a reason for hiding this comment

lmello commented Oct 23, 2014

lmello Oct 23, 2014

Choose a reason for hiding this comment

lmello commented Oct 23, 2014

ferventcoder commented Oct 23, 2014

lmello commented Oct 23, 2014

kylog commented Oct 25, 2014