URL: https://github.com/SSSD/sssd/pull/183 Author: fidencio Title: #183: More socket-activation fixes Action: opened
PR body: """ This series contain two patches for bugs reported by @sgallagher. Please, see the patches ... """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/183/head:pr183 git checkout pr183
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/183 Author: fidencio Title: #183: More socket-activation fixes Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/183/head:pr183 git checkout pr183
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ Patch set has been updated. Please, take a look on the commit messages for a good explanation (mainly for the "Avoid TOCTOU vulnerability with the log files" patch). """
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285362014
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ NACK to removing non-privileged user from all services. Ticket https://pagure.io/SSSD/sssd/issue/3322 is only about sssd-nss.service We might use numeric values `/bin/chown 0:0` in sssd-nss.service or drop `chown` from _sssd-nss.service_ ; because root can write even to files owned by non-privilegen user.
@sgallagher The purpose of calling `chown` in `ExecStartPre` is to allow starting responders as non-privileged from beginning. Systemd drops permissions before `exec`.
`chown_debug_file` is used only with monitor + non-privileged user because we have capability to change owner of file as root and then drop privileges. There might be TOCTOU but it is not related to socket activated services.
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285617124
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
sgallagher commented: """ @lslebodn
@sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use `/usr/bin/chown` for this, because it unconditionally calls `getpwnam()`/`getpwuid()` in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality `/usr/libexec/sssd/sss_chown` that calls only the low-level system function. """
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285667642
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ @sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
In other others, it would solve the situation but only for the NSS responder.
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
That's my suggestion ... but I'd go with whatever you guys agree on ...
Best Regards, -- Fabiano Fidêncio
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285673416
On (10/03/17 14:50), fidencio wrote:
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ @sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
we can call "getpwnam()/getpwuid()" call responders except sssd-nss and monitor.
In other others, it would solve the situation but only for the NSS responder.
Is there any problem to start other responders after sssd-nss?
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
static uid/gid is not a solution; it's just a workaround.
LS
On (10/03/17 17:50), Lukas Slebodnik wrote:
On (10/03/17 14:50), fidencio wrote:
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ @sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
we can call "getpwnam()/getpwuid()" call responders except sssd-nss and monitor.
In other others, it would solve the situation but only for the NSS responder.
Is there any problem to start other responders after sssd-nss?
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
static uid/gid is not a solution; it's just a workaround.
UPS wron mail; I will reply to the PR
LS
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ @sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
In other others, it would solve the situation but only for the NSS responder.
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
That's my suggestion ... but I'd go with whatever you guys agree on ...
Best Regards, -- Fabiano Fidêncio
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285673416
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
jhrozek commented: """ On Fri, Mar 10, 2017 at 05:50:58AM -0800, fidencio wrote:
@sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
In other others, it would solve the situation but only for the NSS responder.
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
btw I think this is better instead of providing a hack because by default, even if the service is started explicitly in the [sssd] section, it runs as root. As long as we track switching to nonroot in the next release, I prefer running as root over adding hacks to the code.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
That's my suggestion ... but I'd go with whatever you guys agree on ...
Best Regards,
Fabiano Fidêncio
-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/SSSD/sssd/pull/183#issuecomment-285673416
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285674937
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ On (10/03/17 05:50), fidencio wrote:
@sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
we can call "getpwnam()/getpwuid()" in responders except sssd-nss (and monitor).
In monitor we should set/unset env variable `_SSS_LOOPS`
In other others, it would solve the situation but only for the NSS responder.
Is there any problem to start other responders after sssd-nss?
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
static uid/gid is not a solution; it's just a workaround.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285722632
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ On (10/03/17 05:57), Jakub Hrozek wrote:
On Fri, Mar 10, 2017 at 05:50:58AM -0800, fidencio wrote:
@sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling chown in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
In other others, it would solve the situation but only for the NSS responder.
What I'm proposing is to take a step back and do *not* support unprivileged users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
btw I think this is better instead of providing a hack because by default, even if the service is started explicitly in the [sssd] section, it runs as root. As long as we track switching to nonroot in the next release, I prefer running as root over adding hacks to the code.
sssd-nss runs as root by default.
We just need to remove chown from sssd-nss.service
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285723904
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ On Fri, Mar 10, 2017 at 5:54 PM, lslebodn notifications@github.com wrote:
On (10/03/17 05:50), fidencio wrote:
@sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <
notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling
chown
in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
we can call "getpwnam()/getpwuid()" in responders except sssd-nss (and monitor).
We can, for sure. However, as far as I understand, @sgallagh would like to avoid SSSD having a circular dependency.
In monitor we should set/unset env variable `_SSS_LOOPS`
In other others, it would solve the situation but only for the NSS responder.
Is there any problem to start other responders after sssd-nss?
What I'm proposing is to take a step back and do *not* support
unprivileged
users for socket-activated services for now. Get the socket-activation working without cycle dependency on SSSD and avoid the TUCTOU issue.
Once we have the static uid for the sssd user on Fedora then I can start bugging Debian/Ubuntu/openSUSE/SUSE maintainers in order to provide the same and we get back to supporting the unprivileged user for socket-activated services.
static uid/gid is not a solution; it's just a workaround.
LS
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/183#issuecomment-285722632, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4ekDwZez-izo2Lix_iao2kIP46GYMks5rkYBcgaJpZM4MVyT_ .
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285735941
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ On (10/03/17 09:45), fidencio wrote:
On Fri, Mar 10, 2017 at 5:54 PM, lslebodn notifications@github.com wrote:
On (10/03/17 05:50), fidencio wrote:
@sgallah, @lslebodn
On Fri, Mar 10, 2017 at 2:22 PM, Stephen Gallagher <
notifications@github.com
wrote:
@lslebodn https://github.com/lslebodn
@sgallagher https://github.com/sgallagher The purpose of calling
chown
in ExecStartPre is to allow starting responders as non-privileged from beginning. Systemd drops permissions before exec.
Yeah, I get that. And I told @fidencio https://github.com/fidencio on IRC that we can live with the TOCTOU for the time being and figure out a better option later. That said, we cannot use /usr/bin/chown for this, because it unconditionally calls getpwnam()/getpwuid() in its execution, which causes a problem when socket-activating. I suggested that we might want to just create a reduced-functionality /usr/libexec/sssd/sss_chown that calls only the low-level system function.
Well, considering we write our own sss_chown binary ... as we still don't have a static uid for the sssd user we would end up calling getpwnam()/getpwuid() for the unprivileged user.
we can call "getpwnam()/getpwuid()" in responders except sssd-nss (and monitor).
We can, for sure. However, as far as I understand, @sgallagh would like to avoid SSSD having a circular dependency.
There is not any circullar dependency between responders.
e.g. sssd-nss -> sssd-pam -> sssd-nss
If yes then please describe them.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-285740096
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ @lslebodn: As far as I understand from @sgallagher comments ... he would like to avoid having a dependency in the sssd-nss.service.
Anyways, I've asked him on IRC and he said it's "okay" for now.
I'm submitting a new patch set that removes the chown() done in the sssd-nss.service, removes the SocketUser=@SSSD_USER and SocketGroup=@SSSD_USER@ from the sssd-nss.socket and also sets "Before=sssd-autofs.socket sssd-pam.socket ..." in the sssd-nss.socket. """
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286182946
URL: https://github.com/SSSD/sssd/pull/183 Author: fidencio Title: #183: More socket-activation fixes Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/183/head:pr183 git checkout pr183
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ I would prefer to remove `open_and_fchown_debug_file` from the last patch. it requires more testing and root can write to any file. It will simplify testing. and `open_and_fchown_debug_file` shoudl fix TOCTOU just for monitor non-root mode.
the patch should just remove `ExecStartPre=-/bin/chown root:root @logpath@/sssd_nss.log` from sssd-nss.service
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286737064
URL: https://github.com/SSSD/sssd/pull/183 Author: fidencio Title: #183: More socket-activation fixes Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/183/head:pr183 git checkout pr183
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ @lslebodn: Patch set updated according to your comments. """
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286740540
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ ACK """
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286767772
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ master:
* ecaf0bb271812c3af3e5916f14da0e37d26994d2 * e19327b3b06e723e5162f0c91cb77ba254bb3dc7 * b7430c4f4b98efe08d9d13d202fbb76229628b30
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286769949
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
lslebodn commented: """ BTW feel free to fix TOCTOU in different PR """
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286774565
URL: https://github.com/SSSD/sssd/pull/183 Author: fidencio Title: #183: More socket-activation fixes Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/183/head:pr183 git checkout pr183
URL: https://github.com/SSSD/sssd/pull/183 Title: #183: More socket-activation fixes
fidencio commented: """ On Wed, Mar 15, 2017 at 4:17 PM, lslebodn notifications@github.com wrote:
BTW feel free to fix TOCTOU in different PR
Sure thing. I'll open an issue and fix the problem in a different PR.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/183#issuecomment-286774565, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4eqpBaR9g7uvNzqXACpZghYZi1ZLbks5rmAEMgaJpZM4MVyT_ .
"""
See the full comment at https://github.com/SSSD/sssd/pull/183#issuecomment-286776664
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
lslebodn
-
Lukas Slebodnik -
sgallagher