Adduser consistency #73

dupuy · 2015-03-18T16:27:46Z

This is just a preliminary PR to get some feedback on scope (i.e. please don't merge!). If there is interest or you have other suggestions, I will add tests and docs and everything else per the guidelines.

Although Bernhard's UID_MIN/GID_MIN PR#62 prevents problems when you have some range of fixed UIDs or GIDs that you need to keep away from, it only does so when using the useradd command. Debian/Ubuntu systems (maybe some others?) also have a "friendlier" front-end called adduser that uses a separate configuration file for some reason. To really change the minimum UID/GID on a system where adduser is not just a symlink to useradd, you also need to update that separate configuration file.

This PR does that, in two commits - the first only updates fields in /etc/adduser.conf that correspond to fields in /etc/login.defs that is being modified per hardening. The second commit goes a bit further in trying to enforce consistency between adduser and useradd, and also updates another configuration file used by useradd: /etc/default/useradd. There is a bit of diminishing returns here - the extra configuration fields are not really security-related, and bringing these files under Chef control makes it harder to configure them manually or in other cookbooks/recipes. The default values for all these fields are the same between the two tools.

Anyhow, interested to know whether you think the second commit (or even the first one) are something you would want to merge, and I will polish them up if you give the green light.

chris-rock · 2015-03-24T08:52:53Z

@dupuy Great work. I really like your approach and it is worth the effort to harmonize the user UID/GID across tools. I need to think further, if we should extract this part from hardening or leave it as a part. I will discuss this topic with @arlimus and @atomic111

chris-rock · 2015-04-10T08:10:16Z

recipes/login_defs.rb

    uid_min: node['auth']['uid_min'],
    gid_min: node['auth']['gid_min']
  )
 end
+
+adduser_conf = '/etc/adduser.conf'


can we move this to attributes?

chris-rock · 2015-04-10T08:11:24Z

@dupuy I apologize for the late response. I discussed with @arlimus Please go ahead. I recommended some changes.

dupuy · 2015-04-10T08:26:37Z

No problem. Just to be perfectly clear (line numbers of comments are not always unambiguous references), you are recommending that

/etc/adduser.conf filename variable be moved to attributes
(which is certainly the right thing to do, not sure why I didn't the first time, just expediency I suspect), and
the new templates be moved to a new, separate adduser.rb recipe

That's reasonable too, although the second new template (for /etc/default/useradd) should probably be moved to a third (new) useradd.rb recipe, as it is a configuration file for the useradd command (which also uses and shares with login/pam/etc. the login.defs config file) and doesn't have anything to do with adduser (except for the functional overlap and confusingly similar names). There should also be a separate variable in attributes/default.rb for the /etc/default/useradd filename, since you never know when you'll need to change one or all of the confusingly overlapping configuration filenames (it's almost -- but not quite -- enough to make you wish for MS Registry so that all the configuration is in one place with one format :-) ).

I'll try to get to this over the weekend.

chris-rock · 2015-04-10T08:31:57Z

@dupuy Yeah, i think that nails it. The additional split makes sense for me. This keeps components small and manageable.

Since this useradd vs adduser can be confusing for newcomers, I also think we should properly document what is for what. Do you need help for that?

dupuy · 2015-04-10T08:35:10Z

I can certainly add documentation if you can suggest the most appropriate place.

There is another issue where I could use help, which is that to avoid feature slip between Chef and Puppet versions of the hardening, all of this (probably including Bernhard's original change as well), probably ought to be implemented there as well, and I know no more about Puppet than I did about Chef four months ago...

chris-rock · 2015-04-10T08:39:35Z

@dupuy Of course. Lets first finish the chef implementation. Afterwards I can migrate it to puppet.

chris-rock · 2015-04-20T19:28:03Z

@dupuy Anything I can do to bring this forward?

dupuy · 2015-05-28T11:45:26Z

Okay, I revised this PR to reflect the changes (I had a transatlantic flight without internet so I couldn't work on the other stuff that has kept this on the back burner) and added some basic tests and even documentation (just in the README). I rebased it all on the latest master, so should be a clean merge. There might be a problem with the tests in that the adduser recipe doesn't create the /etc/adduser.conf file if it doesn't exist (e.g. on non-Debian/Ubuntu systems, or ones where the Apt package for adduser isn't installed) but I couldn't find a way to express that in the Chefspec tests. If there's a problem with the Travis CI build, I'll see what I can do to fix that.

chris-rock · 2015-06-01T14:28:04Z

@dupuy great work. Let's rebase this PR to the latest master to ensure we pass all tests.

dupuy · 2015-06-01T14:41:13Z

OK, rebased, will see how Travis goes...

dupuy · 2015-06-02T14:34:25Z

OK, fixed up some more things revealed by the tests (a Pythonism in the actual code, and a bunch of problems with the Chefspec tests themselves), and I think that it should really go through this time. I did run the kitchen tests (or thought I did) before pushing these (and earlier) changes out for Travis, but maybe there's a problem with that, since there's no output from that stage of the kitchen run:

$ kitchen test default-ubuntu-1404
...
       Running handlers:
       Running handlers complete
       Chef Client finished, 23/29 resources updated in 27.161592943 seconds
       Finished converging <default-ubuntu-1404> (1m8.76s).
-----> Setting up <default-ubuntu-1404>...
       Finished setting up <default-ubuntu-1404> (0m0.00s).
-----> Verifying <default-ubuntu-1404>...
       Finished verifying <default-ubuntu-1404> (0m0.00s).
-----> Destroying <default-ubuntu-1404>...
       ==> default: Forcing shutdown of VM...
       ==> default: Destroying VM and associated drives...
       Vagrant instance <default-ubuntu-1404> destroyed.
       Finished destroying <default-ubuntu-1404> (0m4.08s).
       Finished testing <default-ubuntu-1404> (1m54.03s).
-----> Kitchen is finished. (1m55.40s)

chris-rock · 2015-06-02T14:51:10Z

Looks like you havn't used bundle exec thor kitchen:fetch-remote-tests. To meet the lint and chef specs, just call bundle exec rake lint or bundle exec rake spec

dupuy · 2015-06-02T15:35:26Z

Thanks for those pointers, I had thought the vagrant kitchen VM would run things directly (this is how Bernhard set up Chef cookbooks that we're using). Running bundle exec rake spec on my Mac isn't useful (system is not Linux, just too different), and doesn't work in the vagrant box as it needs to have the tests shipped over to it (Bernhard uses Busser for that). So I can run the lint checks locally without vagrant, guess that's good enough, and I think I might have found the last problem with the tests in the last Travis run, so will try once more...

dupuy · 2015-06-02T15:43:14Z

One other note, the Rakefile seems not to properly detect failures in the 1.9.3 environment, as Job #153.1 shows:

Running Chefspec tests
/home/travis/.rvm/rubies/ruby-1.9.3-p551/bin/ruby -I/home/travis/.rvm/gems/ruby-1.9.3-p551/gems/rspec-core-3.2.3/lib:/home/travis/.rvm/gems/ruby-1.9.3-p551/gems/rspec-support-3.2.2/lib /home/travis/.rvm/gems/ruby-1.9.3-p551/gems/rspec-core-3.2.3/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
.F...........
Failures:
  1) os-hardening::adduser uses uid_min, gid_min, usergroups and umask in /etc/adduser.conf
     Failure/Error: expect(chef_run).to render_file(conffile).
       expected Chef run to render "/etc/adduser.conf" matching:

       (?-mix:^DIRMODE=0710$)

       but got:
...
Finished in 54.51 seconds (files took 5.11 seconds to load)
13 examples, 1 failure
Failed examples:
rspec ./spec/recipes/adduser_spec.rb:43 # os-hardening::adduser uses uid_min, gid_min, usergroups and umask in /etc/adduser.conf
ChefSpec Coverage report generated...
  Total Resources:   19
  Touched Resources: 5
  Touch Coverage:    26.32%
Untouched Resources:
...
[Coveralls] Submitting with config:
...
[Coveralls] Submitting to https://coveralls.io/api/v1
[Coveralls] Job #153.1
[Coveralls] https://coveralls.io/jobs/6354731
Coverage is at 100.0%.
Coverage report sent to Coveralls.
The command "bundle exec rake" exited with 0.

chris-rock · 2015-06-03T08:49:58Z

we have to pull in the tests, because we share the same tests across all implementations: Chef, Puppet ansible. Therefore it's not possible to ship it directly with this repository. This is not required if you have just one DevOps tool.
To run bundle exec rake spec you do not need vagrant or kitchen, because it uses chefspec delivered in this project. The only requirement is an installation of ChefDK on Mac.

chris-rock · 2015-06-20T10:40:51Z

@dupuy amazing work, I need to add some fixes to os-hardening to meet the latest serverspec implementation before I am able to merge. Additionally I need to ensure this PR works great on RHEL-based systems. Maybe we need to restrict adduser to debian/ubuntu systems

dupuy · 2015-06-21T23:05:37Z

Additionally I need to ensure this PR works great on RHEL-based systems. Maybe we need to restrict adduser to debian/ubuntu systems

I haven't explicitly tested on RHEL/Fedora, but the adduser command can be absent on Debian/Ubuntu as well (admittedly much more likely to be absent on RHEL/Fedora) - to handle this regardless of platform the creation (update) of /etc/adduser.conf is restricted by an only_if "the (configuration) file already exists" in recipes/adduser.rb: https://github.com/dupuy/chef-os-hardening/commit/a0bf434ab045a64977f0902fea271f63f0ae05e8#diff-25d9e00951dcb9a6355cbca279561d4eR22

No other actions (apart from configuration file modifications) relative to adduser (or useradd) is taken by these recipes, so I think this should be sufficient restriction.

including description of ['env']['umask'] and its new homedir aspect, and also ['auth']['root_ttys'] list, which was previously overlooked

bugfix: adjust travis to work with chef12/ruby2

artem-sidorenko · 2017-10-21T19:50:36Z

I'm closing this PR, as its not mergeable in the current state. Sorry for that, please ping me if you thing its still needed, I'll try to pick it up and to get it to the mergeable state

chris-rock reviewed Apr 10, 2015
View reviewed changes

chris-rock mentioned this pull request Apr 10, 2015

Adduser consistency dev-sec/puppet-os-hardening#49

Closed

dupuy added 5 commits July 3, 2015 14:50

update /etc/adduser.conf for consistency with /etc/login.defs

d759a4e

add tests for adduser.conf consistency with login.defs

1d9f2fa

more useradd/adduser consistency/configurability: /etc/default/useradd

904987e

add tests for /etc/adduser.conf consistency with /etc/default/useradd

d65e647

document adduser/useradd configuration settings

084615a

including description of ['env']['umask'] and its new homedir aspect, and also ['auth']['root_ttys'] list, which was previously overlooked

rollbrettler pushed a commit to rollbrettler/chef-os-hardening that referenced this pull request Sep 16, 2016

Merge pull request dev-sec#73 from TelekomLabs/travis4ruby2+

4953f40

bugfix: adjust travis to work with chef12/ruby2

artem-sidorenko closed this Oct 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adduser consistency #73

Adduser consistency #73

dupuy commented Mar 18, 2015

chris-rock commented Mar 24, 2015

chris-rock Apr 10, 2015

chris-rock commented Apr 10, 2015

dupuy commented Apr 10, 2015

chris-rock commented Apr 10, 2015

dupuy commented Apr 10, 2015

chris-rock commented Apr 10, 2015

chris-rock commented Apr 20, 2015

dupuy commented May 28, 2015

chris-rock commented Jun 1, 2015

dupuy commented Jun 1, 2015

dupuy commented Jun 2, 2015

chris-rock commented Jun 2, 2015

dupuy commented Jun 2, 2015

dupuy commented Jun 2, 2015

chris-rock commented Jun 3, 2015

chris-rock commented Jun 20, 2015

dupuy commented Jun 21, 2015

artem-sidorenko commented Oct 21, 2017

Adduser consistency #73

Adduser consistency #73

Conversation

dupuy commented Mar 18, 2015

chris-rock commented Mar 24, 2015

chris-rock Apr 10, 2015

Choose a reason for hiding this comment

chris-rock commented Apr 10, 2015

dupuy commented Apr 10, 2015

chris-rock commented Apr 10, 2015

dupuy commented Apr 10, 2015

chris-rock commented Apr 10, 2015

chris-rock commented Apr 20, 2015

dupuy commented May 28, 2015

chris-rock commented Jun 1, 2015

dupuy commented Jun 1, 2015

dupuy commented Jun 2, 2015

chris-rock commented Jun 2, 2015

dupuy commented Jun 2, 2015

dupuy commented Jun 2, 2015

chris-rock commented Jun 3, 2015

chris-rock commented Jun 20, 2015

dupuy commented Jun 21, 2015

artem-sidorenko commented Oct 21, 2017