On Nov 13, 2020, at 4:14 AM, Daniel Gustafsson <dan...@yesql.se> wrote: >> On 12 Nov 2020, at 23:12, Jacob Champion <pchamp...@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/
nss-fix-hang-when-hashing-certificates.patch
Description: nss-fix-hang-when-hashing-certificates.patch