Re: nsswitch reviewer wanted

Hi Michael,

On Thu, Nov 03, 2005 at 04:11:15PM +0300, Michael Bushkov wrote:
> 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.

Glad to be of some help.  Don't take anything I say as any kind of
authority though.

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

Thanks.

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

As I mentioned, I'm no guru, but that seems to be conventional.

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

I hadn't realised that.  I'd be interested to see that patch if you
still have a copy, as it would answer the question of how much work
doing such a thing would be.  So far as I'm concerned there is no issue
leaving that in there, I just wanted to make sure that it hadn't slipped
in by mistake.

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

OK, that's an good enough explanation for me.  Thanks.

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

Superb!

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

I thought that was the intended meaning.  That's great.

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

Cool.

Thanks again for working on this.

Ceri
-- 
Only two things are infinite, the universe and human stupidity, and I'm
not sure about the former.			  -- Einstein (attrib.)