On 08/29/2016 11:47 PM, Sumit Bose wrote:
On Thu, Aug 25, 2016 at 01:03:19PM -0400, Simo Sorce wrote:
> On Thu, 2016-08-25 at 14:24 +0300, Nikolai Kondrashov wrote:
>> On 08/24/2016 11:24 PM, Simo Sorce wrote:
>>> On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote:
>>>> Attached is the third version of work-in-progress SSSD/tlog integration
>>>> patches. I'm sending them in the hope that somebody takes a look and
>>>> perhaps points out some wrong bits I can fix before I'm too dependent
on
>>>> them.
>>>>
>>>> The changes from the last version is some refactoring of the NSS and the
>>>> common parts, plus start of the PAM part of the implementation.
>>>>
>>>> Also, at this point, I think I could contribute some general fixes and
>>>> prerequisite refactoring patches separately.
>>>
>>> So I have been going through the patchset and I have concerns about how
>>> you are determining if the shell needs to be substituted with the
>>> session recording shell.
>>> It seem you do this work every single time a getpwname/uis/etc request
>>> is run. this is very expensive as you do a full group search on each of
>>> those requests, to find data that arguably rarely changes.
>>>
>>> I think in general this should be done at "write" time not at
"read"
>>> time.
>>> Ie whenever the the session recording configuration changes or when a
>>> new user is written in the cache, then you should check if session
>>> recording apply to this user and write an attribute in the user entry.
>>>
>>> On getpwnam/uid/ent calls you would look for those calls and replace the
>>> shell entry accordingly.
>>>
>>> Unless there is some very good reason to do it always at query time this
>>> is, I am afraid, a nack on the approach.
>>
>> I'm not sure I fully understand your suggestion, but I like it so far.
>> I wanted to do something like this from the start, but Sumit had some concerns
>> which I can't quite remember now.
>>
>> Sumit, maybe you can take a look at this?
Sorry for the delay, I wanted to test the patches first. And the good
news is that imo the NSS part works quite smoothly (nevertheless there
might be one or two ares for improvement :-).
That's without doubt :)
First a general comment about the refactoring patches for the NSS
responder. Pavel is working on replacing the current code with a
cache_req_* based one which I think would simplify a lot of things for
you as well. Please try to coordinate with him, maybe he has a WIP
branch which you can you as a base for your patches.
I will.
Then you do a normal user lookup but if there are groups configured
in
the session_recording section you always do a SSS_DP_INITGROUPS instead
of a SSS_DP_USER if the entry is expired. I think here you can add some
improvement to reduce the number of SSS_DP_INITGROUPS calls which are
more expensive than SSS_DP_USER. User entries in the cache have two
timeout values SYSDB_CACHE_EXPIRE and SYSDB_INITGR_EXPIRE. The first is
the expiration time for the general user data (name, home directory,
shell etc) and is checked during the normal user lookup. The second is
the expiration time of the group-membership data. To make sure that
SSS_DP_INITGROUPS is only used when needed I would recommend to
additionally check if SYSDB_INITGR_EXPIRE is expired and only do a
SSS_DP_INITGROUPS in this case and a SSS_DP_USER in all other cases.
Currently both timestamps might not differ often but future enhancements
to the backends might change this.
Alright, I'll try to figure out how to do that.
Finally you call sysdb_initgroups_with_views() to get the list of
groups
the user is a member of and compare them with the groups form the
session_recording configuration. Since you compare the DNs I think this
can be improved a bit. sysdb_initgroups_with_views() does a dereference
search based on the memberOf attribute of the user which holds the DNs
of all the groups the user is a member of. But since you only need
the DNs of the groups for the comparison you can just add the memberOf
attribute to the attribute list in nss_cmd_getpwnam_search() and friends
to make the DNs of the groups available in
session_recording_is_enabled().
Ah, yes, I forgot, you suggested that before. Shall I add that attribute
unconditionally, regardless of session recording settings?
Based in the result of session_recording_is_enabled() it would be
possible to add a new attribute indicating if session recording is
required for the user or not. But since we have the memory cache I
wonder if the comparison of the DNs will hurt that much. Additionally I
would like to wait with such kind of optimizations until we have a good
understanding how the solution we plan for IPA will be implemented and
what kind of optimizations might be needed here to hopefully find a
solution which can give good performance in both cases.
I agree with this. There are too many unknowns still.
Thanks a lot for the review, Sumit!
Nick