[PATCH] pg_hba.conf error messages for logical replication connections
Hey, all,
When creating a logical replication connection that isn't allowed by the
current pg_hba.conf, the error message states that a "replication
connection" is not allowed.
This error message is confusing because although the user is trying to
create a replication connection and specified "replication=database" in
the connection string, the special "replication" pg_hba.conf keyword
does not apply. I believe the error message should just refer to a
regular connection and specify the database the user is trying to
connect to.
When connecting using "replication" in a connection string, the variable
am_walsender is set to true. When "replication=database" is specified,
the variable am_db_walsender is also set to true [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/postmaster/postmaster.c;h=7de27ee4e0171863faca2f24d62488b773a7636e;hb=HEAD#l2154.
When checking whether a pg_hba.conf rule matches in libpq/hba.c, we only
check for the "replication" keyword when am_walsender && !am_db_walsender [2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/hba.c;h=371dccb852fd5c0775c7ebd82b67de3f20dc70af;hb=HEAD#l640.
But then when reporting error messages in libpq/auth.c, only
am_walsender is used in the condition that chooses whether to specify
"replication connection" or "connection" to a specific database in the
error message [3]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/auth.c;h=545635f41a916c740aacd6a8b68672d10378b7ab;hb=HEAD#l420 [4]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/auth.c;h=545635f41a916c740aacd6a8b68672d10378b7ab;hb=HEAD#l487.
In this patch I have modified the conditions in libpq/auth.c to check
am_walsender && !am_db_walsender, as in hba.c. I have also added a
clarification in the documentation for pg_hba.conf.
The value `replication` specifies that the record matches if a physical replication connection is requested (note that replication - connections do not specify any particular database). + connections do not specify any particular database), but it does not + match logical replication connections that specify + `replication=database` and a `dbname` in their connection string.
Thanks,
Paul
Attachments:
pg_hba_conf_error_message_patch_v00.diffapplication/octet-stream; name=pg_hba_conf_error_message_patch_v00.diffDownload
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c4b9971a20..d8b31d94f3 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -230,7 +230,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
virtue of being a superuser.
The value <literal>replication</literal> specifies that the record
matches if a physical replication connection is requested (note that
- replication connections do not specify any particular database).
+ replication connections do not specify any particular database), but
+ it does not match logical replication connections that specify
+ <literal>replication=database</literal> and a <literal>dbname</literal>
+ in their connection string.
Otherwise, this is the name of
a specific <productname>PostgreSQL</productname> database.
Multiple database names can be supplied by separating them with
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 545635f41a..baa0712c0f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -417,7 +417,7 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
@@ -484,7 +484,7 @@ ClientAuthentication(Port *port)
gai_strerror(port->remote_hostname_errcode)) : \
0))
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
On Thu, Jan 28, 2021 at 1:51 PM Paul Martinez <paulmtz@google.com> wrote:
Hey, all,
When creating a logical replication connection that isn't allowed by the
current pg_hba.conf, the error message states that a "replication
connection" is not allowed.This error message is confusing because although the user is trying to
create a replication connection and specified "replication=database" in
the connection string, the special "replication" pg_hba.conf keyword
does not apply.
Right.
I believe the error message should just refer to a
regular connection and specify the database the user is trying to
connect to.
What exactly are you bothered about here? Is the database name not
present in the message your concern or the message uses 'replication'
but actually it doesn't relate to 'replication' specified in
pg_hba.conf your concern? I think with the current scheme one might
say that replication word in error message helps them to distinguish
logical replication connection error from a regular connection error.
I am not telling what you are proposing is wrong but just that the
current scheme of things might be helpful to some users. If you can
explain a bit how the current message mislead you and the proposed one
solves that confusion that would be better?
--
With Regards,
Amit Kapila.
On Thu, Jan 28, 2021 at 8:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
What exactly are you bothered about here? Is the database name not
present in the message your concern or the message uses 'replication'
but actually it doesn't relate to 'replication' specified in
pg_hba.conf your concern? I think with the current scheme one might
say that replication word in error message helps them to distinguish
logical replication connection error from a regular connection error.
I am not telling what you are proposing is wrong but just that the
current scheme of things might be helpful to some users. If you can
explain a bit how the current message misled you and the proposed one
solves that confusion that would be better?
My main confusion arose from conflating the word "replication" in the
error message with the "replication" keyword in pg_hba.conf.
In my case, I was actually confused because I was creating logical
replication connections that weren't getting rejected, despite a lack
of any "replication" rules in my pg_hba.conf. I had the faulty
assumption that replication connection requires "replication" keyword,
and my change to the documentation makes it explicit that logical
replication connections do not match the "replication" keyword.
I was digging through the code trying to understand why it was working,
and also making manual connections before I stumbled upon these error
messages.
The fact that the error message doesn't include the database name
definitely contributed to my confusion. It only mentions the word
"replication", and not a database name, and that reinforces the idea
that the "replication" keyword rule should apply, because it seems
"replication" has replaced the database name.
But overall, I would agree that the current messages aren't wrong,
and my fix could still cause confusion because now logical replication
connections won't be described as "replication" connections.
How about explicitly specifying physical vs. logical replication in the
error message, and also adding hints for clarifying the use of
the "replication" keyword in pg_hba.conf?
if physical replication
Error "pg_hba.conf rejects physical replication connection ..."
Hint "Physical replication connections only match pg_hba.conf rules
using the "replication" keyword"
else if logical replication
Error "pg_hba.conf rejects logical replication connection to database %s ..."
// Maybe add this?
Hint "Logical replication connections do not match pg_hba.conf rules
using the "replication" keyword"
else
Error "pg_hba.conf rejects connection to database %s ..."
If I did go with this approach, would it be better to have three
separate branches, or to just introduce another variable for the
connection type? I'm not sure what is optimal for translation. (If both
types of replication connections get hints then probably three branches
is better.)
const char *connection_type;
connection_type =
am_db_walsender ? _("logical replication connection") :
am_walsender ? _("physical replication connection") :
_("connection")
- Paul
On Sat, Jan 30, 2021 at 12:24 AM Paul Martinez <paulmtz@google.com> wrote:
On Thu, Jan 28, 2021 at 8:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
What exactly are you bothered about here? Is the database name not
present in the message your concern or the message uses 'replication'
but actually it doesn't relate to 'replication' specified in
pg_hba.conf your concern? I think with the current scheme one might
say that replication word in error message helps them to distinguish
logical replication connection error from a regular connection error.
I am not telling what you are proposing is wrong but just that the
current scheme of things might be helpful to some users. If you can
explain a bit how the current message misled you and the proposed one
solves that confusion that would be better?My main confusion arose from conflating the word "replication" in the
error message with the "replication" keyword in pg_hba.conf.In my case, I was actually confused because I was creating logical
replication connections that weren't getting rejected, despite a lack
of any "replication" rules in my pg_hba.conf. I had the faulty
assumption that replication connection requires "replication" keyword,
and my change to the documentation makes it explicit that logical
replication connections do not match the "replication" keyword.
I think it is good to be more explicit in the documentation but we
already mention "physical replication connection" in the sentence. So
it might be better that we add a separate sentence related to logical
replication.
I was digging through the code trying to understand why it was working,
and also making manual connections before I stumbled upon these error
messages.The fact that the error message doesn't include the database name
definitely contributed to my confusion. It only mentions the word
"replication", and not a database name, and that reinforces the idea
that the "replication" keyword rule should apply, because it seems
"replication" has replaced the database name.But overall, I would agree that the current messages aren't wrong,
and my fix could still cause confusion because now logical replication
connections won't be described as "replication" connections.How about explicitly specifying physical vs. logical replication in the
error message, and also adding hints for clarifying the use of
the "replication" keyword in pg_hba.conf?
Yeah, hints or more details might improve the situation but I am not
sure we want to add more branching here. Can we write something
similar to HOSTNAME_LOOKUP_DETAIL for hints? Also, I think what you
are proposing to write is more of a errdetail kind of message. See
more error routines in the docs [1]https://www.postgresql.org/docs/devel/error-message-reporting.html.
[1]: https://www.postgresql.org/docs/devel/error-message-reporting.html
--
With Regards,
Amit Kapila.
On Fri, Jan 29, 2021 at 8:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Yeah, hints or more details might improve the situation but I am not
sure we want to add more branching here. Can we write something
similar to HOSTNAME_LOOKUP_DETAIL for hints? Also, I think what you
are proposing to write is more of a errdetail kind of message. See
more error routines in the docs [1].
Alright, I've updated both sets of error messages to use something like
HOSTNAME_LOOKUP_DETAIL, both for the error message itself, and for the
extra detail message about the replication keyword. Since now we specify
both an errdetail (sent to the client) and an errdetail_log (sent to the
log), I renamed HOSTNAME_LOOKUP_DETAIL to HOSTNAME_LOOKUP_DETAIL_LOG.
- Paul
Attachments:
pg_hba_conf_error_message_patch_v01.diffapplication/octet-stream; name=pg_hba_conf_error_message_patch_v01.diffDownload
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c4b9971a20..2a859caac6 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -230,7 +230,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
virtue of being a superuser.
The value <literal>replication</literal> specifies that the record
matches if a physical replication connection is requested (note that
- replication connections do not specify any particular database).
+ replication connections do not specify any particular database);
+ it does not match logical replication connections, which specify
+ <literal>replication=database</literal> and a <literal>dbname</literal>
+ in their connection string.
Otherwise, this is the name of
a specific <productname>PostgreSQL</productname> database.
Multiple database names can be supplied by separating them with
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 545635f41a..c2f38702a5 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -382,6 +382,18 @@ ClientAuthentication(Port *port)
errmsg("connection requires a valid client certificate")));
}
+ /*
+ * The "replication" keyword in pg_hba.conf only applies to physical
+ * replication connections and not logical replication connections,
+ * so we'll just remind the user of that in case they've gotten confused.
+ */
+#define PG_HBA_REPLICATION_RULE_DETAIL(am_walsender, am_db_walsender) \
+ (am_walsender ? \
+ (am_db_walsender ? \
+ errhint("Note logical replication connections do not match pg_hba.conf rules using the \"replication\" keyword") : \
+ errhint("Note physical replication connections only match pg_hba.conf rules using the \"replication\" keyword")) : \
+ 0)
+
/*
* Now proceed to do the actual authentication check
*/
@@ -417,21 +429,22 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
- if (am_walsender)
- ereport(FATAL,
- (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
- /* translator: last %s describes encryption state */
- errmsg("pg_hba.conf rejects replication connection for host \"%s\", user \"%s\", %s",
- hostinfo, port->user_name,
- encryption_state)));
- else
- ereport(FATAL,
- (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
- /* translator: last %s describes encryption state */
- errmsg("pg_hba.conf rejects connection for host \"%s\", user \"%s\", database \"%s\", %s",
- hostinfo, port->user_name,
- port->database_name,
- encryption_state)));
+#define REJECT_ERROR_MESSAGE(am_walsender, am_db_walsender, hostinfo, port, encryption_state) \
+ (am_walsender ? \
+ (am_db_walsender ? \
+ errmsg("pg_hba.conf rejects logical replication connection for host \"%s\", user \"%s\", database \"%s\", %s", \
+ hostinfo, port->user_name, port->database_name, encryption_state) : \
+ errmsg("pg_hba.conf rejects physical replication connection for host \"%s\", user \"%s\", %s", \
+ hostinfo, port->user_name, encryption_state)) : \
+ errmsg("pg_hba.conf rejects for host \"%s\", user \"%s\", database \"%s\", %s", \
+ hostinfo, port->user_name, port->database_name, encryption_state))
+
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ /* translator: last %s describes encryption state */
+ REJECT_ERROR_MESSAGE(am_walsender, am_db_walsender,
+ hostinfo, port, encryption_state),
+ PG_HBA_REPLICATION_RULE_DETAIL(am_walsender, am_db_walsender)));
break;
}
@@ -463,7 +476,17 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
-#define HOSTNAME_LOOKUP_DETAIL(port) \
+#define IMPLICIT_REJECT_ERROR_MESSAGE(am_walsender, am_db_walsender, hostinfo, port, encryption_state) \
+ (am_walsender ? \
+ (am_db_walsender ? \
+ errmsg("no pg_hba.conf entry for logical replication connection from host \"%s\", user \"%s\", database \"%s\", %s" : \
+ hostinfo, port->user_name, port->database_name, encryption_state) : \
+ errmsg("no pg_hba.conf entry for physical replication connection from host \"%s\", user \"%s\", %s", \
+ hostinfo, port->user_name, encryption_state)) : \
+ errmsg("no pg_hba.conf entry for host \"%s\", user \"%s\", database \"%s\", %s" \
+ hostinfo, port->user_name, port->database_name, encryption_state))
+
+#define HOSTNAME_LOOKUP_DETAIL_LOG(port) \
(port->remote_hostname ? \
(port->remote_hostname_resolv == +1 ? \
errdetail_log("Client IP address resolved to \"%s\", forward lookup matches.", \
@@ -484,23 +507,13 @@ ClientAuthentication(Port *port)
gai_strerror(port->remote_hostname_errcode)) : \
0))
- if (am_walsender)
- ereport(FATAL,
- (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
- /* translator: last %s describes encryption state */
- errmsg("no pg_hba.conf entry for replication connection from host \"%s\", user \"%s\", %s",
- hostinfo, port->user_name,
- encryption_state),
- HOSTNAME_LOOKUP_DETAIL(port)));
- else
- ereport(FATAL,
- (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
- /* translator: last %s describes encryption state */
- errmsg("no pg_hba.conf entry for host \"%s\", user \"%s\", database \"%s\", %s",
- hostinfo, port->user_name,
- port->database_name,
- encryption_state),
- HOSTNAME_LOOKUP_DETAIL(port)));
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ /* translator: last %s describes encryption state */
+ IMPLICIT_REJECT_ERROR_MESSAGE(am_walsender, am_db_walsender,
+ hostinfo, port, encryption_state)
+ HOSTNAME_LOOKUP_DETAIL_LOG(port),
+ PG_HBA_REPLICATION_RULE_DETAIL(am_walsender, am_db_walsender));
break;
}
On Tue, Feb 2, 2021 at 1:43 AM Paul Martinez <paulmtz@google.com> wrote:
On Fri, Jan 29, 2021 at 8:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Yeah, hints or more details might improve the situation but I am not
sure we want to add more branching here. Can we write something
similar to HOSTNAME_LOOKUP_DETAIL for hints? Also, I think what you
are proposing to write is more of a errdetail kind of message. See
more error routines in the docs [1].Alright, I've updated both sets of error messages to use something like
HOSTNAME_LOOKUP_DETAIL, both for the error message itself, and for the
extra detail message about the replication keyword. Since now we specify
both an errdetail (sent to the client) and an errdetail_log (sent to the
log), I renamed HOSTNAME_LOOKUP_DETAIL to HOSTNAME_LOOKUP_DETAIL_LOG.
I don't think we need to update the error messages, it makes the code
a bit difficult to parse without much benefit. How about just adding
errdetail? See attached and let me know what you think?
--
With Regards,
Amit Kapila.
Attachments:
pg_hba_conf_error_message_patch_v02.patchapplication/octet-stream; name=pg_hba_conf_error_message_patch_v02.patchDownload
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c4b9971a20..2a859caac6 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -230,7 +230,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
virtue of being a superuser.
The value <literal>replication</literal> specifies that the record
matches if a physical replication connection is requested (note that
- replication connections do not specify any particular database).
+ replication connections do not specify any particular database);
+ it does not match logical replication connections, which specify
+ <literal>replication=database</literal> and a <literal>dbname</literal>
+ in their connection string.
Otherwise, this is the name of
a specific <productname>PostgreSQL</productname> database.
Multiple database names can be supplied by separating them with
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 545635f41a..2314adb699 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -417,7 +417,7 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
@@ -431,7 +431,9 @@ ClientAuthentication(Port *port)
errmsg("pg_hba.conf rejects connection for host \"%s\", user \"%s\", database \"%s\", %s",
hostinfo, port->user_name,
port->database_name,
- encryption_state)));
+ encryption_state),
+ am_db_walsender ?
+ errdetail("Logical replication connections do not match pg_hba.conf rules using the \"replication\" keyword.") : 0));
break;
}
@@ -463,7 +465,7 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
-#define HOSTNAME_LOOKUP_DETAIL(port) \
+#define HOSTNAME_LOOKUP_DETAIL_LOG(port) \
(port->remote_hostname ? \
(port->remote_hostname_resolv == +1 ? \
errdetail_log("Client IP address resolved to \"%s\", forward lookup matches.", \
@@ -484,14 +486,14 @@ ClientAuthentication(Port *port)
gai_strerror(port->remote_hostname_errcode)) : \
0))
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
errmsg("no pg_hba.conf entry for replication connection from host \"%s\", user \"%s\", %s",
hostinfo, port->user_name,
encryption_state),
- HOSTNAME_LOOKUP_DETAIL(port)));
+ HOSTNAME_LOOKUP_DETAIL_LOG(port)));
else
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
@@ -500,7 +502,9 @@ ClientAuthentication(Port *port)
hostinfo, port->user_name,
port->database_name,
encryption_state),
- HOSTNAME_LOOKUP_DETAIL(port)));
+ am_db_walsender ?
+ errdetail("Logical replication connections do not match pg_hba.conf rules using the \"replication\" keyword.") : 0,
+ HOSTNAME_LOOKUP_DETAIL_LOG(port)));
break;
}
On Tue, Feb 16, 2021 at 2:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
I don't think we need to update the error messages, it makes the code
a bit difficult to parse without much benefit. How about just adding
errdetail? See attached and let me know what you think?
Yeah, I think that looks good. Thanks!
- Paul
On Tue, Feb 16, 2021 at 10:40 PM Paul Martinez <paulmtz@google.com> wrote:
On Tue, Feb 16, 2021 at 2:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
I don't think we need to update the error messages, it makes the code
a bit difficult to parse without much benefit. How about just adding
errdetail? See attached and let me know what you think?Yeah, I think that looks good. Thanks!
Okay, I think normally it might not be a good idea to expose
additional information about authentication failure especially about
pg_hba so as to reduce the risk of exposing information to potential
attackers but in this case, it appears to me that it would be helpful
for users. Just in case someone else has any opinion, for logical
replication connection failures, the messages before and after fix
would be:
Before fix
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
replication connection for host "::1", user "KapilaAm", no encryption
After fix error:
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
connection for host "::1", user "KapilaAm", database "postgres", no
encryption
DETAIL: Logical replication connections do not match pg_hba.conf
rules using the "replication" keyword.
Does anyone see a problem with the DETAIL message or the change of
error message (database name appears in the new message) in this case?
Attached patch with the updated commit message.
--
With Regards,
Amit Kapila.
Attachments:
pg_hba_conf_error_message_patch_v03.patchapplication/octet-stream; name=pg_hba_conf_error_message_patch_v03.patchDownload
From 7d2bfe07eb32e33ed61ee64ac5af774742f5be4f Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Wed, 17 Feb 2021 16:21:12 +0530
Subject: [PATCH v3] Improve the error message details for authentication
failures.
The authentication failure error messages weren't distinguishing whether
it is a physical replication or logical replication connection failure and
gives unclear information on what led to failure in case of logical
replication connection failures.
Add the errdetail to give appropriate information.
Author: Paul Martinez, with changes by me
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CACqFVBYahrAi2OPdJfUA3YCvn3QMzzxZdw0ibSJ8wouWeDtiyQ@mail.gmail.com
---
doc/src/sgml/client-auth.sgml | 5 ++++-
src/backend/libpq/auth.c | 16 ++++++++++------
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c4b9971a20..2a859caac6 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -230,7 +230,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
virtue of being a superuser.
The value <literal>replication</literal> specifies that the record
matches if a physical replication connection is requested (note that
- replication connections do not specify any particular database).
+ replication connections do not specify any particular database);
+ it does not match logical replication connections, which specify
+ <literal>replication=database</literal> and a <literal>dbname</literal>
+ in their connection string.
Otherwise, this is the name of
a specific <productname>PostgreSQL</productname> database.
Multiple database names can be supplied by separating them with
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 545635f41a..2314adb699 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -417,7 +417,7 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
@@ -431,7 +431,9 @@ ClientAuthentication(Port *port)
errmsg("pg_hba.conf rejects connection for host \"%s\", user \"%s\", database \"%s\", %s",
hostinfo, port->user_name,
port->database_name,
- encryption_state)));
+ encryption_state),
+ am_db_walsender ?
+ errdetail("Logical replication connections do not match pg_hba.conf rules using the \"replication\" keyword.") : 0));
break;
}
@@ -463,7 +465,7 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
-#define HOSTNAME_LOOKUP_DETAIL(port) \
+#define HOSTNAME_LOOKUP_DETAIL_LOG(port) \
(port->remote_hostname ? \
(port->remote_hostname_resolv == +1 ? \
errdetail_log("Client IP address resolved to \"%s\", forward lookup matches.", \
@@ -484,14 +486,14 @@ ClientAuthentication(Port *port)
gai_strerror(port->remote_hostname_errcode)) : \
0))
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
errmsg("no pg_hba.conf entry for replication connection from host \"%s\", user \"%s\", %s",
hostinfo, port->user_name,
encryption_state),
- HOSTNAME_LOOKUP_DETAIL(port)));
+ HOSTNAME_LOOKUP_DETAIL_LOG(port)));
else
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
@@ -500,7 +502,9 @@ ClientAuthentication(Port *port)
hostinfo, port->user_name,
port->database_name,
encryption_state),
- HOSTNAME_LOOKUP_DETAIL(port)));
+ am_db_walsender ?
+ errdetail("Logical replication connections do not match pg_hba.conf rules using the \"replication\" keyword.") : 0,
+ HOSTNAME_LOOKUP_DETAIL_LOG(port)));
break;
}
--
2.28.0.windows.1
On Wed, Feb 17, 2021, at 8:01 AM, Amit Kapila wrote:
Before fix
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
replication connection for host "::1", user "KapilaAm", no encryptionAfter fix error:
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
connection for host "::1", user "KapilaAm", database "postgres", no
encryption
DETAIL: Logical replication connections do not match pg_hba.conf
rules using the "replication" keyword.
The new message is certainly an improvement because it provides an additional
component (database name) that could be used to figure out what's wrong with
the logical replication connection. However, I wouldn't like to add a DETAIL
message for something that could be easily inspected in the pg_hba.conf. The
old message leaves a doubt about which rule was used (absence of database name)
but the new message makes this very clear. IMO with this new message, we don't
need a DETAIL message. If in doubt, user can always read that documentation
(the new sentence clarifies the "replication" usage for logical replication
connections).
Regarding the documentation, I think the new sentence a bit confusing. The
modified sentence is providing detailed information about "replication" in the
database field then you start mentioned "replication=database". Even though it
is related to the connection string, it could confuse the reader for a second.
I would say "it does not match logical replication connections". It seems
sufficient to inform the reader that he/she cannot use records with
"replication" to match logical replication connections.
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Thu, Feb 18, 2021 at 5:59 AM Euler Taveira <euler@eulerto.com> wrote:
On Wed, Feb 17, 2021, at 8:01 AM, Amit Kapila wrote:
Before fix
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
replication connection for host "::1", user "KapilaAm", no encryptionAfter fix error:
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
connection for host "::1", user "KapilaAm", database "postgres", no
encryption
DETAIL: Logical replication connections do not match pg_hba.conf
rules using the "replication" keyword.The new message is certainly an improvement because it provides an additional
component (database name) that could be used to figure out what's wrong with
the logical replication connection. However, I wouldn't like to add a DETAIL
message for something that could be easily inspected in the pg_hba.conf. The
old message leaves a doubt about which rule was used (absence of database name)
but the new message makes this very clear. IMO with this new message, we don't
need a DETAIL message.
You have a point. Paul, do you have any thoughts on this?
If in doubt, user can always read that documentation
(the new sentence clarifies the "replication" usage for logical replication
connections).Regarding the documentation, I think the new sentence a bit confusing. The
modified sentence is providing detailed information about "replication" in the
database field then you start mentioned "replication=database". Even though it
is related to the connection string, it could confuse the reader for a second.
I would say "it does not match logical replication connections". It seems
sufficient to inform the reader that he/she cannot use records with
"replication" to match logical replication connections.
Fair point.
--
With Regards,
Amit Kapila.
On Thu, Feb 18, 2021 at 2:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Feb 18, 2021 at 5:59 AM Euler Taveira <euler@eulerto.com> wrote:
On Wed, Feb 17, 2021, at 8:01 AM, Amit Kapila wrote:
Before fix
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
replication connection for host "::1", user "KapilaAm", no encryptionAfter fix error:
ERROR: could not connect to the publisher: connection to server at
"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejects
connection for host "::1", user "KapilaAm", database "postgres", no
encryption
DETAIL: Logical replication connections do not match pg_hba.conf
rules using the "replication" keyword.The new message is certainly an improvement because it provides an additional
component (database name) that could be used to figure out what's wrong with
the logical replication connection. However, I wouldn't like to add a DETAIL
message for something that could be easily inspected in the pg_hba.conf. The
old message leaves a doubt about which rule was used (absence of database name)
but the new message makes this very clear. IMO with this new message, we don't
need a DETAIL message.You have a point. Paul, do you have any thoughts on this?
Changed as per suggestion.
If in doubt, user can always read that documentation
(the new sentence clarifies the "replication" usage for logical replication
connections).Regarding the documentation, I think the new sentence a bit confusing. The
modified sentence is providing detailed information about "replication" in the
database field then you start mentioned "replication=database". Even though it
is related to the connection string, it could confuse the reader for a second.
I would say "it does not match logical replication connections". It seems
sufficient to inform the reader that he/she cannot use records with
"replication" to match logical replication connections.Fair point.
I have used a bit of different wording here to make things clear.
Let me know what you think of the attached?
--
With Regards,
Amit Kapila.
Attachments:
pg_hba_conf_error_message_patch_v04.patchapplication/octet-stream; name=pg_hba_conf_error_message_patch_v04.patchDownload
From 4e6926b6bb2c0766d426debfc57a8c95c1a67d0e Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Sat, 20 Feb 2021 15:30:49 +0530
Subject: [PATCH v4] Change the error message for logical replication
authentication failures.
The authentication failure error message wasn't distinguishing whether
it is a physical replication or logical replication connection failure and
was giving incomplete information on what led to failure in case of logical
replication connection.
Author: Paul Martinez and Amit Kapila
Reviewed-by: Euler Taveira and Amit Kapila
Discussion: https://postgr.es/m/CACqFVBYahrAi2OPdJfUA3YCvn3QMzzxZdw0ibSJ8wouWeDtiyQ@mail.gmail.com
---
doc/src/sgml/client-auth.sgml | 6 ++++--
src/backend/libpq/auth.c | 4 ++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c4b9971a20..b420486a0a 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -229,8 +229,10 @@ hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceabl
members of the role, directly or indirectly, and not just by
virtue of being a superuser.
The value <literal>replication</literal> specifies that the record
- matches if a physical replication connection is requested (note that
- replication connections do not specify any particular database).
+ matches if a physical replication connection is requested, however, it
+ doesn't match with logical replication connections. Note that physical
+ replication connections do not specify any particular database whereas
+ logical replication connections do specify it.
Otherwise, this is the name of
a specific <productname>PostgreSQL</productname> database.
Multiple database names can be supplied by separating them with
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 545635f41a..baa0712c0f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -417,7 +417,7 @@ ClientAuthentication(Port *port)
#endif
_("no encryption");
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
@@ -484,7 +484,7 @@ ClientAuthentication(Port *port)
gai_strerror(port->remote_hostname_errcode)) : \
0))
- if (am_walsender)
+ if (am_walsender && !am_db_walsender)
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
/* translator: last %s describes encryption state */
--
2.28.0.windows.1
On Sat, Feb 20, 2021, at 7:33 AM, Amit Kapila wrote:
I have used a bit of different wording here to make things clear.
Let me know what you think of the attached?
WFM.
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Mon, Feb 22, 2021 at 6:08 PM Euler Taveira <euler@eulerto.com> wrote:
On Sat, Feb 20, 2021, at 7:33 AM, Amit Kapila wrote:
I have used a bit of different wording here to make things clear.
Let me know what you think of the attached?
WFM.
Thanks, Pushed!
--
With Regards,
Amit Kapila.