dblink vs SQL/MED

Tom Lane wrote:

Peter wrote:

SQL/MED catalog manipulation facilities

This doesn't do any remote or external things yet, but it gives modules
like plproxy and dblink a standardized and future-proof system for
managing their connection information.

It seems this is a pile of pretty useless code, so far as the core
distribution is concerned, unless somebody fixes dblink to use it.
Is that on anyone's radar for 8.4?

How much time is left to get it done? I might be able to find some time
on the weekend after Christmas, or the weekend after New Years day -- is
that soon enough?

Joe

Joe Conway <mail@joeconway.com> writes:

Tom Lane wrote:

It seems this is a pile of pretty useless code, so far as the core
distribution is concerned, unless somebody fixes dblink to use it.
Is that on anyone's radar for 8.4?

How much time is left to get it done? I might be able to find some time
on the weekend after Christmas, or the weekend after New Years day -- is
that soon enough?

Well, it seems quite clear to me that we're not entering beta on Jan 1.
And even if we were, we've traditionally cut more slack for contrib
modules than the core database. So if you can do something about it in
the next few weeks, that'd be great.

regards, tom lane

On Saturday 20 December 2008 19:33:17 Tom Lane wrote:

Peter wrote:

SQL/MED catalog manipulation facilities

This doesn't do any remote or external things yet, but it gives modules
like plproxy and dblink a standardized and future-proof system for
managing their connection information.

It seems this is a pile of pretty useless code, so far as the core
distribution is concerned, unless somebody fixes dblink to use it.
Is that on anyone's radar for 8.4?

Martin had sent some code for that with his original patch series. I or
someone can review that next.

Peter Eisentraut wrote:

On Saturday 20 December 2008 19:33:17 Tom Lane wrote:

Peter wrote:

SQL/MED catalog manipulation facilities

This doesn't do any remote or external things yet, but it gives modules
like plproxy and dblink a standardized and future-proof system for
managing their connection information.

It seems this is a pile of pretty useless code, so far as the core
distribution is concerned, unless somebody fixes dblink to use it.
Is that on anyone's radar for 8.4?

Martin had sent some code for that with his original patch series. I or
someone can review that next.

Here is what I would propose (still needs documentation and regression
test changes, but wanted feedback first). I think it is better to keep
the changes to dblink very simple.

After looking for an already existing dblink rconn, the passed connstr
is checked to see if it matches a valid foreign data server prior to
being used as a connstr. If so, a connstr is constructed from the
foreign server and user mapping options (for current user). The
resulting connstr is then treated exactly as if it were one that was
passed directly to dblink.

Two specific questions on this approach:
1. This implies that the exact same dblink_connstr_check() is performed
on a predefined foreign server and user mapping as a raw connstr --
is this desireable? I'm not entirely clear on the intended purpose
and use of foreign data wrappers yet.
2. It seems like get_connect_string() is generically useful to any
client of postgresql_fdw.c -- should it go there instead of dblink?

Thanks,

Joe

Joe Conway wrote:

Peter Eisentraut wrote:

Martin had sent some code for that with his original patch series. I
or someone can review that next.

Here is what I would propose (still needs documentation and regression
test changes, but wanted feedback first). I think it is better to keep
the changes to dblink very simple.

The attached now includes documentation and regression test changes. It
also includes the refactoring to pull dblink_send_query() out of
dblink_record_internal() and the fix for the bug reported by Oleksiy
Shchukin.

After looking for an already existing dblink rconn, the passed connstr
is checked to see if it matches a valid foreign data server prior to
being used as a connstr. If so, a connstr is constructed from the
foreign server and user mapping options (for current user). The
resulting connstr is then treated exactly as if it were one that was
passed directly to dblink.

Two specific questions on this approach:
1. This implies that the exact same dblink_connstr_check() is performed
on a predefined foreign server and user mapping as a raw connstr --
is this desireable? I'm not entirely clear on the intended purpose
and use of foreign data wrappers yet.

