Replace shared root login with user accounts #628

edunham · 2017-03-27T18:12:19Z

These changes:

Create individual user account for everyone who has an SSH key
Add each SSH key to its person's account instead of to root
Forbid root login at ssh config level (OSX and Ubuntu)
Permit passwordless sudo for each user who has an SSH key. This is not great but it is no worse than the current state of affairs. The options for moving away from passwordless sudo will all potentially break peoples' workflows, so I'd like to write and test them as a separate set of changes from this one.

r? @aneeshusa to catch my inevitable Salt room-for-improvement

cc @larsbergstrom for thoughts on if/how we should actively disallow root login on Windows

Labeling please-don't-merge because I'm 90% sure something in here will break Travis.

This change is


        Create individual accounts


        explicitly forbid root login

edunham · 2017-03-27T18:13:55Z

Oh actually I had a couple old PRs about this which were broken for various reasons. I'll integrate the suggestions from them before consuming anyone's review time.


        Consolidate user account management into single state


        Deploy Mozilla-recommended SSHD config

See https://wiki.mozilla.org/Security/Guidelines/OpenSSH


        single ID for sudoers line


        reduce account creation to cross-platform arguments

larsbergstrom · 2017-04-01T15:16:15Z

From the logs, it looks like the creation step needs to be depended upon by the keyfile installation step? There are a bunch of OSX errors about [ERROR ] Failed to add the ssh key. Is the home directory available, and/or does the key file exist?

aneeshusa

I haven't tested this, but looks good overall so far. I left some Salt comments.

aneeshusa · 2017-04-11T02:54:15Z

admin/files/sshd_config

+# Using regular users in combination with /bin/su or /usr/bin/sudo ensure a clear audit track.
+PermitRootLogin No
+
+# Use kernel sandbox mechanisms where possible in unprivilegied processes


spelling: unprivileged

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

+
+{% for ssh_user in admin.ssh_users %}
+{{ ssh_user }}:
+    user.present:


Before user.present, we should also use group.present to create a group for each user with the same name. We should require the group state from the user state and set gid_from_name: True in the user state.

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

+{{ ssh_user }}:
+    user.present:
+        - empty_password: True
+        - optional_groups:


We should use groups instead of optional_groups so Salt complains if this doesn't work.

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

+    user.present:
+        - empty_password: True
+        - optional_groups:
+            - wheel


Might be nice to add a group.present state for the wheel group itself.

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

+        - empty_password: True
+        - optional_groups:
+            - wheel
+    ssh_auth.present:


This should require the user.present state.

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

+
+# FIXME This is just as bad as all sharing root login.
+/etc/sudoers:
+    file.append:


I've personally never been a fan of file.append, I prefer to use file.managed and control the whole contents instead of hoping that Salt is always smart enough to do the right thing with file.append.

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

+/etc/sudoers:
+    file.append:
+        - text:
+            {% for ssh_user in admin.ssh_users %}


We could provide permissions to the wheel group instead of each user individually.

aneeshusa · 2017-04-11T02:54:15Z

admin/init.sls

-{% for ssh_user in admin.ssh_users %}
-sshkey-{{ ssh_user }}:
-  ssh_auth.present:
+# FIXME we should also explicitly AllowUsers usera userb userc


We could also use AllowGroups wheel, I think.

I believe we want to end up with as few people having root access as possible, so most users who can access the hosts to examine logs or whatever will not be in wheel. Thus they'll either need to be individually added with AllowUsers, or all added to a group which in turn is added using AllowGroups. While a dedicated group would have the same effect as individually allowing, it adds a layer of indirection when trying to figure out which users have access compared to just looking at sshd_config.

So my FIXME was pretty unclear, but I'm pretty sure it's an issue that AllowGroups wheel does not adequately address.

aneeshusa · 2017-04-11T02:56:51Z

It might also be nice to switch to a non-standard port to reduce log spam, since we've set the log level to verbose.

aneeshusa · 2017-04-25T22:38:36Z

It might also be cool to integrate https://github.com/mozilla/ssh_scan as a test.

aneeshusa · 2017-05-06T20:41:04Z

If we want to implement #657, I suggest the following changes:

Don't provide sudoers access to these per-user accounts; we'll implement the Publisher ACL in a separate PR
Keep root SSH access enabled for now

After testing Publisher ACL, we can enable sudoers access if necessary and drop root SSH access.

bors-servo · 2017-06-14T18:53:22Z

