syntax check should only check the file it needs to

[TJM]

The syntax check visudo -c || ( rm -f '/etc/sudoers.d/01_sensu' && exit 1) will prevent any new sudo rules from being added in if someone "else" has created a file in /etc/sudoers.d with the wrong permissions (i.e. not setting mode at all). This results in an error that looks like:

Notice: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/File[01_sensu]/ensure: defined content as '{md5}98070207913a6c126f19208bf2b2e5ea'
Info: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/File[01_sensu]: Scheduling refresh of Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu]
Debug: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/File[01_sensu]: The container Sudo::Conf[sensu] will propagate my refresh event
Debug: Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu](provider=posix): Executing 'visudo -c || ( rm -f '/etc/sudoers.d/01_sensu' && exit 1)'
Debug: Executing: 'visudo -c || ( rm -f '/etc/sudoers.d/01_sensu' && exit 1)'
Notice: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu]/returns: /etc/sudoers.d/10_oes_sudoers: bad permissions, should be mode 0440
Notice: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu]/returns: /etc/sudoers: parsed OK
Notice: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu]/returns: /etc/sudoers.d/01_sensu: parsed OK
Error: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu]: Failed to call refresh: 'visudo -c || ( rm -f '/etc/sudoers.d/01_sensu' && exit 1)' returned 1 instead of one of [0]
Error: /Stage[main]/Sudo::Configs/Sudo::Conf[sensu]/Exec[sudo-syntax-check for file /etc/sudoers.d/01_sensu]: 'visudo -c || ( rm -f '/etc/sudoers.d/01_sensu' && exit 1)' returned 1 instead of one of [0]
Debug: Sudo::Conf[sensu]: Resource is being skipped, unscheduling all events
Info: Sudo::Conf[sensu]: Unscheduling all events on Sudo::Conf[sensu]

Where the file that it complains about (/etc/sudoers.d/10_oes_sudoers) is not the file it is checking/removing (/etc/sudoers.d/01_sensu).

I have had a chat with the people that created the bogus file (they obviously had seen the filename format that saz-sudo uses, given the name they picked). I changed their code to use "sudo::conf" instead of "file" and everything was fine, but the fact remains that we should probably add the -f FILE option to our visudo command.

Suggested Fix: use visudo -c -f 'FILENAME' || ( rm -f 'FILENAME' && exit 1)

[nmaludy]

+1 this just bit me

[lelutin]

arguably there is already a check on the specific files being installed with the "validate_cmd" param on the file resource in sudo::conf.

I would guess that the more desctuctive check triggered by $delete_on_error had the purpose of avoiding to create configuration that breaks sudo access. however, given the reported issue, it is not being super helpful in those cases.

