URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ On Tue, Nov 29, 2016 at 9:33 AM, Pavel Březina notifications@github.com wrote:

One thing I don't like about those patches is that we always recreate the idle timer. Can we get around this (maybe also for the client idle timeout)? What I have i mind is this:

1. Instead of resetting the timer, remember time(NULL) of the last

communication, say last_request_time 2. Create a periodic timer that fires each responder_idle_timeout / 2 3. In the timer check that last_request_time + responder_idle_timeout < time(NULL)

It won't be as precise as your solution, but I don't think we need it to be but it will save us lots of memory operations. What do you think?

Fine by me. Your suggestion has been used in a few other places of the our codebase as well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/94#issuecomment-263507800, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4euHebmR2oC8k9VYhGYHpTzyt5RFvks5rC-NigaJpZM4K8AJs .

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-263514555

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

pbrezina commented: """ Sounds good to me. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-263818341

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ On Thu, Dec 1, 2016 at 1:03 PM, Pavel Březina notifications@github.com wrote:

Hi,

if (ret != EOK) { #ifdef HAVE_SYSTEMD if (ret != ENOENT) #endif { DEBUG(SSSDBG_FATAL_FAILURE, "No services configured!\n"); return EINVAL; } }

can you separate above to:

#ifdef HAVE_SYSTEMDif (ret != EOK && ret != ENOENT) { DEBUG(SSSDBG_FATAL_FAILURE, "No services configured!\n"); return EINVAL; } #else ... #endif

Okay,

We should also amend services option man page to describe what happens if a service is not listed there, that they are automatically activated when needed

Okay.

.

Now I have few more comments to the timeouts.

1. I believe you can remove "RESPONDER: Shutdown

{dbus,socket}-activated responders in case they're idle" in favor of "Change the timeout logic".

The "Change timeout logic" commit is a leftover that must have been squashed to "RESPONDER: Shutdown {dbus,socket}-activated responders in case they're idle". Sorry for messing it up.

Squashing it there is okay for you?

1. Please use the same logic in sbus code. I think you just want to

pass a pointer to last_request_time into sbus (to remove the need for function pointers) and let the sbus code update it when appropriate even for our private communication (in other responders). Because if a communication is happening between responder and provider, the responder is still not idle (it may be awaiting reply from data provider so it can be send to the client).

Okay, I can pass just the pointer to the last_request_time var into sbus. Please, here I need a bit more pointers in order to be sure I understand your suggestion.

Please, correct me if I'm wrong, it's updating the last_request_time even for our private communication, no? Are you talking specifically about IFP provider or the others? Because the others have their last_request_time() updated everytime something goes through their sockets and, as far as I understand, it should be enough, right? If not, why not?

1. Resetting the timeout when sbus signal is received is not enough.

You want to reset it everytime any communication on the bus is happening. I actually think that signals are the only communication that we can skip for two reasons (One - we don't use any signals except NameOwnerChanged, Second

• those are asynchronous events that do not require any reply). I think you

want to reset the time in sbus_message_handler when you got a valid handler (interface and method combination. The you don't need the iface validator.

Hmmm. I see your point and it makes sense. But I have one question ... We do receive signals from org.freedesktop.sssd.infopipe.* and they should be treated, right? So we can't ignore it completely.

In case those signals end up in sbus_message_handler() as well, then I agree with you. Is that the case?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/94#issuecomment-264156064, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4ev7GREjEnsOvsuU94yxTOH6T1zj5ks5rDreogaJpZM4K8AJs .

Thanks a lot for the review, Pavel.

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-264162692

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

jhrozek commented: """ Hi, I read the patches, so far I didn't do a whole lot of testing, so maybe some questions here are invalid, but nonetheless, here is my take on the patchset:

1. currently the patches don't work well if sssd is running with `user=root` because then the confdb is owned by root and the responders, which always start as the sssd user cannot read it. I think we could solve this particular problem by owning the confdb as sssd/sssd from the start -- I don't think there would be any information leak because the confdb is then only readable to sssd and root and the sssd user's shell is hopefully` /sbin/nologin`. However, just the presence of the sssd user is currently conditional (`--with-sssd-user` still defaults to root). I wonder if it was the easiest to conditionalize the unit and socket files and expand the sssd user instead of hardcoding sssd there?

1. There seems to be some issue with PAM socket ownership because I can't authenticate with socket-activated PAM responder: `Dec 12 11:36:11 client.ipa.test su[29517]: pam_sss(su-l:auth): Request to sssd failed. Public socket has wrong ownership or permissions.`

