Christoph Berg wrote:
> Re: To Tom Lane 2016-02-19 <20160219115334.GB26862@msg.df7cb.de>
> > Updated patch attached.
>
> *Blush* I though I had compile-tested the patch, but without
> --enable-openssl it wasn't built :(.
>
> The attached patch has successfully been included in the 9.6 Debian
> package, passed the regression tests there, and I've also done some
> chmod/chown tests on the filesystem to verify it indeed catches the
> cases laid out by Tom.
This looks like a pretty reasonable change to me.
For the sake of cleanliness, I propose splitting out the checks for
regular file and for owned-by-root-or-us from the actual chmod-level
checks at the same time. That way we can provide more specific error
messages for each case. (Furthermore, I'm pretty sure that the check
for superuserness could be applied on Windows also -- in the attached
patch it's still #ifdef'ed out, because I don't know how to write it.)
After doing that change I started to look at the details of the check
and found some mistakes:
* it said "g=w" instead of "g=r", in contradiction with the numeric
specification: g=w means 020 rather than 040. We want the file to be
group-readable, not group-writeable.
* it failed to check for S_IXUSR, so permissions 0700 were okay, in
contradiction with what the error message indicates. This is a
preexisting bug actually. Do we want to fix it by preventing a
user-executable file (possibly breaking compability with existing
executable key files), or do we want to document what the restriction
really is?
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services