Обсуждение: Logging of matching pg_hba.conf entry during auth skips trust auth, potential security issue

Hi everyone,

This started as a conversation on Discord. Someone asked if Postgres
logs which line in pg_hba.conf matched against a certain login
attempt, and I said no. That's not quite right, as enabling
log_connections includes a line like this:

2023-08-15 13:26:03.159 PDT [692166] postgres@snip LOG: connection
authenticated: identity="postgres" method=md5
(/etc/postgresql/15/main/pg_hba.conf:107)

But I wasn't getting that output. I finally gave up and looked at the
code, where I found that this particular output is only generated by
the set_authn_id function. So if that function is never called,
there's no message saying which line from the pg_hba.conf file matched
a particular login.

The switch statement that decodes port->hba->auth_method ends by
simply setting status = STATUS_OK; with no supplementary output since
it never calls set_authn_id. So in theory, a malicious user could add
a trust line to pg_hba.conf and have unlimited unlogged access to the
database. Unless you happen to notice that the "connection
authenticated" line has disappeared, it would look like normal
activity.

Would it make sense to decouple the hba info from set_authn_id so that
it is always logged even when new auth methods get added in the
future? Or alternatively create a function call specifically for that
output so it can be produced from the trust case statement and
anywhere else that needs to tag the auth line. I personally would love
to see if someone got in through a trust line, ESPECIALLY if it isn't
supposed to be there. Like:

2023-08-15 13:26:03.159 PDT [692166] postgres@snip LOG: connection
authenticated: identity="postgres" method=trust
(/etc/postgresql/15/main/pg_hba.conf:1)

Perhaps I'm being too paranoid; It just seemed to be an odd omission.
Euler Taveira clued me into the initial patch which introduced the
pg_hba.conf tattling:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=9afffcb833d3c5e59a328a2af674fac7e7334fc1

I read through the discussion, and it doesn't seem like the security
aspect of simply hiding trust auths from the log was considered. Since
this is a new capability, I suppose nothing is really different from
say Postgres 14 and below. Still, it never hurts to ask.

Cheers!

-- 
Shaun Thomas
High Availability Architect
EDB
www.enterprisedb.com

On Tue, Aug 15, 2023 at 04:49:47PM -0500, Shaun Thomas wrote:
> The switch statement that decodes port->hba->auth_method ends by
> simply setting status = STATUS_OK; with no supplementary output since
> it never calls set_authn_id. So in theory, a malicious user could add
> a trust line to pg_hba.conf and have unlimited unlogged access to the
> database.

That's the same as giving access to your data folder.  Updating
pg_hba.conf is only the tip of the iceberg if one has write access to
your data folder.

> Unless you happen to notice that the "connection
> authenticated" line has disappeared, it would look like normal
> activity.

"trust" is not really an anthentication method because it does
nothing, it just authorizes things to go through, so you cannot
really say that it can have an authn ID (grep for "pedantic" around
here):
https://www.postgresql.org/message-id/c55788dd1773c521c862e8e0dddb367df51222be.camel@vmware.com

> Would it make sense to decouple the hba info from set_authn_id so that
> it is always logged even when new auth methods get added in the
> future? Or alternatively create a function call specifically for that
> output so it can be produced from the trust case statement and
> anywhere else that needs to tag the auth line. I personally would love
> to see if someone got in through a trust line, ESPECIALLY if it isn't
> supposed to be there. Like:
>
> 2023-08-15 13:26:03.159 PDT [692166] postgres@snip LOG: connection
> authenticated: identity="postgres" method=trust
> (/etc/postgresql/15/main/pg_hba.conf:1)

You mean outside the switch/case in ClientAuthentication().  Some
methods have an authn that is implementation-dependent, like ldap.  I
am not sure if switching the logic would lead to a gain, like calling
once set_authn_id() vs passing up a string to set in a single call of
set_authn_id().

> I read through the discussion, and it doesn't seem like the security
> aspect of simply hiding trust auths from the log was considered. Since
> this is a new capability, I suppose nothing is really different from
> say Postgres 14 and below. Still, it never hurts to ask.

The first message from Jacob outlines the idea behind the handling of
trust.  We could perhaps add one extra set_authn_id() for the uaTrust
case (not uaCert!) if that's more helpful.
--
Michael