1. Would it make any sense to own the sockets by the sssd user as well? Currently it seems the sockets are owned by the sssd user for services started by monitor if they are running un-privileged but as root for systemd-activated services. Because the

1. There were some SELinux denials on my test VM, but granted, I run F-24 there. We need to make sure that no SELinux AVC denials are present in Fedora later.

1. In the upstream reference specfile, should we use the systemd macros as we do for services so that all sockets are enabled and started?

1. The documentation should be improved a bit. At least we should say that the services line is only optional on systemd platforms. I think we should also add an example that lists how to disable a socket if the admin wants to totally disable some service.

1. I'm wondering if the restart on-failure is correct. I think it is, but I would like to discuss it a bit. Currently the restart is done on any return code other than 0. The only issue I see is that the restart is retried several times if the service fails to start up, but in general I think it's much safer than trying to be too smart and only restart with on-abnormal. Do you agree?

That's all for now, I will continue reading the patches and testing them later. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-266407528

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

pbrezina commented: """ **Unit files** I wonder if the user and group in unit files should take --with-sssd-user value from configure.

**MAN: Mention that the services' list is optional** It should also say that it is socket activated if it is missing.

**sbus timeout** ```c struct sbus_service_timeout { errno_t (*setup_timeout_fn)(void *); errno_t (*reset_timeout_fn)(void *); void *timeout_data; }; ``` `setup_timeout_fn` is not used in sbus, it is used in monitor code so it should not be part of sbus. I agree to setup the idle timeout in monitor when we know that the service was bus activated. But I still don't think we need to expose this logic in sbus. Pass the function pointer to the monitor (although I think it can be hardcoded) and only pointer to time_t of last request time to sbus. Also I would prefer to delete `reset_responder_idle_timer` and only set rctx->last_request_time directly without all those checks and a debug message. I believe it will be more simple and those checks are not really necessary (it doesn't matter if the timer is set or not, we still can remember last request time).

**sbus_destructor** Destructors with talloc can be typed directly and still keep type checking with gcc: ```c static int sbus_connection_destructor(void *ctx) -> static int sbus_connection_destructor(struct sbus_connection *conn) ``` **ensure that timeout is at least 60 seconds** ```c + /* Idle timeout set to 0 means that no timeout will be set up to + * the responder */ + if (rctx->idle_timeout != 0) { + /* Ensure that the responder timeout is at least sixty seconds */ + if (rctx->idle_timeout < 60) { + rctx->idle_timeout = 60; + } } ``` Print a warning here, please.

**sudo** Sudo is a little bit special responder. Since it requires some periodic task in the backend, we decided a long time ago to disable sudo in backend unless it is explicitly configured, either by putting "services = sudo" or "sudo_provider = provider". I think this is a good time to treat it the same as all other responders, i.e. keep it always enabled unless it is explicitly disabled with `sudo_provider = none`. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-266420800

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ On Mon, Dec 12, 2016 at 12:40 PM, Jakub Hrozek notifications@github.com wrote:

Hi, I read the patches, so far I didn't do a whole lot of testing, so maybe some questions here are invalid, but nonetheless, here is my take on the patchset:

currently the patches don't work well if sssd is running with user=root because then the confdb is owned by root and the responders, which always start as the sssd user cannot read it. I think we could solve this particular problem by owning the confdb as sssd/sssd from the start -- I don't think there would be any information leak because the confdb is then only readable to sssd and root and the sssd user's shell is hopefully /sbin/nologin. However, just the presence of the sssd user is currently conditional (--with-sssd-user still defaults to root). I wonder if it was the easiest to conditionalize the unit and socket files and expand the sssd user instead of hardcoding sssd there?

Hmmm. Yes, I guess the best thing to do here is to expand --with-sssd-user parameter in the unit files.

There seems to be some issue with PAM socket ownership because I can't authenticate with socket-activated PAM responder: Dec 12 11:36:11 client.ipa.test su[29517]: pam_sss(su-l:auth): Request to sssd failed. Public socket has wrong ownership or permissions.

Most likely caused by the 1.

Would it make any sense to own the sockets by the sssd user as well? Currently it seems the sockets are owned by the sssd user for services started by monitor if they are running un-privileged but as root for systemd-activated services. Because the

Hmm. Probably it does, but I'd like to hear other developers' opinion here

as well. And seems that we missed some part of your explanation.

There were some SELinux denials on my test VM, but granted, I run F-24 there. We need to make sure that no SELinux AVC denials are present in Fedora later.

Lukáš replied this one, thanks!

In the upstream reference specfile, should we use the systemd macros as we do for services so that all sockets are enabled and started?

I'll check, but most-likely, yes!

The documentation should be improved a bit. At least we should say that the services line is only optional on systemd platforms. I think we should also add an example that lists how to disable a socket if the admin wants to totally disable some service.

Well, my patch in the manual explicitly mentions that the services line is optional and the sentence is added only on systemd platforms. For sure I can expand it.

I'm wondering if the restart on-failure is correct. I think it is, but I would like to discuss it a bit. Currently the restart is done on any return code other than 0. The only issue I see is that the restart is retried several times if the service fails to start up, but in general I think it's much safer than trying to be too smart and only restart with on-abnormal. Do you agree?

Had the same thought when adding it. I'd prefer to keep it as it is instead of trying to be smart (and failing on that).

That's all for now, I will continue reading the patches and testing them

later.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/94#issuecomment-266407528, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4esxEc1sdyvB65jm3C1y3pdo8ojI7ks5rHTKbgaJpZM4K8AJs .

I'll go through the other comments and prepare a new version of the series. Thanks for the review!

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-266428998

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

jhrozek commented: """ On Mon, Dec 12, 2016 at 05:19:18AM -0800, fidencio wrote:

...

Would it make any sense to own the sockets by the sssd user as well? Currently it seems the sockets are owned by the sssd user for services started by monitor if they are running un-privileged but as root for systemd-activated services. Because the

Hmm. Probably it does, but I'd like to hear other developers' opinion here

as well. And seems that we missed some part of your explanation.

I don't remembe what exactly did I want to say here :-) I think just that because the sockets are normally world-writable, except the private pam one, the permissions don't matter that much, but at least with the private pam socket care should be taken its permissions are the same as if sssd created the socket itself.

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-266442892

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ On Mon, Dec 12, 2016 at 7:13 PM, Fabiano Fidêncio fidencio@redhat.com wrote:

On Mon, Dec 12, 2016 at 2:26 PM, Fabiano Fidêncio fidencio@redhat.com wrote:

...

On Mon, Dec 12, 2016 at 1:34 PM, Pavel Březina notifications@github.com wrote:

...

*Unit files* I wonder if the user and group in unit files should take --with-sssd-user value from configure.

Yeah, it seems the way to go! Jakub also pointed it out during his review.

...

*MAN: Mention that the services' list is optional* It should also say that it is socket activated if it is missing.

Right!

...

*sbus timeout*

struct sbus_service_timeout { errno_t (*setup_timeout_fn)(void *); errno_t (*reset_timeout_fn)(void *); void *timeout_data; };

setup_timeout_fn is not used in sbus, it is used in monitor code so it should not be part of sbus. I agree to setup the idle timeout in monitor when we know that the service was bus activated. But I still don't think we need to expose this logic in sbus. Pass the function pointer to the monitor (although I think it can be hardcoded) and only pointer to time_t of last request time to sbus. Also I would prefer to delete reset_responder_idle_timer and only set rctx->last_request_time directly without all those checks and a debug message. I believe it will be more simple and those checks are not really necessary (it doesn't matter if the timer is set or not, we still can remember last request time).

Hmm. Okay, let me think a little bit about it and do some tests.

After doing some changes in the code I have to say it doesn't look cleaner at all.

...

...

*sbus_destructor* Destructors with talloc can be typed directly and still keep type checking with gcc:

static int sbus_connection_destructor(void *ctx) -> static int sbus_connection_destructor(struct sbus_connection *conn)

Right.

Well, not exactly. GCC will complain in case it's done:

../src/sbus/sssd_dbus_connection.c: In function ‘sbus_init_connection’: ../src/sbus/sssd_dbus_connection.c:181:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] talloc_set_destructor((TALLOC_CTX *)conn, sbus_connection_destructor);

So, if you don't mind I'll avoid the warning and keep it as it is.

...

...

*ensure that timeout is at least 60 seconds*

• /* Idle timeout set to 0 means that no timeout will be set up to+ * the responder */
• if (rctx->idle_timeout != 0) {

•    /* Ensure that the responder timeout is at least sixty seconds */
  

•    if (rctx->idle_timeout < 60) {
  

•        rctx->idle_timeout = 60;
  

•    }
  

  }

Print a warning here, please.

Not sure if I understand your comment. The reason I'm checking wether the timeout is set to zero or not is because I'm following Lukáš suggestion to disable the timeout in that case. So, yes, I can print a warning message in this case. But I also think that keeping Lukáš suggestion makes sense, right?

...

*sudo* Sudo is a little bit special responder. Since it requires some periodic task in the backend, we decided a long time ago to disable sudo in backend unless it is explicitly configured, either by putting "services = sudo" or "sudo_provider = provider". I think this is a good time to treat it the same as all other responders, i.e. keep it always enabled unless it is explicitly disabled with sudo_provider = none.

I'm not sure whether I completely understand your suggestion here. You're saying there's some differences between sudo responder and the others. So, your suggestion is to drop these differences as soon as this patch series lands? Or to do these changes within this patch series?

...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/94#issuecomment-266420800, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4eniHdCjuqepQFbAeSFIcT_fOqjk7ks5rHT9XgaJpZM4K8AJs .

Thanks a lot for the review!

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-266558041

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

pbrezina commented: """

*sudo*

Sudo is a little bit special responder. Since it requires some periodic task in the backend, we decided a long time ago to disable sudo in backend unless it is explicitly configured, either by putting "services = sudo" or "sudo_provider = provider". I think this is a good time to treat it the same as all other responders, i.e. keep it always enabled unless it is explicitly disabled with sudo_provider = none.

I'm not sure whether I completely understand your suggestion here. You're saying there's some differences between sudo responder and the others. So, your suggestion is to drop these differences as soon as this patch series lands? Or to do these changes within this patch series?

Drop those differences within this patch set. See `dp_target_sudo_enabled()`. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-267005317

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

Label: -Changes requested

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

jhrozek commented: """ Coverity seems to have detected a warning: ```` Error: CHECKED_RETURN (CWE-252): sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:323: check_return: Calling "sss_cmd_empty_packet" without checking return value (as is done elsewhere 4 out of 5 times). sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1401: example_assign: Example 1: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1402: example_checked: Example 1 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1424: example_assign: Example 2: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1425: example_checked: Example 2 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1097: example_assign: Example 3: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1098: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/common/responder_cmd.c:85: example_assign: Example 4: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/common/responder_cmd.c:86: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0". # 321| DEBUG(SSSDBG_TRACE_FUNC, "setautomntent did not find requested map\n"); # 322| /* Notify the caller that this entry wasn't found */ # 323|-> sss_cmd_empty_packet(pctx->creq->out); # 324| } else { # 325| DEBUG(SSSDBG_TRACE_FUNC, "setautomntent found data\n"); ```` I'm not sure if it's legit or if we just passed a threshold of checked/unchecked ratio, but it would be nice to not add any new warnings with new commits.

Could you please add a more verbose comment to the commits that enable the responder socket activation (either the first one, autofs, or just copy to all) that explain why the BindsTo option was added and what the workflow is for socket-activated responders and starting and stopping the sssd service?

Similarly, can you please explain in the commit message that adds the `--unprivileged-start` option that the log files are chowned by the unit file and the socket files created by sssd in the most common scenario of this option? I was even wondering if the option would be better named `--socket-activated-start` but I don't have strong feelings about it.

There is a typo in the commit that changes the PAM responder `unprivileged_unprivileged_start`, which would be nice to fix in the commit where it was introduced, so that every commit can be compiled on its own and we can always use bisect.

I have a bit of trouble reading `client_registration()` after ` MONITOR: Deal with socket-activated responders`. Could we please change `client_registration()` so that the ifdefs are a bit less interleaved with the non-systemd code? Even at the cost of a little code duplication, I think this: ``` #ifdef HAVE_SYSTEMD systemd_client_registration(args..) #else managed_client_registration(args..) #endif /* HAVE_SYSTEMD */ ``` Is IMO preferred over the ifdefs being sprinkled around in the code.

About ` MAN: Mention that the services' list is optional`, are you sure just enabling the socket is all that is needed? Doesn't the admin also need to enable the service in addition to the socket?

`MONITOR: Let the responder know whether it was socket-activated`: do you think this commit is needed? Could the responder learn that it's socket activated when it goes through `activate_unix_sockets()` or is that too late? Please note I'm not against this commit per se, I'm just trying to see if we can simplify the code.

I'm not sure I understand the `time_t` pointer being added to the responder context. Shouldn't we only care about the requests from the client, like NSS or D-Bus?

I mostly just read the code, but I'm afraid I'm still having issues with the socket-activated PAM responder. My sssd.conf is as follows: ``` [sssd] services = nss user = sssd

domains = ipa.test ```

I enabled and started the sssd-pam responder socket, then tried to log in as an IPA user, but I'm getting: ``` Dec 21 09:57:04 client.ipa.test su[30415]: pam_sss(su-l:auth): Request to sssd failed. Public socket has wrong ownership or permissions ```

