cloud-init icon indicating copy to clipboard operation
cloud-init copied to clipboard

Support sudoers in the "usr merge" location

Open rjschwei opened this issue 1 year ago • 5 comments

Proposed Commit Message

When sudo is compiled with the secure-path option and the configuration file exists in /usr/etc and not in /etc take the configuration in /usr/etc into account. If the desired include directive exists in the system (/usr/etc ) configuration there is nothing to do if there is no file in etc.

Additional Context

Test Steps

Checklist

Merge type

  • [x] Squash merge using "Proposed Commit Message"
  • [ ] Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

rjschwei avatar Apr 08 '24 21:04 rjschwei

@blackboxsw need some help with the test please, I do not understand why I am triggering a permission error in the mock for the file write. Thanks

rjschwei avatar Apr 08 '24 21:04 rjschwei

OK, thanks for the questions, made me realize that I had a logic error. There is no need to copy the content from /usr/etc/sudoers if it exists. If the "secure path" file does not have the desired include directive and the "sysadmin location", i.e. /etc/sudoers does not exist we simply write the include directive to /etc/sudoers . If /etc/sudoers exists but does not have the include directive we append there to preserve the current behavior.

rjschwei avatar Apr 25 '24 22:04 rjschwei

Looks pretty good for handling the case of /usr/etc/sudoers vs an absent /etc/sudoers file. Some general questions I have are:

  • Is the a changeset represented in upstream sudo project that points to this behavior and/or /usr/etc/sudoers overrides?

Not that I can find. The existence of /usr/etc appears to be quite old and can be when building the upstream sources with --enable-secure-path. For SUSE distributions we set the configuration explicitly with

%if %{defined _distconfdir}
    --prefix=/usr \
    --sysconfdir=%{_distconfdir} \
    --enable-adminconf=%{_sysconfdir} \
%endif

The condition on defined _distconfdir exists to differentiate the build between distributions that have /usr/etc and distributions that do not.

https://build.opensuse.org/projects/openSUSE:Factory/packages/sudo/files/sudo.spec?expand=1

  • what documentation tells us we should ignore /usr/etc/sudoers when /etc/sudoers exits?

I changed that in the current update. I twisted myself into a knot with the first go around. The logic is more straight forwrad now, IMHO.

  • What should be the behavior on a system with both /etc/sudoers.d and /usr/etc/sudoers.d? Do all the sudo config parts.d directory get aggregated?

The default SUSE configuration is to include /usr/etc/sudoers.d and then /etc/sudoers.d. So the answer is yes they do. The updated logic makes sure that /etc/sudoers.d is included last if no include directive exists in either /etc/sudoers or /usr/etc/sudoers preserving the current behavior.

rjschwei avatar Apr 25 '24 22:04 rjschwei

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

github-actions[bot] avatar May 15 '24 00:05 github-actions[bot]

@blackboxsw I think I addressed the requested changes. Anything else?

rjschwei avatar May 15 '24 12:05 rjschwei