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
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
*
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
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
pbrezina commented:
"""
Ack. Thank you.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/136#issuecomment-305426853
___
sssd-devel mailing list --
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
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:
"""
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
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
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
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
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
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
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
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
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:
"""
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
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
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
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
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;
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
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
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
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
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
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
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
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
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
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,
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
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
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
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
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
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
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.
"""
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
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
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
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
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
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
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:
"""
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.
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.
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,
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,
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
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:
"""
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
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,
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
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
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
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
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
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
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
60 matches
Mail list logo
