Обсуждение: [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].

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].

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] [4].

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

[1]:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/postmaster/postmaster.c;h=7de27ee4e0171863faca2f24d62488b773a7636e;hb=HEAD#l2154

[2]:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/hba.c;h=371dccb852fd5c0775c7ebd82b67de3f20dc70af;hb=HEAD#l640

[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

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].

[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

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.

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.

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.

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 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.
>
> 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.