Re: nsswitch reviewer wanted

Hello, Ceri!

Ceri Davies wrote:

>I realise that you weren't asking for comments, but I took a quick look
>at http://www.rsu.ru/~bushman/nsswitch_cached/nss_cached.patch and have
>some.  I'll send this to the original lists too.
>  
>
Thanks for comments! I need them much.

>o There aren't nearly enough comments.  Granted, I'm not a C aficionado,
>  but it's ~ line 3200 of that patch before we come to a new non-trivial
>  comment, and there aren't many after that.
>  
>
Ok - I'll comment the code.

>o cached/config.c has magic numbers in create_def_configuration_entry(),
>  which probably belong as #defines in config.h instead.  I'm not sure
>  what style(9) says about that though, so am happy to be ignored.
>  
>
Yes, that's right - I guess, the most correct way is to move them to 
config.h

>o There is a single mention of a ssh_hostkeys cache in
>  include/nsswitch.h, and no code to implement it.
>  
>
I've implemented the patch for OpenSSH, which allows it to use nsswitch 
for the hostkeys. It should be committed by the OpenSSH team. That's why 
the ssh_hostkeys cache is mentioned in the nsswitch.h. This line can 
easialy be removed - as it doesn't affect anything.

>o On line 15448, there is a whitespace nit.  Also, in this area, are we
>  sure that there is no benefit in continuing to key by euid/egid if
>  perform_actual_lookup is enabled; can we send the euid/egid with the
>  lookup request?
>  
>
You are talking about passing euid/egid to the underlying nsswitch 
modules, right? This will require significant changes in these modules, 
and, as far as I'm converned, won't gain us any benefits.

I can't see any benefit of keying by euid/egud when the 
'perform_actual_lookups' mode is enabled. By ignoring them, we make the 
cache common for all users, and no user can poison it - because all 
requests are made solely by ourselves. If we won't ignore the euid/egid, 
then for every user, we'll have to make similar requests, this will also 
affect the cache size.
When perform_actual_lookups mode is off, cache should be certainly 
separated by eud/egid for (basically) security reasons.

>o A user should be able to invalidate one of their caches (e.g.,
>  "cached -i hosts" to flush their hosts cache).  Root should be able
>  to do it for all users with a single command (e.g., "cached -I hosts"
>  to flush all hosts caches).
>
>  
>
Ok - I'll do that.

>o The manual for cached.conf is unclear over whether it's OK to name
>  an "unknown" cache in cached.conf.
>
>  
>
I'll corect this. In fact, it's ok to do that.

>o The location of cached.conf is defaulted to /usr/local/etc in
>  cached/cached.c and should be changed on commit.
>
>  
>
Will be corrected.

>Ceri
>  
>
--
Michael