Re: BUG #18379: LDAP bind password exposed

On Thu, Mar 7, 2024 at 1:34 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings,
>
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > While I agree that users should take steps to secure their log files,
> > > I'd argue that it's best practice to avoid dumping sensitive data into
> > > log files, which it seems like it would be in this case. I'm not
> > > suggesting that this is bug-worthy or that we should go to excessive
> > > lengths to try and prevent every such case, but if someone showed up
> > > with a reasonable patch to replace the sensitive information in a
> pg_hba
> > > line with ****, I would be on the side of supporting that.
> >
> > I dunno, I think it would mostly serve to set false expectations.
>
> I appreciate that concern. I don't think it will though.
>
> > We've repeatedly rejected requests to scrub the log of passwords
> > found in CREATE/ALTER USER commands, for example. I think some
> > of the same issues that led to that conclusion would apply here,
> > notably that a syntax error could lead to failing to recognize
> > at all that some substring is a password. (A visibly erroneous
> > pg_hba line would not get quoted in the specific context the OP
> > complains of, but I'm pretty sure we'd print it while logging
> > the configuration reload failure.)
>
> While this may not be popular, I'd be in support of doing away with
> support for cleartext passwords being accepted through CREATE/ALTER USER
> commands, therefore eliminating this issue entirely. As discussed on
> this very thread, passing passwords in the clear from the client to the
> server, in any context, goes against best practices, which is why PG's
> ldap auth method is discouraged as it does exactly that.
>
> Doing this would, ideally at least, result in a lot more use of libpq's
> PQchangePassword() and \password in psql, which would further reduce the
> chances of a syntax error ending up dumping sensitive data into the log
> (the server's SCRAM password authenticator would still be considered
> sensitive and worthy of hiding, imv, but I wouldn't push as hard on that
> as it's clearly less of a risk than the user's cleartext password).
>
> Sure, mistakes could still happen and we couldn't do anything about it
> (a user might *try* to pass in a cleartext password via ALTER USER *and*
> have a syntax mistake), but now we're really looking at low probability
> issues.
>
>

All the philosophical stuff aside, I think this could be avoided by the
deployment of an ldap_password_hook, which is designed to allow mangling of
the bindpasswd before it's passed to the ldap server. There's a trivial
example in src/test/modules/ldap_password_func

It's available in release 16, so the OP would need to upgrade.

cheers

andrew