☔ The latest upstream changes (presumably #694) made this pull request unmergeable. Please resolve the merge conflicts.


        speeling


        Temporarily leave root login till folks fix their passwords


        Groups and requiring them


        no sudoers access per-user.

edunham · 2017-10-26T17:55:05Z

Feedbacks addressed; merging in master and fixing the conflicts now


        Merge branch 'master' of github.com:servo/saltfs into accts

sshkeys should not be needed because the states for individual user login should be smart enough to make the directories they need. Should. We will briefly need similar states to these back for removing all the authorized keys once we verify that user logins are working.

aneeshusa

Thanks for updating this! I'd made #725 in the meantime with a more robust SSH config and a test, so how do you feel about taking the SSH bits out of this PR and having this PR about creating individual accounts?

aneeshusa · 2017-10-27T05:15:30Z

admin/init.sls

+        - groups:
+            - wheel
+        - require:
+            - group:


Unless this is a new feature, I don't think this works and we'll need:

- group: wheel - group: {{ ssh_user }}

as two separate lines.

oops, yeah. Fixed.

aneeshusa · 2017-10-27T05:15:30Z

admin/init.sls

+{% for ssh_user in admin.ssh_users %}
+{{ ssh_user }}:
+    group.present:
+        - members:


We need to create the group first, and then the user, so this state shouldn't have a members key as that would be a circular dependency. The gid_from_name parameter in the user.present state will take care of this.

FYI, since the group.present will be empty at that point, just set it to an empty list as so to get the YAML to parse correctly: https://docs.saltstack.com/en/2016.3/topics/troubleshooting/yaml_idiosyncrasies.html#yaml-does-not-like-double-short-decs

aneeshusa · 2017-10-27T05:15:30Z

admin/init.sls

+    - mode: 644
+    - source: salt://{{ tpldir }}/files/sshd_config
+
+wheel:


I agree with your comment about only providing some users root access. To prevent confusion, let's rename this group to sshusers, and reserve the wheel group for users that will have root perms.

aneeshusa · 2017-10-27T05:15:30Z

admin/init.sls

+            - group:
+                - wheel
+                - {{ ssh_user }}
+    ssh_auth.present:


We'll need to keep the two-state solution of file.managed to continue being able to handle SSH key rotation and revocation.

aneeshusa · 2017-10-27T05:15:30Z

admin/init.sls

-    - mode: 700
-
-sshkeys:
+/etc/sshd_config:


We should keep the states that set up the SSH keys for the root user, as we don't want to lock ourselves out (and at least for now want to keep installing root SSH keys on new machines).

aneeshusa · 2017-10-27T05:18:39Z

Also, since it doesn't look like the Publisher ACL feature panned out as I was expecting, feel free to bring back the sudoers-related states. A rebase would also be helpful for cleaning up the history a bit.


        Clean up individual accounts

edunham · 2017-10-27T18:12:30Z

@aneeshusa Should be more-right. I un-clobbered your SSH states, restricted it to not trying to do Mac because getting account creation right on OSX is 🤷‍♂️, but kept the sshd config addition.

Looking ready for squash?

Can't find a "I made the requested changes" button, on the review... should be ready for another look now though


        Merge branch 'master' of github.com:servo/saltfs into accts

edunham added 2 commits Mar 27, 2017

Create individual accounts

dd752e0

explicitly forbid root login

Loading status checks…

7d61ae0

edunham added the DO NOT MERGE YET label Mar 27, 2017

highfive assigned aneeshusa Mar 27, 2017

highfive added the S-awaiting-review label Mar 27, 2017

Consolidate user account management into single state

Loading status checks…

e841322

edunham mentioned this pull request Mar 27, 2017

Create user accounts instead of sharing root #253

Closed

Deploy Mozilla-recommended SSHD config

Loading status checks…

bb9d9eb

See https://wiki.mozilla.org/Security/Guidelines/OpenSSH

edunham mentioned this pull request Mar 27, 2017

Deploy Mozilla-recommended sshd config #254

Closed

single ID for sudoers line

Loading status checks…

edffbd7

edunham force-pushed the edunham:accts branch from 1fc0c9d to edffbd7 Mar 27, 2017

reduce account creation to cross-platform arguments

Loading status checks…

aa76c08

aneeshusa suggested changes Apr 11, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Apr 11, 2017

aneeshusa mentioned this pull request May 5, 2017

Use Salt publisher ACL system to allow Salt usage without root access #657

Closed

highfive added the S-needs-rebase label Jun 14, 2017

aneeshusa mentioned this pull request Sep 30, 2017

Manage sshd_config file #725

Closed

edunham added 4 commits Oct 26, 2017

speeling

5095c29

Temporarily leave root login till folks fix their passwords

72ef667

Groups and requiring them

10873fa

no sudoers access per-user.

04ba2b3

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 26, 2017

edunham force-pushed the edunham:accts branch from 9306996 to eb905c0 Oct 26, 2017

aneeshusa reviewed Oct 27, 2017

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Oct 27, 2017

Clean up individual accounts

Loading status checks…

5d41c38

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 27, 2017

Merge branch 'master' of github.com:servo/saltfs into accts

Loading status checks…

273cdd3

edunham closed this May 21, 2020

servo / saltfs

Replace shared root login with user accounts #628

Replace shared root login with user accounts #628

edunham commented Mar 27, 2017 •

edited by larsbergstrom

edunham commented Mar 27, 2017

larsbergstrom commented Apr 1, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Apr 11, 2017

aneeshusa commented Apr 25, 2017

aneeshusa commented May 6, 2017

bors-servo commented Jun 14, 2017

edunham commented Oct 26, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Oct 27, 2017

edunham commented Oct 27, 2017

servo / saltfs

Join GitHub today

Replace shared root login with user accounts #628

Replace shared root login with user accounts #628

Conversation

edunham commented Mar 27, 2017 • edited by larsbergstrom

edunham commented Mar 27, 2017

larsbergstrom commented Apr 1, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Apr 11, 2017

aneeshusa commented Apr 25, 2017

aneeshusa commented May 6, 2017

bors-servo commented Jun 14, 2017

edunham commented Oct 26, 2017

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Oct 27, 2017

edunham commented Oct 27, 2017

edunham commented Mar 27, 2017 •

edited by larsbergstrom