Skip to content
This repository has been archived by the owner on Apr 23, 2019. It is now read-only.

Add version 0.1.o of sysadmins: a way to add sudo users #116

Closed
wants to merge 26 commits into from

Conversation

berkes
Copy link
Collaborator

@berkes berkes commented Sep 26, 2014

This is a first go at #79

It introduces a small cookbook "sysadmins" that takes sysadmins from the node json file and creates these on the server.

Please have a look and tell me what you think. If you like, I will add a commit that adds this cookbook to the base role and bump the version.

I could not find any tests in the main repo, but maybe I am looking wrong? Am I supposed to run/add tests? And if so, where?

@@ -0,0 +1,7 @@
name 'sysadmins'

Choose a reason for hiding this comment

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

Put one space between the method name and the first argument.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -1,6 +1,6 @@
name 'base'
description 'Base bootstrap for every box'
run_list "recipe[sudo]", "recipe[apt]", "recipe[build-essential]"
run_list "recipe[sysadmins]", "recipe[sudo]", "recipe[apt]", "recipe[build-essential]"

Choose a reason for hiding this comment

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

Line is too long. [86/80]

@berkes
Copy link
Collaborator Author

berkes commented Sep 28, 2014

One important issue to discuss is the default sudo-behaviour.

As this PR piggybacks the sudo-file managed by the included sudo cookbook, it gets the behaviour for the "sysadmin group" from there.

This means all sysadmins can sudo without using their passwords.

I -personally- don't like that very much. But I cannot really think of an actual reason why it makes me uncomfortable. What do you think?

@jvanbaarsen
Copy link
Contributor

@berkes Thanks for working on this! Are there ways to prevent the using sudo without password? Or would it be super hard? I have the same as you, don't really like it, but cant think of a reason why. @michiels What do you think?

@michiels
Copy link
Member

Let me think on this a little bit. My first thought would be to double down on security and require a password. But since we're using SSH keys to connect this would seem a bit weird AND I don't want to put user's passwords in the Chef attributes.

An alternative would be to require password, and have the user add a password when they log in with their SSH key for the first time. So until they choose a password, they can't sudo. This way we do have double-security for this.

I'm not sure what the "best practice" is for this. Maybe we can find some pointers. I'll ask around too :)

@berkes
Copy link
Collaborator Author

berkes commented Sep 29, 2014

My gut-feeling says to go with what Ubuntu does by default, rather than what the "sudo" cookbook does by default.

Ubuntu's (server) default sudo behaviour is:

# Members of the admin group may gain root privileges
%admin ALL=(ALL) ALL

# Allow members of group sudo to execute any command
%sudo   ALL=(ALL:ALL) ALL

The first user on the system automatically gets "admin" group. Any new users can get admin group.
Users belonging to admin group need to provide their password when using sudo.

Here's what I -personally- would prefer for my servers:
I provide a list of users that get "admin" in my node.json file. I provide the password hash in that list; and optionally, provide one or more ssh-key's that allow even easier access for that user.
Any user in that list has access and sudo-access in the "default" Ubuntu-way:

  • You log in (over SSH) using either the provided keys, or the password.
  • You then have sudo-access, which asks for your password again.

I don't know all the ins- and outs, and I cannot oversee all the security-impacts of all alternatives. It might be more secure, but might also open some unintended security-hole: scio me nescire.
As such, I rather simply trust that others, in this case the Ubuntu architects, have made the right desisions. And follow their system.

In that case: please don't yet apply his PR. I will refactor and make sure that the sudo-cookbook behaves in the normal Ubuntu-way; I will assign the default "admin" group as used in Ubuntu, rather than a new "sysadmin" group.

@michiels
Copy link
Member

@berkes 100% agree with your comment about using Ubuntu's defaults and setting those things accordingly with our Chef recipes. Would be great if you can change this PR to make that work. Thanks!

* Use a group called "admin"
* Require a password for sudo.

Requiring a password breaks Vagrant's provision and setup
and as such, we don't require a password in Vagrant.
"include_sudoers_d" => true,
"sudoers_default" => [
'env_reset',
'mail_badpass',

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@berkes
Copy link
Collaborator Author

berkes commented Sep 29, 2014

This should bring it in line with Ubuntu's default behaviour.

jvanbaarsen and others added 18 commits October 8, 2014 23:24
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
Signed-off-by: Jeroen van Baarsen <[email protected]>
@@ -0,0 +1,7 @@
name "backups"

Choose a reason for hiding this comment

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

Put one space between the method name and the first argument.

@berkes
Copy link
Collaborator Author

berkes commented Oct 8, 2014

Github is completely borking my rebase. Will re-open a proper PR in a sec.

@berkes berkes closed this Oct 8, 2014
@berkes berkes deleted the feature/79-sudo-users branch October 8, 2014 21:29
@berkes berkes mentioned this pull request Oct 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants