URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration WIP Action: opened
PR body: """ @lslebodn, @pbrezina, this is the work-in-progress tlog integration patchset I'd like to work on with you. This is not for merging as it is. We can go over it when we meet :) """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration WIP Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration WIP
spbnick commented: """ I've pushed a draft rewrite using cache_req and data provider. The functionality is very basic and is mostly a proof-of-concept with no intent to be efficient.
For each entry returned for a request for user information in cache_req, it fires off an initgr request. On the data provider side, that initgr request is post-processed to include a "sessionRecording" attribute, if selective session recording is enabled. That attribute specifies if the user name, or names of the groups it's member of, match any of the user or group names in the session recording configuration. Back in cache_req, that attribute is copied over the returned entry.
Once the entries get to NSS, if unconditional session recording is enabled (scope = all), or if selective session recording is enabled (scope = some) and the entry has sessionRecording attribute set to true, the user shell is replaced with the session recording shell.
Things still to do:
* retrieve and use override_space instead of hardcoding * make sure initgr is fired only if there are groups to match against and SYSDB_INITGR_EXPIRE has expired * things I missed (please tell!)
Regarding the second item, isn't cache_req already ensuring that initgr request is only sent when SYSDB_INITGR_EXPIRE is sent, so we don't have to do anything about that?
I'd be glad to hear @pbrezina and @sumit-bose opinions on this so far. Thanks!
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-288411475
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration WIP
spbnick commented: """ One thing bothering me is that `dp_req_initgr_pp` doesn't seem well suited for what we're doing. I.e. nothing cares if it fails. Can we do something about it? Change the interface, put that code somewhere else? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-288413469
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration WIP Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration WIP
spbnick commented: """ Alright, this version loads override_space, and only does initgr request if there are groups to match. I think I'll maybe add checking for SYSDB_INITGR_EXPIRE to avoid firing initgr for entries which haven't expired yet. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-289055189
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration WIP
spbnick commented: """ @sumit-bose, @pbrezina, could you please take a look? I don't need a detailed review yet, just feedback on approach. Thanks! """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-289364772
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration WIP Action: edited
Changed field: title Original value: """ Tlog integration WIP """
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Alright. Aside from `dp_req_initgr_pp` not letting us report a failure, I think this is close to what we need, and is ready for a proper review. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-289480431
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Alright, this one includes PAM exporting the original shell as well. One thing that bothers me about the implementation is that now all responders are reading the shell settings from the NSS section. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-290127604
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ A better CI result: http://sssd-ci.duckdns.org/logs/job/66/48/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-290197456
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Hi Nick, thank you for the patches. The changes are not big, they are well structured and commented. Thank you for that. You put a good work into it!
Here is the review. Please, do not be alarmed, it is only lots of quotation but required changes are small and there isn't many of them.
* **CONFIG: Add session_recording section** - options needs to go also into cfg_rules.ini for the validator * **UTIL: Add session recording conf management module** - `session_recording_conf_load()` -- please, check that the conf pointet is not `NULL` - I would welcome a little bit shorter `enum` values for scope, e.g. `SESSION_RECORDING_SCOPE_NONE`. Or do you think that the `CONF` word is important? * **DP: Overlay sessionRecording attribute on initgr** - Few minor styling issues: ```c ctx->gids[ctx->gnum] = ldb_msg_find_attr_as_uint(res->msgs[i], SYSDB_GIDNUM, 0); ^ we wrap to parenthesis, i.e.: SYSDB_GIDNUM, 0); ``` ```c groupname = (ctx->gnum == 0) ? NULL : sss_nss_get_name_from_msg(ctx->domain_info, res->msgs[i]);
It is not shorter and does not improve readibility, please use if/else ``` - Logic issues ```c /* FIXME Do we need to/can we retrieve the primary group name? */ ``` - Primary group is specified with `gidNumber` and suplementary groups are specified with `memberUid`/`memberOf`. Primary group may or may not be part of suplementary groups so yes, you need to special case it here. ```c static errno_t dp_initgroups(struct sbus_request *sbus_req, struct dp_client *dp_cli, const char *key, uint32_t dp_flags, struct dp_id_data *data) { [...]
ret = sysdb_initgroups(sbus_req, domain, data->filter_value, &res); if (ret == ENOENT || (ret == EOK && res->count == 0)) { /* There is no point in concacting NSS responder. Proceed as usual. */ return EAGAIN;
^^^ we shortcut here and run request without postprocess function
} else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get initgroups [%d]: %s\n", ret, sss_strerror(ret)); goto done; }
ctx = create_initgr_ctx(sbus_req, data->domain, domain, res); if (ctx == NULL) { ret = ENOMEM; goto done; }
dp_req_with_reply_pp(dp_cli, data->domain, "Initgroups", key, sbus_req, DPT_ID, DPM_ACCOUNT_HANDLER, dp_flags, data, dp_req_initgr_pp, ctx, struct dp_initgr_ctx, dp_req_reply_std, struct dp_reply_std);
ret = EOK;
done: talloc_free(res); return ret; } ``` - If initgroups was not yet performed for this user, we do not run the postprocess function a proceed without it. I believe you want to set the `sessionRecording` attribute even in this case so the postprocess function must always be executed.
* **CACHE_REQ: Pull sessionRecording attrs from initgr** ```c static errno_t cache_req_sr_overlay(struct tevent_req *req) { [...]
/* If we have group names to match against */ if (rctx->sr_conf.groups != NULL && *rctx->sr_conf.groups != NULL) {
^^^ please, use rctx->sr_conf.groups[0] != NULL so the intention is clearer }
static errno_t cache_req_sr_overlay_match_users(struct tevent_req *req) { [...]
/* Create per-message talloc context */ tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr, "Failed creating temporary talloc context\n"); ret = ENOMEM; goto done; }
You can create one `tmp_ctx` for the whole function and then free only its children in each cycle with `talloc_free_children()`. It will save some allocations. } ``` - Also please take this `cache_req_sr_overlay()` and move it into seperate `cache_req_session_recording.c` (or similar) module and convert it to an independent tevent request. This way, you are calling `tevent_req_done` inside `cache_req_sr_overlay_match_all_step_done()` which is not very clean. So the code in `cache_req.c` will change to something like this:
```c static errno_t cache_req_search_domains(struct tevent_req *req, struct cache_req_domain *cr_domain, bool check_next, bool bypass_cache, bool bypass_dp) { [...]
subreq = cache_req_search_domains_send(state, state->ev, state->cr, cr_domain, check_next, bypass_cache, bypass_dp); if (subreq == NULL) { return ENOMEM; }
----tevent_req_set_callback(subreq, cache_req_done, req); ++++tevent_req_set_callback(subreq, cache_req_search_domains_done, req);
return EAGAIN; }
static void cache_req_search_domains_done(...) { ...
subreq = cache_req_session_recording_send(...); if (subreq == NULL) { return ENOMEM; }
tevent_req_set_callback(subreq, cache_req_done, req); }
static void cache_req_search_done(...) { cache_req_session_recording_recv() ... } ```
* **NSS: Substitute session recording shell** - Changes here looks like they may go into separate function.
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-292153700
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Hi Pavel, thanks a lot for a thorough review and for your kind words! I'll be fixing the code according to your comments and will answer your question below.
I would welcome a little bit shorter enum values for scope, e.g. SESSION_RECORDING_SCOPE_NONE. Or do you think that the CONF word is important?
Not really, I can rename the module to just "session_recording.c" and remove "_CONF" from the names. It's an artifact from the time there were more "session_recording" modules in the same directory, and, anyway, SSSD is not adhering to "module name is the symbol name prefix" policy anyway. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-292191969
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pardon for the review request, Pavel. Trying to figure out how they work on GitHub. Please ignore. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293225994
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """
Primary group is specified with gidNumber and suplementary groups are specified with memberUid/memberOf. Primary group may or may not be part of suplementary groups so yes, you need to special case it here.
Pavel, how can we get the primary group name here, in case it is not in the supplementary groups? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293244825
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pavel, another question: aren't we skipping the request altogether in dp_initgroups, instead of just the post-processing callback here: ```C ret = sysdb_initgroups(sbus_req, domain, data->filter_value, &res); if (ret == ENOENT || (ret == EOK && res->count == 0)) { /* There is no point in contacting NSS responder. Proceed as usual. */ return EAGAIN; } else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get initgroups [%d]: %s\n", ret, sss_strerror(ret)); goto done; } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293271093
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pavel, another question: aren't we skipping the request altogether in dp_initgroups, instead of just the post-processing callback here: ```C ret = sysdb_initgroups(sbus_req, domain, data->filter_value, &res); if (ret == ENOENT || (ret == EOK && res->count == 0)) { /* There is no point in contacting NSS responder. Proceed as usual. */ return EAGAIN; } else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get initgroups [%d]: %s\n", ret, sss_strerror(ret)); goto done; } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293271093
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ You can obtain name of the primary group by calling `sysdb_getgrgid` on `SYSDB_GIDNUMBER` attribute of the user. You can introduce new function to sysdb that will do this, e.g. `sysdb_get_primary_group`.
If we hit the branch that returns EAGAIN, we issue the request without post-process function in the caller: ``` dp_get_account_info_handler: if ((data->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_INITGROUPS) { ret = dp_initgroups(sbus_req, dp_cli, key, dp_flags, data); if (ret != EAGAIN) { goto done; } }
dp_req_with_reply(dp_cli, domain, "Account", key, sbus_req, DPT_ID, DPM_ACCOUNT_HANDLER, dp_flags, data, dp_req_reply_std, struct dp_reply_std); ``` """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293520669
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ You can obtain name of the primary group by calling `sysdb_getgrgid` on `SYSDB_GIDNUMBER` attribute of the user. You can introduce new function to sysdb that will do this, e.g. `sysdb_get_primary_group`.
If we hit the branch that returns EAGAIN, we issue the request without post-process function in the caller: ``` dp_get_account_info_handler: if ((data->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_INITGROUPS) { ret = dp_initgroups(sbus_req, dp_cli, key, dp_flags, data); if (ret != EAGAIN) { goto done; } }
dp_req_with_reply(dp_cli, domain, "Account", key, sbus_req, DPT_ID, DPM_ACCOUNT_HANDLER, dp_flags, data, dp_req_reply_std, struct dp_reply_std); ``` """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293520669
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Thanks, Pavel! Is it ensured that the primary group name will always be in sysdb at that point? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293521162
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Hmm, good question. In theory, yes. If user is cached and group object with this gid exists in LDAP then the primary group is in cache as well. However, the primary group object doesn't have to exists in LDAP and in this case sssd doesn't create a mock object so it won't be in cache. I'm not really sure from the top of my head though, please, try it. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293524535
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Thanks, Pavel.
I'm still confused, though. At the moment we invoke `create_initgr_ctx` with an `ldb_result` returned from `sysdb_initgroups`. If `sysdb_initgroups` returned `ENOENT`, or produced `res->count == 0`, we will have nothing to supply `create_initgr_ctx` with, and even if it worked, `dp_req_initgr_pp_sr_overlay` would have nothing to match against.
I guess I don't understand the whole mechanism: first we get the result from the cache, then we do the request. Doesn't make sense to me. Could you please explain what's going on here?
Thank you
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-293816585
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ So the question is why we first search the cache then do the request?
The current code is to notify nss responder about changes: 1. Call `sysdb_initgroups` and remember the groups that are cached at the moment. 2. Update the cache with the content from LDAP. 3. Send original groups to NSS responder so it can decide if anything has changed. If so, it invalidates in-memory cache for initgroups.
Thinking about it, this is something that can be moved to NSS responder now. Anyway...
What you want to achieve is: 1. Update the cache 2. Call `sysdb_initgroups` to fetch the groups 3. Match the groups.
I didn't realize it during the review that you are not doing it in this way, sorry about that. So you don't have really that much in common with the NSS request, you just want to hook into the postprocess function which is called when the request is finished, before the result is send to the responder.
Does this answer your question? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-294822182
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Thank you. Yes, I think it answers my question. I'll do another sysdb_initgroups in the postprocess function. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-294827141
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pavel, after your suggestion on IRC I replaced `sysdb_initgroups` with `sysdb_initgroups_with_views` in the data provider, and also switched to using `sss_view_ldb_msg_find_attr_as_uint64` and `sysdb_getgrgid_with_views` for retrieving the primary group, but the result is the same.
All the test_session_recording_some_groups_overridden* tests fail, there is no trace of group overrides being in effect. I suspect I'm missing something basic. Do you have any suggestions? Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-295768680
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pavel, after your suggestion on IRC I replaced `sysdb_initgroups` with `sysdb_initgroups_with_views` in the data provider, and also switched to using `sss_view_ldb_msg_find_attr_as_uint64` and `sysdb_getgrgid_with_views` for retrieving the primary group, but the result is the same.
All the test_session_recording_some_groups_overridden* tests fail, there is no trace of group overrides being in effect. I suspect I'm missing something basic. Do you have any suggestions? Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-295768680
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pavel, I tried to address all your comments, and also added the fix you made to data provider initialization regarding overrides. I also improved the tests. This is ready for another review. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-296623885
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Hi, here are some comments. Mostly nitpicks.
Can you also share some link how to enable tlog and test this code?
* **CACHE_REQ: Rename search_domains_done to search_domains_next_done```** As far as tevent coding style goes `cache_req_search_domains_done` is correct since the request name is `cache_req_search_domains`. Name `cache_req_search_domains_next_done` is incorrect in this context where it serves as the last callback.
The tevent coding style should keep the following form: ``` struct tevent_req *request_name_send(...) errno_t request_name_$step(...) void request_name_$stepdone(...) void request_name_done(...) errno_t request_name_recv(..) ``` Where `$step` and `$stepdone` is something that describes this intermediate step. `$stepdone` may be simply `$step_done` where it works or any other string if it works better. But the last callback name must be `request_name_done`. Please, discard this patch.
* **CACHE_REQ: Rename done to search_domains_done** Here is the same problem now. But in addtion, you are messing up with the order of the functions where the last callback called is place before another callback. So I propose the following order and name changes:
``` !. cache_req_send 2. cache_req_process_input 3. cache_req_input_parsed 4. cache_req_select_domains 5. cache_req_search_domains 6. cache_req_search_domains_done => cache_req_process_result 7. cache_req_sr_overlay_done => cache_req_done ``` So just change those names and switch order of these two functions. Please, discard this patch and do this change in commit where you create `cache_req_sr_overlay_done`.
* **DP: Overlay sessionRecording attribute on initgr** Please remove unneeded parameters instead of silencing the compilator with `(void)req_name; (void)reply;`. Also remove unused `reply` from `dp_req_initgr_pp_nss_notify`.
* **CACHE_REQ: Pull sessionRecording attrs from initgr** See previous tevent comment and also please read [this comment](https://github.com/SSSD/sssd/pull/154#issuecomment-284717277) where I explained tevent coding style to Fabiano and I would like to obey it in `cache_req_sr_overlay.c`. The style is mostly about keeping order and proper names of function. This code should read as: ``` cache_req_sr_overlay_send cache_req_sr_overlay_match_users cache_req_sr_overlay_match_all cache_req_sr_overlay_done cache_req_sr_overlay_recv ``` """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297006120
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Hi Pavel, thank you for your review! I'll be addressing your comments soon, but for now here is how to test this.
The patches add support for a new section in sssd.conf: `session_recording`. The section can have up to three options: `scope`, `users`, and `groups`. The `scope` option accepts one of three values: `none`, `all`, and `some`. They mean "no users will be recorded", "all users will be recorded", and "some (specified) users and/or groups will be recorded", respectively.
If `scope` is set to `some`, then the `users` option accepts a whitespace-separated list of users to have session recording enabled, same goes for `groups` option, but only for groups.
Enabling session recording for a user should result in SSSD reporting user shell as `/usr/bin/tlog-rec` through NSS, and exporting `TLOG_REC_SHELL` environment variable during PAM session setup. That variable should contain the actual user shell.
Testing for those should be sufficient, but if you'd like, you can get tlog here: https://github.com/Scribery/tlog
You can either build and install it yourself, or use an RPM from the latest release: https://github.com/Scribery/tlog/releases/tag/v3
You can also take a look at the tests in `src/tests/intg/test_session_recording.py` for configuration examples.
Please let me know if you needed some other information. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297016412
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ BTW, should I perhaps include an update to the sssd.conf man page? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297017382
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Yes, that is a good idea. And although the configuration itself is rather simple, it may be a good thing to also create sssd-tlog or sssd-session-recording manpage with few paragraphs describing the future and how to use it in more details just to promote it. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297309335
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Alright, I'll work on the manpages as well.
Another thing: should we also make a configure option for enabling session recording, so that we can have this enabled and have an RPM dependency in Fedora, where tlog is already available, and disabled in RHEL and everywhere else, until it is available there? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297404214
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Hmm. There is no code dependency so SSSD can be built fine even without tlog rpms, only when enabled the tlog shell won't exist. @lslebodn how do you want to package this?
I don't think we have to have the code conditionally built, maybe is sufficient to hide manual pages. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297678575
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Perhaps we shouldn't do *any* conditional building. Just note in the manpages that the tlog package must be installed for this to work. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297738135
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Pavel, I tried to address your latest comments. I had a difficulty with the last one, though, the comment regarding "CACHE_REQ: Pull sessionRecording attrs from initgr". I think I fixed the order, but I'm not sure if you wanted me to change any names, and if so, to what. Please explain what you need me do, if I did it wrong.
I made the draft manpage updates too, please take a look. I think perhaps we shouldn't do any conditional building of tlog support, but I worry what RHEL customers will say when they know tlog is not available in RHEL yet. Perhaps we can get it in EPEL?
Another thing: I'll likely replace `tlog-rec` with something else, perhaps something called `tlog-session`, because I want to have `tlog-rec` purely for recording and not track user sessions. Just a heads-up. If you'd like details, see Scribery/tlog#92. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297766450
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """
Hmm. There is no code dependency so SSSD can be built fine even without tlog rpms, only when >enabled the tlog shell won't exist. @lslebodn how do you want to package this?
IIUC it is a runtime dependency because sssd need to compile on el7 :-) We can do similar think as with the option `enable_files_domain`. Default is `false` and there is configure time option `--enable-files-domain` to change default value.
https://pagure.io/SSSD/sssd/blob/master/f/src/confdb/confdb.c#_1833
Does this answer the question? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297939607
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Hi Lukas,
I'm sorry I don't quite understand. I agree that SSSD would have a runtime dependency on tlog to use this feature, i.e. tlog would need to be installed to work. However, how does this come from SSSD needing to compile on el7? Also what part of enable_files_domain we can reuse?
Pavel, perhaps you understand and can explain?
Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297973256
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """
I'm sorry I don't quite understand.
And I do not understand what do you want to achieve with tlog :-)
Also what part of enable_files_domain we can reuse?
I meant the approach as with `enable_files_domain` might help with tlog. But I am not sure what do you want to achieve.
man sssd.conf says: ``` enable_files_domain (boolean) When this option is enabled, SSSD prepends an implicit domain with “id_provider=files” before any explicitly configured domains. ```
The default value depends on the configure time option `--enable-files-domain` and xml for an page contains: ``` <para condition="no_enable_files_domain"> Default: false </para> <para condition="enable_files_domain"> Default: true </para> ```
By default implicit files domain is not enabled and in f26+ spec file explicitly use `--enable-files-domain`
Hope it helps. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297987874
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Ah, I see. I understand what you meant regarding enable-files-domain. I'll try to explain what we're trying to do with tlog.
First of all, session recording should always be disabled by default. No matter if tlog is present or not. Next, SSSD will function perfectly, also no matter if tlog is present or not, even if session recording is enabled and configured. The only problem is the users won't be able to login if session recording is enabled for them and tlog is not installed.
A little recap on how session recording setup works in SSSD: you get to choose if session recording is disabled for all, enabled for all, or enabled for some users/groups. In the latter case you get to choose which users/groups have it enabled. For enabled users SSSD makes NSS report their shell as tlog, and puts the original shell into an environment variable when PAM sets up the session, so tlog can start the original shell. So if tlog is not installed, users simply get non-existing shell.
I was thinking we could just have session recording support built and documented in manpages unconditionally, and only add it to SSSD dependencies on distros where tlog is present. However, I worry that RHEL customers will then be misled, because I expect tlog is not going to be available for RHEL for a while.
So the question is do we build/document session recording conditionally, selected at configure time, or not. Also, do we add tlog to dependencies if we have session recording support built. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297999419
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Ah, I see. I understand what you meant regarding enable-files-domain. I'll try to explain what we're trying to do with tlog.
First of all, session recording should always be disabled by default. No matter if tlog is present or not. Next, SSSD will function perfectly, also no matter if tlog is present or not, even if session recording is enabled and configured. The only problem is the users won't be able to login if session recording is enabled for them and tlog is not installed.
A little recap on how session recording setup works in SSSD: you get to choose if session recording is disabled for all, enabled for all, or enabled for some users/groups. In the latter case you get to choose which users/groups have it enabled. For enabled users SSSD makes NSS report their shell as tlog, and puts the original shell into an environment variable when PAM sets up the session, so tlog can start the original shell. So if tlog is not installed, users simply get non-existing shell.
I was thinking we could just have session recording support built and documented in manpages unconditionally, and only add it to SSSD dependencies on distros where tlog is present. However, I worry that RHEL customers will then be misled, because I expect tlog is not going to be available for RHEL for a while.
So the question is do we build/document session recording conditionally, selected at configure time, or not. Also, do we add tlog to dependencies if we have session recording support built. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-297999419
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """
The only problem is the users won't be able to login if session recording is enabled for them and tlog is not installed.
That sounds like misconfiguration. In fedora, I would prefer to have weak dependency[1] on tlog in sssd. With "recommends" it would be installed by default but users would be able remove tlog package. Therefore we cannot rely on existing binary on system. But we can detect that after starting sssd that session recording is enabled and there is missing tlog shell(missing execute bit ...). Then we have two options: 1. fail due to misconfiguration; 2. scream to syslog + disable session recording and continue as it was not enabled.
[1] http://rpm.org/user_doc/dependencies.html#weak-dependencies
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298012520
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing. If it can be fixed by just installing tlog, then we shouldn't break SSSD, and let users fix the situation with as little fuss as possible. I.e. without the need to restart SSSD.
I wouldn't say it's a misconfiguration of SSSD, it's a system misconfiguration, and it's not SSSD's business to abort because of that. As far as SSSD is concerned its configuration is valid. However, I think SSSD can shout in the log that tlog is missing. The question is at which point. Maybe on PAM session setup? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298024703
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing. If it can be fixed by just installing tlog, then we shouldn't break SSSD, and let users fix the situation with as little fuss as possible. I.e. without the need to restart SSSD.
I wouldn't say it's a misconfiguration of SSSD, it's a system misconfiguration, and it's not SSSD's business to abort because of that. As far as SSSD is concerned its configuration is valid. However, I think SSSD can shout in the log that tlog is missing. The question is at which point. Maybe on PAM session setup? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298024703
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing. If it can be fixed by just installing tlog, then we shouldn't break SSSD, and we should let users fix the situation with as little fuss as possible. I.e. without the need to restart SSSD.
I wouldn't say it's a misconfiguration of SSSD, it's a system misconfiguration, and it's not SSSD's business to abort because of that. As far as SSSD is concerned its configuration is valid. However, I think SSSD can shout in the log that tlog is missing. The question is at which point. Maybe on PAM session setup? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298024703
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """ On (28/04/17 08:11), Nikolai Kondrashov wrote:
Sure, a weak dependency sounds right.
I would not like disabling SSSD or session recording in case tlog is missing.
It is not disabling SSSD; because SSSD will not even start any responder or back-end :-). And sssd can fail to start even due to other misconfiguration e.g. wrong permissions on /etc/sssd/sssd.conf ...
If it can be fixed by just installing tlog, then we shouldn't break SSSD, and let users fix the situation with as little fuss as possible. I.e. without the need to restart SSSD.
* sssd won't start and log message to syslog that session recording is enabled (scope != none) and tlog is missing. * user will try to find a reason why sssd failed to start; check log files and find out that there is missing binary for tlog shell * user will install tlog * start sssd and everything works like a magic :-)
It is just an idea how to simply solve this case. But feel free to use other way.
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298048544
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Yeah, perhaps not starting SSSD can help.
Still, there is the issue of tlog not being available for RHEL at all, at least not from official channels. Should SSSD have session recording documented and supported then? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298063944
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ There is one other problem: integration tests. If we make SSSD fail on startup when it doesn't find tlog-rec, then we can't run SSSD as is in integration tests. That is without requiring tlog-rec be installed for integration testing, which is hassle, as tlog is only present in Fedora so far.
So, either we make it a warning, which will be harder to notice for users, or perhaps add a configure option to make the session recording shell something that we know always exists, like `/bin/true`.
I don't like the complications of the latter, but I think it might be better for user.
Pavel, Lukas? """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298608203
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Integration tests can be skipped if tlog is not present. We already do this for secrets responder that is skipped if sssd_secrets does not exist.
I agree that we should fail to start as ignoring session recording configuration may be considered as security issue (administrator wants audit but no audit is done). """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298610282
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Integration tests don't actually need tlog to verify that SSSD's job is done properly. Also, tlog is not going to be officially present for a while yet on most distros we test. OTOH, nothing prevents users installing it there themselves and so SSSD functionality needs to be verified.
Ignoring absence of tlog would not be a security issue, but only a functionality issue, because recorded users simply won't be able to login. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298612435
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """ On (02/05/17 04:43), Nikolai Kondrashov wrote:
Integration tests don't actually need tlog to verify that SSSD's job is done properly.
In that case, we needn't skip tests but Cwrap integration testes can be called with `--with-session-recording-shell="$$prefix"/bin/tlog-rec`
and `"$$prefix"/bin/tlog-rec` can a shell script or it needn't be present at all if it is not required for testing
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298623373
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """ Actually, it need to be available otherwise sssd would not start :-) """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298623501
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Yes, that's what I meant. Only perhaps we can just use `--with-session-recording-shell=/bin/false` and not have any scripts. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298629188
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
lslebodn commented: """ On (02/05/17 06:03), Nikolai Kondrashov wrote:
Yes, that's what I meant. Only perhaps we can just use `--with-session-recording-shell=/bin/false` and not have any scripts.
I glad there is not misunderstanding
BTW it does not matter which binary will be passed to --with-session-recording-shell. It can be even `/usr/bin/getent` :-) But it should not be standard shell e.g. `/sbin/nologin`
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298637807
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Alright, here is another version of the patchset. Changes include:
* abort SSSD startup if session recording is enabled, but session recording shell is not present, or not executable * add configure option disabling session recording (`--without-session-recording`); * fix the `--with-session-recording-shell` configure option; * rename `pam_reply_export_shell` to `pam_reply_sr_export_shell` as it deals with session recording exclusively.
If we decide we don't need the `--without-session-recording` configure option, we can just drop the last patch. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-298836574
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ @pbrezina, here is a rebased patchset, with the changes you requested. I dropped the --without-session-recording configure option as agreed with @jhrozek and @lslebodn. Please review. Thank you! """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-301030273
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ CI results: http://sssd-ci.duckdns.org/logs/job/69/46/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-301045414
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ This update rebases on latest master and also changes "tlog-rec" to "tlog-rec-session", as the latter is now supposed to be used as the login shell in upstream tlog. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-302695875
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Thank you. I will look into it during this week. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-303039439
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Hi Nick, thank your for the changes. One last nitpick and I will ack those patches. Please, switch order of `cache_req_process_result` and `cache_req_done` so it reads: ```c cache_req_process_result() cache_req_done() ``` """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-305137103
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ I'm on it, thanks for review @pbrezina!
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-305148541
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Rebased, and swapped `cache_req_process_result` and `cache_req_done`, as requested. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-305174143
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ CI result: http://sssd-ci.duckdns.org/logs/commit/17/e2bb8c81901721fc35f7807f8c000aea95... """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-305196182
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
pbrezina commented: """ Ack. Thank you. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-305426853
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
fidencio commented: """ As 1.15.3 was released and per @pbrezina's ACK, I'm adding the "Accepted" label to this patch set. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-318153855
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
jhrozek commented: """ I've just fired one more CI run (because there were so many patches since pbrezina's ACK) and when that arrives, I'll push. """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-318165730
URL: https://github.com/SSSD/sssd/pull/136 Author: spbnick Title: #136: Tlog integration Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/136/head:pr136 git checkout pr136
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
jhrozek commented: """ Since we released 1.15.3 (finally!) ealier this week, I merged the patches: * 27c30eb5f046d6c43276b139706110906cdacb9b * 53a4219e2f51cd0443931aa931505bf0b4bf5a45 * 49d24ba630544632e29ed397627c97352523165d * 836dae913497e150bd0ec11eee1e256e4fcc0bb7 * 382a972a80ac571cdbf70d88571f6de49fe1cd23 * 24b3a7b91a54b5b55cfddb52b3d5ac565afdcff1 * 200787df74510f6edc9387cf9c33f133ccfc0ae3 * bac0c0df377de4469c8f9310179eef04c7b091fa * 90fb7d3e61423ff1375e9f552f4b58e5173ad3d1 * 5ea60d18ddb8eaff25d274c22c7db7df57b6ec4d * 29dd456102dc995aa59a56483363087071bb84d6 * 99b96048b79b0228c3f7c431ea12010f7bd5b362 * d802eba25e7c1304e5036684261bcf41540532d8 * 555f43b491f40e0237b8677565a748b929092bee * 9759333b3dd404c6787ef0186984c5d4256eb5bb * c31065ecc0793e836066035d0c692b050b5f6f55 * cb89693cf5ccdedf69fa304c6d43d618a7bc18b2
"""
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-318342201
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration
spbnick commented: """ Woo-hoo :D! Thanks a lot for all the work, Pavel, Lukas and Jakub :)! """
See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-318342564
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
lslebodn
-
pbrezina
-
spbnick