On Tue, Apr 26, 2022 at 12:07:32AM +0200, Daniel Gustafsson wrote: > In this particular codepath I think we can afford clearing it on the way out, > with a comment explaining why. It's easily reproducible and adding a call and > a comment is a good documentation for ourselves of this OpenSSL behavior. > That > being said, clearing on the way in is the important bit.
+ * consumed an error, but cipher initialization can in FIPS enabled It seems to me that this comment needs a hyphen, as of "FIPS-enabled". I am a bit annoyed to assume that having only a localized ERR_clear_error() in the error code path of the init() call is the only problem that would occur, only because that's the first one we'd see in a hash computation. So my choice would be to call ERR_get_error() within SSLerrmessage() and clear the queue after fetching the error code via ERR_get_error() for both cryptohash_openssl.c and hmac_openssl.c, but I won't fight hard against both of you on this point, either. Perhaps this should be reported to the upstream folks? We'd still need this code for already released versions, but getting two errors looks like a mistake. > pgcrypto doesn't really consume or even inspect the OpenSSL errors, but pass > on > a PXE error based on the context of the operation. We could clear the queue > as > we leave, but as you say we already clear it before calling in other places so > it's not clear that it's useful. We've had EVP in pgcrypto for some time > without seeing issues from error queues, one error left isn't that different > from two when not consumed. Okay. I did not recall the full error stack used in pgcrypto. It is annoying to not get from OpenSSL the details of the error, though. With FIPS enabled, one computing a hash with pgcrypto would just know about the initialization error, but would miss why the computation failed. It looks like we could use a new error code to tell px_strerror() to look at the OpenSSL error queue instead of one of the hardcoded strings. Just saying. > The attached 0002 does however correctly (IMO) report the error as an init > error instead of the non-descript generic error, which isn't really all that > helpful. I think that also removes the last consumer of the generic error, > but > I will take another look with fresh eyes to confirm that. > > 0003 removes what I think is a weirdly placed questionmark from the message > that make it seem strangely ambiguous. This needs to update the test answer > files as well, but I first wanted to float the idea before doing that. Good catches. -- Michael
signature.asc
Description: PGP signature