Re: Docs: Encourage strong server verification with SCRAM

Greetings,

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> On Tue, May 23, 2023 at 05:02:50PM -0400, Stephen Frost wrote:
> > * Jacob Champion (jchampion(at)timescale(dot)com) wrote:
> >> As touched on in past threads, our SCRAM implementation is slightly
> >> nonstandard and doesn't always protect the entirety of the
> >> authentication handshake:
> >>
> >> - the username in the startup packet is not covered by the SCRAM
> >> crypto and can be tampered with if channel binding is not in effect,
> >> or by an attacker holding only the server's key
> >
> > Encouraging channel binding when using TLS certainly makes a lot of
> > sense, particularly in environments where the trust anchors are not
> > strongly managed (that is- if you trust the huge number of root
> > certificates that a system may have installed). Perhaps explicitly
> > encouraging users to *not* trust every root server installed on their
> > client for their PG connections would also be a good idea. We should
> > probably add language to that effect to this section.
>
> Currently the user name is not treated by the backend, like that in
> auth-scram.c:
>
> /*
> * Read username. Note: this is ignored. We use the username from the
> * startup message instead, still it is kept around if provided as it
> * proves to be useful for debugging purposes.
> */
> state->client_username = read_attr_value(&p, 'n');
>
> Could it make sense to cross-check that with the contents of the
> startup package instead, with or without channel binding?

Not without breaking things we support today and for what seems like an
unclear benefit given that we've got channel binding today (though
perhaps we need to make sure there's ways to force it on both sides to
be on and to encourage everyone to do that- which is what this change is
generally about, I think).

As I recall, the reason we do it the way we do is because the SASL spec
that SCRAM is implemented under requires the username to be utf8 encoded
while we support other encodings, and I don't think we want to break
non-utf8 usage.

Thanks,

Stephen