2:02 p.m.
Hi everyone,
Please find attached proof-of-concept patches for a part of NSS integration
with tlog. Namely, addition of shell substitution for getpwnam requests.
The code is supposed to replace a user's shell with /usr/bin/tlog-rec, if
session recording is enabled for all users, if it is enabled for that
particular user, or for a group that it belongs to.
The configuration is done in a dedicated section of sssd.conf named
"session_recording", which can contain three options "scope",
"users", and
"groups". Those correspond to the scope of session recording: "none",
"some",
and "all", corresponding in order to: disabled session recording, session
recording enabled for the specified users/groups, and session recording
enabled for all users handled by SSSD.
An example of a configuration can be:
[session_recording]
; Disabled
scope = none
or
[session_recording]
; Enabled for everyone
scope = all
or
[session_recording]
; Enabled for some users and groups
scope = some
users = user1, user2
groups = group1, group2
The parts to be done still are adding support for getpwuid and getpwent
requests, exporting of the original shell in pam_sss, and of course cleaning
it up and doing it according to your comments and requirements.
The code has some documentation in doxygen format, which I can change later if
we decide on some other format, or no documentation at all.
Please, tell me if I'm doing anything wrong this far already, or suggest
better ways to do it.
Thank you!
Nick
P.S. I'm on PTO for two weeks starting next week, so might not be able to
answer quickly.
6:46 a.m.
New subject: [RFCv2] NSS tlog integration
Hi everyone,
Here is the second version of RFC for SSSD/tlog integration. This one covers
support for all necessary NSS requests: getpwnam, getpwuid and getpwent.
I don't think it handles caching well, is probably not very fast, has little
tests, and still tries to document code, apart from likely being wrong in many
parts otherwise, but I would like to show it to you, so you can correct me
earlier rather than later.
I'll proceed to the PAM part and adding more tests in the meantime.
Thank you!
Nick
9:24 a.m.
New subject: [RFCv3] NSS tlog integration
Hi everyone,
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.
Nick
3:24 p.m.
New subject: [RFCv3] NSS tlog integration
On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote:
Hi everyone,
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.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
6:24 a.m.
New subject: [RFCv3] NSS tlog integration
Hi Simo,
Thanks for looking at the patches and for the feedback!
I'm replying below.
On 08/24/2016 11:24 PM, Simo Sorce wrote:
On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote:
> Hi everyone,
>
> 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?
I'll try to describe what I think you mean. So, we can modify the
user cache-filling machinery to look at the session recording configuration
upon adding a new user, and if session recording is enabled for the user
(in whatever way), add an attribute signifying that. Then NSS could just check
that attribute and substitute the shell when needed. I assume PAM would also
be able to do that. Although it would still need the shell override logic,
currently residing in NSS (I started another thread on this). Is that what you
mean?
Still, I have a bunch of concerns about this, which perhaps you can help me
resolve.
Would it be OK for the cache machinery to also pull groups into the cache, for
which session recording is enabled, so that it can check if the user belongs
to them? It would need to fetch them by names that the administrator would put
into the configuration. I.e. with all the overrides, whitespace, and other
changes.
Similarly, would the cache machinery be in a good position to understand the
names of the users listed in the session recording configuration? I.e. I
expect the administrators would like to put there names as they see them. Also
with overrides, whitespace and other tweaks NSS does.
Then, I expect we need to check whether a user should be recorded also on
cache refresh, not only on initial addition and configuration change. Because
groups can go away, or their names might change due to overrides.
Lastly, user names might change due to overrides. Can this approach handle it?
Thank you!
Nick
12:03 p.m.
New subject: [RFCv3] NSS tlog integration
On Thu, 2016-08-25 at 14:24 +0300, Nikolai Kondrashov wrote:
Hi Simo,
Thanks for looking at the patches and for the feedback!
I'm replying below.
On 08/24/2016 11:24 PM, Simo Sorce wrote:
> On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote:
>> Hi everyone,
>>
>> 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?
I'll try to describe what I think you mean. So, we can modify the
user cache-filling machinery to look at the session recording configuration
upon adding a new user, and if session recording is enabled for the user
(in whatever way), add an attribute signifying that. Then NSS could just check
that attribute and substitute the shell when needed. I assume PAM would also
be able to do that. Although it would still need the shell override logic,
currently residing in NSS (I started another thread on this). Is that what you
mean?
Yes
Still, I have a bunch of concerns about this, which perhaps you can
help me
resolve.
Would it be OK for the cache machinery to also pull groups into the cache, for
which session recording is enabled, so that it can check if the user belongs
to them? It would need to fetch them by names that the administrator would put
into the configuration. I.e. with all the overrides, whitespace, and other
changes.
I am a bit concerned that this list is based on group names in general,
how do you deal with local groups that list a remote user as member ?
Similarly, would the cache machinery be in a good position to
understand the
names of the users listed in the session recording configuration? I.e. I
expect the administrators would like to put there names as they see them. Also
with overrides, whitespace and other tweaks NSS does.
You have access to the same configuration rules that the frontend has
access to if you need, so you can do whatever mapping is needed at store
time.
Then, I expect we need to check whether a user should be recorded
also on
cache refresh, not only on initial addition and configuration change. Because
groups can go away, or their names might change due to overrides.
The check needs to be performed any time that:
- a user is added or refreshed
- a group is added or refreshed (memberships changed)
- the session recording configuration is changed
Lastly, user names might change due to overrides. Can this approach
handle it?
afaik overrides are also store in cache so it can be taken in account.
I am curious about what are Sumit concerns though.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
3:47 p.m.
New subject: [RFCv3] NSS tlog integration
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:
> Hi Simo,
>
> Thanks for looking at the patches and for the feedback!
> I'm replying below.
>
> On 08/24/2016 11:24 PM, Simo Sorce wrote:
> > On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote:
> >> Hi everyone,
> >>
> >> 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 :-).
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.
About Simo's concern. If there are groups defined in the
session_recording configuration you look up those groups with
cache_req_group_by_name_send(). As long as the groups are in the cache
and not expired they are returned directly from the cache. I think this
lookup cannot be avoided because in theory the groups name, be it the
original name or an override name, might change. And you want to be sure
that the name listed in the configuration is still a valid group name.
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.
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().
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.
HTH
bye,
Sumit
> >
> > I'll try to describe what I think you mean. So, we can modify the
> > user cache-filling machinery to look at the session recording configuration
> > upon adding a new user, and if session recording is enabled for the user
> > (in whatever way), add an attribute signifying that. Then NSS could just check
> > that attribute and substitute the shell when needed. I assume PAM would also
> > be able to do that. Although it would still need the shell override logic,
> > currently residing in NSS (I started another thread on this). Is that what you
> > mean?
>
> Yes
>
> > Still, I have a bunch of concerns about this, which perhaps you can help me
> > resolve.
> >
> > Would it be OK for the cache machinery to also pull groups into the cache, for
> > which session recording is enabled, so that it can check if the user belongs
> > to them? It would need to fetch them by names that the administrator would put
> > into the configuration. I.e. with all the overrides, whitespace, and other
> > changes.
>
> I am a bit concerned that this list is based on group names in general,
> how do you deal with local groups that list a remote user as member ?
>
> > Similarly, would the cache machinery be in a good position to understand the
> > names of the users listed in the session recording configuration? I.e. I
> > expect the administrators would like to put there names as they see them. Also
> > with overrides, whitespace and other tweaks NSS does.
>
> You have access to the same configuration rules that the frontend has
> access to if you need, so you can do whatever mapping is needed at store
> time.
>
> > Then, I expect we need to check whether a user should be recorded also on
> > cache refresh, not only on initial addition and configuration change. Because
> > groups can go away, or their names might change due to overrides.
>
> The check needs to be performed any time that:
> - a user is added or refreshed
> - a group is added or refreshed (memberships changed)
> - the session recording configuration is changed
>
> > Lastly, user names might change due to overrides. Can this approach handle it?
>
> afaik overrides are also store in cache so it can be taken in account.
>
> I am curious about what are Sumit concerns though.
>
> Simo.
>
> --
> Simo Sorce * Red Hat, Inc * New York
>
4:18 p.m.
New subject: [RFCv3] NSS tlog integration
On Mon, 2016-08-29 at 22:47 +0200, Sumit Bose wrote:
About Simo's concern. If there are groups defined in the
session_recording configuration you look up those groups with
cache_req_group_by_name_send(). As long as the groups are in the cache
and not expired they are returned directly from the cache. I think
this lookup cannot be avoided because in theory the groups name, be it
the original name or an override name, might change. And you want to
be sure that the name listed in the configuration is still a valid
group name.
My concern is that an initgroups call can be quite expensive in certain
domains where a user is a member of many groups.
If we really want to check memberships based on session recording
configuration *at query time* rather than at storing time, I would think
it would be more efficient to resolve only the group memberships of the
groups listed in the session recording configuration, and then check if
the user is member of any of those groups.
Given in general this kind of access is geared toward auditing the work
of small groups of people (admins with root access or similar), it would
probably be more efficient than calling initgroups, as it will end up
pulling a less data, less often.
If session recording is invoked often those groups will be cached so
very few lookups would be made even if many different users log into the
system. If you call initgroups instead you may end up updating several
dozen groups for each user login.
What do you think ?
Simo.
--
Simo Sorce * Red Hat, Inc * New York
6:21 a.m.
New subject: [RFCv3] NSS tlog integration
On Mon, Aug 29, 2016 at 05:18:58PM -0400, Simo Sorce wrote:
On Mon, 2016-08-29 at 22:47 +0200, Sumit Bose wrote:
> About Simo's concern. If there are groups defined in the
> session_recording configuration you look up those groups with
> cache_req_group_by_name_send(). As long as the groups are in the cache
> and not expired they are returned directly from the cache. I think
> this lookup cannot be avoided because in theory the groups name, be it
> the original name or an override name, might change. And you want to
> be sure that the name listed in the configuration is still a valid
> group name.
My concern is that an initgroups call can be quite expensive in certain
domains where a user is a member of many groups.
If we really want to check memberships based on session recording
configuration *at query time* rather than at storing time, I would think
it would be more efficient to resolve only the group memberships of the
groups listed in the session recording configuration, and then check if
the user is member of any of those groups.
Given in general this kind of access is geared toward auditing the work
of small groups of people (admins with root access or similar), it would
probably be more efficient than calling initgroups, as it will end up
pulling a less data, less often.
If session recording is invoked often those groups will be cached so
very few lookups would be made even if many different users log into the
system. If you call initgroups instead you may end up updating several
dozen groups for each user login.
What do you think ?
I share your concerns but I think we are between a rock and a hard place
here. For AD e.g. I think we have quite a efficient way to do initgroups
with the tokenGroups request and hopefully will have the PAC with
S4U2Self soon. Group lookups on AD might be more costly due to nested
groups.
Additionally we have to take the ignore_group_members options into
account which might be enabled in environments with a larger number of
users and groups.
As mentioned in my previous mail I would recommend to only do an
initgroups request to the backend if SYSDB_INITGR_EXPIRE is exprired and
not the user entry (SYSDB_CACHE_EXPIRE). Currently the timeout for both
is configured with entry_cache_user_timeout. But we can add an option
which would allow to set a higher timeout to SYSDB_INITGR_EXPIRE so that
initgroups in the backend is called more rarely. This might mitigate the
situation a bit.
Unfortunately we cannot rely on the forced initgroups request we do
during authentication because "all" applications (I haven't checked all
but it is a very typical pattern) call getpwnam() first and then call
pam_authenticate(). And chances are high that they just use the shell
returned by this getpwnam().
While thinking about it a session recording attribute in the user entry
as you suggested might help in the following way (maybe this is even
what you suggested but I didn't understood it while reading :-):
- attribute present:
* getpw* requests in the NSS responder will return the result
according to the value of the attribute
* only initgroups requests will update the attribute
- attribute not present:
* all requests will set the attribute but only if SYSDB_INITGR_EXPIRE
is expired a plain user request is replaced by an initgroups
request to the backends
I would prefer that the handling of this attribute happens in the NSS
responder because I think the backends should not be aware of this
"application specific" attribute.
So only the first getpwnam() for a user might have extra costs (besides
the refresh of the session recording groups which should typically be
covered by the mid-point refresh).
What do you think?
bye,
Sumit
bye,
Sumit
>
> Simo.
>
> --
> Simo Sorce * Red Hat, Inc * New York
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
6:32 a.m.
New subject: [RFCv3] NSS tlog integration
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
5:42 a.m.
New subject: [RFCv3] NSS tlog integration
Hi Simo,
On 08/29/2016 11:47 PM, Sumit Bose wrote:
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.
I'm looking at this and am getting confused (old news, I know :).
Two cases where I use SSS_DP_INITGROUPS are invocations of check_cache, which
has this code:
/* if we have any reply let's check cache validity */
if (res->count > 0) {
/* ... */
if (req_type == SSS_DP_INITGROUPS) {
cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
SYSDB_INITGR_EXPIRE,
0);
} else {
cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
SYSDB_CACHE_EXPIRE,
0);
}
/* ... */
}
It seems to me that it already checks the cache as you suggested. Is that so?
The only other case is in nss_cmd_setpwent_step, which fetches users for
getpwent using sss_dp_get_account_send and it doesn't seem that I could/should
do anything there. Is that right?
Thank you.
Nick
6:57 a.m.
New subject: [RFCv3] NSS tlog integration
On 08/29/2016 11:47 PM, Sumit Bose wrote:
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().
And one more question: the above basically means that we need to make
sysdb_getpwnam retrieve "memberOf" as well. Do we want to do that?
If yes, do we want to do that unconditionally, or change the interface to have
that optional (ugh)?
Do we want to change sysdb_enumpwent_filter (used in setpwent) to do that as
well?
We went through some of this in a chat a while ago, but I just want to be
sure.
Thanks!
Nick
4:12 a.m.
New subject: [RFCv3] NSS tlog integration
On Wed, Sep 07, 2016 at 01:42:32PM +0300, Nikolai Kondrashov wrote:
Hi Simo,
On 08/29/2016 11:47 PM, Sumit Bose wrote:
> 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.
I'm looking at this and am getting confused (old news, I know :).
Two cases where I use SSS_DP_INITGROUPS are invocations of check_cache, which
has this code:
/* if we have any reply let's check cache validity */
if (res->count > 0) {
/* ... */
if (req_type == SSS_DP_INITGROUPS) {
cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
SYSDB_INITGR_EXPIRE,
0);
} else {
cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0],
SYSDB_CACHE_EXPIRE,
0);
}
/* ... */
}
It seems to me that it already checks the cache as you suggested. Is that so?
The only other case is in nss_cmd_setpwent_step, which fetches users for
getpwent using sss_dp_get_account_send and it doesn't seem that I could/should
do anything there. Is that right?
Yes and no. The idea was to not unconditionally replace SSS_DP_USER with
SSS_DP_INITGROUPS if there are groups in the session recording
configuration. If we then add a new option to configure the
SYSDB_INITGR_EXPIRE independently of SYSDB_CACHE_EXPIRE (currently both
use the entry_cache_user_timeout config option) it would be possible to
reduce the number of SSS_DP_INITGROUPS to the backend while have the
user entry itself updated on the default rate with a plain SSS_DP_USER
call.
But all this might not be needed if a session recording attribute is
used as suggested by Simo. See my reply to Simo's email in this thread
for details about how this can be implemented.
bye,
Sumit
>
> Thank you.
>
> Nick
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
4:17 a.m.
New subject: [RFCv3] NSS tlog integration
On Wed, Sep 07, 2016 at 02:57:15PM +0300, Nikolai Kondrashov wrote:
On 08/29/2016 11:47 PM, Sumit Bose wrote:
> 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().
And one more question: the above basically means that we need to make
sysdb_getpwnam retrieve "memberOf" as well. Do we want to do that?
If yes, do we want to do that unconditionally, or change the interface to have
that optional (ugh)?
It might be better to not do it unconditionally because it might cause
some unwanted processing and memory allocations. A new
sysdb_getpwnam_ex() call with new parameter for the needed attributes
might work. But as with your other questions, all this might not be
needed if a session recording attribute as suggested by Simo can be
used. Then only this attribute must be added to the default list of
sysdb_getpwnam().
bye,
Sumit
>
> Do we want to change sysdb_enumpwent_filter (used in setpwent) to do that as
> well?
>
> We went through some of this in a chat a while ago, but I just want to be
> sure.
>
> Thanks!
>
> Nick
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
5:39 a.m.
New subject: [RFCv3] NSS tlog integration
On 08/30/2016 02:21 PM, Sumit Bose wrote:
On Mon, Aug 29, 2016 at 05:18:58PM -0400, Simo Sorce wrote:
> On Mon, 2016-08-29 at 22:47 +0200, Sumit Bose wrote:
>> About Simo's concern. If there are groups defined in the
>> session_recording configuration you look up those groups with
>> cache_req_group_by_name_send(). As long as the groups are in the cache
>> and not expired they are returned directly from the cache. I think
>> this lookup cannot be avoided because in theory the groups name, be it
>> the original name or an override name, might change. And you want to
>> be sure that the name listed in the configuration is still a valid
>> group name.
>
> My concern is that an initgroups call can be quite expensive in certain
> domains where a user is a member of many groups.
> If we really want to check memberships based on session recording
> configuration *at query time* rather than at storing time, I would think
> it would be more efficient to resolve only the group memberships of the
> groups listed in the session recording configuration, and then check if
> the user is member of any of those groups.
>
> Given in general this kind of access is geared toward auditing the work
> of small groups of people (admins with root access or similar), it would
> probably be more efficient than calling initgroups, as it will end up
> pulling a less data, less often.
>
> If session recording is invoked often those groups will be cached so
> very few lookups would be made even if many different users log into the
> system. If you call initgroups instead you may end up updating several
> dozen groups for each user login.
>
> What do you think ?
I share your concerns but I think we are between a rock and a hard place
here. For AD e.g. I think we have quite a efficient way to do initgroups
with the tokenGroups request and hopefully will have the PAC with
S4U2Self soon. Group lookups on AD might be more costly due to nested
groups.
Additionally we have to take the ignore_group_members options into
account which might be enabled in environments with a larger number of
users and groups.
As mentioned in my previous mail I would recommend to only do an
initgroups request to the backend if SYSDB_INITGR_EXPIRE is exprired and
not the user entry (SYSDB_CACHE_EXPIRE). Currently the timeout for both
is configured with entry_cache_user_timeout. But we can add an option
which would allow to set a higher timeout to SYSDB_INITGR_EXPIRE so that
initgroups in the backend is called more rarely. This might mitigate the
situation a bit.
Unfortunately we cannot rely on the forced initgroups request we do
during authentication because "all" applications (I haven't checked all
but it is a very typical pattern) call getpwnam() first and then call
pam_authenticate(). And chances are high that they just use the shell
returned by this getpwnam().
While thinking about it a session recording attribute in the user entry
as you suggested might help in the following way (maybe this is even
what you suggested but I didn't understood it while reading :-):
- attribute present:
* getpw* requests in the NSS responder will return the result
according to the value of the attribute
* only initgroups requests will update the attribute
- attribute not present:
* all requests will set the attribute but only if SYSDB_INITGR_EXPIRE
is expired a plain user request is replaced by an initgroups
request to the backends
I would prefer that the handling of this attribute happens in the NSS
responder because I think the backends should not be aware of this
"application specific" attribute.
So only the first getpwnam() for a user might have extra costs (besides
the refresh of the session recording groups which should typically be
covered by the mid-point refresh).
What do you think?
Simo, I know this was a long time ago, but could you please answer Sumit?
AFAIU, he suggests we don't change the approach in general, but only do
replace a user info request with initgroups if SYSDB_INITGR_EXPIRE has
elapsed.
Sumit, I like your suggestion regarding the attribute, but what we're going to
do with local overrides? E.g. what if a group listed in the groups to record
was renamed locally? How would it work?
Thank you.
Nick
9:46 a.m.
New subject: [RFCv3] NSS tlog integration
On Wed, Dec 07, 2016 at 01:39:54PM +0200, Nikolai Kondrashov wrote:
On 08/30/2016 02:21 PM, Sumit Bose wrote:
> On Mon, Aug 29, 2016 at 05:18:58PM -0400, Simo Sorce wrote:
> > On Mon, 2016-08-29 at 22:47 +0200, Sumit Bose wrote:
> > > About Simo's concern. If there are groups defined in the
> > > session_recording configuration you look up those groups with
> > > cache_req_group_by_name_send(). As long as the groups are in the cache
> > > and not expired they are returned directly from the cache. I think
> > > this lookup cannot be avoided because in theory the groups name, be it
> > > the original name or an override name, might change. And you want to
> > > be sure that the name listed in the configuration is still a valid
> > > group name.
> >
> > My concern is that an initgroups call can be quite expensive in certain
> > domains where a user is a member of many groups.
> > If we really want to check memberships based on session recording
> > configuration *at query time* rather than at storing time, I would think
> > it would be more efficient to resolve only the group memberships of the
> > groups listed in the session recording configuration, and then check if
> > the user is member of any of those groups.
> >
> > Given in general this kind of access is geared toward auditing the work
> > of small groups of people (admins with root access or similar), it would
> > probably be more efficient than calling initgroups, as it will end up
> > pulling a less data, less often.
> >
> > If session recording is invoked often those groups will be cached so
> > very few lookups would be made even if many different users log into the
> > system. If you call initgroups instead you may end up updating several
> > dozen groups for each user login.
> >
> > What do you think ?
>
> I share your concerns but I think we are between a rock and a hard place
> here. For AD e.g. I think we have quite a efficient way to do initgroups
> with the tokenGroups request and hopefully will have the PAC with
> S4U2Self soon. Group lookups on AD might be more costly due to nested
> groups.
>
> Additionally we have to take the ignore_group_members options into
> account which might be enabled in environments with a larger number of
> users and groups.
>
> As mentioned in my previous mail I would recommend to only do an
> initgroups request to the backend if SYSDB_INITGR_EXPIRE is exprired and
> not the user entry (SYSDB_CACHE_EXPIRE). Currently the timeout for both
> is configured with entry_cache_user_timeout. But we can add an option
> which would allow to set a higher timeout to SYSDB_INITGR_EXPIRE so that
> initgroups in the backend is called more rarely. This might mitigate the
> situation a bit.
>
> Unfortunately we cannot rely on the forced initgroups request we do
> during authentication because "all" applications (I haven't checked
all
> but it is a very typical pattern) call getpwnam() first and then call
> pam_authenticate(). And chances are high that they just use the shell
> returned by this getpwnam().
>
> While thinking about it a session recording attribute in the user entry
> as you suggested might help in the following way (maybe this is even
> what you suggested but I didn't understood it while reading :-):
>
> - attribute present:
> * getpw* requests in the NSS responder will return the result
> according to the value of the attribute
> * only initgroups requests will update the attribute
> - attribute not present:
> * all requests will set the attribute but only if SYSDB_INITGR_EXPIRE
> is expired a plain user request is replaced by an initgroups
> request to the backends
>
> I would prefer that the handling of this attribute happens in the NSS
> responder because I think the backends should not be aware of this
> "application specific" attribute.
>
> So only the first getpwnam() for a user might have extra costs (besides
> the refresh of the session recording groups which should typically be
> covered by the mid-point refresh).
>
> What do you think?
Simo, I know this was a long time ago, but could you please answer Sumit?
AFAIU, he suggests we don't change the approach in general, but only do
replace a user info request with initgroups if SYSDB_INITGR_EXPIRE has
elapsed.
Sumit, I like your suggestion regarding the attribute, but what we're going to
do with local overrides? E.g. what if a group listed in the groups to record
was renamed locally? How would it work?
Local overrides are treated in the same way as the non-default IPA
overrides. So there should be not difference handling them.
bye,
Sumit
>
> Thank you.
>
> Nick
2688
days inactive
2830
days old
sssd-devel@lists.fedorahosted.org
15 comments
3 participants
participants (3)
-
Nikolai Kondrashov -
Simo Sorce -
Sumit Bose