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) 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.
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.