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 650321e

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.