Re: Experiments with Postgres and SSL

Committed this. Thank you to everyone involved!

On 04/04/2024 14:08, Matthias van de Meent wrote:
> Patch 0003:
>
> The read size in secure_raw_read is capped to port->raw_buf_remaining
> if the raw buf has any data. While the user will probably call into
> this function again, I think that's a waste of cycles.

Hmm, yeah, I suppose we could read more data in the same call. It seems
simpler not to. The case that "raw_buf_remaining > 0" is a very rare.

> pq_buffer_has_data now doesn't have any protections against
> desynchronized state between PqRecvLength and PqRecvPointer. An
> Assert(PqRecvLength >= PqRecvPointer) to that value would be
> appreciated.

Added.

> 0006:
>
> In CONNECTION_FAILED, we use connection_failed() to select whether we
> need a new connection or stop trying altogether, but that function's
> description states:
>
>> + * Out-of-line portion of the CONNECTION_FAILED() macro
>> + *
>> + * Returns true, if we should retry the connection with different encryption method.
>
> Which to me reads like we should reuse the connection, and try a
> different method on that same connection. Maybe we can improve the
> wording to something like
> + * Returns true, if we should reconnect with a different encryption method.
> to make the reconnect part more clear.

Changed to "Returns true, if we should reconnect and retry with a
different encryption method".

> In select_next_encryption_method, there are several copies of this pattern:
>
> if ((remaining_methods & ENC_METHOD) != 0)
> {
> conn->current_enc_method = ENC_METHOD;
> return true;
> }
>
> I think a helper macro would reduce the verbosity of the scaffolding,
> like in the attached SELECT_NEXT_METHOD.diff.txt.

Applied.

In addition to the above, I made heavy changes to the tests. I wanted to
test not just the outcome (SSL, GSSAPI, plaintext, or fail), but also
the steps and reconnections needed to get there. To facilitate that, I
rewrote how the expected outcome was represented in the test script. It
now uses a table-driven approach, with a line for each test iteration,
ie. for each different combination of options that are tested.

I then added some more logging, so that whenever the server receives an
SSLRequest or GSSENCRequest packet, it logs a line. That's controlled by
a new not-in-sample GUC ("trace_connection_negotiation"), intended only
for the test and debugging. The test scrapes the log for the lines that
it prints, and the expected output includes a compact trace of expected
events. For example, the expected output for "user=testuser
gssencmode=prefer sslmode=prefer sslnegotiation=direct", when GSS and
SSL are both disabled in the server, looks like this:

# USER GSSENCMODE SSLMODE SSLNEGOTIATION EVENTS -> OUTCOME
testuser prefer prefer direct connect,
directsslreject, reconnect, sslreject, authok -> plain

That means, we expect libpq to first try direct SSL, which is rejected
by the server. It should then reconnect and attempt traditional
negotiated SSL, which is also rejected. Finally, it should try plaintext
authentication, without reconnecting, which succeeds.

That actually revealed a couple of slightly bogus behaviors with the
current code. Here's one example:

# XXX: libpq retries the connection unnecessarily in this case:
nogssuser require allow connect, gssaccept, authfail,
reconnect, gssaccept, authfail -> fail

That means, with "gssencmode=require sslmode=allow", if the server
accepts the GSS encryption but refuses the connection at authentication,
libpq will reconnect and go through the same motions again. The second
attempt is pointless, we know it's going to fail. The refactoring to the
libpq state machine fixed that issue as a side-effect.

I removed the server ssl_enable_alpn and libpq sslalpn options. The idea
was that they could be useful for testing, but we didn't actually have
any tests that would use them, and you get the same result by testing
with an older server or client version. I'm open to adding them back if
we also add tests that benefit from them, but they were pretty pointless
as they were.

One important open item now is that we need to register a proper ALPN
protocol ID with IANA.

--
Heikki Linnakangas
Neon (https://neon.tech)