Note that #10289 is now pending on this as well

I have tweaked a test for this already. Docs to be added today

Documentation added. @romen, are you happy with the result?

Finally got all the docs bits right. Final review, please?

Let's move the discussion away from specific diff spots, shall we?

[for those not quite following, the discussion is about EVP_PKEY_size() not being quite suitable for what it does, how to implement it for provided keys, and whether it should be divided into operation specific size functions]

@romen said:

For the EC operations for example it is completely left to the whims of the provider developer what to return in this case: he could return the estimated output either of a signature operation or of a kexchange operation.

With legacy keys, this is implemented in the EVP_PKEY_ASN1_METHOD, which is as close to the keymgmt interface as you can get.

Something to be noted is that EVP_PKEY_size returns a number large enough for any operation, i.e. in the case you mention, it would return MAX(sigsize, kexsize).

I'm quite doubtful that having EVP_PKEY_size() make an exhaustive search for the exact size is useful when the max size is good enough for its intended use. Something to think of is the extra churn that generates.

I've wrapped my brain around the idea of operation specific size functions and could give that a go in a separate PR.

@levitte

What if we reduce the size of this PR by excluding the bits (pun intended) about EVP_PKEY_size() and its counterpart into the provider API?

That way we can proceed with the rest of this PR that seems pretty much ready, and debate to our hearts content about what to do for the naming and the ambiguity of EVP_PKEY_size().

Does it sound a plausible approach?

(also I am out of cycles for reviews for today, so it is unlikely I can look at big changes before tomorrow)

Something to be noted is that EVP_PKEY_size returns a number large enough for any operation, i.e. in the case you mention, it would return MAX(sigsize, kexsize).

@levitte I think that this could become a good interim definition of what EVP_PKEY_size() and OSSL_PKEY_PARAM_SIZE should return. At least it removes the ambiguity for provider implementers and it also marginally improves the situation for developers linking against libcrypto that at least now would have a description of what EVP_PKEY_size() returns that matches more closely with the current and legacy behavior.

@romen said:

What if we reduce the size of this PR by excluding the bits (pun intended) about EVP_PKEY_size() and its counterpart into the provider API?

The problem is that this is used everywhere to allocate a suitable buffer. Without this change, anything that uses provider-only keys (such as those created with EVP_PKEY_fromdata(), or those generated in #10289) will not be able to do any signing, because EVP_PKEY_size() returns zero for such keys.

This is actually a very serious stumbling block (of many) that has kinda halted #10289 for quite some time.

@romen, there's one thing I don't quite understand, and it's why a KEYMGMT shouldn't be able to return a known max buffer size for all operations that will use the particular key. I mean this technically rather than on principle.

However, I realise that you might not be entirely aware of one little fact: the KEYMGMT used with any operation must belong to the same provider as the operation that uses the keys. This means that as far as provider authors' "whims" are concerned, it should not be a concern, because that provider will have to be self-consistent to work properly, i.e. what the KEYMGMT part returns must be correct for all the operations that the same provider implements.

In other words, an exhaustive search for a maximum size should not be necessary, technically speaking. It can be provided once, by the provider author.

I've added some more changes that eliminate quite a lot of our internal use of EVP_PKEY_size() (including a number that went against our documentation!).

I haven't touched libssl at all. The code in ssl/statem/ scares me. @mattcaswell, would you be willing to have a look? Most specifically, these calls go against documented recommendations (*):

(*) doc/man3/EVP_DigestSignInit.pod explicitly says:

The use of EVP_PKEY_size() with these functions is discouraged because some
signature operations may have a signature length which depends on the
parameters set. As a result EVP_PKEY_size() would have to return a value
which indicates the maximum possible signature for any set of parameters.

@levitte Re: #10778 (comment)

With the definition of "is the maximum size of any supported operation on
this key" I don't see a technical problem.

I was against it when the definition seemed to be "it must return the first
meaningful size to conform with the legacy behavior of EVP_PKEY_size: i.e.
return signature size if supported, alternatively encryption size,
alternatively key exchange size, etc."

The first definition is self contained, the second is pushing hacks to
support the ambiguity of EVP_PKEY_size from core to providers!

That being said, I still feel that it is not particularly elegant to
hardcode in keymgmt.c a number that depends on operations defined in a
different source files (say signature.c and exchange.c).
It means that as a provider implementer I have to remember to recheck
the maximum size of every possible operation even if I am modifying
only say exchange.c to introduce a new kdf ctrl flag.

It is bad enough that I have to consider every possible combination of
parameters for a single operation, having to recheck it also for the other
files might be problematic and error prone (in addition to being easily
forgotten!).

Anyway, while my first objection felt like something worth holding this PR,
elegance seems secondary and I would be fine with approving this PR even
against my second objection (if we opened an issue to keep track of the
fact that we wish to improve it as soon as possible)

I was against it when the definition seemed to be "it must return the first
meaningful size to conform with the legacy behavior of EVP_PKEY_size: i.e.
return signature size if supported, alternatively encryption size,
alternatively key exchange size, etc."

That's not the legacy behaviour either. The number returned for one legacy key is constant for that key and is large enough to accommodate a signature done using that key, as well as a secret key encrypted with key, as well as a derived key (DH). It returns a maximum meaningful size for all. That explains why it returns ECDSA_size() for our EC keys, that's simply because it's a larger number than ECDH_size() returns, i.e. whoever authored that code already knew the result of MAX(ECDSA_size(), ECDH_size()).

Actually, this also strengthens my believe that the description of EVP_PKEY_size() that makes you think that it's only about signatures is incorrect. This has never matched the actual behaviour, or how we use it in our own code.

I've added some more changes that eliminate quite a lot of our internal use of EVP_PKEY_size() (including a number that went against our documentation!).

That was a fun experiment, but I'm going to move it to another branch / PR. For the intents and purposes of this PR, that was a bit over the top.

Let's move the discussion away from specific diff spots, shall we?

[for those not quite following, the discussion is about EVP_PKEY_size() not being quite suitable for what it does, how to implement it for provided keys, and whether it should be divided into operation specific size functions]

Indeed, I'm a bit lost and unsure where the discussion about EVP_PKEY_size() ended up. GitHub's web interface makes it very hard to reconstruct the discussion thread behind all those hidden resolved comments.

@romen have all your concerns about EVP_PKEY_size() now been resolved? If you still think that the function is misnamed and has problems, it would be good if you could take the time to open a separate issue which summarizes your concerns and the current status-quo.

Yes all my concerns about EVP_PKEY_size() have been addressed so far, we are just trying to please the CIs

Current travis failure is not relevant, it's just a worker that's slow (it seems to be one and the same, I've seen that job fail with early timeouts for a while now)

The extra commits that came just now solve the nits @romen had for me. From where I stand, the approval still stands.

Yes, I confirm that my approval was implicitly granted!

I actually found it rather explicit 😉

Merged.

6508e85 EVP: make EVP_PKEY_{bits,security_bits,size} work with provider only keys
9e5aaf7 PROV: Adapt the RSA, DSA and DH KEYMGMT implementations
81a624f TEST: Adapt test/evp_pkey_provided_test.c to check the key size
6942a0d DOC: New file for EVP_PKEY_size(), EVP_PKEY_bits() and EVP_PKEY_security_bits()
03d65ca DOC: Make EVP_SignInit.pod conform with man-pages(7)
f17268d Add CHANGES entry regarding the documentation of EVP_PKEY_size() et al
806253f DSA: Move DSA_security_bits() and DSA_bits()