@TJM you could disable the visudo -c || (rm .. check by setting $delete_on_error to false on the main class

[TJM]

Disabling syntax checking or recovery from syntax errors are crude workarounds. An unrelated issue can cause this specific resource to fail in strange an mysterious ways. If we only check the file being deployed, it should only fail if that specific file has a syntax issue. If the other file has permissions that are non-compliant, then let that "unmanaged" rule fail.

~tommy

[lelutin]

@TJM I'm sorry I've used words that were misleading (I'm re-reading myself now).

that parameter that I mentioned, $delete_on_error, if set to false it will not disable any check but rather it will prevent the module from removing the deployed file on error. this will make puppet runs show an error but won't prevent you from installing new sudo rules.

I do agree though that something is not right in that exec that checks syntax errors for all files (and does it for all sudo::conf resources) such a general check should happen only once in init.pp just to make puppet runs produce an error if sudo has a syntax error, and I would advocate to get rid of $delete_on_error

[saz]

Looking at the whole visudo check, this looks more complicated than it should or I'm missing something.

The change has been introduced in https://github.com/saz/puppet-sudo/commit/650321efd922fe7cd0455d3a48a8779792b39b1a

As far as I can see, if sudo::validate_single is set to true, visudo will be used for only the changed file as validate_cmd on the file resource.

Shouldn't it be enough to change the default of validate_single to true and we will have a check for each file and a complete check with the exec itself?

[TJM]

I thought the same thing, way overcomplicated. To me, it is invalid logic (bad assumption) to add a new file, check all the files, and if any of them fail, remove the new file. It only makes sense to check the new file itself, and if it fails, remove it. In my opinion, validate_single should be true by default.

However, validate_single would not disable the logic of checking all files and removing the new one. For that you would also have to set delete_on_error to false, but then it tries to modify the file, appending the error as a comment?

Checking the syntax of ALL the files and issuing a failure message can still be an option, but it shouldn't cause a removal of the newly added file unless that file was the one that was bad.

I suggest making validate_single true by default, and defanging the other "sudo syntax check" (exec). Either remove that logic entirely, or possibly make it an option to syntax check all unmanaged files too? Only makes sense without purge probably, and then you would have to do some trickery with exec like:

  exec {"sudo-syntax-check for all files":
    command     => "visudo -c",
    unless      => "visudo -c",
    path        => $sudo_syntax_path,
  }

... that would effectively run visudo -c every time, and if it returned in error, it would run it a second time (super efficient, right?), and display the error result? (if I got the logic correct)

What do you think? ~tommy

[h0tw1r3]

Generally speaking, prefer the current behavior. A sudo::conf may be valid by itself but in the context of the whole configuration (eg. order dependent, duplicate definitions, ???) the configuration breaks.

That said, I haven't looked at the code. Perhaps that could use some ❤️

[TJM]

I am not sure if it makes sense to have a problem (even just file permissions) in another file prevent adding a new one.

[oraziobattaglia]

Hello, I had the same problem, a sudoer file with wrong permissions, created out of puppet, deleted all my sudoers :-O! I agree with TJM that will be better a single file approch like "visudo -c -f 'FILENAME' || ( rm -f 'FILENAME' && exit 1)".

I upgraded the module today, we had an old version, but I was forced to roll back. Thanks

[jamesps-ebi]

Any movement on this?

I'm in agreement that the behaviour seems wrong. We should only be checking validity on the files controlled by Puppet. i.e.

visudo -c -f 'FILENAME' || ( rm -f 'FILENAME' && exit 1)

But I can also see a use-case for checking the entire sudoers config as a whole. Maybe we can run visudo -c and just return the output as a warning if the command gives a non-0 exit status.

[tdlc]

Hello,

I had the same problem - some application created a sudoers file with permission 0400. But visudo expects 0440 and returns 1 / error. Then new sudoers files cannot be created by saz-sudo. This is independent from the setting validate_single. No matter if it's true or false saz-sudo will always check all sudoers files.

The problem is at the end of conf.pp:

  exec { "sudo-syntax-check for file ${cur_file}":
    command     => "visudo -c || ${delete_cmd}",
    refreshonly => true,
    path        => $sudo_syntax_path,
  }

Here all files will be checked unconditionally.

Best wishes

[tdlc]

I have made a pull request which is basically the same as the one by jamesps-ebi, but I also removed the parameters delete_on_error, validate_single and sudo_syntax_path which don't make sense anymore.

Puppet will not create an invalid sudo file because the file resource uses the validate_cmd:

  if $ensure == 'present' {
    $validate_cmd_real = 'visudo -c -f %'
  } else {
    $validate_cmd_real = undef
  }

  file { "${priority_real}_${dname}":
    ensure       => $ensure,
    path         => $cur_file_real,
    owner        => 'root',
    group        => $sudo::params::config_file_group,
    mode         => $sudo::params::config_file_mode,
    source       => $source,
    content      => $content_real,
    require      => File[$sudo_config_dir_real],
    validate_cmd => $validate_cmd_real,
  }

Made some tests on RHEL 8.

[saz]

@tdlc Thanks for picking this topic up again. I now understand your comment in your PR.

I'd rather keep the parameters, as this makes (or keeps) the module flexible, and maybe add another one: full_syntax_check

1. Change the default of validate_single from false to true
2. Make the full syntax check configurable, with false by default

At first, this makes things more complicated, but there's a history to how things are. See https://github.com/saz/puppet-sudo/issues/125 for an example of a valid case, when a full config check will be good and prevent you from running into an issue.

[tdlc]

Ok I get the point of #125 that a full syntax check of all files can be desirable in some situations. Anyway the are some problems with the parameters as they are now:

• the exec in conf.pp is probably run for each file separately. So the validation of all files runs multiple times. It should probably be moved to init.pp or somewhere else and only run once.
• It is impossible to validate only a single file because $notify_real = Exec["sudo-syntax-check for file ${cur_file}"] is always set no matter which value validate_single has. The exec is always called additionally and will validate all files.
• If validate_single is true and delete_on_error is true an invalid file would not be created because the file resources's validate_cmd will prevent that. Nevertheless $delete_cmd could try to delete it if there's a global syntax error in some file.
• If delete_on_error is false I think the code will append an error message to the file endlessly. See $delete_cmd.

All this problems led me to the conclusion that I want to keep it simple and I removed the parameters. But it should be possible to write some sensible code containing some sort of full_syntax_check.

[tdlc]

I have quickly hacked some code not thoroughly tested. Take a look at this branch if you like.

[tdlc]

I have made up my mind on this issue again and will add a new mr:

• Changed validate_single to really validate a single file when true. When false it will validate all files. This is a little different from previous behaviour where the exec would always validate all files. I now think it makes sense to run the full validation also per file as each single file could break configuration as a whole.
• Removed delete_on_error as invalid files should never be created. And might lead to infinite messages in file when false.
• Removed sudo_syntax_path as it can't be used for validate_cmd.

This is completely untested code - beware. I can test this on RHEL 7 8 9 if you are interested. Are you interested?