Re: [HACKERS] expand_dbname in postgres_fdw

On Thu, Jul 27, 2017 at 12:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The main problem to my mind is that a connection string could possibly
> override items meant to be specified elsewhere.  In particular it ought
> not be allowed to specify the remote username or password, because those
> are supposed to come from the user mapping object not the server object.
> I suspect you could break things by trying to specify client_encoding
> there, as well.

Attached patch allows dbname expansion and makes sure that it doesn't
contain any invalid options. Whether you decide to commit it or not
(at the moment I don't see any security implications, at least not more than
in usual dbname expansion usage, e.g. in psql, but who knows), it seems
to me that the documentation should be updated since currently it is not
clear on the subject, as the beginning of this thread proves.
From 09e3924501d219331b8b8fca12a9ea35a689ba10 Mon Sep 17 00:00:00 2001
From: Arseny Sher <sher-ars@yandex.ru>
Date: Thu, 27 Jul 2017 12:41:17 +0300
Subject: [PATCH] Allow 'dbname' option contain full-fledged connstring in
 postgres_fdw

And if this is the case, validate that it doesn't contain any invalid options
such as client_encoding or user.
---
 contrib/postgres_fdw/connection.c |  2 +-
 contrib/postgres_fdw/option.c     | 80 ++++++++++++++++++++++++++++++++-------
 2 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index be4ec07cf9..bfcd9ed2e3 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -263,7 +263,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
         /* verify connection parameters and make connection */
         check_conn_params(keywords, values);
 
-        conn = PQconnectdbParams(keywords, values, false);
+        conn = PQconnectdbParams(keywords, values, true);
         if (!conn || PQstatus(conn) != CONNECTION_OK)
             ereport(ERROR,
                     (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 67e1c59951..26c11e5179 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -50,8 +50,10 @@ static PQconninfoOption *libpq_options;
  * Helper functions
  */
 static void InitPgFdwOptions(void);
+static void validate_dbname(const char *dbname);
+static void get_opts_hint(StringInfo buf, Oid context, bool libpq_options);
 static bool is_valid_option(const char *keyword, Oid context);
-static bool is_libpq_option(const char *keyword);
+static bool is_libpq_option(const char *keyword, Oid context);
 
 
 /*
@@ -86,16 +88,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
              * Unknown option specified, complain about it. Provide a hint
              * with list of valid options for the object.
              */
-            PgFdwOption *opt;
             StringInfoData buf;
-
-            initStringInfo(&buf);
-            for (opt = postgres_fdw_options; opt->keyword; opt++)
-            {
-                if (catalog == opt->optcontext)
-                    appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
-                                     opt->keyword);
-            }
+            get_opts_hint(&buf, catalog, false);
 
             ereport(ERROR,
                     (errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
@@ -143,6 +137,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
                          errmsg("%s requires a non-negative integer value",
                                 def->defname)));
         }
+        else if (strcmp(def->defname, "dbname") == 0)
+        {
+            validate_dbname(defGetString(def));
+        }
     }
 
     PG_RETURN_VOID();
@@ -250,6 +248,59 @@ InitPgFdwOptions(void)
 }
 
 /*
+ * If dbname param specified, make sure it doesn't carry invalid
+ * options if it is a connstring.
+ */
+static void
+validate_dbname(const char *dbname)
+{
+    PQconninfoOption *lopts;
+    PQconninfoOption *lopt;
+
+    /* If it is not a connstring, just skip the check */
+    if ((lopts = PQconninfoParse(dbname, NULL)) != NULL)
+    {
+        for (lopt = lopts; lopt->keyword; lopt++)
+        {
+            if (lopt->val != NULL && !is_libpq_option(lopt->keyword,
+                                                      ForeignServerRelationId))
+            {
+                StringInfoData buf;
+                get_opts_hint(&buf, ForeignServerRelationId, true);
+
+                ereport(ERROR,
+                        (errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
+                         errmsg("invalid option in dbname connstring \"%s\"",
+                                lopt->keyword),
+                         errhint("Valid options in this context are: %s",
+                                 buf.data)));
+            }
+        }
+        PQconninfoFree(lopts);
+    }
+}
+
+/*
+ * Put to 'buf' a hint with list of valid options for the object 'context'. If
+ * libpq_options is true, only libpq options are provided. 'buf' must point to
+ * existing StringInfoData struct when called.
+ */
+static void
+get_opts_hint(StringInfo buf, Oid context, bool libpq_options)
+{
+    PgFdwOption *opt;
+
+    initStringInfo(buf);
+    for (opt = postgres_fdw_options; opt->keyword; opt++)
+    {
+        if (context == opt->optcontext &&
+            (!libpq_options || opt->is_libpq_opt))
+            appendStringInfo(buf, "%s%s", (buf->len > 0) ? ", " : "",
+                             opt->keyword);
+    }
+}
+
+/*
  * Check whether the given option is one of the valid postgres_fdw options.
  * context is the Oid of the catalog holding the object the option is for.
  */
@@ -271,9 +322,11 @@ is_valid_option(const char *keyword, Oid context)
 
 /*
  * Check whether the given option is one of the valid libpq options.
+ * context is the Oid of the catalog holding the object the option is for;
+ * if it is InvalidOid, don't check the catalog.
  */
 static bool
-is_libpq_option(const char *keyword)
+is_libpq_option(const char *keyword, Oid context)
 {
     PgFdwOption *opt;
 
@@ -281,7 +334,8 @@ is_libpq_option(const char *keyword)
 
     for (opt = postgres_fdw_options; opt->keyword; opt++)
     {
-        if (opt->is_libpq_opt && strcmp(opt->keyword, keyword) == 0)
+        if (opt->is_libpq_opt && strcmp(opt->keyword, keyword) == 0 &&
+            (context == InvalidOid || context == opt->optcontext))
             return true;
     }
 
@@ -308,7 +362,7 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
     {
         DefElem    *d = (DefElem *) lfirst(lc);
 
-        if (is_libpq_option(d->defname))
+        if (is_libpq_option(d->defname, InvalidOid))
         {
             keywords[i] = d->defname;
             values[i] = defGetString(d);
-- 
2.11.0



--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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