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

Only add to important and existing groups + sudo #50

Closed
wants to merge 1 commit into from

Conversation

jonhoo
Copy link

@jonhoo jonhoo commented Jul 22, 2017

Fixes #49.

@hartwork
Copy link
Owner

hartwork commented Jul 23, 2017

I had a bit of time to think about it now and I would rather do that for Arch, only: Group names and defaults may vary across distributions. I'm considering to extend existing class ArchStrategy like this, instead:

class ArchStrategy(DistroStrategy):
[..]
    def adjust_cloud_cfg_dict(self, cloud_cfg_dict):
        super(ArchStrategy, self).adjust_cloud_cfg_dict(cloud_cfg_dict)

        system_info__default_user = system_info.setdefault('default_user', {})
        system_info__default_user.setdefault('sudo', ['ALL=(ALL) NOPASSWD:ALL'])
        # Get rid of groups cdrom, dailout, dip, netdev, plugdev, sudo:
        system_info__default_user['groups'] = ['adm']

What do you think?

@jonhoo
Copy link
Author

jonhoo commented Jul 23, 2017

I believe adm applies on every distro that has systemd (though I could be mistaken). I also think it's fine for that to be the only group the cloud user is a member of, as all the others listed are largely unnecessary (and can be regained through sudo if need be). The sudo rule will also work on all distros (it's a part of cloud-init, and is not Arch-specific).

@hartwork
Copy link
Owner

For debian, users should be in the sudo group, according to https://wiki.debian.org/sudo. So just adm is not friendly with Debian.

Yes, the sudo rule would work everywhere but may not fit the approach that distros has towards sudo, organically, e.g. the wheel group is common use for sudo on Gentoo.

Would you adjust #50 (comment) further with just Arch in mind?
Are you using image-bootstrap for something other than Arch btw?

@jonhoo
Copy link
Author

jonhoo commented Jul 23, 2017

I'm not entirely sure I agree with that assessment: The sudo/wheel groups are convenient shorthands in some distros for giving a set of users sudo access, but they are not necessary in order to provide sudo. The cloud-init sudo directive is specifically meant for giving sudo-access for a specific user (i.e., the cloud user) in a distro-independent manner. And it does it by adding a file to /etc/sudoers.d/, which is the standard and recommended way to give permissions to only a particular user. It is also worth pointing out that some distros (Arch included) do not have such a group by default — you can modify /etc/sudoers to make sudo or wheel "special" groups, but that's a manual process.

adm is an entirely different story, and is unrelated to sudo. It is a special user for systemd purposes (it is distro-independent), and gives password-less access to journalctl among other things. Regardless of whether you choose to add the user to sudo/wheel, they should also be added to adm.

To answer your question, no, I would not modify it further. I believe that config is the minimal config for any distro. And no, I'm not, though that may change if support for Alpine lands.

@hartwork
Copy link
Owner

I guess you can either have the sudoers extention isolated in a way that works for all distros, or integrated with the common pattern of that distro. I think you have a point here that the isolated version works for all.

I also notice that we have a mix of two different things here: A patch that we want for all distros — the sudo part — and the group fix, that only Arch may need (and that is rather "static"). So I have split that up now. I made #53 for that now, please review.

@jonhoo
Copy link
Author

jonhoo commented Jul 23, 2017

Yup, I prefer #53 too.

@jonhoo jonhoo closed this Jul 23, 2017
@jonhoo jonhoo deleted the fix-user-creation branch July 25, 2017 17:11
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