API modernization PK_KEM_Encryptor/Decryptor

[reneme]

This is in response of #3608 (comment). It modernizes the PK_KEM_Encryptor interface using std::span<>. Also, it introduces PK_KEM_Encryptor::encrypt without out-params that returns a KEM_Encapsulation struct containing the encapsulated and plaintext shared secrets.

General Question/Discussion

Do we actually want to use std::span<> as out-params like that? It provides maximum flexibility, but comes with some draw-backs. Note, though, that the std::span out-param overloads are not mandatory. Users may use the managed overload that returns a pre-allocated KEM_Encapsulation, for instance.

Advantages

• Less copies: End-user can embed the KEM-output straight into some larger user-defined buffer
• Container-independence: End-user can decide to use secure_vector<> or anything they want

Drawbacks

• Buffer management: Upstream code is responsible to provide output buffers of the correct size
• Explicit bounds checks: Given the above, our code must be vigilant about memory bounds (i.e. more crucial and explicit BOTAN_ASSERTs)
• Explicit copies: if downstream code is not yet adapted to use std::span out-params, copies are still necessary.

[coveralls]

coverage: 91.738% (+0.003%) from 91.735% when pulling a9bdf73 on Rohde-Schwarz:chore/pubkey_operators into 054e13f on randombit:master.

[randombit]

The new return struct looks good. I'm leery of the std::span output buffers because it may be difficult for an application to know what length to use, and misuse will cause memory errors.

It does have the advantage of allowing precise placement of a KEM output in some network buffer, etc. We do support something very similar in the cipher modes where you pass a buffer along with a ignored offset, for the precise reason of supporting such usage. I would argue that a KEM is a bit different because KEM encapsulation or decapsulation is much slower relative to say AES/GCM. So the additional overhead of an extra copy is pretty trivial compared to the KEM operation itself.

Also, it completely precludes supporting a KEM which has a variable length output in its unprocessed form. I don't think we have any such KEMs currently and it is hard to imagine one being standardized in the future, but I don't like boxing ourselves in on this point.

[reneme]

Frankly, I think we're already somewhat boxed into fixed-size KEM outputs because of the size_t encapsulated_key_length() method in KEM_Encryptor. I.e. adaptions to the API would be needed in any case, to support such a KEM.

Otherwise, I agree though: It isn't exactly obvious for a user what buffer length is required for the std::span out-param. However, I would imagine most new users would go for the convenience of the new return struct anyway. I view the overload with a std::span out-param as an alternative to support zero-copy (or low-allocation) use cases. One could certainly make this clearer in the documentation.

This leaves the potential for memory issues which we need to keep in mind with explicit bounds checks (e.g. by religiously using BufferStuffer to fill those std::span out-params).

[randombit]

because of the size_t encapsulated_key_length() method in KEM_Encryptor

Oh indeed. Every other function of similar form is documented as returning the upper bound of the length, but here we claim it's the exact length...

[randombit]

This leaves the potential for memory issues which we need to keep in mind with explicit bounds checks (e.g. by religiously using BufferStuffer to fill those std::span out-params).

Either that or just an explicit check that the span is equal in length to the return value of encapsulated_key_length - that would probably be much easier to diagnose for someone who provides an insufficient output buffer.

[reneme]

Either that or just an explicit check that the span is equal in length to the return value of encapsulated_key_length

That's already implemented with BOTAN_ARG_CHECK: https://github.com/randombit/botan/pull/3611/files#diff-1bbeccd351eec59d7ff980cb2357e54ded37b816ad96485a5e60d226a625bb5d

Additionally, the internal implementations of PK_Ops::KEM_Encrytion/Decryption have their own bounds-checks to be doubly sure.

[randombit]

Sorry I had missed the checks at the top level - that was what I had in mind. LGTM

[randombit]

Can you update the KEM docs to mention the new signatures?

[reneme]

Can you update the KEM docs to mention the new signatures?

Done. I don't have my YubiKey handy right now. Will sign and re-push tomorrow or Monday morning.

[reneme]

Will sign and re-push tomorrow or Monday morning.

Done.

[randombit]

Can you rebase to pick up the regular CI run clang-tidy check added in #3620? Otherwise lgtm

[randombit]

Let's hold off on merging this to master until after I can release 3.1.1 with #3623 addressed