The socket was created as: ``` srw-rw-rw-. 1 sssd sssd 0 Dec 21 09:56 /var/lib/sss/pipes/pam ```

I built sssd from source. Please shout if you'd like to have access to my test machine. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-268482769

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ On Wed, Dec 21, 2016 at 10:59 AM, Jakub Hrozek notifications@github.com wrote:

Coverity seems to have detected a warning:

Error: CHECKED_RETURN (CWE-252): sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:323: check_return: Calling "sss_cmd_empty_packet" without checking return value (as is done elsewhere 4 out of 5 times). sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1401: example_assign: Example 1: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1402: example_checked: Example 1 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1424: example_assign: Example 2: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1425: example_checked: Example 2 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1097: example_assign: Example 3: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/autofs/autofssrv_cmd.c:1098: example_checked: Example 3 (cont.): "ret" has its value checked in "ret != 0". sssd-1.14.90/src/responder/common/responder_cmd.c:85: example_assign: Example 4: Assigning: "ret" = return value from "sss_cmd_empty_packet(pctx->creq->out)". sssd-1.14.90/src/responder/common/responder_cmd.c:86: example_checked: Example 4 (cont.): "ret" has its value checked in "ret != 0". # 321| DEBUG(SSSDBG_TRACE_FUNC, "setautomntent did not find requested map\n"); # 322| /* Notify the caller that this entry wasn't found */ # 323|-> sss_cmd_empty_packet(pctx->creq->out); # 324| } else { # 325| DEBUG(SSSDBG_TRACE_FUNC, "setautomntent found data\n");

I'm not sure if it's legit or if we just passed a threshold of checked/unchecked ratio, but it would be nice to not add any new warnings with new commits.

Thanks for catching this one. I'll take a look whether they make sense or not.

Could you please add a more verbose comment to the commits that enable the responder socket activation (either the first one, autofs, or just copy to all) that explain why the BindsTo option was added and what the workflow is for socket-activated responders and starting and stopping the sssd service?

Sure.

Similarly, can you please explain in the commit message that adds the --unprivileged-start option that the log files are chowned by the unit file and the socket files created by sssd in the most common scenario of this option? I was even wondering if the option would be better named --socket-activated-start but I don't have strong feelings about it.

Sure.

There is a typo in the commit that changes the PAM responder unprivileged_unprivileged_start, which would be nice to fix in the commit where it was introduced, so that every commit can be compiled on its own and we can always use bisect.

Oh, sorry, my fault messing up with rebase :-\

I have a bit of trouble reading client_registration() after MONITOR: Deal with socket-activated responders. Could we please change client_registration() so that the ifdefs are a bit less interleaved with the non-systemd code? Even at the cost of a little code duplication, I think this:

#ifdef HAVE_SYSTEMD systemd_client_registration(args..) #else managed_client_registration(args..) #endif /* HAVE_SYSTEMD */

Is IMO preferred over the ifdefs being sprinkled around in the code.

Okay.

About MAN: Mention that the services' list is optional, are you sure just enabling the socket is all that is needed? Doesn't the admin also need to enable the service in addition to the socket?

In the testes I've done here (mainly generating the sssd tarball and installing it on my VM) that's the only thing needed. The services will be started by any activity on the sockets, doesn't matter whether it's enabled or not.

MONITOR: Let the responder know whether it was socket-activated: do you think this commit is needed? Could the responder learn that it's socket activated when it goes through activate_unix_sockets() or is that too late? Please note I'm not against this commit per se, I'm just trying to see if we can simplify the code.

Hmm. I don't see how it could be done there, to be honest. Correct me if I'm mistaken, but that code would be run even if the socket was set in the services list, no?

I'm not sure I understand the time_t pointer being added to the responder context. Shouldn't we only care about the requests from the client, like NSS or D-Bus?

Pavel suggested to track internal communication as well. So, that's the reason we update the "last_request_time" on every internal communication as well.

I mostly just read the code, but I'm afraid I'm still having issues with the socket-activated PAM responder. My sssd.conf is as follows:

[sssd] services = nss user = sssd

domains = ipa.test

I enabled and started the sssd-pam responder socket, then tried to log in as an IPA user, but I'm getting:

Dec 21 09:57:04 client.ipa.test su[30415]: pam_sss(su-l:auth): Request to sssd failed. Public socket has wrong ownership or permissions

The socket was created as:

srw-rw-rw-. 1 sssd sssd 0 Dec 21 09:56 /var/lib/sss/pipes/pam

