-
Notifications
You must be signed in to change notification settings - Fork 248
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
A couple of cosmetic changes related to IFP: #6868
Conversation
alexey-tikhonov
commented
Aug 9, 2023
- warn loudly in case of dbus service policy misconfiguration - this might be common pitfall during transition toward non-privileged sssd
- remove excessive "Exec=" from dbus service file in case SSSD is built with system support (it had a wrong value anyway, imo)
Well... it works, but https://dbus.freedesktop.org/doc/dbus-specification.html says:
I think this is just outdated wording, pre-"SystemdService" key was introduced, but formally it says so. Still I'm not sure what should be a value of "Exec=". @pbrezina, what would you say? |
To summarize, there are 3 different ways how 'sssd_ifp' can be started: "Exec=" option in dbus service description is formally mandatory and in case There is clearly a contradiction between (1) and (3) We can either disallow starting 'sssd_ifp' via 'monitor' (as this is more or less the case with 'sssd_kcm'), or stop keeping (3) in mind while maintaining config files. |
426ed3a
to
c9c9f67
Compare
Ok, I opted for
and just added a dynamic comment that explains "Exec=" option for cases (1) and (2). |
Makefile.am
Outdated
@@ -105,6 +106,7 @@ if SSSD_NON_ROOT_USER | |||
additional_caps = CAP_DAC_OVERRIDE | |||
endif | |||
else | |||
ifp_dbus_exec_comment = \# "sss_sinal" is used to force SSSD monitor to trigger "sssd_ifp" reconnection to dbus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/sss_sinal/sss_signal/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor typo, but besides that Ack LGTM.
to avoid potential confusion
c9c9f67
to
aaa3db8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.