Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make uid_min and gid_min of login.defs configurable #62

Merged
merged 6 commits into from
Nov 28, 2014

Conversation

bkw
Copy link
Contributor

@bkw bkw commented Nov 28, 2014

This PR lets UID_MIN and GID_MIN in login.defs come from attributes/default.rb, defaulting to same values as before. In order to achieve that, I fixed some minor issues along the way:

  • fix some rubocop issues
  • update to chefspec 4.1.1
  • bump sysctl dependency to 0.6.0 as described in the readme
  • use quotes for file mode of login.defs template
  • add a chefspec test for login_defs creation

Tell me if you want me to split these into smaller PRs.

@@ -109,14 +109,14 @@ PASS_MIN_DAYS <%= @password_min_age.to_s %>
PASS_WARN_AGE 7

# Min/max values for automatic uid selection in useradd
UID_MIN 1000
UID_MIN <%= @uid_min.to_s %>
UID_MAX 60000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we start to make min configurable, why not max?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazyness as a virtue - I just did not need that for my usecase ;-)

The lower limit of 1000 is much more often a problem, for instance if you create your admins at uid 2300+ with fixed uids. The next automatic install might add a user with uid 2305, and when you add the next admin user, it will conflict on that system. I can't envision a similar thing happening because of wrong max settings.
But feel free to add them, if you do ;-)

@chris-rock
Copy link
Member

@bkw great work.

@@ -29,7 +29,7 @@
supports 'redhat', '>= 6.4'
supports 'oracle', '>= 6.4'

depends 'sysctl', '>= 0.3.0'
depends 'sysctl', '>= 0.6.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkw: any reason, why we require a bump here? i am not sure if this breaks existing usage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was because of /your/ commit: 0310465

:-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright. good point ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just double checked. I missed
https://github.com/TelekomLabs/chef-os-hardening/blob/master/recipes/sysctl.rb#L24

We support version greater 0.3.0, but will drop support lt 0.6.0 in our next major release

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, should I remove the bump commit from this pr then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please

@bkw
Copy link
Contributor Author

bkw commented Nov 28, 2014

force-pushed a new version without the sysctl-0.6.0 dependency bump.

@chris-rock
Copy link
Member

thanks @bkw

chris-rock added a commit that referenced this pull request Nov 28, 2014
make uid_min and gid_min of login.defs configurable
@chris-rock chris-rock merged commit a8250f3 into dev-sec:master Nov 28, 2014
@bkw bkw deleted the uid_min+gid_min branch November 28, 2014 20:05
@dupuy dupuy mentioned this pull request Mar 18, 2015
rollbrettler pushed a commit to rollbrettler/chef-os-hardening that referenced this pull request Sep 16, 2016
fixes dev-sec#62

Signed-off-by: Dominik Richter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants