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

Feature #79 sudo users #125

Merged
merged 7 commits into from
Oct 31, 2014
Merged

Conversation

berkes
Copy link
Collaborator

@berkes berkes commented Oct 8, 2014

Another 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?

"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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, @houndci, however, the other lines were already single-quoted. Making you happy, would mean that I introduce inconsistency: arguably worse.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your thinking but I would prefer to change the lines surrounding your change, that way we can go towards the code-guideline slowly.

@jvanbaarsen
Copy link
Contributor

@berkes I've added this to my todolist for next sunday.

"authorization": {
"sudo": {
"users": ["<your sudo user>"]
"sysadmins": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you also try the option to use the username as the hash key? that way you can ommit the array. Like so:

"sysadmins": {
  "<username_1>": {
    "password": "<hashed password: openssl passwd -1 'plaintextpassword'>",
    "ssh_keys": [
      "ssh-rsa AAA123...xyz== foo",
      "ssh-rsa AAA456...uvw== bar"
    ]
  },
  "<username_2>": {
    "password": "<hashed password: openssl passwd -1 'plaintextpassword'>",
    "ssh_keys": [
      "ssh-rsa AAA123...xyz== foo",
      "ssh-rsa AAA456...uvw== bar"
    ]
  }
}

@jvanbaarsen
Copy link
Contributor

@michiels What do you think?

"authorization" => {
"sudo" => {
"groups" => ["admin"],
"passwordless" => false,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be true by default? This way you don't need to specify it in the node configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if I understand your question. The reason I made this false, is to make sure that all sudo-users have to provide their password in order to gain sudo-rights. This is the "Default Ubuntu Way". Somehow, the chef-sudo-cookbook changes this and kindof "breaks" the standard Ubuntu way.

Users can still choose passwordless sudo-usage, if they really don't like the Default Ubuntu Way by changing this setting in the node configuration.

@michiels
Copy link
Member

@berkes @jvanbaarsen Thank you for the work so far! I posted to little comments.

Another question I have is if we should include this in the base role or not. What has happened in the past a few times is that if you call the sudo cookbook in a role, without passing in the sudoers information you'll end up with a server you're locked out off because the sudo-ers cookbook will clear out all the sudo users.

Maybe it's wise to include this in a specific role instead of in the base role? What do you guys think?

@jvanbaarsen
Copy link
Contributor

I see the locked-out problem, since its a big risk for people using this, i think it would be good to have it in a specific role, also maybe add a warning about this in the docs?

@michiels
Copy link
Member

On the other hand. If we set the "admin" group to be included for sudoers by default in the role defaults, users that are added to the "admin" group once, will still work in there even if you don't pass anything to the cookbook.

I still suggest we add it to a separate role, just to be sure people know what they are doing :)

@berkes
Copy link
Collaborator Author

berkes commented Oct 20, 2014

Having this as a separate role is fine for me. How would this be used than? Should we add a note to the example node-configuration where people can include such a role?

Furthermore, I've changed the configuration of the sudo cookbook to match how Ubuntu has the sudo set-up. That was not the case in the current set-up. By sticking close to the Ubuntu default (e.g. using admins as group) stuff should not break, unless people had previously set up a server with a sudo-configuration that did not match Ubuntu default, off course.

* 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.
Don't run it by default, but make it a role.
@jvanbaarsen
Copy link
Contributor

@berkes I think it is a good idea to have it in sample_node.json so people know its available, also maybe add a short description to the README explaining how people can use this! Furthermore, thanks for all the amazing work!

@@ -6,6 +6,15 @@
"server_repl_password": "<enter a random password>"
},
"packages": ["<option list of system wide packages>"],
"sysadmins": {
"<username>":
Copy link
Contributor

Choose a reason for hiding this comment

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

Your missing a { here

@jvanbaarsen
Copy link
Contributor

@berkes I've just tested this, it works great! I'm going to merge this in, and will fix a couple of things myself: The missing { and make sure this entry is in the correct Changelog.

Thanks for all this amazing work!

jvanbaarsen added a commit that referenced this pull request Oct 31, 2014
@jvanbaarsen jvanbaarsen merged commit 255b611 into intercity:master Oct 31, 2014
@berkes
Copy link
Collaborator Author

berkes commented Nov 3, 2014

Thanks for your time too. Sorry I did not report back, I was away this weekend.

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