Re: [patch] plproxy v2

On 7/22/08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
> > On 7/21/08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I looked through this a bit, and my principal reaction was "what are
> >> the security implications?
> > There are 2 aspects to it:
> > 1. Function can be created only by superuser.
>
> What I'm concerned about is who they can be *called* by. I'd be happier
> if the default behavior was that there was no public execute privilege
> for plproxy functions.
>
> I think right now that could be enforced by having plproxy's validator
> procedure replace any null proacl entry with something that explicitly
> refuses public execute. That's a bit of a hack though. Maybe it'd be
> worth inventing per-PL default ACLs, instead of having a
> one-size-fits-all policy?

Note1 that if user (admin) wants he can also do user filtering/mapping
in plproxy.* functions.

Note2 - instead of restricting privileges on actual functions, we could
instead restrict privilege on the 2 functions under 'plproxy' schema,
or directly on schema. Seems simpler. Eg. create simple default
installation with REVOKE ALL FROM PUBLIC.

> > 2. If cluster connection strings do not have 'user=' key,
> > ' user=' || current_username() is appended to it.
>
>
> Cool, I missed that. At minimum the documentation has to explain this
> point and emphasize the security implications.

Ok.

> Is it a good idea
> to allow user= in the cluster strings at all?

I think so - if the plproxy database itself already is main point of
authentication, both can-connect and can-execute sense, then it's
good to avoid complicating setup and send queries away under single
user that has minimal rights on partition dbs and can only execute
requested functions.

> > Also, plroxy does
> > _nothing_ with passwords. That means the password for remote
> > connection must be in postgres user's .pgpass,
>
>
> That seems *exactly* backwards, because putting the password in postgres
> user's .pgpass is as good as disabling password auth altogether.
> Consider that it would also hand all the keys to the kingdom over to
> someone who had access to dblink on the same machine (not even the same
> cluster, so long as it was run by the same postgres user!).

Good point. Some ideas for password handling:

1. Require that user always provider both username and password in
plproxy.get_cluster_partitions(). We could add separate function
for that or add fields to plproxy.get_partitions(), although
this is not necessary - user can add them simply to connect string.

Main problems with this is that maybe you don't want to show the
passwords to anyone who can execute plproxy.* functions?

2. Let PL/Proxy fetch user password hash from pg_shadow, add API to libpq
to use the hash on authentication instead plaintext password.
This ofcourse expects that remote server uses same auth method as
current one.

Despite appearance it does not have security problems - the hashes
are already equivalent to plaintext password.

> > But I don't think plproxy can and should protect dumb admins who
> > create remote_exec(sql) function and allow untrusted users to
> > execute it.
>
> We regularly get beat up about any aspect of our security apparatus
> that isn't "secure by default". This definitely isn't, and from
> a PR point of view (if nothing else) that doesn't seem a good idea.
>
> I repeat that I don't feel comfortable in the least with plproxy's
> security model.

Btw, I'm very thankful for your review. I would really like improve the
security of plproxy whatever the merge decision will be, so hopefully
we can discuss it further.

To make discussion easier, here are list of possible problems/fixes
discussed thus far (as I see):

Problems:

- restrict users who can access remote dbs by default.
- avoid spreading passwords too far.
- .pgpass gives them to any libpq client
- inside connect string they are visible to calling user
(although only his own?)

Documentation/default setup fixes:

1. Restrict access to 'plproxy' schema or functions under that schema.
Only users that have grants can use plproxy functions.

2. Create default setup that does user filtering/mapping by default.
Have the permissions on the functions and tables carefully tuned
to allow minimal access.

3. Make the default setup also handle passwords from tables.
So instead user adding password handling insecurely, he can use it
or remove it from already secure setup.

Code fixes:

4. Create plproxy functions without execute permissions by default.
(Seems unnecessary as 1, 2 already give that?)

5. Let plproxy use user password hash directly from pg_shadow.
(Unless user= or password= is given on connection string?)

Seems like restricting access is easy, but only 5) gives secure
password handling.

--
marko