postgres_fdw super user checks

Started by Jeff Janesabout 9 years ago37 messages
#1Jeff Janes
jeff.janes@gmail.com
1 attachment(s)

postgres_fdw has some checks to enforce that non-superusers must connect to
the foreign server with a password-based method. The reason for this is to
prevent the authentication to the foreign server from happening on the
basis of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid of the
USER MAPPING being used for the connection, not the userid of currently
logged on user.

That is, I think the last line in this script should succeed: ('jjanes' is
both a superuser, and a database):

CREATE EXTENSION IF NOT EXISTS postgres_fdw WITH SCHEMA public;
CREATE SERVER foo FOREIGN DATA WRAPPER postgres_fdw;
CREATE USER MAPPING FOR jjanes SERVER foo;
CREATE TABLE foobar1 ( x integer);
CREATE FOREIGN TABLE foobar2 ( x integer) SERVER foo OPTIONS ( table_name
'foobar1');
CREATE VIEW foobar3 AS SELECT foobar2.x FROM foobar2;
CREATE USER test;
GRANT SELECT ON TABLE foobar3 TO test;
\c jjanes test
select * from foobar3;

It connects back to itself, simply for demonstration purposes.

The attached patch implements this change in auth checking.

Cheers,

Jeff

Attachments:

postgres_fdw_superuser.patchapplication/octet-stream; name=postgres_fdw_superuser.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
new file mode 100644
index bcdddc2..11ee50f
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
*************** static bool xact_got_connection = false;
*** 65,71 ****
  
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
! static void check_conn_params(const char **keywords, const char **values);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
  static void begin_remote_xact(ConnCacheEntry *entry);
--- 65,71 ----
  
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
! static void check_conn_params(const char **keywords, const char **values, UserMapping *user);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
  static void begin_remote_xact(ConnCacheEntry *entry);
*************** connect_pg_server(ForeignServer *server,
*** 222,228 ****
  		keywords[n] = values[n] = NULL;
  
  		/* verify connection parameters and make connection */
! 		check_conn_params(keywords, values);
  
  		conn = PQconnectdbParams(keywords, values, false);
  		if (!conn || PQstatus(conn) != CONNECTION_OK)
--- 222,228 ----
  		keywords[n] = values[n] = NULL;
  
  		/* verify connection parameters and make connection */
! 		check_conn_params(keywords, values, user);
  
  		conn = PQconnectdbParams(keywords, values, false);
  		if (!conn || PQstatus(conn) != CONNECTION_OK)
*************** connect_pg_server(ForeignServer *server,
*** 247,253 ****
  		 * otherwise, he's piggybacking on the postgres server's user
  		 * identity. See also dblink_security_check() in contrib/dblink.
  		 */
! 		if (!superuser() && !PQconnectionUsedPassword(conn))
  			ereport(ERROR,
  				  (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  				   errmsg("password is required"),
--- 247,253 ----
  		 * otherwise, he's piggybacking on the postgres server's user
  		 * identity. See also dblink_security_check() in contrib/dblink.
  		 */
! 		if (!superuser_arg(user->userid) && !PQconnectionUsedPassword(conn))
  			ereport(ERROR,
  				  (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  				   errmsg("password is required"),
*************** connect_pg_server(ForeignServer *server,
*** 280,286 ****
   * contrib/dblink.)
   */
  static void
! check_conn_params(const char **keywords, const char **values)
  {
  	int			i;
  
--- 280,286 ----
   * contrib/dblink.)
   */
  static void
! check_conn_params(const char **keywords, const char **values, UserMapping *user)
  {
  	int			i;
  
*************** check_conn_params(const char **keywords,
*** 288,293 ****
--- 288,296 ----
  	if (superuser())
  		return;
  
+ 	if (superuser_arg(user->userid))
+ 		return;
+ 
  	/* ok if params contain a non-empty password */
  	for (i = 0; keywords[i] != NULL; i++)
  	{
#2Michael Paquier
michael.paquier@gmail.com
In reply to: Jeff Janes (#1)
Re: postgres_fdw super user checks

On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

postgres_fdw has some checks to enforce that non-superusers must connect to
the foreign server with a password-based method. The reason for this is to
prevent the authentication to the foreign server from happening on the basis
of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid of the
USER MAPPING being used for the connection, not the userid of currently
logged on user.

So, if the user mapping user is a superuser locally, this would allow
any lambda user of the local server to attempt a connection to the
remote server. It looks dangerous rather dangerous to me to authorize
that, even if the current behavior is a bit inconsistent I agree.

Your patch breaks the join pushdown logic when multiple user IDs are
involved. Per se make check.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Jeff Janes (#1)
Re: postgres_fdw super user checks

On Mon, Oct 17, 2016 at 12:03 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

postgres_fdw has some checks to enforce that non-superusers must connect to
the foreign server with a password-based method. The reason for this is to
prevent the authentication to the foreign server from happening on the basis
of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid of the
USER MAPPING being used for the connection, not the userid of currently
logged on user.

That is, I think the last line in this script should succeed: ('jjanes' is
both a superuser, and a database):

CREATE EXTENSION IF NOT EXISTS postgres_fdw WITH SCHEMA public;
CREATE SERVER foo FOREIGN DATA WRAPPER postgres_fdw;
CREATE USER MAPPING FOR jjanes SERVER foo;
CREATE TABLE foobar1 ( x integer);
CREATE FOREIGN TABLE foobar2 ( x integer) SERVER foo OPTIONS ( table_name
'foobar1');
CREATE VIEW foobar3 AS SELECT foobar2.x FROM foobar2;
CREATE USER test;
GRANT SELECT ON TABLE foobar3 TO test;
\c jjanes test
select * from foobar3;

It connects back to itself, simply for demonstration purposes.

The attached patch implements this change in auth checking.

I agree with your analysis, that any passwordless foreign server
access with super user's user mapping should be allowed. If it's safe
to access a foreign server without password for a superuser, then it
should be safe to do so when corresponding user mapping is used even
when login user is non-superuser.

But there's one problem with the patch.

login as some useruser and run following commands.

create extension postgres_fdw;
create server foo foreign data wrapper postgres_fdw options (dbname 'postgres');
create user test;
grant USAGE ON FOREIGN server foo to test;
set role test;
create user mapping for test server foo;
create foreign table fpg_class (oid oid) server foo options
(table_name 'pg_class', schema_name 'pg_catalog');
create view fview as select * from fpg_class;
set role <some superuser>;
select * from fview limit 0;

With your patch it gives error
ERROR: password is required
DETAIL: Non-superuser cannot connect if the server does not request a password.
HINT: Target server's authentication method must be changed.

Without the patch it does not give any error.

Is that intentional?

I guess, this is because of asymmetry in check_conn_params() and
connect_pg_server(). The first one does not check any params if the
logged in user is a superuser but the later checks if only the user in
the mapping is superuser.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Michael Paquier (#2)
Re: postgres_fdw super user checks

On Mon, Oct 17, 2016 at 11:48 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

postgres_fdw has some checks to enforce that non-superusers must connect to
the foreign server with a password-based method. The reason for this is to
prevent the authentication to the foreign server from happening on the basis
of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid of the
USER MAPPING being used for the connection, not the userid of currently
logged on user.

So, if the user mapping user is a superuser locally, this would allow
any lambda user of the local server to attempt a connection to the
remote server. It looks dangerous rather dangerous to me to authorize
that, even if the current behavior is a bit inconsistent I agree.

A lambda user can use a user mapping same as a superuser if a. that
user mapping is public and/or b. it uses a view owned by super user
(RangeTblEntry::checkuser). When a is true but not b, the the user in
UserMapping is set to lambda and not superuser, so this patch is
correct here. If b is true, and lambda is able to access the view, the
superuser has granted it permissions to do so and thus intends to let
lambda use a super user user mapping. Since we trust super users to do
the right thing, I don't see why it's unsafe. Any other objects
accesses by lambda, will use a different user mapping based on the
object being accessed.

Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#2)
Re: postgres_fdw super user checks

On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

postgres_fdw has some checks to enforce that non-superusers must connect to
the foreign server with a password-based method. The reason for this is to
prevent the authentication to the foreign server from happening on the basis
of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid of the
USER MAPPING being used for the connection, not the userid of currently
logged on user.

So, if the user mapping user is a superuser locally, this would allow
any lambda user of the local server to attempt a connection to the
remote server. It looks dangerous rather dangerous to me to authorize
that, even if the current behavior is a bit inconsistent I agree.

I don't know what "any lambda user" means. Did you mean to write "any
random user"?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#5)
Re: postgres_fdw super user checks

On Mon, Oct 17, 2016 at 10:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

postgres_fdw has some checks to enforce that non-superusers must connect to
the foreign server with a password-based method. The reason for this is to
prevent the authentication to the foreign server from happening on the basis
of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid of the
USER MAPPING being used for the connection, not the userid of currently
logged on user.

So, if the user mapping user is a superuser locally, this would allow
any lambda user of the local server to attempt a connection to the
remote server. It looks dangerous rather dangerous to me to authorize
that, even if the current behavior is a bit inconsistent I agree.

I don't know what "any lambda user" means. Did you mean to write "any
random user"?

Yes, in this context that would be "any non-superuser" or "any user
without superuser rights". Actually that's a French-ism. I just
translated it naturally to English to define a user that has no access
to advanced features :)
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Haribabu Kommi
kommi.haribabu@gmail.com
In reply to: Michael Paquier (#6)
Re: postgres_fdw super user checks

On Tue, Oct 18, 2016 at 10:38 AM, Michael Paquier <michael.paquier@gmail.com

wrote:

On Mon, Oct 17, 2016 at 10:51 PM, Robert Haas <robertmhaas@gmail.com>
wrote:

On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes <jeff.janes@gmail.com>

wrote:

postgres_fdw has some checks to enforce that non-superusers must

connect to

the foreign server with a password-based method. The reason for this

is to

prevent the authentication to the foreign server from happening on the

basis

of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid

of the

USER MAPPING being used for the connection, not the userid of currently
logged on user.

So, if the user mapping user is a superuser locally, this would allow
any lambda user of the local server to attempt a connection to the
remote server. It looks dangerous rather dangerous to me to authorize
that, even if the current behavior is a bit inconsistent I agree.

I don't know what "any lambda user" means. Did you mean to write "any
random user"?

Yes, in this context that would be "any non-superuser" or "any user
without superuser rights". Actually that's a French-ism. I just
translated it naturally to English to define a user that has no access
to advanced features :)

Thanks for the patch, but it breaking the existing functionality as per the
other
mails. Marked as "returned with feedback" in 2016-11 commitfest.

Regards,
Hari Babu
Fujitsu Australia

#8Jeff Janes
jeff.janes@gmail.com
In reply to: Haribabu Kommi (#7)
1 attachment(s)
Re: postgres_fdw super user checks

On Thu, Dec 1, 2016 at 7:11 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Tue, Oct 18, 2016 at 10:38 AM, Michael Paquier <
michael.paquier@gmail.com> wrote:

On Mon, Oct 17, 2016 at 10:51 PM, Robert Haas <robertmhaas@gmail.com>
wrote:

On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes <jeff.janes@gmail.com>

wrote:

postgres_fdw has some checks to enforce that non-superusers must

connect to

the foreign server with a password-based method. The reason for this

is to

prevent the authentication to the foreign server from happening on

the basis

of the OS user who is running the non-foreign server.

But I think these super user checks should be run against the userid

of the

USER MAPPING being used for the connection, not the userid of

currently

logged on user.

So, if the user mapping user is a superuser locally, this would allow
any lambda user of the local server to attempt a connection to the
remote server. It looks dangerous rather dangerous to me to authorize
that, even if the current behavior is a bit inconsistent I agree.

I don't know what "any lambda user" means. Did you mean to write "any
random user"?

Yes, in this context that would be "any non-superuser" or "any user
without superuser rights". Actually that's a French-ism. I just
translated it naturally to English to define a user that has no access
to advanced features :)

Thanks for the patch, but it breaking the existing functionality as per
the other
mails. Marked as "returned with feedback" in 2016-11 commitfest.

Here is an updated patch. This version allows you use the password-less
connection if you either are the super-user directly (which is the existing
committed behavior), or if you are using the super-user's mapping because
you are querying a super-user-owned view which you have been granted access
to.

It first I thought the currently committed behavior might be a security bug
as a directly logged in superuser can use another user's user-defined
mapping but without the password restriction when querying a view made by
someone else. But consulting with the security list nearly a year ago, the
conclusion was that it is never a good idea for a superuser to blindly
query from other users' views, and that the current situation is no worse
for postgres_fdw than it is for other features, and so nothing needs to be
done about it. So that is why I've decided to allow the passwordless
solution in either situation--a superuser using someone else mapping, or
someone else using a super user's mapping.

I didn't update any comments because the existing ones seem to apply
equally well to the new code as the old code.

The regression test passes with this version because I still allow the old
behavior. I didn't add a new test to also test the new behavior, because I
don't know how to do that with the existing make check framework, and a TAP
test seems like overkill.

Cheers,

Jeff

Attachments:

postgres_fdw_superuser_v2.patchapplication/octet-stream; name=postgres_fdw_superuser_v2.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
new file mode 100644
index be4ec07..34d64a5
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
*************** static bool xact_got_connection = false;
*** 75,81 ****
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
  static void disconnect_pg_server(ConnCacheEntry *entry);
! static void check_conn_params(const char **keywords, const char **values);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
  static void begin_remote_xact(ConnCacheEntry *entry);
--- 75,81 ----
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
  static void disconnect_pg_server(ConnCacheEntry *entry);
! static void check_conn_params(const char **keywords, const char **values, UserMapping *user);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
  static void begin_remote_xact(ConnCacheEntry *entry);
*************** connect_pg_server(ForeignServer *server,
*** 261,267 ****
  		keywords[n] = values[n] = NULL;
  
  		/* verify connection parameters and make connection */
! 		check_conn_params(keywords, values);
  
  		conn = PQconnectdbParams(keywords, values, false);
  		if (!conn || PQstatus(conn) != CONNECTION_OK)
--- 261,267 ----
  		keywords[n] = values[n] = NULL;
  
  		/* verify connection parameters and make connection */
! 		check_conn_params(keywords, values, user);
  
  		conn = PQconnectdbParams(keywords, values, false);
  		if (!conn || PQstatus(conn) != CONNECTION_OK)
*************** connect_pg_server(ForeignServer *server,
*** 276,282 ****
  		 * otherwise, he's piggybacking on the postgres server's user
  		 * identity. See also dblink_security_check() in contrib/dblink.
  		 */
! 		if (!superuser() && !PQconnectionUsedPassword(conn))
  			ereport(ERROR,
  					(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  					 errmsg("password is required"),
--- 276,282 ----
  		 * otherwise, he's piggybacking on the postgres server's user
  		 * identity. See also dblink_security_check() in contrib/dblink.
  		 */
! 		if (!superuser() && !superuser_arg(user->userid) && !PQconnectionUsedPassword(conn))
  			ereport(ERROR,
  					(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  					 errmsg("password is required"),
*************** disconnect_pg_server(ConnCacheEntry *ent
*** 322,328 ****
   * contrib/dblink.)
   */
  static void
! check_conn_params(const char **keywords, const char **values)
  {
  	int			i;
  
--- 322,328 ----
   * contrib/dblink.)
   */
  static void
! check_conn_params(const char **keywords, const char **values, UserMapping *user)
  {
  	int			i;
  
*************** check_conn_params(const char **keywords,
*** 330,335 ****
--- 330,338 ----
  	if (superuser())
  		return;
  
+ 	if (superuser_arg(user->userid))
+ 		return;
+ 
  	/* ok if params contain a non-empty password */
  	for (i = 0; keywords[i] != NULL; i++)
  	{
#9Andreas Karlsson
andreas@proxel.se
In reply to: Jeff Janes (#8)
Re: postgres_fdw super user checks

On 07/27/2017 09:45 PM, Jeff Janes wrote:> Here is an updated patch. 
This version allows you use the password-less

connection if you either are the super-user directly (which is the
existing committed behavior), or if you are using the super-user's
mapping because you are querying a super-user-owned view which you have
been granted access to.

I have tested the patch and it passes the tests and works, and the code
looks good (I have a small nitpick below).

The feature seems useful, especially for people who already use views
for security, so the question is if this is a potential footgun. I am
leaning towards no since the superuser should be careful when grant
access to is views anyway.

It would have been nice if there was a more generic way to handle this
since 1) the security issue is not unique to postgres_fdw and 2) this
requires you to create a view. But since the patch is simple, an
improvement in itself and does not prevent any future further
improvements in this era I see no reason to let perfect be the enemy of
good.

= Nitpicking/style

I would prefer if

/* no check required if superuser */
if (superuser())
return;

if (superuser_arg(user->userid))
return;

was, for consistency with the if clause in connect_pg_server(), written as

/* no check required if superuser */
if (superuser() || superuser_arg(user->userid))
return;

Andreas

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Jeff Janes
jeff.janes@gmail.com
In reply to: Andreas Karlsson (#9)
1 attachment(s)
Re: postgres_fdw super user checks

On Tue, Sep 12, 2017 at 1:13 AM, Andreas Karlsson <andreas@proxel.se> wrote:

On 07/27/2017 09:45 PM, Jeff Janes wrote:> Here is an updated patch. This
version allows you use the password-less

connection if you either are the super-user directly (which is the
existing committed behavior), or if you are using the super-user's mapping
because you are querying a super-user-owned view which you have been
granted access to.

I have tested the patch and it passes the tests and works, and the code
looks good (I have a small nitpick below).

The feature seems useful, especially for people who already use views for
security, so the question is if this is a potential footgun. I am leaning
towards no since the superuser should be careful when grant access to is
views anyway.

It would have been nice if there was a more generic way to handle this
since 1) the security issue is not unique to postgres_fdw and 2) this
requires you to create a view. But since the patch is simple, an
improvement in itself and does not prevent any future further improvements
in this era I see no reason to let perfect be the enemy of good.

Thanks for the review.

I think that foreign tables ought to behave as views do, where they run as
the owner rather than the invoker. No one has talked me out of it, but no
one has supported me on it either. But I think it is too late to change
that now. Wrapping it in a view is not hard, but it sure clutters up a
schema. I don't think this can be made too generic, because each database
has a quite different security model, so the solution will be much
different.

Attached is a new patch which fixes the style issue you mentioned.

Cheers,

Jeff

Attachments:

postgres_fdw_superuser_v3.patchapplication/octet-stream; name=postgres_fdw_superuser_v3.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
new file mode 100644
index be4ec07..256d484
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
*************** static bool xact_got_connection = false;
*** 75,81 ****
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
  static void disconnect_pg_server(ConnCacheEntry *entry);
! static void check_conn_params(const char **keywords, const char **values);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
  static void begin_remote_xact(ConnCacheEntry *entry);
--- 75,81 ----
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
  static void disconnect_pg_server(ConnCacheEntry *entry);
! static void check_conn_params(const char **keywords, const char **values, UserMapping *user);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
  static void begin_remote_xact(ConnCacheEntry *entry);
*************** connect_pg_server(ForeignServer *server,
*** 261,267 ****
  		keywords[n] = values[n] = NULL;
  
  		/* verify connection parameters and make connection */
! 		check_conn_params(keywords, values);
  
  		conn = PQconnectdbParams(keywords, values, false);
  		if (!conn || PQstatus(conn) != CONNECTION_OK)
--- 261,267 ----
  		keywords[n] = values[n] = NULL;
  
  		/* verify connection parameters and make connection */
! 		check_conn_params(keywords, values, user);
  
  		conn = PQconnectdbParams(keywords, values, false);
  		if (!conn || PQstatus(conn) != CONNECTION_OK)
*************** connect_pg_server(ForeignServer *server,
*** 276,282 ****
  		 * otherwise, he's piggybacking on the postgres server's user
  		 * identity. See also dblink_security_check() in contrib/dblink.
  		 */
! 		if (!superuser() && !PQconnectionUsedPassword(conn))
  			ereport(ERROR,
  					(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  					 errmsg("password is required"),
--- 276,282 ----
  		 * otherwise, he's piggybacking on the postgres server's user
  		 * identity. See also dblink_security_check() in contrib/dblink.
  		 */
! 		if (!superuser() && !superuser_arg(user->userid) && !PQconnectionUsedPassword(conn))
  			ereport(ERROR,
  					(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
  					 errmsg("password is required"),
*************** disconnect_pg_server(ConnCacheEntry *ent
*** 322,333 ****
   * contrib/dblink.)
   */
  static void
! check_conn_params(const char **keywords, const char **values)
  {
  	int			i;
  
  	/* no check required if superuser */
! 	if (superuser())
  		return;
  
  	/* ok if params contain a non-empty password */
--- 322,333 ----
   * contrib/dblink.)
   */
  static void
! check_conn_params(const char **keywords, const char **values, UserMapping *user)
  {
  	int			i;
  
  	/* no check required if superuser */
! 	if (superuser() || superuser_arg(user->userid))
  		return;
  
  	/* ok if params contain a non-empty password */
#11Robert Haas
robertmhaas@gmail.com
In reply to: Jeff Janes (#10)
Re: postgres_fdw super user checks

On Thu, Sep 14, 2017 at 2:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

I think that foreign tables ought to behave as views do, where they run as
the owner rather than the invoker. No one has talked me out of it, but no
one has supported me on it either. But I think it is too late to change
that now.

That's an interesting point. I think that you can imagine use cases
for either method. Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right. But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster. You want each person's own credentials to carry
over to everything they do remotely.

I feel like the USER MAPPING stuff is a pretty clunky and annoying way
of trying to make this work, no matter which of those use cases you
happen to have. But I'm not exactly sure what would be better,
either, and like you say, it's a bit late to be breaking compatibility
at this point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Andreas Karlsson
andreas@proxel.se
In reply to: Jeff Janes (#10)
Re: postgres_fdw super user checks

On 09/14/2017 08:33 PM, Jeff Janes wrote:> Attached is a new patch which
fixes the style issue you mentioned.

Thanks, the patch looks good no,w and as far as I can tell there was no
need to update the comments or the documentation so I am setting this as
ready for committer.

Andreas

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Jeff Janes
jeff.janes@gmail.com
In reply to: Robert Haas (#11)
Re: postgres_fdw super user checks

On Thu, Sep 14, 2017 at 1:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 14, 2017 at 2:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

I think that foreign tables ought to behave as views do, where they run

as

the owner rather than the invoker. No one has talked me out of it, but

no

one has supported me on it either. But I think it is too late to change
that now.

That's an interesting point. I think that you can imagine use cases
for either method. Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right. But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster. You want each person's own credentials to carry
over to everything they do remotely.

OK. And if you want the first one, you can wrap it in a view currently,
but if it were changed I don't know what you would do if you want the 2nd
one (other than having every user create their own set of foreign tables).
So I guess the current situation is more flexible.

It does seem like it would then be a good idea to have a user mapping
option of "pass_the_hash" which would look up md5 hash from the pg_authid
(if the local username is the same as the remote user name) and use that to
connect to the foreign server, as an alternative option to recording the
password in plain text in the mapping itself. But that would require some
changes to libpq, not just postgres_fdw.

And that wouldn't work for SCRAM. I guess that SCRAM does have some
feature to allow this kind of delegation, but I don't know enough about it
to know how hard it would be to implement in postgres_fdw or how useful it
would be to have.

I feel like the USER MAPPING stuff is a pretty clunky and annoying way
of trying to make this work, no matter which of those use cases you
happen to have. But I'm not exactly sure what would be better,
either, and like you say, it's a bit late to be breaking compatibility
at this point.

Yeah, I have not been finding it enjoyable. How much flexibility does the
SQL/MED spec even give us (I don't have access to the spec)? From what I
could tell, it requires USER MAPPING to exist but doesn't give any details,
and doesn't say there can't be something else one could optionally use
instead.

Cheers,

Jeff

#14Robert Haas
robertmhaas@gmail.com
In reply to: Jeff Janes (#13)
Re: postgres_fdw super user checks

On Wed, Oct 4, 2017 at 6:13 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

OK. And if you want the first one, you can wrap it in a view currently, but
if it were changed I don't know what you would do if you want the 2nd one
(other than having every user create their own set of foreign tables). So I
guess the current situation is more flexible.

So where does that leave this patch? I don't think it makes this
patch a bad idea, although I kind of lean towads the view that the
patch should just be checking superuser_arg(), not superuser() ||
superuser_arg().

It does seem like it would then be a good idea to have a user mapping option
of "pass_the_hash" which would look up md5 hash from the pg_authid (if the
local username is the same as the remote user name) and use that to connect
to the foreign server, as an alternative option to recording the password in
plain text in the mapping itself. But that would require some changes to
libpq, not just postgres_fdw.

And that wouldn't work for SCRAM. I guess that SCRAM does have some feature
to allow this kind of delegation, but I don't know enough about it to know
how hard it would be to implement in postgres_fdw or how useful it would be
to have.

We really need some kind of method for delegating authentication. I
don't know how it should work.

Generally, password authentication is a silly choice for automated
logins because then you've got to store the password someplace from
which it can be digitally stolen; stealing a password from someone's
brain is a different kind of problem. It's not even a good method for
this situation, yet it's the only one we allow. I think that bites,
but I don't really know what to do about it.

Yeah, I have not been finding it enjoyable. How much flexibility does the
SQL/MED spec even give us (I don't have access to the spec)?

Sorry, I don't know.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Jeff Janes
jeff.janes@gmail.com
In reply to: Robert Haas (#14)
Re: postgres_fdw super user checks

On Thu, Oct 5, 2017 at 6:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 4, 2017 at 6:13 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

OK. And if you want the first one, you can wrap it in a view currently,

but

if it were changed I don't know what you would do if you want the 2nd one
(other than having every user create their own set of foreign tables).

So I

guess the current situation is more flexible.

So where does that leave this patch?

Sorry, I thought we were just having a digression. I didn't think that
part was about this patch specifically, but what more could be done later.

I don't think it makes this
patch a bad idea, although I kind of lean towads the view that the
patch should just be checking superuser_arg(), not superuser() ||
superuser_arg().

I don't see a reason to block a directly-logged-in superuser from using a
mapping. I asked in the closed list whether the current (released)
behavior was a security bug, and the answer was no. And I don't know why
else to block superusers from doing something other than as a security
bug. Also it would create a backwards compatibility hazard to revoke the
ability now.

Cheers,

Jeff

#16Nico Williams
nico@cryptonector.com
In reply to: Robert Haas (#11)
Re: postgres_fdw super user checks

On Thu, Sep 14, 2017 at 04:08:08PM -0400, Robert Haas wrote:

On Thu, Sep 14, 2017 at 2:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

I think that foreign tables ought to behave as views do, where they run as
the owner rather than the invoker. No one has talked me out of it, but no
one has supported me on it either. But I think it is too late to change
that now.

That's an interesting point. I think that you can imagine use cases
for either method. Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right. But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster. You want each person's own credentials to carry
over to everything they do remotely.

Hmmm, I don't think that's really right.

What I'd like instead is for the FDW client to tell the FDW server the
session_user/current_user on behalf of which it's acting, and let the
FDW server decide how to proceed. This could be done by doing a SET
SESSION fdw.client.session_user... and so on.

We use Kerberos principal names as PG user/role names, _with_ @REALM
included, so a user foo@BAR is likely to make sense to the FDW server.

Of course, if you're not using Kerberos then the local and remote user
namespaces might be completely distinct, but by letting the FDW server
know a) the FDW client's username (via authentication) and b) the true
username on the client side (via SET/set_config()), the server might
have enough information to decide whether it trusts (a) to impersonate
(b) and how to map (b) to a local user.

Nico
--

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17Simon Riggs
simon@2ndquadrant.com
In reply to: Jeff Janes (#13)
Re: postgres_fdw super user checks

On 4 October 2017 at 18:13, Jeff Janes <jeff.janes@gmail.com> wrote:

On Thu, Sep 14, 2017 at 1:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Sep 14, 2017 at 2:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

I think that foreign tables ought to behave as views do, where they run
as
the owner rather than the invoker. No one has talked me out of it, but
no
one has supported me on it either. But I think it is too late to change
that now.

That's an interesting point. I think that you can imagine use cases
for either method. Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right. But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster. You want each person's own credentials to carry
over to everything they do remotely.

OK. And if you want the first one, you can wrap it in a view currently, but
if it were changed I don't know what you would do if you want the 2nd one
(other than having every user create their own set of foreign tables). So I
guess the current situation is more flexible.

Sounds like it would be a useful option on a Foreign Server to allow
it to run queries as either the invoker or the owner. We have that
choice for functions, so we already have the concept and syntax
available. We could have another default at FDW level that specifies
what the default is for that type of FDW, and if that is not
specified, we keep it like it currently is.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#18Robert Haas
robertmhaas@gmail.com
In reply to: Jeff Janes (#15)
Re: postgres_fdw super user checks

On Thu, Oct 5, 2017 at 1:02 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

I don't see a reason to block a directly-logged-in superuser from using a
mapping. I asked in the closed list whether the current (released)
behavior was a security bug, and the answer was no. And I don't know why
else to block superusers from doing something other than as a security bug.
Also it would create a backwards compatibility hazard to revoke the ability
now.

Well, my thought was that we ought to be consistent about whose
authorization matters. If we're using the view owner's credentials in
general, then we also (defensibly, anyway) ought to use the view
owner's superuser-ness to decide whether to enforce this restriction.
Using either the view owner's superuser-ness or the session user's
superuser-ness kind of puts you halfway in the middle. The view
owner's rights are what matters mostly, but your own rights also
matter a little bit around the edges. That's a little strange.

I don't have violently strong opinions about this - does anyone else
have a view?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#19Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#18)
Re: postgres_fdw super user checks

Robert, all,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Oct 5, 2017 at 1:02 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

I don't see a reason to block a directly-logged-in superuser from using a
mapping. I asked in the closed list whether the current (released)
behavior was a security bug, and the answer was no. And I don't know why
else to block superusers from doing something other than as a security bug.
Also it would create a backwards compatibility hazard to revoke the ability
now.

Well, my thought was that we ought to be consistent about whose
authorization matters. If we're using the view owner's credentials in
general, then we also (defensibly, anyway) ought to use the view
owner's superuser-ness to decide whether to enforce this restriction.
Using either the view owner's superuser-ness or the session user's
superuser-ness kind of puts you halfway in the middle. The view
owner's rights are what matters mostly, but your own rights also
matter a little bit around the edges. That's a little strange.

I don't have violently strong opinions about this - does anyone else
have a view?

I haven't been following this closely, but I tend to agree with you- if
we're using the view owner's privileges then that's what everything
should be based on, not whether the caller is a superuser or not.

Consider a security-definer function. Clearly, such a function should
always run as the owner of the function, even if the caller is a
superuser. Running as the caller instead of the owner of the function
when the caller is a superuser because that would allow the function to
access more clearly isn't a good idea, imv.

Yes, that means that sometimes when superusers run things they get
permission denied errors. That's always been the case, and is correct.

Thanks!

Stephen

#20Robert Haas
robertmhaas@gmail.com
In reply to: Nico Williams (#16)
Re: [HACKERS] postgres_fdw super user checks

On Thu, Oct 5, 2017 at 1:16 PM, Nico Williams <nico@cryptonector.com> wrote:

That's an interesting point. I think that you can imagine use cases
for either method. Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right. But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster. You want each person's own credentials to carry
over to everything they do remotely.

Hmmm, I don't think that's really right.

What I'd like instead is for the FDW client to tell the FDW server the
session_user/current_user on behalf of which it's acting, and let the
FDW server decide how to proceed. This could be done by doing a SET
SESSION fdw.client.session_user... and so on.

Isn't that the same thing as the second use case I mentioned?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#21Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#19)
Re: [HACKERS] postgres_fdw super user checks

On Thu, Oct 12, 2017 at 9:21 PM, Stephen Frost <sfrost@snowman.net> wrote:

Yes, that means that sometimes when superusers run things they get
permission denied errors. That's always been the case, and is correct.

OK, let me try to summarize where we are with this.

Currently, postgres_fdw requires a password unless you are logged in
as a superuser. Jeff proposes to change that so that it requires a
password if you are EITHER logged in as a superuser OR using a
superuser's credentials - e.g. by selecting from a view created by a
superuser. Stephen and I propose that it should be one thing or the
other, and therefore that we should CHANGE the behavior to depend on
whose credentials you are using, not make it an either-or thing. So
when selecting from a view, whether or not you need a password would
depend entirely on who owns the view, not who you are. So that gives
us three options all of which are easy to implement: (1) leave it
alone [favored by nobody], (2) allow based on view owner OR current
user [favored by Jeff], (3) allowed based on view owner only [favored
by Stephen and me].

Taken in complete isolation, this would, maybe, constitute a marginal
consensus for option (2). However, Simon weighed in proposing a much
broader rethink in how foreign tables work -- letting them run with
either the owner's privileges or the accessor's privileges, rather
than always using the accessor's privileges as they do today. Nico
Williams, along with Jeff and I and others, had a lengthy discussion
of the desirability of some kind of authentication-forwarding
mechanisms. I view all of these questions as somewhat irrelevant to
the immediate decision insofar as we could do that stuff later, or
not, but they matter to the extent that they constitute tacit votes on
what to do about the patch on hand. Unless we can get a clearer
consensus here, I think we should just mark this patch as Rejected. I
hate to do nothing here, but neither doing something with which I
disagree nor trying to unilaterally impose my own opinion seem better.

Last call for other votes (or changes of opinion).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#22Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#21)
Re: [HACKERS] postgres_fdw super user checks

Robert Haas <robertmhaas@gmail.com> writes:

OK, let me try to summarize where we are with this.

Currently, postgres_fdw requires a password unless you are logged in
as a superuser. Jeff proposes to change that so that it requires a
password if you are EITHER logged in as a superuser OR using a
superuser's credentials - e.g. by selecting from a view created by a
superuser. Stephen and I propose that it should be one thing or the
other, and therefore that we should CHANGE the behavior to depend on
whose credentials you are using, not make it an either-or thing. So
when selecting from a view, whether or not you need a password would
depend entirely on who owns the view, not who you are. So that gives
us three options all of which are easy to implement: (1) leave it
alone [favored by nobody], (2) allow based on view owner OR current
user [favored by Jeff], (3) allowed based on view owner only [favored
by Stephen and me].

FWIW, option (3) feels like the right thing to me. It does not seem
right that a view would behave differently depending on who calls it.
We are using the view owner's user mapping, no?

Now, the core reason why there's any restriction at all is that we
do not want non-superusers to be able to piggyback on credentials
belonging to the postgres OS user. For example, without a user-
supplied password, libpq might successfully make a connection
because it got a password out of ~postgres/.pgpass, or because the
"peer" auth method authenticated the connection as coming from a
postgres-owned process, etc. So there's a question as to which of
these options squares with that concern. But again it seems like
(3) is a better choice. As-is, a non-superuser-owned view might
successfully scrape a password out of ~postgres/.pgpass if it's
being called by a superuser, which doesn't seem especially desirable.
If we change, then a superuser-owned view would successfully connect
when being run by a non-superuser, which does seem desirable (given
that the non-superuser must have been given privileges on the view).

Taken in complete isolation, this would, maybe, constitute a marginal
consensus for option (2). However, Simon weighed in proposing a much
broader rethink in how foreign tables work -- letting them run with
either the owner's privileges or the accessor's privileges, rather
than always using the accessor's privileges as they do today.

While I'm not objecting to the concept of a rethink, I don't think
that "either-or" is what we want for security considerations. What
would make sense to me, if we're going to consider the foreign table
owner as relevant, is always using the owner's privileges. A view
owner's privileges, for example, should determine whether the view
is allowed to select from the foreign table --- but then the foreign
table owner's identity should determine what happens while connecting
to the remote server.

Anyway, one thing I'm not for is whacking this around at this time
and then whacking it around some more later. If there's any serious
interest in a more global rethink, I think we ought to mark this
Returned With Feedback pending someone doing that rethink.

... Unless we can get a clearer
consensus here, I think we should just mark this patch as Rejected.

"Rejected" seems to me to imply "not only do we not want this patch,
but we're uninterested in any likely successor either". That seems
overly strong, hence my suggestion of RWF. Although maybe switching
to owner privileges would be so different as to constitute an unrelated
patch.

regards, tom lane

#23Stephen Frost
sfrost@snowman.net
In reply to: Tom Lane (#22)
Re: [HACKERS] postgres_fdw super user checks

Tom, Robert, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

OK, let me try to summarize where we are with this.

Currently, postgres_fdw requires a password unless you are logged in
as a superuser. Jeff proposes to change that so that it requires a
password if you are EITHER logged in as a superuser OR using a
superuser's credentials - e.g. by selecting from a view created by a
superuser. Stephen and I propose that it should be one thing or the
other, and therefore that we should CHANGE the behavior to depend on
whose credentials you are using, not make it an either-or thing. So
when selecting from a view, whether or not you need a password would
depend entirely on who owns the view, not who you are. So that gives
us three options all of which are easy to implement: (1) leave it
alone [favored by nobody], (2) allow based on view owner OR current
user [favored by Jeff], (3) allowed based on view owner only [favored
by Stephen and me].

FWIW, option (3) feels like the right thing to me. It does not seem
right that a view would behave differently depending on who calls it.

Just to make it clear, I continue to agree with (3) and agree with Tom
that we shouldn't be behaving differently depending on who is calling
the view.

Now, the core reason why there's any restriction at all is that we
do not want non-superusers to be able to piggyback on credentials
belonging to the postgres OS user. For example, without a user-
supplied password, libpq might successfully make a connection
because it got a password out of ~postgres/.pgpass, or because the
"peer" auth method authenticated the connection as coming from a
postgres-owned process, etc. So there's a question as to which of
these options squares with that concern. But again it seems like
(3) is a better choice. As-is, a non-superuser-owned view might
successfully scrape a password out of ~postgres/.pgpass if it's
being called by a superuser, which doesn't seem especially desirable.
If we change, then a superuser-owned view would successfully connect
when being run by a non-superuser, which does seem desirable (given
that the non-superuser must have been given privileges on the view).

Right, agreed.

Taken in complete isolation, this would, maybe, constitute a marginal
consensus for option (2). However, Simon weighed in proposing a much
broader rethink in how foreign tables work -- letting them run with
either the owner's privileges or the accessor's privileges, rather
than always using the accessor's privileges as they do today.

While I'm not objecting to the concept of a rethink, I don't think
that "either-or" is what we want for security considerations. What
would make sense to me, if we're going to consider the foreign table
owner as relevant, is always using the owner's privileges. A view
owner's privileges, for example, should determine whether the view
is allowed to select from the foreign table --- but then the foreign
table owner's identity should determine what happens while connecting
to the remote server.

I agree that "either-or" is bad for security considerations.

Anyway, one thing I'm not for is whacking this around at this time
and then whacking it around some more later. If there's any serious
interest in a more global rethink, I think we ought to mark this
Returned With Feedback pending someone doing that rethink.

The "global rethink" being contemplated seems to be more about
authentication forwarding than it is about this specific change. If
there's some 'global rethink' which is actually applicable to this
specific deviation from the usual "use the view's owner for privilege
checks", then it's unclear to me what that is.

... Unless we can get a clearer
consensus here, I think we should just mark this patch as Rejected.

"Rejected" seems to me to imply "not only do we not want this patch,
but we're uninterested in any likely successor either". That seems
overly strong, hence my suggestion of RWF. Although maybe switching
to owner privileges would be so different as to constitute an unrelated
patch.

I tend to agree with RWF also in this case, though hopefully we can have
something done in the next CF which implements what seems to be the
consensus here from those looking specifically at this issue.

Thanks!

Stephen

#24Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Stephen Frost (#23)
Re: [HACKERS] postgres_fdw super user checks

On Wed, Nov 29, 2017 at 4:56 AM, Stephen Frost <sfrost@snowman.net> wrote:

Just to make it clear, I continue to agree with (3) and agree with Tom
that we shouldn't be behaving differently depending on who is calling
the view.

I also would vote for 3. That looks consistent with the way we handle
accesses based on owner of a view generally (without foreign tables
involved).

The "global rethink" being contemplated seems to be more about
authentication forwarding than it is about this specific change. If
there's some 'global rethink' which is actually applicable to this
specific deviation from the usual "use the view's owner for privilege
checks", then it's unclear to me what that is.

Global rethink may constitute other authentication methods like
certificate based authentication. But I am not clear about global
rethink in the context of owner privileges problem being discussed
here.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

#25Stephen Frost
sfrost@snowman.net
In reply to: Ashutosh Bapat (#24)
Re: [HACKERS] postgres_fdw super user checks

Ashutosh,

* Ashutosh Bapat (ashutosh.bapat@enterprisedb.com) wrote:

On Wed, Nov 29, 2017 at 4:56 AM, Stephen Frost <sfrost@snowman.net> wrote:

The "global rethink" being contemplated seems to be more about
authentication forwarding than it is about this specific change. If
there's some 'global rethink' which is actually applicable to this
specific deviation from the usual "use the view's owner for privilege
checks", then it's unclear to me what that is.

Global rethink may constitute other authentication methods like
certificate based authentication. But I am not clear about global
rethink in the context of owner privileges problem being discussed
here.

Right, I'm all for an independent discussion about how we can have
same-cluster or cross-cluster trust relationships set up to make it
easier for users in one cluster/database to access tables in another
that they should be allowed to, but that's a different topic from this.

In other words, I think we're agreeing here. :)

Thanks!

Stephen

#26Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Stephen Frost (#25)
Re: [HACKERS] postgres_fdw super user checks

On Wed, Nov 29, 2017 at 7:42 PM, Stephen Frost <sfrost@snowman.net> wrote:

Ashutosh,

* Ashutosh Bapat (ashutosh.bapat@enterprisedb.com) wrote:

On Wed, Nov 29, 2017 at 4:56 AM, Stephen Frost <sfrost@snowman.net> wrote:

The "global rethink" being contemplated seems to be more about
authentication forwarding than it is about this specific change. If
there's some 'global rethink' which is actually applicable to this
specific deviation from the usual "use the view's owner for privilege
checks", then it's unclear to me what that is.

Global rethink may constitute other authentication methods like
certificate based authentication. But I am not clear about global
rethink in the context of owner privileges problem being discussed
here.

Right, I'm all for an independent discussion about how we can have
same-cluster or cross-cluster trust relationships set up to make it
easier for users in one cluster/database to access tables in another
that they should be allowed to, but that's a different topic from this.

In other words, I think we're agreeing here. :)

Yes.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

#27Michael Paquier
michael.paquier@gmail.com
In reply to: Ashutosh Bapat (#26)
Re: [HACKERS] postgres_fdw super user checks

On Thu, Nov 30, 2017 at 12:15 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Wed, Nov 29, 2017 at 7:42 PM, Stephen Frost <sfrost@snowman.net> wrote:

Ashutosh,

* Ashutosh Bapat (ashutosh.bapat@enterprisedb.com) wrote:

On Wed, Nov 29, 2017 at 4:56 AM, Stephen Frost <sfrost@snowman.net> wrote:

The "global rethink" being contemplated seems to be more about
authentication forwarding than it is about this specific change. If
there's some 'global rethink' which is actually applicable to this
specific deviation from the usual "use the view's owner for privilege
checks", then it's unclear to me what that is.

Global rethink may constitute other authentication methods like
certificate based authentication. But I am not clear about global
rethink in the context of owner privileges problem being discussed
here.

Right, I'm all for an independent discussion about how we can have
same-cluster or cross-cluster trust relationships set up to make it
easier for users in one cluster/database to access tables in another
that they should be allowed to, but that's a different topic from this.

In other words, I think we're agreeing here. :)

Yes.

I am moving this patch to next CF 2018-01.
--
Michael

#28Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#27)
1 attachment(s)
Re: [HACKERS] postgres_fdw super user checks

On Fri, Dec 1, 2017 at 12:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

I am moving this patch to next CF 2018-01.

There now seems to be a consensus for superuser -> superuser_arg
rather than what Jeff did originally; that approach has 4 votes and
nothing else has more than 1. So, here's a patch that does it that
way.

I tried to see if some documentation update was needed, but I think
the documentation already reflects the proposed new behavior. It
says:

<para>
Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal> option
for user mappings belonging to non-superusers.
</para>

Currently, however, that's not accurate. Right now you need to
specify the password option for user mappings that will be *used by*
non-superusers, not user mappings *belonging to* non-superusers. So
this patch is, I think, just making the actual behavior match the
documented behavior. Not sure if anyone has any other suggestions
here. I think this is definitely a master-only change; should we try
to insert some kind of warning into the back-branch docs? I
definitely think this should be called out in the v11 release notes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

postgres-fdw-superuser.patchapplication/octet-stream; name=postgres-fdw-superuser.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index be4ec07cf9..4fbf043860 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -75,7 +75,7 @@ static bool xact_got_connection = false;
 /* prototypes of private functions */
 static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
 static void disconnect_pg_server(ConnCacheEntry *entry);
-static void check_conn_params(const char **keywords, const char **values);
+static void check_conn_params(const char **keywords, const char **values, UserMapping *user);
 static void configure_remote_session(PGconn *conn);
 static void do_sql_command(PGconn *conn, const char *sql);
 static void begin_remote_xact(ConnCacheEntry *entry);
@@ -261,7 +261,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 		keywords[n] = values[n] = NULL;
 
 		/* verify connection parameters and make connection */
-		check_conn_params(keywords, values);
+		check_conn_params(keywords, values, user);
 
 		conn = PQconnectdbParams(keywords, values, false);
 		if (!conn || PQstatus(conn) != CONNECTION_OK)
@@ -276,7 +276,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 		 * otherwise, he's piggybacking on the postgres server's user
 		 * identity. See also dblink_security_check() in contrib/dblink.
 		 */
-		if (!superuser() && !PQconnectionUsedPassword(conn))
+		if (!superuser_arg(user->userid) && !PQconnectionUsedPassword(conn))
 			ereport(ERROR,
 					(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
 					 errmsg("password is required"),
@@ -322,12 +322,12 @@ disconnect_pg_server(ConnCacheEntry *entry)
  * contrib/dblink.)
  */
 static void
-check_conn_params(const char **keywords, const char **values)
+check_conn_params(const char **keywords, const char **values, UserMapping *user)
 {
 	int			i;
 
 	/* no check required if superuser */
-	if (superuser())
+	if (superuser_arg(user->userid))
 		return;
 
 	/* ok if params contain a non-empty password */
#29Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#28)
Re: [HACKERS] postgres_fdw super user checks

Robert, all,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, Dec 1, 2017 at 12:31 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

I am moving this patch to next CF 2018-01.

There now seems to be a consensus for superuser -> superuser_arg
rather than what Jeff did originally; that approach has 4 votes and
nothing else has more than 1. So, here's a patch that does it that
way.

I've taken a quick look and this looks good to me.

I tried to see if some documentation update was needed, but I think
the documentation already reflects the proposed new behavior. It
says:

<para>
Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal> option
for user mappings belonging to non-superusers.
</para>

Currently, however, that's not accurate. Right now you need to
specify the password option for user mappings that will be *used by*
non-superusers, not user mappings *belonging to* non-superusers. So
this patch is, I think, just making the actual behavior match the
documented behavior. Not sure if anyone has any other suggestions
here. I think this is definitely a master-only change; should we try
to insert some kind of warning into the back-branch docs? I
definitely think this should be called out in the v11 release notes.

I'm not a fan of having *only* warning in the back-branches. What I
would think we'd do here is correct the back-branch documentation to be
correct, and then add a warning that it changes in v11.

You didn't suggest an actual change wrt the back-branch warning, but it
seems to me like it'd end up being morally equivilant to "ok, forget
what we just said, what really happens is X, but we fix it in v11."

Thanks!

Stephen

#30Robert Haas
robertmhaas@gmail.com
In reply to: Stephen Frost (#29)
Re: [HACKERS] postgres_fdw super user checks

On Sun, Dec 3, 2017 at 3:42 PM, Stephen Frost <sfrost@snowman.net> wrote:

I'm not a fan of having *only* warning in the back-branches. What I
would think we'd do here is correct the back-branch documentation to be
correct, and then add a warning that it changes in v11.

You didn't suggest an actual change wrt the back-branch warning, but it
seems to me like it'd end up being morally equivilant to "ok, forget
what we just said, what really happens is X, but we fix it in v11."

Yeah, I'm very unclear what, if anything, to do about the back-branch
documentation. Suggestions appreciated.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#31Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Robert Haas (#30)
Re: [HACKERS] postgres_fdw super user checks

On Tue, Dec 5, 2017 at 2:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Dec 3, 2017 at 3:42 PM, Stephen Frost <sfrost@snowman.net> wrote:

I'm not a fan of having *only* warning in the back-branches. What I
would think we'd do here is correct the back-branch documentation to be
correct, and then add a warning that it changes in v11.

You didn't suggest an actual change wrt the back-branch warning, but it
seems to me like it'd end up being morally equivilant to "ok, forget
what we just said, what really happens is X, but we fix it in v11."

Yeah, I'm very unclear what, if anything, to do about the back-branch
documentation. Suggestions appreciated.

I think the real behaviour can be described as something like this:

"Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal>
option for user mappings that may be used by non-superusers." But
which user mappings may be used by non-superusers can not be defined
without explaining views owned by superusers. I don't think we should
be talking about views in that part of documentation.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

#32Robert Haas
robertmhaas@gmail.com
In reply to: Ashutosh Bapat (#31)
Re: [HACKERS] postgres_fdw super user checks

On Mon, Dec 4, 2017 at 5:57 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

I think the real behaviour can be described as something like this:

"Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal>
option for user mappings that may be used by non-superusers." But
which user mappings may be used by non-superusers can not be defined
without explaining views owned by superusers. I don't think we should
be talking about views in that part of documentation.

Well, if we don't, then I'm not sure we can really make this clear.

Anyhow, I've committed the patch to master for now; we can keep
arguing about what, if anything, to do for back-branch documentation.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#33Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#32)
Re: [HACKERS] postgres_fdw super user checks

Robert, Ashutosh,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Mon, Dec 4, 2017 at 5:57 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

I think the real behaviour can be described as something like this:

"Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal>
option for user mappings that may be used by non-superusers." But
which user mappings may be used by non-superusers can not be defined
without explaining views owned by superusers. I don't think we should
be talking about views in that part of documentation.

Well, if we don't, then I'm not sure we can really make this clear.

Yeah, I'm pretty sure we need to spell out the situation around views
here because it's different from how views normally work as discussed in
Rules and Privileges.

I'll note that the Rules and Privileges section could use a bit of love
too- the v10 docs have:

"Due to rewriting of queries by the PostgreSQL rule system, other
tables/views than those used in the original query get accessed. When
update rules are used, this can include write access to tables."

Which isn't really accurate since simple updatable views were added.

Looking at it more though, really, I think that whole page needs to be
re-cast to be about *views* and stop talking about rules. That's really
a seperate discussino to have though.

Anyhow, I've committed the patch to master for now; we can keep
arguing about what, if anything, to do for back-branch documentation.

Thanks!

Stephen

#34Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Robert Haas (#32)
Re: [HACKERS] postgres_fdw super user checks

On Wed, Dec 6, 2017 at 1:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:

"Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal>
option for user mappings that may be used by non-superusers." But
which user mappings may be used by non-superusers can not be defined
without explaining views owned by superusers. I don't think we should
be talking about views in that part of documentation.

Well, if we don't, then I'm not sure we can really make this clear.

Anyhow, I've committed the patch to master for now; we can keep
arguing about what, if anything, to do for back-branch documentation.

Ok, something like this:

"Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal>
option for user mappings that may be used by non-superusers. Hence
always specify the <literal>password</literal> option for a user
mapping for a non-superuser. Consider a view referencing a foreign
table and owned by a superuser but accessible to a non-superuser. When
the non-superuser executes a query referencing the view, it uses
superuser's user mapping to connect to the foreign server. Since a
non-superuser is using the user mapping, it requires password, even
though its a super-user's mapping. Hence specify the
<literal>password</literal> option for a user mapping for a superuser,
if the superuser has such views."

That's a lot of explanation. And somehow we will have to say that this
behaviour will change in the next version.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

#35Jeff Janes
jeff.janes@gmail.com
In reply to: Simon Riggs (#17)
Re: [HACKERS] postgres_fdw super user checks

On Thu, Oct 5, 2017 at 10:49 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 4 October 2017 at 18:13, Jeff Janes <jeff.janes@gmail.com> wrote:

OK. And if you want the first one, you can wrap it in a view currently,
but

if it were changed I don't know what you would do if you want the 2nd one
(other than having every user create their own set of foreign tables).

So I

guess the current situation is more flexible.

Sounds like it would be a useful option on a Foreign Server to allow
it to run queries as either the invoker or the owner. We have that
choice for functions, so we already have the concept and syntax
available. We could have another default at FDW level that specifies
what the default is for that type of FDW, and if that is not
specified, we keep it like it currently is.

To go further off topic, I'd like to have the invoker vs definer security
options available even for plain old views as well. Sometimes I want
create a view so that I can let people see, in a controlled manner, things
they couldn't otherwise see. But more often I just want to provide a
convenience wrapper around ugly SQL without accidentally granting people
additional privileges.

Cheers,

Jeff

#36Jeff Janes
jeff.janes@gmail.com
In reply to: Robert Haas (#32)
Re: [HACKERS] postgres_fdw super user checks

On Tue, Dec 5, 2017 at 8:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Dec 4, 2017 at 5:57 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

I think the real behaviour can be described as something like this:

"Only superusers may connect to foreign servers without password
authentication, so always specify the <literal>password</literal>
option for user mappings that may be used by non-superusers." But
which user mappings may be used by non-superusers can not be defined
without explaining views owned by superusers. I don't think we should
be talking about views in that part of documentation.

Well, if we don't, then I'm not sure we can really make this clear.

Anyhow, I've committed the patch to master for now; we can keep
arguing about what, if anything, to do for back-branch documentation.

Thank you for the changes and commit. Alas, I don't know how to document
this clearly, either.

Cheers,

Jeff

#37Jeff Janes
jeff.janes@gmail.com
In reply to: Nico Williams (#16)
Re: [HACKERS] postgres_fdw super user checks

On Thu, Oct 5, 2017 at 10:16 AM, Nico Williams <nico@cryptonector.com>
wrote:

On Thu, Sep 14, 2017 at 04:08:08PM -0400, Robert Haas wrote:

On Thu, Sep 14, 2017 at 2:33 PM, Jeff Janes <jeff.janes@gmail.com>

wrote:

I think that foreign tables ought to behave as views do, where they

run as

the owner rather than the invoker. No one has talked me out of it,

but no

one has supported me on it either. But I think it is too late to

change

that now.

That's an interesting point. I think that you can imagine use cases
for either method. Obviously, if what you want to do is drill a hole
through the Internet to another server and then expose it to some of
your fellow users, having the FDW run with the owner's permissions
(and credentials) is exactly right. But there's another use case too,
which is where you have something that looks like a multi-user
sharding cluster. You want each person's own credentials to carry
over to everything they do remotely.

Hmmm, I don't think that's really right.

What I'd like instead is for the FDW client to tell the FDW server the
session_user/current_user on behalf of which it's acting, and let the
FDW server decide how to proceed. This could be done by doing a SET
SESSION fdw.client.session_user... and so on.

So then the FDW client would still have to authenticate itself, presumably
as a superuser, to the FDW server, and just tell the server "trust me on
who I'm representing here"?

We use Kerberos principal names as PG user/role names, _with_ @REALM
included, so a user foo@BAR is likely to make sense to the FDW server.

Of course, if you're not using Kerberos then the local and remote user
namespaces might be completely distinct, but by letting the FDW server
know a) the FDW client's username (via authentication) and b) the true
username on the client side (via SET/set_config()), the server might
have enough information to decide whether it trusts (a) to impersonate
(b) and how to map (b) to a local user.

With Kerberos, shouldn't the ultimate client be able (in theory) to
authenticate to the end server through the intermediate server, without
giving the intermediate server general impersonation privileges? If that
can be done in theory, any idea on how to implement it?

Cheers,

Jeff