On Tue, Aug 15, 2023 at 3:24 PM Michael Paquier <michael@paquier.xyz> wrote:
> The first message from Jacob outlines the idea behind the handling of
> trust.  We could perhaps add one extra set_authn_id() for the uaTrust
> case (not uaCert!) if that's more helpful.

I'm not super comfortable with saying "connection authenticated" when
it explicitly hasn't been (nor with switching the meaning of a
non-NULL SYSTEM_USER from "definitely authenticated somehow" to "who
knows; parse it apart to see"). But adding a log entry ("connection
trusted:" or some such?) with the pointer to the HBA line that made it
happen seems like a useful audit helper to me.

--Jacob

On Tue, Aug 15, 2023 at 03:39:10PM -0700, Jacob Champion wrote:
> I'm not super comfortable with saying "connection authenticated" when
> it explicitly hasn't been (nor with switching the meaning of a
> non-NULL SYSTEM_USER from "definitely authenticated somehow" to "who
> knows; parse it apart to see"). But adding a log entry ("connection
> trusted:" or some such?) with the pointer to the HBA line that made it
> happen seems like a useful audit helper to me.

Yeah, thanks for confirming.  That's also the impression I get after
reading again the original thread and the idea of how this code path
is handled in this commit.

We could do something like a LOG "connection: method=%s user=%s
(%s:%d)", without the "authenticated" and "identity" terms from
set_authn_id().  Just to drop an idea.
--
Michael

> We could do something like a LOG "connection: method=%s user=%s
> (%s:%d)", without the "authenticated" and "identity" terms from
> set_authn_id().  Just to drop an idea.

That would be my inclination as well. Heck, just slap a log message
right in the specific case statements that don't have actual auth as
defined by set_authn_id. This assumes anyone really cares about it
that much, of course. :D

-- 
Shaun

On Wed, Aug 16, 2023 at 6:27 AM Shaun Thomas
<shaun.thomas@enterprisedb.com> wrote:
>
> > We could do something like a LOG "connection: method=%s user=%s
> > (%s:%d)", without the "authenticated" and "identity" terms from
> > set_authn_id().  Just to drop an idea.
>
> That would be my inclination as well. Heck, just slap a log message
> right in the specific case statements that don't have actual auth as
> defined by set_authn_id. This assumes anyone really cares about it
> that much, of course. :D

Maybe something like the attached?

- I made the check more generic, rather than hardcoding it inside the
trust statement, because my OAuth proposal would add a method that
only calls set_authn_id() some of the time.
- I used the phrasing "connection not authenticated" in the hopes that
it's a bit more greppable than just "connection", especially in
combination with the existing "connection authenticated" lines.

(I'm reminded that we're reflecting an unauthenticated username as-is
into the logs, but I also don't think this makes things any worse than
they are today with the "authorized" lines.)

--Jacob

Greetings,

* Jacob Champion (jchampion@timescale.com) wrote:
> Maybe something like the attached?

> - I used the phrasing "connection not authenticated" in the hopes that
> it's a bit more greppable than just "connection", especially in
> combination with the existing "connection authenticated" lines.

That doesn't seem quite right ... admittedly, 'trust' isn't performing
authentication but there can certainly be an argument made that the
basic 'matched a line in pg_hba.conf' is a form of authentication, and
worse really, saying 'not authenticated' would seem to imply that we
didn't allow the connection when, really, we did, and that could be
confusing to someone.

Maybe 'connection allowed' instead..?

Thanks,

Stephen

On Thu, Aug 17, 2023 at 9:01 AM Stephen Frost <sfrost@snowman.net> wrote:
> That doesn't seem quite right ... admittedly, 'trust' isn't performing
> authentication but there can certainly be an argument made that the
> basic 'matched a line in pg_hba.conf' is a form of authentication

I'm not personally on board with this argument, but...

> and
> worse really, saying 'not authenticated' would seem to imply that we
> didn't allow the connection when, really, we did, and that could be
> confusing to someone.

...with this one, I agree.

> Maybe 'connection allowed' instead..?

Hm. It hasn't really been allowed yet, either. To illustrate what I mean:

    LOG:  connection received: host=[local]
    LOG:  connection allowed: user="jacob" method=trust
(/home/jacob/src/data/pg16/pg_hba.conf:117)
    LOG:  connection authorized: user=jacob database=postgres
application_name=psql

Maybe "unauthenticated connection:"? "connection without
authentication:"? "connection skipped authentication:"?

--Jacob

Greetings,

* Jacob Champion (jchampion@timescale.com) wrote:
> On Thu, Aug 17, 2023 at 9:01 AM Stephen Frost <sfrost@snowman.net> wrote:
> > Maybe 'connection allowed' instead..?
>
> Hm. It hasn't really been allowed yet, either. To illustrate what I mean:
>
>     LOG:  connection received: host=[local]
>     LOG:  connection allowed: user="jacob" method=trust
> (/home/jacob/src/data/pg16/pg_hba.conf:117)
>     LOG:  connection authorized: user=jacob database=postgres
> application_name=psql
>
> Maybe "unauthenticated connection:"? "connection without
> authentication:"? "connection skipped authentication:"?

Don't like 'skipped' but that feels closer.

How about 'connection bypassed authentication'?

Thanks,

Stephen

On Thu, Aug 17, 2023 at 9:46 AM Stephen Frost <sfrost@snowman.net> wrote:
> Don't like 'skipped' but that feels closer.
>
> How about 'connection bypassed authentication'?

Works for me; see v2.

Thanks!
--Jacob

On Thu, Aug 17, 2023 at 12:54 PM Jacob Champion <jchampion@timescale.com> wrote:
> On Thu, Aug 17, 2023 at 9:46 AM Stephen Frost <sfrost@snowman.net> wrote:
> > Don't like 'skipped' but that feels closer.
> >
> > How about 'connection bypassed authentication'?
>
> Works for me; see v2.

For what it's worth, my vote would be for "connection authenticated:
... method=trust". The only reason we're not doing that is because
there's some argument that trusting that the client is who they say
they are is not really authentication at all. But this seems silly,
because we put "trust" in the "METHOD" column of pg_hba.conf, so in
that case we already treat it as an authentication method. Also, any
such line in pg_hba.conf still matches against the supplied IP address
mask, which I suppose could be viewed as a form of authentication. Or
maybe not. But I wonder if we're just being too persnickety about
language here, in a way that maybe isn't consistent with our previous
practice.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Aug 17, 2023 at 03:29:28PM -0400, Stephen Frost wrote:
> On Thu, Aug 17, 2023 at 15:23 Robert Haas <robertmhaas@gmail.com> wrote:
>> For what it's worth, my vote would be for "connection authenticated:
>> ... method=trust".
>
> I don’t have any particular objection to this language and agree that it’s
> actually closer to how we talk about the trust auth method in our
> documentation.

After sleeping on it, I think that I'd just agree with Robert's point
to just use the same language as the message, while also agreeing with
the patch to not set MyClientConnectionInfo.authn_id in the uaTrust
case, only logging something under log_connections.

+        * No authentication was actually performed; this happens e.g. when the
+        * trust method is in use.

This comment should be reworded a bit, say "No authentication identity
was set; blah ..".

> Maybe if we decided to rework the documentation … or perhaps just ripped
> “trust” out entirely … but those are whole different things from what we
> are trying to accomplish here.

Not sure I see any point in doing that these days.
--
Michael

On Fri, Aug 18, 2023 at 08:49:16AM +0900, Michael Paquier wrote:
> After sleeping on it, I think that I'd just agree with Robert's point
> to just use the same language as the message, while also agreeing with
> the patch to not set MyClientConnectionInfo.authn_id in the uaTrust
> case, only logging something under log_connections.
>
> +        * No authentication was actually performed; this happens e.g. when the
> +        * trust method is in use.
>
> This comment should be reworded a bit, say "No authentication identity
> was set; blah ..".

Attached is a v3 to do these two things, with adjustments for two SSL
tests.  Any objections about it?

(Note: no backpatch)
--
Michael

On Sun, Aug 20, 2023 at 7:58 PM Michael Paquier <michael@paquier.xyz> wrote:
> Attached is a v3 to do these two things, with adjustments for two SSL
> tests.  Any objections about it?

+ * No authentication identity was set; this happens e.g. when the
+ * trust method is in use.  For audit purposes, log a breadcrumb to
+ * explain where in the HBA this happened.

Proposed rewrite: "Normally, if log_connections is set, the call to
set_authn_id will log the connection. However, if that function is
never called, perhaps because the trust method is in use, then we
handle the logging here instead."

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sun, Aug 20, 2023 at 4:58 PM Michael Paquier <michael@paquier.xyz> wrote:
> Attached is a v3 to do these two things, with adjustments for two SSL
> tests.  Any objections about it?

(Sorry for the long weekend delay.) No objections; you may want to
adjust the comment above the test block in t/001_password.pl, as well.

I will ask -- more as a rhetorical question than something to resolve
for this patch, since the topic is going to come back with a vengeance
for OAuth -- what purpose the consistency here is serving. If the OP
wants to notice when a connection that should be using strong
authentication is not, is it helpful to make that connection "look the
same" in the logs? I understand we've been carrying the language
"trust authentication method" for a long time, but is that really the
only hang-up, or would there be pushback if I tried to change that
too, sometime in the future?

Thanks,
--Jacob

On Mon, Aug 21, 2023 at 09:27:51AM -0400, Robert Haas wrote:
> + * No authentication identity was set; this happens e.g. when the
> + * trust method is in use.  For audit purposes, log a breadcrumb to
> + * explain where in the HBA this happened.
>
> Proposed rewrite: "Normally, if log_connections is set, the call to
> set_authn_id will log the connection. However, if that function is
> never called, perhaps because the trust method is in use, then we
> handle the logging here instead."

WFM.
--
Michael

On Mon, Aug 21, 2023 at 10:49:16AM -0700, Jacob Champion wrote:
> On Sun, Aug 20, 2023 at 4:58 PM Michael Paquier <michael@paquier.xyz> wrote:
> > Attached is a v3 to do these two things, with adjustments for two SSL
> > tests.  Any objections about it?
>
> (Sorry for the long weekend delay.) No objections; you may want to
> adjust the comment above the test block in t/001_password.pl, as well.

There are additionally two more comments in the SSL tests that could
be removed, I guess.  Here's a v4, with Robert's latest suggestion
added.

> I will ask -- more as a rhetorical question than something to resolve
> for this patch, since the topic is going to come back with a vengeance
> for OAuth -- what purpose the consistency here is serving. If the OP
> wants to notice when a connection that should be using strong
> authentication is not, is it helpful to make that connection "look the
> same" in the logs? I understand we've been carrying the language
> "trust authentication method" for a long time, but is that really the
> only hang-up, or would there be pushback if I tried to change that
> too, sometime in the future?

I am not sure that we need to change this historic term, TBH.  Perhaps
it would be shorter to just rip off the trust method from the tree
with a deprecation period but that's not something I'm much in favor
off either (I use it daily for my own stuff, as one example).
Another, more conservative approach may be to make it a developer-only
option and discourage more its use in the docs.
--
Michael

On Mon, Aug 21, 2023 at 4:22 PM Michael Paquier <michael@paquier.xyz> wrote:
> There are additionally two more comments in the SSL tests that could
> be removed, I guess.  Here's a v4, with Robert's latest suggestion
> added.

LGTM.

> I am not sure that we need to change this historic term, TBH.  Perhaps
> it would be shorter to just rip off the trust method from the tree
> with a deprecation period but that's not something I'm much in favor
> off either (I use it daily for my own stuff, as one example).
> Another, more conservative approach may be to make it a developer-only
> option and discourage more its use in the docs.

I don't think we should get rid of anonymous connections; there are
ways to securely authorize a client connection without ever
authenticating the entity at the other end. I'd just like the server
to call them what they are, because I think the distinction is
valuable for DBAs who are closely watching their systems.

--Jacob

On Mon, Aug 21, 2023 at 07:43:56PM -0400, Isaac Morland wrote:
> I hope we're not really considering removing the "trust" method. For
> testing and development purposes it's very handy — just tell the database,
> running in a VM, to allow all connections and just believe who they say
> they are from a client process running in the same or a different VM, with
> no production data anywhere in site and no connection to the real network.

For some benchmarking scenarios, it can actually be useful when
testing cases where new connections are spawned as it bypasses
entirely the authentication path, moving the bottlenecks to different
areas one may want to stress.
--
Michael

On Mon, Aug 21, 2023 at 04:44:33PM -0700, Jacob Champion wrote:
> On Mon, Aug 21, 2023 at 4:22 PM Michael Paquier <michael@paquier.xyz> wrote:
>> There are additionally two more comments in the SSL tests that could
>> be removed, I guess.  Here's a v4, with Robert's latest suggestion
>> added.
>
> LGTM.

Okay.  Hearing nothing else, I have gone ahead and applied v4.
--
Michael