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

Fixing sssd.service: #7783

Closed
wants to merge 2 commits into from
Closed

Conversation

alexey-tikhonov
Copy link
Member

@@ -9,7 +9,7 @@ Also=sssd-kcm.socket

[Service]
Environment=DEBUG_LOGGER=--logger=files
ExecStartPre=+-/bin/chown -f -R -h root:@SSSD_USER@ @sssdconfdir@
ExecStartPre=+-/bin/chown -f -R -H root:@SSSD_USER@ @sssdconfdir@
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mgerstner,

do you think this change is reasonable taking into account that:

  1. it requires root to write to /etc (and so to make /etc/sssd a symlink)
  2. with -H and -R only command line argument is dereferenced
    ?

#7781 has some details/reason.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recursion logic (-R) of GNU chown is already safe against race conditions and symbolic links appearing in the directory tree, that is correct.

If you want to support the @sssdconfdir being a symlink and if it reasonable to assume that the location it points to is only root controlled, which is the case for /etc/sssd, then passing -H should be fine. I would document the reason for passing -H here then, though, because it's use is a bit special.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

to support use case where /etc/sssd is a symlink.

'-H' only allows following a command line argument itself,
everything else encountered due to '-R' isn't followed.

This is an update to a20fa0f

Resolves: SSSD#7781
@alexey-tikhonov
Copy link
Member Author

Rebased and added comments.

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the fix and adding the comments, ACK.

bye,
Sumit

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for taking care of it.

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7783

  • master
    • 561c51b - SYSTEMD: fix missing 'g+x' on /etc/sssd and subdirs
    • 4b35ac3 - SYSTEMD: traverse 'sssdconfdir' symlink while chown-ing
  • sssd-2-10
    • a0ab704 - SYSTEMD: fix missing 'g+x' on /etc/sssd and subdirs
    • fa26915 - SYSTEMD: traverse 'sssdconfdir' symlink while chown-ing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants