Re: dblink patches for comment - Mailing list pgsql-hackers

Tom Lane wrote:
> It's hard to review it without any docs that say what it's supposed to do.
> (And you'd need to patch the docs anyway, eh?)

Here's a much simpler SQL/MED support patch for dblink.

This enforces security in the same manner for FOREIGN SERVER connections
as that worked out over time for other dblink connections. Essentially,
the FOREIGN SERVER and associated user MAPPING provides the needed info
for the libpq connection, but otherwise behavior is the same.

I've also attached a doc patch.

Comments?

Joe
Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.77
diff -c -r1.77 dblink.c
*** dblink.c    1 Jan 2009 17:23:31 -0000    1.77
--- dblink.c    31 May 2009 21:21:16 -0000
***************
*** 46,51 ****
--- 46,52 ----
  #include "catalog/pg_type.h"
  #include "executor/executor.h"
  #include "executor/spi.h"
+ #include "foreign/foreign.h"
  #include "lib/stringinfo.h"
  #include "miscadmin.h"
  #include "nodes/execnodes.h"
***************
*** 96,101 ****
--- 97,103 ----
  static void dblink_connstr_check(const char *connstr);
  static void dblink_security_check(PGconn *conn, remoteConn *rconn);
  static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
+ static char *get_connect_string(const char *servername);

  /* Global */
  static remoteConn *pconn = NULL;
***************
*** 165,171 ****
              } \
              else \
              { \
!                 connstr = conname_or_str; \
                  dblink_connstr_check(connstr); \
                  conn = PQconnectdb(connstr); \
                  if (PQstatus(conn) == CONNECTION_BAD) \
--- 167,177 ----
              } \
              else \
              { \
!                 connstr = get_connect_string(conname_or_str); \
!                 if (connstr == NULL) \
!                 { \
!                     connstr = conname_or_str; \
!                 } \
                  dblink_connstr_check(connstr); \
                  conn = PQconnectdb(connstr); \
                  if (PQstatus(conn) == CONNECTION_BAD) \
***************
*** 210,215 ****
--- 216,222 ----
  Datum
  dblink_connect(PG_FUNCTION_ARGS)
  {
+     char       *conname_or_str = NULL;
      char       *connstr = NULL;
      char       *connname = NULL;
      char       *msg;
***************
*** 220,235 ****

      if (PG_NARGS() == 2)
      {
!         connstr = text_to_cstring(PG_GETARG_TEXT_PP(1));
          connname = text_to_cstring(PG_GETARG_TEXT_PP(0));
      }
      else if (PG_NARGS() == 1)
!         connstr = text_to_cstring(PG_GETARG_TEXT_PP(0));

      if (connname)
          rconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext,
                                                    sizeof(remoteConn));

      /* check password in connection string if not superuser */
      dblink_connstr_check(connstr);
      conn = PQconnectdb(connstr);
--- 227,247 ----

      if (PG_NARGS() == 2)
      {
!         conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(1));
          connname = text_to_cstring(PG_GETARG_TEXT_PP(0));
      }
      else if (PG_NARGS() == 1)
!         conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(0));

      if (connname)
          rconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext,
                                                    sizeof(remoteConn));

+     /* first check for valid foreign data server */
+     connstr = get_connect_string(conname_or_str);
+     if (connstr == NULL)
+         connstr = conname_or_str;
+
      /* check password in connection string if not superuser */
      dblink_connstr_check(connstr);
      conn = PQconnectdb(connstr);
***************
*** 2358,2360 ****
--- 2370,2430 ----
           errcontext("Error occurred on dblink connection named \"%s\": %s.",
                      dblink_context_conname, dblink_context_msg)));
  }
+
+ /*
+  * Obtain connection string for a foreign server
+  */
+ static char *
+ get_connect_string(const char *servername)
+ {
+     ForeignServer       *foreign_server;
+     UserMapping           *user_mapping;
+     ListCell           *cell;
+     StringInfo            buf = makeStringInfo();
+     ForeignDataWrapper *fdw;
+     AclResult            aclresult;
+
+     /* first gather the server connstr options */
+     foreign_server = GetForeignServerByName(servername, true);
+
+     if (foreign_server)
+     {
+         Oid        serverid = foreign_server->serverid;
+         Oid        fdwid = foreign_server->fdwid;
+         Oid        userid = GetUserId();
+
+         user_mapping = GetUserMapping(userid, serverid);
+         fdw    = GetForeignDataWrapper(fdwid);
+
+         /* Check permissions, user must have usage on the server. */
+         aclresult = pg_foreign_server_aclcheck(serverid, userid, ACL_USAGE);
+         if (aclresult != ACLCHECK_OK)
+             aclcheck_error(aclresult, ACL_KIND_FOREIGN_SERVER, foreign_server->servername);
+
+         foreach (cell, fdw->options)
+         {
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         foreach (cell, foreign_server->options)
+         {
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         foreach (cell, user_mapping->options)
+         {
+
+             DefElem           *def = lfirst(cell);
+
+             appendStringInfo(buf, "%s='%s' ", def->defname, strVal(def->arg));
+         }
+
+         return pstrdup(buf->data);
+     }
+     else
+         return NULL;
+ }
Index: expected/dblink.out
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/expected/dblink.out,v
retrieving revision 1.24
diff -c -r1.24 dblink.out
*** expected/dblink.out    3 Jul 2008 03:56:57 -0000    1.24
--- expected/dblink.out    1 Jun 2009 00:06:08 -0000
***************
*** 784,786 ****
--- 784,829 ----
   OK
  (1 row)

+ -- test foreign data wrapper functionality
+ CREATE USER dblink_regression_test;
+ CREATE FOREIGN DATA WRAPPER postgresql;
+ CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+ CREATE USER MAPPING FOR public SERVER fdtest;
+ GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
+ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
+ \set ORIGINAL_USER :USER
+ \c - dblink_regression_test
+ -- should fail
+ SELECT dblink_connect('myconn', 'fdtest');
+ ERROR:  password is required
+ DETAIL:  Non-superusers must provide a password in the connection string.
+ -- should succeed
+ SELECT dblink_connect_u('myconn', 'fdtest');
+  dblink_connect_u
+ ------------------
+  OK
+ (1 row)
+
+ SELECT * FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[]);
+  a  | b |       c
+ ----+---+---------------
+   0 | a | {a0,b0,c0}
+   1 | b | {a1,b1,c1}
+   2 | c | {a2,b2,c2}
+   3 | d | {a3,b3,c3}
+   4 | e | {a4,b4,c4}
+   5 | f | {a5,b5,c5}
+   6 | g | {a6,b6,c6}
+   7 | h | {a7,b7,c7}
+   8 | i | {a8,b8,c8}
+   9 | j | {a9,b9,c9}
+  10 | k | {a10,b10,c10}
+ (11 rows)
+
+ \c - :ORIGINAL_USER
+ REVOKE USAGE ON FOREIGN SERVER fdtest FROM dblink_regression_test;
+ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_test;
+ DROP USER dblink_regression_test;
+ DROP USER MAPPING FOR public SERVER fdtest;
+ DROP SERVER fdtest;
+ DROP FOREIGN DATA WRAPPER postgresql;
Index: sql/dblink.sql
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/sql/dblink.sql,v
retrieving revision 1.20
diff -c -r1.20 dblink.sql
*** sql/dblink.sql    6 Apr 2008 16:54:48 -0000    1.20
--- sql/dblink.sql    1 Jun 2009 00:05:29 -0000
***************
*** 364,366 ****
--- 364,391 ----
  SELECT dblink_cancel_query('dtest1');
  SELECT dblink_error_message('dtest1');
  SELECT dblink_disconnect('dtest1');
+
+ -- test foreign data wrapper functionality
+ CREATE USER dblink_regression_test;
+
+ CREATE FOREIGN DATA WRAPPER postgresql;
+ CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (dbname 'contrib_regression');
+ CREATE USER MAPPING FOR public SERVER fdtest;
+ GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
+ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO dblink_regression_test;
+
+ \set ORIGINAL_USER :USER
+ \c - dblink_regression_test
+ -- should fail
+ SELECT dblink_connect('myconn', 'fdtest');
+ -- should succeed
+ SELECT dblink_connect_u('myconn', 'fdtest');
+ SELECT * FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[]);
+
+ \c - :ORIGINAL_USER
+ REVOKE USAGE ON FOREIGN SERVER fdtest FROM dblink_regression_test;
+ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) FROM dblink_regression_test;
+ DROP USER dblink_regression_test;
+ DROP USER MAPPING FOR public SERVER fdtest;
+ DROP SERVER fdtest;
+ DROP FOREIGN DATA WRAPPER postgresql;
Index: dblink.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/dblink.sgml,v
retrieving revision 1.6
diff -c -r1.6 dblink.sgml
*** dblink.sgml    12 Nov 2008 15:52:44 -0000    1.6
--- dblink.sgml    1 Jun 2009 00:12:22 -0000
***************
*** 42,47 ****
--- 42,59 ----
      only one unnamed connection is permitted at a time.  The connection
      will persist until closed or until the database session is ended.
     </para>
