[SSSD] [sssd PR#136][comment] Tlog integration
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 mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
URL: https://github.com/SSSD/sssd/pull/136 Title: #136: Tlog integration spbnick commented: """ CI result: http://sssd-ci.duckdns.org/logs/commit/17/e2bb8c81901721fc35f7807f8c000aea950c27/7063/summary.html """ See the full comment at https://github.com/SSSD/sssd/pull/136#issuecomment-305196182 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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: ``` Default: false Default: true ``` 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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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,
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration WIP
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration WIP
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration WIP
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#136][comment] Tlog integration WIP
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 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