On the one hand, why be any less stringent on an fdw server than any
other connect string? But on the other hand, the fdw server definition
has supposedly been vetted by a superuser. Any thoughts of this?

2. It seems like get_connect_string() is generically useful to any
client of postgresql_fdw.c -- should it go there instead of dblink?

I'm pretty sure get_connect_string() should be moved to postgresql_fdw.c
-- objections?

Thanks,

Joe

Joe Conway wrote:

Two specific questions on this approach:
1. This implies that the exact same dblink_connstr_check() is performed
on a predefined foreign server and user mapping as a raw connstr --
is this desireable? I'm not entirely clear on the intended purpose
and use of foreign data wrappers yet.

On the one hand, why be any less stringent on an fdw server than any
other connect string? But on the other hand, the fdw server definition
has supposedly been vetted by a superuser. Any thoughts of this?

I'd vote for allowing aribitrary connect strings -- ordinary users cannot
create servers and user mappings unless explicitly granted the privileges.
It probably should be noted in the documentation that allowing ordinary
users to create user mappings enables the use of postgres .pgpass file.
Not sure where to put this at the moment.

2. It seems like get_connect_string() is generically useful to any
client of postgresql_fdw.c -- should it go there instead of dblink?

I'm pretty sure get_connect_string() should be moved to postgresql_fdw.c
-- objections?

There is some discussion in another thread about this:
http://archives.postgresql.org/pgsql-hackers/2008-12/msg01875.php
http://archives.postgresql.org/pgsql-hackers/2009-01/msg00021.php

The initial approach was to let each foreign data wrapper provide its own
connection string/list builder function. Latest is to provide the lookup
functions in foreign.c, and use the same functions for all the different
fdw's. I was about to implement those but got distracted. Will resume now.

regards,
Martin

(changed the subject to hopefully get a few more eyes looking at this
thread)

Martin Pihlak wrote:

I'd vote for allowing aribitrary connect strings -- ordinary users cannot
create servers and user mappings unless explicitly granted the privileges.
It probably should be noted in the documentation that allowing ordinary
users to create user mappings enables the use of postgres .pgpass file.
Not sure where to put this at the moment.

I'm mainly concerned about re-opening security holes that we spent a lot
of time debating and subsequently closing. I suspect if we assume that
any FDW-derived connect string can bypass the checks we put in place, we
will regret it later. But I'm open to arguments on both sides...

2. It seems like get_connect_string() is generically useful to any
client of postgresql_fdw.c -- should it go there instead of dblink?

I'm pretty sure get_connect_string() should be moved to postgresql_fdw.c
-- objections?

There is some discussion in another thread about this:
http://archives.postgresql.org/pgsql-hackers/2008-12/msg01875.php
http://archives.postgresql.org/pgsql-hackers/2009-01/msg00021.php

The initial approach was to let each foreign data wrapper provide its own
connection string/list builder function. Latest is to provide the lookup
functions in foreign.c, and use the same functions for all the different
fdw's. I was about to implement those but got distracted. Will resume now.

It seems to me that get_connect_string() (or whatever we decide to call
it), is very libpq specific, and therefore belongs with postgresql_fdw.c
rather than foreign.c. But if we can't reach a consensus there is no
harm in leaving it as a dblink.c specific static function either.

Joe

Joe Conway wrote:

I'm mainly concerned about re-opening security holes that we spent a lot
of time debating and subsequently closing. I suspect if we assume that
any FDW-derived connect string can bypass the checks we put in place, we
will regret it later. But I'm open to arguments on both sides...

Can you elaborate what security issues you are concerned about?

It seems to me that get_connect_string() (or whatever we decide to call
it), is very libpq specific, and therefore belongs with postgresql_fdw.c
rather than foreign.c. But if we can't reach a consensus there is no
harm in leaving it as a dblink.c specific static function either.

postgresql_fdw.c is a module with a defined API. I don't see what you
would achieve by putting an ad hoc function in there.

Joe Conway wrote:

I'm mainly concerned about re-opening security holes that we spent a lot
of time debating and subsequently closing. I suspect if we assume that
any FDW-derived connect string can bypass the checks we put in place, we
will regret it later. But I'm open to arguments on both sides...

In order to create a foreign server, the user needs USAGE on the foreign
data wrapper. Creating user mappings requires the user to be the owner of
the server. Both need explicit grants or alters by the superuser. This is
a bit more relaxed than the current superuser check, but still only trusted
users can define arbitrary connections.

Also, allowing passwordless user mappings adds some flexibility for defining
connections - storing passwords in .pgpass, pgservice or not using a password
at all (pg_hba trust etc.).

regards,
Martin

Peter Eisentraut wrote:

Joe Conway wrote:

I'm mainly concerned about re-opening security holes that we spent a
lot of time debating and subsequently closing. I suspect if we assume
that any FDW-derived connect string can bypass the checks we put in
place, we will regret it later. But I'm open to arguments on both
sides...

Can you elaborate what security issues you are concerned about?

For example: on a freshly installed postgres installation, that does
*not* require authentication, you would get the following behavior which
previously was found to be unacceptable:

8<--------------------------------------------------------------
--
-- as the superuser
--
CREATE FOREIGN DATA WRAPPER postgresql LIBRARY 'postgresql_fdw'
LANGUAGE C;
CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql
OPTIONS (dbname 'contrib_regression');
CREATE USER dblink_regression_test;
CREATE USER MAPPING FOR dblink_regression_test SERVER fdtest;
\c - dblink_regression_test
psql (8.4devel)
You are now connected to database "contrib_regression" as user
"dblink_regression_test".
--
-- now as untrusted user dblink_regression_test
--
contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
dblink_connect
----------------
OK
(1 row)

contrib_regression=> SELECT * FROM dblink('myconn','SELECT
current_user') AS t(u text);
u
----------
postgres
(1 row)
8<--------------------------------------------------------------

Now, we can blame the superuser for explicitly allowing this, but I
don't see that as much different from the previously voiced security
concerns. I'm sure there are other privilege escalation scenarios, but I
haven't tried at all hard to think of them...

Joe

On Tuesday 06 January 2009 05:54:14 Joe Conway wrote:

--
-- now as untrusted user dblink_regression_test
--
contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
  dblink_connect
----------------
  OK
(1 row)

I think you want some permission checking on fdtest then, right?

Peter Eisentraut <peter_e@gmx.net> writes:

I think you want some permission checking on fdtest then, right?

What about the permissions on the system catalogs themselves?
AFAICT, the pg_user_mappings view will expose user passwords to
the "owner" of the foreign server, which doesn't seem good.

regards, tom lane

On Tuesday 06 January 2009 19:50:51 Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I think you want some permission checking on fdtest then, right?

What about the permissions on the system catalogs themselves?
AFAICT, the pg_user_mappings view will expose user passwords to
the "owner" of the foreign server, which doesn't seem good.

Well, no one is forcing you to put a password there. dblink has had its
mechanisms for obtaining passwords until now, and those are not invalidated
by this. There are as always limited use cases for hardcoding passwords, but
in a fully multiuser environment you probably want to use a different
authentication mechanism. Eventually, when we allow these modules to
actually call out, we will have to seriously evaluate that. But for right
now, if you don't want your password in there, don't put it there.

Peter Eisentraut <peter_e@gmx.net> writes:

On Tuesday 06 January 2009 19:50:51 Tom Lane wrote:

What about the permissions on the system catalogs themselves?
AFAICT, the pg_user_mappings view will expose user passwords to
the "owner" of the foreign server, which doesn't seem good.

Well, no one is forcing you to put a password there. dblink has had its
mechanisms for obtaining passwords until now, and those are not invalidated
by this. There are as always limited use cases for hardcoding passwords, but
in a fully multiuser environment you probably want to use a different
authentication mechanism. Eventually, when we allow these modules to
actually call out, we will have to seriously evaluate that. But for right
now, if you don't want your password in there, don't put it there.

Huh? The advertised reason for putting in all this stuff was to provide
a thought-through, secure mechanism for dealing with connection
information. If we haven't done that thinking yet, I'm of the opinion
the whole thing should be ripped out until we have. It's of exactly
zero value if it cannot be trusted with a password.

regards, tom lane

Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I think you want some permission checking on fdtest then, right?

What about the permissions on the system catalogs themselves?
AFAICT, the pg_user_mappings view will expose user passwords to
the "owner" of the foreign server, which doesn't seem good.

Usually it would have been the server owner who created those user
mappings in the first place -- so the passwords are already known
to him/her. Of course it is possible to create the mappings first
and later change the ownership of the server, thus exposing the
passwords to a new role. But IMHO, it would be reasonable to assume
that the owner of the server has full control over its user mappings.

regards,
Martin

Peter Eisentraut wrote:

On Tuesday 06 January 2009 05:54:14 Joe Conway wrote:

contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
dblink_connect
----------------
OK
(1 row)

I think you want some permission checking on fdtest then, right?

The proposed "connection lookup" functions have USAGE check on the
server.

About the connstr validation -- it would be best done in the connection
lookup function. IMO it would make sense to validate the connstring if the
foreign server is not OWNED by a superuser. This would enable less trusted
to create and own servers but would force them to provide a username and
password (validate in CreateUserMapping and GetForeignConnectionOptions).
And superuser could still set up a connection that makes use of .pgpass,
pgservice etc. Comments?

regards,
Martin

Peter Eisentraut wrote:

On Tuesday 06 January 2009 05:54:14 Joe Conway wrote:

--
-- now as untrusted user dblink_regression_test
--
contrib_regression=> SELECT dblink_connect('myconn', 'fdtest');
dblink_connect
----------------
OK
(1 row)

I think you want some permission checking on fdtest then, right?

I don't see anything documented under GRANT which controls privileges on
a mapping, and the USAGE on a server only controls what a user can see
by query. I assume that if the superuser creates a mapping from user foo
to server bar, foo can still use bar via the mapping, even if they don't
have USAGE granted on the server. It isn't clear from the docs what is
intended, so I could have that wrong.

But even if foo is granted USAGE on bar, I think you miss the point. If you:

1. grant a non-superuser (foo) access to a server (bar)
2. create a mapping for foo to bar which includes no password
3. configure bar to not require authentication (trust)

you will get the privilege escalation as shown (e.g. foo becomes
postgres on bar).

Joe

Martin Pihlak <martin.pihlak@gmail.com> writes:

Usually it would have been the server owner who created those user
mappings in the first place -- so the passwords are already known
to him/her. Of course it is possible to create the mappings first
and later change the ownership of the server, thus exposing the
passwords to a new role. But IMHO, it would be reasonable to assume
that the owner of the server has full control over its user mappings.

So the DBA should know his users' passwords for remote sites? That's
not normally considered good security practice.

If the passwords were encrypted strings it might be acceptable, but
without some libpq changes I think they'd have to be cleartext :-(

regards, tom lane

Martin Pihlak wrote:

Usually it would have been the server owner who created those user
mappings in the first place -- so the passwords are already known
to him/her. Of course it is possible to create the mappings first
and later change the ownership of the server, thus exposing the
passwords to a new role. But IMHO, it would be reasonable to assume
that the owner of the server has full control over its user mappings.

Maybe we should rethink that. In the SQL standard, it says that USAGE
on the server is required to create a user mapping. I think we could
let users create mappings for their own user name if they have USAGE.
And change the system views so a user can only see his own user mapping
options.

More generally, using passwords in this way is always going to be a
mediocre security solution, since the plaintext password will leak in
all kinds of directions: system catalogs readable by superuser, server
log readable by adm group members (depends on OS), psql_history, etc. I
imagine that a proper authentication solution would involve credentials
forwarding with either Kerberos or SSL or the like.