On Nov 13, 2020, at 4:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 12 Nov 2020, at 23:12, Jacob Champion <pchampion@vmware.com> wrote:
>>
>> I'm not completely sure why this is exposed so easily with an OpenSSL
>> server -- I'm guessing the implementation slices up its packets
>> differently on the wire, causing a read event before NSS is able to
>> decrypt a full record -- but it's worth noting that this case also shows
>> up during NSS-to-NSS psql connections, when handling notifications at
>> the end of every query. PQconsumeInput() reports a hard failure with the
>> current implementation, but its return value is ignored by
>> PrintNotifications(). Otherwise this probably would have showed up
>> earlier.
>
> Should there perhaps be an Assert there to catch those?
Hm. From the perspective of helping developers out, perhaps, but from
the standpoint of "don't crash when an endpoint outside our control does
something strange", I think that's a harder sell. Should the error be
bubbled all the way up instead? Or perhaps, if psql isn't supposed to
treat notification errors as "hard" failures, it should at least warn
the user that something is fishy?
>> (What's the best way to test this case? Are there lower-level tests for
>> the protocol/network layer somewhere that I'm missing?)
>
> Not AFAIK. Having been knee-deep now, do you have any ideas on how to
> implement?
I think that testing these sorts of important edge cases needs a
friendly DSL -- something that doesn't want to make devs tear their hair
out while building tests. I've been playing a little bit with Scapy [1]
to understand more of the libpq v3 protocol; I'll see if that can be
adapted for pieces of the TLS handshake in a way that's easy to
maintain. If it can be, maybe that'd be a good starting example.
> I've incorporated this patch as well as the previous patch for the assertion
> failure on private callback data into the attached v19 patchset. I also did a
> spellcheck and pgindent run on it for ease of review.
Commit 6be725e70 got rid of some psql error messaging that the tests
were keying off of, so there are a few new failures after a rebase onto
latest master.
I've attached a patch that gets the SCRAM tests a little further
(certificate hashing was caught in an infinite loop). I also added error
checks to those loops, along the lines of the existing OpenSSL
implementation: if a suitable digest can't be found, the user will see
an error like
psql: error: could not find digest for OID 'PKCS #1 SHA-256 With RSA Encryption'
It's a little verbose but I don't think this case should come up in
normal practice.
--Jacob
[1] https://scapy.net/