I built sssd from source. Please shout if you'd like to have access to my test machine.

That's really really weird. I'll try to reproduce it locally and if not possible I'll ask access to your test machine.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/94#issuecomment-268482769, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4epea6S0_Rl39hsujP_N_VjmQLJzVks5rKPiBgaJpZM4K8AJs .

I'll submit another version soon, Thanks for the review!

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-268489832

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

Label: -Changes requested

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

lslebodn commented: """ Test are broken on all platforms http://sssd-ci.duckdns.org/logs/job/59/96/summary.html """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-272122115

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ Pushed a new version. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-272136455

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ Pushed a new version. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-272136455

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

Label: +Changes requested

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ @jhrozek , thanks for the review.

A new series will be update in a few and I hope all the issues raised are addressed. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-272744838

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

Label: -Changes requested

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

lslebodn commented: """ On (15/01/17 14:54), fidencio wrote:

fidencio commented on this pull request.

...

• }

• /* As the service is a responder and wasn't part of the services' list, it means

• * the service has been socket/dbus activated and has to be configured and added
  

• * to the services' list now */
  

• *_explicitly_configured = false;
• *_svc = NULL;
• if (check_service(svc_name) != NULL) {

•    DEBUG(SSSDBG_FATAL_FAILURE, "Invalid service %s\n", svc_name);
  

•    return EINVAL;
  

• }
• mini->ctx->services_started = true;
• mini->ctx->num_services++;

Yep, we must decrease the num_services. Nice catch.

It might explain why I saw a crash. I will need to test with new version.

``` #0 monitor_service_shutdown (conn=0x7f80357cb880, data=<optimized out>) at src/monitor/monitor.c:2457 2457 if (svc->conn == conn) {

Thread 1 (Thread 0x7f8035380880 (LWP 24632)): #0 monitor_service_shutdown (conn=0x7f80357cb880, data=<optimized out>) at src/monitor/monitor.c:2457 ctx = 0x7f80357a5b30 svc = 0xbebebebebebebebe __FUNCTION__ = "monitor_service_shutdown" #1 0x00007f8030b65447 in _talloc_free_internal (ptr=0x7f80357cb880, location=0x7f8030b68a12 "../talloc.c:2631") at ../talloc.c:1046 d = 0x7f80348dfe80 <sbus_connection_destructor> ptr_to_free = <optimized out> #2 0x00007f8030b6501b in _talloc_free_children_internal (location=0x7f8030b68a12 "../talloc.c:2631", ptr=0x7f80357b5370, tc=0x7f80357b5310) at ../talloc.c:1525 child = 0x7f80357cb880 new_parent = 0x0 #3 _talloc_free_internal (ptr=0x7f80357b5370, location=0x7f8030b68a12 "../talloc.c:2631") at ../talloc.c:1072 ptr_to_free = <optimized out> #4 0x00007f8030b6501b in _talloc_free_children_internal (location=0x7f8030b68a12 "../talloc.c:2631", ptr=0x7f80357a5b30, tc=0x7f80357a5ad0) at ../talloc.c:1525 child = 0x7f80357b5370 new_parent = 0x0 #5 _talloc_free_internal (ptr=0x7f80357a5b30, location=0x7f8030b68a12 "../talloc.c:2631") at ../talloc.c:1072 ptr_to_free = <optimized out> #6 0x00007f8030b6501b in _talloc_free_children_internal (location=0x7f8030b68a12 "../talloc.c:2631", ptr=0x7f80357aca80, tc=0x7f80357aca20) at ../talloc.c:1525 child = 0x7f80357a5b30 new_parent = 0x0 #7 _talloc_free_internal (ptr=0x7f80357aca80, location=0x7f8030b68a12 "../talloc.c:2631") at ../talloc.c:1072 ptr_to_free = <optimized out> #8 0x00007f8030b6501b in _talloc_free_children_internal (location=0x7f8030b68a12 "../talloc.c:2631", ptr=0x7f80357bcf10, tc=0x7f80357bceb0) at ../talloc.c:1525 child = 0x7f80357aca80 new_parent = 0x0 #9 _talloc_free_internal (ptr=0x7f80357bcf10, location=0x7f8030b68a12 "../talloc.c:2631") at ../talloc.c:1072 ptr_to_free = <optimized out> #10 0x00007f8030b5eb63 in _talloc_free_children_internal (location=0x7f8030b68a12 "../talloc.c:2631", ptr=0x7f80357a6f50, tc=0x7f80357a6ef0) at ../talloc.c:1525 child = 0x7f80357bcf10 new_parent = 0x0 #11 _talloc_free_internal (location=<optimized out>, ptr=<optimized out>) at ../talloc.c:1072 ptr_to_free = <optimized out> #12 _talloc_free (ptr=0x7f80357a6f50, location=0x7f8030b68a12 "../talloc.c:2631") at ../talloc.c:1647 No locals. #13 0x00007f80303caa49 in __run_exit_handlers (status=status@entry=1, listp=0x7f803074c6c8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:77 atfct = <optimized out> onfct = <optimized out> cxafct = <optimized out> f = <optimized out> #14 0x00007f80303caa95 in __GI_exit (status=status@entry=1) at exit.c:99 No locals. #15 0x00007f80353d9917 in monitor_quit (ret=ret@entry=1, mt_ctx=0x7f80357a5b30) at src/monitor/monitor.c:1530 svc = <optimized out> pid = <optimized out> status = 0 error = <optimized out> kret = <optimized out> killed = <optimized out> #16 0x00007f80353d9dea in monitor_restart_service (svc=0x7f80357ccbf0) at src/monitor/monitor.c:2639 mt_ctx = 0x7f80357a5b30 now = <optimized out> te = <optimized out> #17 mt_svc_exit_handler (pid=<optimized out>, wait_status=<optimized out>, pvt=<optimized out>) at src/monitor/monitor.c:2602 svc = 0x7f80357ccbf0 __FUNCTION__ = "mt_svc_exit_handler" #18 0x00007f80327acb77 in sss_child_invoke_cb (ev=<optimized out>, imm=0x7f80357ca6d0, pvt=<optimized out>) at src/util/child_common.c:181 cb_pvt = 0x7f80357ce220 child_ctx = 0x7f80357c6ab0 key = {type = HASH_KEY_ULONG, {str = 0x6076 <Address 0x6076 out of bounds>, ul = 24694}} error = 0 __FUNCTION__ = "sss_child_invoke_cb" #19 0x00007f8030d70c34 in tevent_common_loop_immediate (ev=ev@entry=0x7f80357bcf10) at ../tevent_immediate.c:135 im = 0x7f80357ca6d0 handler = 0x7f80327acb00 <sss_child_invoke_cb> private_data = 0x7f80357ce220 #20 0x00007f8030d72384 in poll_event_loop_once (ev=0x7f80357bcf10, location=<optimized out>) at ../tevent_poll.c:649 No locals. #21 0x00007f8030d7040d in _tevent_loop_once (ev=ev@entry=0x7f80357bcf10, location=location@entry=0x7f8034909327 "src/util/server.c:707") at ../tevent.c:533 ret = <optimized out> nesting_stack_ptr = 0x0 #22 0x00007f8030d71ebb in poll_event_loop_wait (ev=0x7f80357bcf10, location=0x7f8034909327 "src/util/server.c:707") at ../tevent_poll.c:677 ret = <optimized out> poll_ev = 0x7f80357ad210 #23 0x00007f80348eb5d3 in server_loop (main_ctx=0x7f80357aca80) at src/util/server.c:707 No locals. #24 0x00007f80353d717a in main (argc=<optimized out>, argv=<optimized out>) at src/monitor/monitor.c:2914 opt = <optimized out> pc = <optimized out> opt_daemon = 0 opt_interactive = 1 opt_genconf = 0 opt_version = 0 opt_netlinkoff = 0 opt_config_file = 0x0 config_file = <optimized out> flags = <optimized out> main_ctx = 0x7f80357aca80 tmp_ctx = 0x7f80357a59b0 monitor = 0x7f80357a5b30 ret = <optimized out> long_options = {{longName = 0x0, shortName = 0 '\000', argInfo = 4, arg = 0x7f8034696320 <poptHelpOptions>, val = 0, descrip = 0x7f80353e0b9a "Help options:", argDescrip = 0x0}, {longName = 0x7f80353e0ba8 "debug-level", shortName = 100 'd', argInfo = 2, arg = 0x7f803117f128 <debug_level>, val = 0, descrip = 0x7f80353e0bb4 "Debug level", argDescrip = 0x0}, {longName = 0x7f80353e015f "debug-to-files", shortName = 102 'f', argInfo = 0, arg = 0x7f803117f124 <debug_to_file>, val = 0, descrip = 0x7f80353e1ca8 "Send the debug output to files instead of stderr", argDescrip = 0x0}, {longName = 0x7f80353e018c "debug-to-stderr", shortName = 0 '\000', argInfo = 1073741824, arg = 0x7f803117f120 <debug_to_stderr>, val = 0, descrip = 0x7f80353e1ce0 "Send the debug output to stderr directly.", argDescrip = 0x0}, {longName = 0x7f80353e0bc0 "debug-timestamps", shortName = 0 '\000', argInfo = 2, arg = 0x7f803117f0fc <debug_timestamps>, val = 0, descrip = 0x7f80353e0bd1 "Add debug timestamps", argDescrip = 0x0}, {longName = 0x7f80353e0be6 "debug-microseconds", shortName = 0 '\000', argInfo = 2, arg = 0x7f803117f0f8 <debug_microseconds>, val = 0, descrip = 0x7f80353e1d10 "Show timestamps with microseconds", argDescrip = 0x0}, {longName = 0x7f80353e0bf9 "daemon", shortName = 68 'D', argInfo = 0, arg = 0x7fffb030becc, val = 0, descrip = 0x7f80353e0c00 "Become a daemon (default)", argDescrip = 0x0}, {longName = 0x7f80353e0c1a "interactive", shortName = 105 'i', argInfo = 0, arg = 0x7fffb030bed0, val = 0, descrip = 0x7f80353e1d38 "Run interactive (not a daemon)", argDescrip = 0x0}, {longName = 0x7f80353e0c26 "disable-netlink", shortName = 0 '\000', argInfo = 1073741824, arg = 0x7fffb030bedc, val = 0, descrip = 0x7f80353e0c36 "Disable netlink interface", argDescrip = 0x0}, {longName = 0x7f80353e3a2d "config", shortName = 99 'c', argInfo = 1, arg = 0x7fffb030bee8, val = 0, descrip = 0x7f80353e1d58 "Specify a non-default config file", argDescrip = 0x0}, {longName = 0x7f80353e0c50 "genconf", shortName = 103 'g', argInfo = 0, arg = 0x7fffb030bed4, val = 0, descrip = 0x7f80353e1d80 "Refresh the configuration database, then exit", argDescrip = 0x0}, {longName = 0x7f80353e3a1f "version", shortName = 0 '\000', argInfo = 0, arg = 0x7fffb030bed8, val = 0, descrip = 0x7f80353e0c58 "Print version number and exit", argDescrip = 0x0}, {longName = 0x0, shortName = 0 '\000', argInfo = 0, arg = 0x0, val = 0, descrip = 0x0, argDescrip = 0x0}} __FUNCTION__ = "main" ```

"""

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-272829452

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

