On Fri, Jan 27, 2012 at 02:43:55PM +0100, Jakub Hrozek wrote:
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:
> > >>>https://fedorahosted.org/sssd/ticket/1111
> > >>>
> > >>>Requires cn=defaults patches.
> > >>>
> > >>>Please note, that the new sudo responder option (cache_timeout)
> > >>>be added to SSSDConfig.py as a part of #1144.
> > >>
> > >>Nack,
> > >>
> > >>Please name the new option "sudo_cache_timeout" to avoid
> > >>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
> > >>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
> > >>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
> > >>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.
> 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.
I'm sorry, I forgot the in-memory cache patch has already been sent for
review and reposted it on this list again: