[PoC] Let libpq reject unexpected authentication requests

From: Jacob Champion <pchampion(at)vmware(dot)com>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: [PoC] Let libpq reject unexpected authentication requests
Date: 2022-03-05 01:04:05
Message-ID: 9e5a8ccddb8355ea9fa4b75a1e3a9edc88a70cd3.camel@vmware.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello,

TL;DR: this patch lets you specify exactly one authentication method in
the connection string, and libpq will fail the connection if the server
doesn't use that method.

(This is not intended for PG15. I'm generally anxious about posting
experimental work during a commitfest, but there's been enough
conversation about this topic recently that I felt like it'd be useful
to have code to point to.)

== Proposal and Alternatives ==

$subject keeps coming up in threads. I think my first introduction to
it was after the TLS injection CVE, and then it came up again in the
pluggable auth thread. It's hard for me to generalize based on "sound
bites", but among the proposals I've seen are

1. reject plaintext passwords
2. reject a configurable list of unacceptable methods
3. allow client and server to negotiate a method

All of them seem to have merit. I'm personally motivated by the case
brought up by the CVE: if I'm expecting client certificate
authentication, it's not acceptable for the server to extract _any_
information about passwords from my system, whether they're plaintext,
hashed, or SCRAM-protected. So I chose not to implement option 1. And
option 3 looked like a lot of work to take on in an experiment without
a clear consensus.

Here is my take on option 2, then: you get to choose exactly one method
that the client will accept. If you want to use client certificates,
use require_auth=cert. If you want to force SCRAM, use
require_auth=scram-sha-256. If the server asks for something different,
libpq will fail. If the server tries to get away without asking you for
authentication, libpq will fail. There is no negotiation.

== Why Force Authn? ==

I think my decision to fail if the server doesn't authenticate might be
controversial. It doesn't provide additional protection against active
attack unless you're using a mutual authentication method (SCRAM),
because you can't prove that the server actually did anything with its
side of the handshake. But this approach grew on me for a few reasons:

- When using SCRAM, it allows the client to force a server to
authenticate itself, even when channel bindings aren't being used. (I
really think it's weird that we let the server get away with that
today.)

- In cases where you want to ensure that your actions are logged for
later audit, you can be reasonably sure that you're connecting to a
database that hasn't been configured with a `trust` setting.

- For cert authentication, it ensures that the server asked for a cert
and that you actually sent one. This is more forward-looking; today, we
always ask for a certificate from the client even if we don't use it.
But if implicit TLS takes off, I'd expect to see more middleware, with
more potential for misconfiguration.

== General Thoughts ==

I like that this approach fits nicely into the existing code. The
majority of the patch just beefs up check_expected_areq(). The new flag
that tracks whether or not we've authenticated is scattered around more
than I would like, but I'm hopeful that some of the pluggable auth
conversations will lead to nice refactoring opportunities for those
internal helpers.

There's currently no way to prohibit client certificates from being
sent. If my use case is "servers shouldn't be able to extract password
info if the client expects certificates", then someone else may very
well say "servers shouldn't be able to extract my client certificate if
I wanted to use SCRAM". Likewise, this feature won't prevent a GSS
authenticated channel -- but we do have gssencmode=disable, so I'm less
concerned there.

I made the assumption that a GSS encrypted channel authenticates both
parties to each other, but I don't actually know what guarantees are
made there. I have not tested SSPI.

I'm not a fan of the multiple spellings of "password" ("ldap", "pam",
et al). My initial thought was that it'd be nice to match the client
setting to the HBA setting in the server. But I don't think it's really
all that helpful; worst-case, it pretends to do something it can't,
since the client can't determine what the plaintext password is
actually used for on the backend. And if someone devises (say) a SASL
scheme for proxied LDAP authentication, that spelling becomes obsolete.

Speaking of obsolete, the current implementation assumes that any SASL
exchange must be for SCRAM. That won't be anywhere close to future-
proof.

Thanks,
--Jacob

Attachment Content-Type Size
0001-libpq-let-client-reject-unexpected-auth-methods.patch text/x-patch 18.8 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2022-03-05 01:12:09 Re: Add the replication origin name and commit-LSN to logical replication worker errcontext
Previous Message Thomas Munro 2022-03-05 00:21:26 Re: Regression tests failures on Windows Server 2019 - on master at commit # d816f366b