Resending, hopefully the mailman issues are gone now..
On Fri, Jan 27, 2012 at 11:39:17AM +0100, Jakub Hrozek wrote:
On Thu, Jan 26, 2012 at 04:35:04PM +0100, Pavel Březina wrote:
> 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:
> >>>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.
> >>Please name the new option "sudo_cache_timeout" to avoid
> >>the general cache timeout. Cache timeouts are being separated in 1.8
> >>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
> >>using "rules" and "num_rules" would be nicer.
> >>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
> 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
> >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.
I should have bring this up sooner but I this it is much cleaner that
the entries are deleted with a tevent timer after their TTL is over than
checking if they are valid when reading them from the hash.
The cache logic can then be reduced to looking up an entry in the hash.
Check out pam_initgr_cache_remove() for an example.