Re: [PATCH] Authentication support for pserver

["Martin Langhoff" <martin.langhoff@gmail.com>]

On Dec 15, 2007 10:08 AM, Ævar Arnfjörð Bjarmason <avar@cpan.org> wrote:
> > Looks good.  I'll queue only so that I won't lose it and wait for Acks
> > from Mart[iy]ns.  Please sign off your patch.

I have been mulling a bit about this change. Seems correct from a code
flow POV. But anon pserver support was added with a lot of
trepidation. Authenticated, write-enabled pserver support fills me
with dread ;-)

A few things that come to mind

 - git/config is very likely to be readable if the site is served via
other means, like dumb http protocol, or git+ssh. So even if the
password scrambling is mickey-mouse. it might make sense to force the
password data to live elsewhere.

 - umasks/file permissions will probably be handled by the underlying
git tools and core.sharedrepository, since we no longer update refs
"manually". So after a bit of thinking, this should not be an issue.

 - I still worry about running cvsserver with an acct that has write
privileges. With anon, all we need write access is the sqlite db, and
that's not even based on user input. And it can be relocated elsewhere
via git/config.

So, wondering about input validation and related matters re-read most
of cvsserver. The only suspicious bit I find is a caller to
transmitfile @ lines 1117, where the 2nd parameter doesn't come from
tempfile() - if a nasty filename sneaked in, we could (perhaps) be in
for a surprise. We do that for diff header prettyness - we could use
--label / -L instead. Will try and prep a patch for that tomorrow if
noone beats me to it (bedtime in nz!).

Hmmm. Does "worried ACK" count as an ACK? Can we add big fat warnings
along the lines of "run this in a locked down environment, pretty
please"?

cheers,



m