lslebodn commented: """ I still can see the crash in latest version. It was not related to the quoted change """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-273046891

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

lslebodn commented: """ As part of different testing I found out that services are not restarted if they die.

There send `kill -9` to some responder which is listed in "services" option. Tested on rhel6 => without systemd. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-273234615

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ @lslebodn: About kill -9 not restarting the responder ... it's already fixed locally by the my patches covering @jhrozek review.

About the second one, I'll dig into it Tomorrow. Thanks for the review so far.

I do believe the crash you saw (even I'm not able to reproduce it) got fixed by the fixup patches that I have locally. But I'll wait for the reproducer anyways. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-273279519

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

pbrezina commented: """ ```c if (conn->last_request_time != NULL) { time_t *last_request_time = conn->last_request_time;

*last_request_time = time(NULL); } ```

This looks weird. Why did you not use simply `conn->last_request_time = time(NULL)`?

Otherwise the code looks good. I'm glad you got rid of those function pointers, especially in `sbus` code. I need to do some testing before final ack but I think we got there. Good job. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-273768810

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

fidencio commented: """ New patch set pushed addressing @lslebodn's review.

@lslebodn, @pbrezina: I'm wondering whether we should drop this patch [0] and make it part of a different series.

[0]: https://github.com/fidencio/sssd/commit/fb6eb85b7032788ca069569e8506816280d0... """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-274345172

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

pbrezina commented: """

...

if (conn->last_request_time != NULL) {
    time_t *last_request_time = conn->last_request_time;

    *last_request_time = time(NULL);
}

This looks weird. Why did you not use simply conn->last_request_time = time(NULL)?

Hmmm. Got reminded the reason ... :-) Take a look on the conn structure and you'll realize that last_request_time is not a time_t, but a time_t *.

I meant: `*conn->last_request_time = time(NULL)` but it's my personal preference, feel free to leave it this way. """

See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-274467906

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Author: fidencio Title: #94: Enable {socket,dbus}-activation for responders Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/94/head:pr94 git checkout pr94

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

Label: +Accepted

URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders

Label: +Pushed