Обсуждение: [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.
Вложения
On Wed, Feb 17, 2021, at 8:01 AM, Amit Kapila wrote:
Before fixERROR: could not connect to the publisher: connection to server at"localhost" (::1), port 5432 failed: FATAL: pg_hba.conf rejectsreplication 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 rejectsconnection for host "::1", user "KapilaAm", database "postgres", noencryptionDETAIL: Logical replication connections do not match pg_hba.confrules 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.
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.