On Wed, 7 Oct 2020 at 20:45, Richard Levitte <levi...@openssl.org> wrote: > > I'm as culpable as anyone re pushing the "convention" that an EVP_PKEY > with a private key should have a public key. I was incorrect.
Sure, my example was not about pointing fingers! It's just to give recent data points on how the documentation written in 2006 is probably not something as stale as one can think, because until recently we were leveraging that assumption at least in some sections of the lower layers. > Regarding avoiding NULL checks, that's actually a separate story, > evem though it overlaps conveniently. There was the idea, for a > while, that we should avoid NULL checks everywhere (unless it's valid > input), and indeed make it a programmer error if they happened to pass > NULL where they shouldn't. > One can see that as an hard assertion, and that has indeed produced > crashes that uncovered bugs we might otherwise have missed. The tide > has changed though, and it seem like the fashion du jour is to check > NULLs and error with an ERR_R_PASSED_NULL_PARAMETER. I'm not sure > that we've made an actual hard decision in either direction, though. > > However, I'm not sure where the NULL check problem comes in. > Operations that don't use the public key parts don't need to look at > the public key parts, so a NULL check there is simply not necessary. I think the lack of NULL checks is intertwined with this problem as long as in some code that does access the public key component we ensured we dereference without making the NULL check because of the assumption. The problem I see with spot checking rather than writing a comprehensive suite of tests, is that given our wide API surface and the compromises taken in the transition from non-opaque stack allocated objects in 1.0.2 to opaque objects in 1.1.1+, we might have non-obvious places where dereferencing the public key without checking for NULL can bite us. I would add, although it's feedback for the vote, not the proposal, that I am not opposed to changing the documented assumption in principle, but with changing it this late in the development of 3.0. I am giving feedback for the text proposal to ensure that during the voting we don't underestimate the high risk of critical bugs coming with adopting this change at this stage, and that it does call for vastly extending our current test coverage beyond what we were currently planning as acceptable. We all feel quite under pressure due to the time constraints, and I feel that in general we have privileged working on implementing the required features (with sufficient level of tests as per our policy) rather than spending major resources in further developing our test coverage with exhaustive positive and negative tests for regular and far-fetched cases, this should probably also have a weight on the decision of committing to this change at this point. Nicola