Migrate Puppet sudo configs to saz/sudo module

added Backlog label

assigned to @lavamind

changed the description

changed milestone to %cleanup and publish the sysadmin codebase

add a new profile::sudo class to create sudo::conf resources from Hiera data

isn't that automatic when you include sudo?

or is that what the profile::sudo class would do, basically?

move the sudoers-file resource in role::network_health_relay to Hiera

thanks.

progressively convert monolithic sudoers to Hiera

i'd keep that out of scope for now: let's deploy that large sudoers file through the new sudo module for now, and progressively remove chunks of it as we see fit.

the reason for this is that it is a long process. that file is huge and will take a long time to convert. i would prefer to have it in a separate ticket, if we really want to do that. and anyways it's something we could already do by using file snippets (but i agree would be better with the new module).

isn't that automatic when you include sudo? or is that what the profile::sudo class would do, basically?

This is what I've used in the past:

class profile::sudo {
  include sudo
  $configs = lookup('profile::sudo::configs', Hash, 'hash', {}) 
  if !empty($configs) {
    $configs.each | $name, $config | { 
      create_resources('::sudo::conf', { $name => $config })
    }   
  }
}

The main reason was to explicitely use the hash Hiera merge strategy (see the lookup() call) for these configs instead of the default first, but I'll admit I don't remember a case when it was useful although I haven't worked with such complex sudo rules as TPO seems to need. Also I think we could also configure the merge strategy for sudo::configs in Hiera itself and just do away with the profile entirely.

i'd keep that out of scope for now: let's deploy that large sudoers file through the new sudo module for now, and progressively remove chunks of it as we see fit.

Alright, that works for me. In this case I would move the relevant bits from sudo_legacy to profile::sudo and do away with sudo_legacy.

Is that what that blob is for in the saz/sudo README file?

lookup_options:
  sudo::configs:
    merge:
      strategy: deep
      merge_hash_arrays: true

I think it might be worth skipping the extra profile thing here. If most stuff is going to be in hiera anyways, maybe it's better to do it cleanly from the start?

Also, I just thought of something: for roles like the network-health relay, I think it makes sense to have the rule in Hiera because there's really nothing else to add to the role (for now?). But wouldn't it be better for some sudo rules to live in the profile if, for example, there are dependency with other resources? I feel that having the sudo rule inside the profile also makes the mechanism clearer...

In fact, now that I think of it, maybe in the case of the network health, we should have created a "tpa-rfc-7" profile to grant that exception, since that is something we're likely to reuse, and it's going to be easier and move obvious that we've done that with a profile. (Plus, we'll be able to easily audit which nodes have that class, if we need to.)

But that's a discussion for the other ticket I guess...

...

On 2022-02-23 19:04:01, Jérôme Charaoui (@lavamind) wrote:

Jérôme Charaoui commented on a discussion: #40633 (comment 2781149)

isn't that automatic when you include sudo? or is that what the profile::sudo class would do, basically?

This is what I've used in the past:
class profile::sudo {
  include sudo
  $configs = lookup('profile::sudo::configs', Hash, 'hash', {}) 
  if !empty($configs) {
    $configs.each | $name, $config | { 
      create_resources('::sudo::conf', { $name => $config })
    }   
  }
}
The main reason was to explicitely use the hash Hiera merge strategy (see the lookup() call) for these configs instead of the default first, but I'll admit I don't remember a case when it was useful although I haven't worked with such complex sudo rules as TPO seems to need. Also I think we could also configure the merge strategy for sudo::configs in Hiera itself and just do away with the profile entirely.

-- Antoine Beaupré torproject.org system administration

good, i changed the task list to reflect that.

mentioned in issue #40630 (closed)

changed the description

added Puppet label

added Icebox label and removed Backlog label

added Backlog label and removed Icebox label

after dealing with this fuckery in a panic this morning, i'd like to re-prioritize this:

https://social.treehouse.systems/@psykose/109967460650885493

basically, i had to go through each sudoers thing out there and figure out what snippets we generate. it's far from obvious. i used git grep sudoers.d as a heuristic, but i'm not sure that matches everything. using the module could also allow us to have rules in puppet to keep this nasty stuff from happening.

we could also consider ditching sudo altogether in favor of something like doas, but maybe that's a bit too involved at this stage.

Agreed, I can get started this week.

added Next label and removed Backlog label

added Doing label and removed Next label

marked the checklist item audit saz/puppet-sudo as completed

I audited saz/puppet-sudo today, tag 7.0.2 (commit id f8fe9fa39acf60b349182cff448709459a1c3547).

Everything checks out, it's a relatively small module, and I didn't see anything that would clash with our infrastructure besides the sudo namespace issue we already identified.

I pushed the sudo-module branch in our tor-puppet repo, containing the audited code.

I think it might be worth skipping the extra profile thing here. If most stuff is going to be in hiera anyways, maybe it's better to do it cleanly from the start?

I think it's inevitable we'll need profile::sudo because currently sudo aka sudo_legacy deploys some sudo-related bits for PAM (libpam-pwdfile package and /etc/pam.d/sudo), and to me makes the most sense to have it there.

Also, I just thought of something: for roles like the network-health relay, I think it makes sense to have the rule in Hiera because there's really nothing else to add to the role (for now?). But wouldn't it be better for some sudo rules to live in the profile if, for example, there are dependency with other resources? I feel that having the sudo rule inside the profile also makes the mechanism clearer...

Yeah I think its fine if we have some rules in Hiera and some in profiles, it's probably also kind of inevitable.

In fact, now that I think of it, maybe in the case of the network health, we should have created a "tpa-rfc-7" profile to grant that exception, since that is something we're likely to reuse, and it's going to be easier and move obvious that we've done that with a profile. (Plus, we'll be able to easily audit which nodes have that class, if we need to.)

We already do have something like that actually: profile::sudo::root_exception.

Worked on this some more today, and its pretty clear that our current sudo class, even renamed to sudo_legacy cannot coexist with the new sudo module because both fight over managing /etc/sudoers which is the main sudo configuration file.

So what I've come up with is minimal modifications to the current legacy sudoers so its appropriate to ship as a /etc/sudoers.d configuration file:

Moved or removed Defaults directives
Removed #includedir statement

So what I propose we ship now is the profile::sudo replacement class I just pushed to the sudo-module branch.

One of the risks is that by default the sudo module will purge any unmanaged configs in /etc/sudoers.d, but that's a feature, not a bug: we'll be able to identify and remedy any of these as we deploy the module.

Would you give it a quick review, @anarcat ?

LGTM. I was puzzled by the #includedir removal, until I understood the main config file moved into a snippet...

I take it the other File resources dropping stuff in sudoers.d will be converted at a later stage and will otherwise correctly be enforced?

...

On 2023-03-20 20:28:11, Jérôme Charaoui (@lavamind) wrote:

Would you give it a quick review, @anarcat ?

I take it the other File resources dropping stuff in sudoers.d will be converted at a later stage and will otherwise correctly be enforced?

Right, for now I think its best to focus on deploying the module, but converting those File resources to Sudo::Conf should be quite easy.

The branch is ready for merging but I got side-tracked this week with helping out #40972 (closed) ... but now Puppet is enabled again everywhere so we should be able to proceed next Monday.

The sudo-module branch has been merged and deployed on all the nodes.

Let's see how this goes until next Monday, and then proceed to convert any remaining File-managed sudoers files to Sudo::Conf.

added Next label and removed Doing label

Migrate Puppet sudo configs to saz/sudo module

Designs

Child items ...

Activity