+
+    <para>
+     The connection string may also be the name of an existing foreign
+     server that utilizes the postgresql_fdw foreign data wrapper library.
+     See the example below, as well as the following:
+     <simplelist type="inline">
+      <member><xref linkend="sql-createforeigndatawrapper" endterm="sql-createforeigndatawrapper-title"></member>
+      <member><xref linkend="sql-createserver" endterm="sql-createserver-title"></member>
+      <member><xref linkend="sql-createusermapping" endterm="sql-createusermapping-title"></member>
+     </simplelist>
+    </para>
+
    </refsect1>

    <refsect1>
***************
*** 113,118 ****
--- 125,172 ----
   ----------------
    OK
   (1 row)
+
+  -- FOREIGN DATA WRAPPER functionality
+  -- Note: local connection must require authentication for this to work properly
+  CREATE USER dblink_regression_test WITH PASSWORD 'secret';
+  CREATE FOREIGN DATA WRAPPER postgresql;
+  CREATE SERVER fdtest FOREIGN DATA WRAPPER postgresql OPTIONS (hostaddr '127.0.0.1', dbname 'contrib_regression');
+
+  CREATE USER MAPPING FOR dblink_regression_test SERVER fdtest OPTIONS (user 'dblink_regression_test', password
'secret');
+  GRANT USAGE ON FOREIGN SERVER fdtest TO dblink_regression_test;
+  GRANT SELECT ON TABLE foo TO dblink_regression_test;
+
+  \set ORIGINAL_USER :USER
+  \c - dblink_regression_test
+  SELECT dblink_connect('myconn', 'fdtest');
+   dblink_connect
+  ----------------
+   OK
+  (1 row)
+
+  SELECT * FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[]);
+   a  | b |       c
+  ----+---+---------------
+    0 | a | {a0,b0,c0}
+    1 | b | {a1,b1,c1}
+    2 | c | {a2,b2,c2}
+    3 | d | {a3,b3,c3}
+    4 | e | {a4,b4,c4}
+    5 | f | {a5,b5,c5}
+    6 | g | {a6,b6,c6}
+    7 | h | {a7,b7,c7}
+    8 | i | {a8,b8,c8}
+    9 | j | {a9,b9,c9}
+   10 | k | {a10,b10,c10}
+  (11 rows)
+
+  \c - :ORIGINAL_USER
+  REVOKE USAGE ON FOREIGN SERVER fdtest FROM dblink_regression_test;
+  REVOKE SELECT ON TABLE foo FROM  dblink_regression_test;
+  DROP USER MAPPING FOR dblink_regression_test SERVER fdtest;
+  DROP USER dblink_regression_test;
+  DROP SERVER fdtest;
+  DROP FOREIGN DATA WRAPPER postgresql;
     </programlisting>
    </refsect1>
   </refentry>