Dne 25.1.2012 20:50, Jakub Hrozek napsal(a):
On Wed, Jan 25, 2012 at 08:44:40PM +0100, Jakub Hrozek wrote:
> On Tue, Jan 24, 2012 at 03:10:32PM +0100, Pavel Březina wrote:
>>
https://fedorahosted.org/sssd/ticket/1111
>>
>> Requires cn=defaults patches.
>>
>> Please note, that the new sudo responder option (cache_timeout) will
>> be added to SSSDConfig.py as a part of #1144.
>
> Nack,
>
> Please name the new option "sudo_cache_timeout" to avoid name-clash with
> the general cache timeout. Cache timeouts are being separated in 1.8
> anyway.
Done.
> Nitpick: sudosrc_cache.c has the diacritics in your surname
mangled.
I don't know how that happened, I hope that it is alright this time.
> I would prefer to have a different prefix than res_ in struct
> sudo_cache_entry. I realize that res is a common name used for
> sysdb_attrs but that's largely used as a shorthand for "result".
Simply
> using "rules" and "num_rules" would be nicer.
Done.
> The way FQDN-only domains are skipped is different from the
> "cn=defaults" patch (and I prefer that approach). In this patch,
> only the first domain is checked for being FQDN-only,
I'm sorry, I don't follow. Could you provide me some more information?
However, I've corrected a bug in sudosrv_cache_lookup() where I was
passing dctx->domain instead of domain as a parameter. It is fixed in
this patch.
I think it would
> be better to move the check into the loop, or (and that probably
better)
> move the cache into sudosrv_get_rules().
Are you getting at this situation?
- We have domains A and B
- User x@B
- x@B calls 'sudo cmd' which stores rules for this user into sysdb and
into in-memory cache
- we create user x@A
- x@A uses sudo but it would use the rules for x@B until the in-memory
cache is expired
Your solution would certainly work for this situation but there wouldn't
be the need of in-memory cache anymore, we can use just sysdb.
The purpose of having in-memory cache is for the sudo request to be as
fast as possible. And I would like to point out that it is still just a
cache - and cache doesn't have to be necessary current.
> Also please remove the FIXME from sudosrv_get_rules().
Thank you. I've also removed the other cache related FIXME, because I
believe it is sorted out this way. Please, correct me if I'm wrong.
One more thing - the new option needs to be documented in man pages
and
settable by the configAPI.
Unfortunately it is not that simple. This affects the config tests as it
requires support of a new responder in the tests. It will be done in #1144.
Thank you for the review.