openssl icon indicating copy to clipboard operation
openssl copied to clipboard

Pipelining support for TLSv1.3 and Provided Ciphers

Open ramenhost opened this issue 2 years ago • 6 comments

Hi,

We are working on a hardware accelerator, and we use OpenSSL Engine/Provider framework to offload crypto. SSL pipelining feature gives us performance benefits and we are planning to extend pipelining support to TLSv1.3 and provided ciphers.

This feature was mentioned by @mattcaswell in PR #19456 discussion recently. Creating this issue for community feedback before starting our work on it. We are looking forward to submit a PR for review once done.

ramenhost avatar Jan 19 '23 12:01 ramenhost

This would be an awesome feature to have.

Any implementation will need to target the master branch (as a new feature it will not be eligible for backport to 3.0, and its not suitable for the upcoming 3.1). Therefore you will be touching the newly refactored record layer code.

Careful consideration will have to be given to how to extend this to to provided ciphers. It might be an idea to rough out a design for this before going too deep into implementation.

mattcaswell avatar Jan 19 '23 12:01 mattcaswell

I might be tempted to split this into 2 PRs. One to extend pipelining support to provided ciphers, and another to add it for TLSv1.3. The two things seem logically separate.

mattcaswell avatar Jan 19 '23 12:01 mattcaswell

That makes sense. We will be starting with support for TLSv1.3.

ramenhost avatar Jan 19 '23 12:01 ramenhost

The tls13_cipher() is using different EVP APIs compared to tls1_cipher().

For AAD

https://github.com/openssl/openssl/blob/e788c772b12eea5ced4ce46619e13acf0e0eb6ba/ssl/record/methods/tls1_meth.c#L263-L264 https://github.com/openssl/openssl/blob/e788c772b12eea5ced4ce46619e13acf0e0eb6ba/ssl/record/methods/tls13_meth.c#L157-L158

For Tag (sending)

https://github.com/openssl/openssl/blob/e788c772b12eea5ced4ce46619e13acf0e0eb6ba/crypto/evp/e_chacha20_poly1305.c#L470-L473 https://github.com/openssl/openssl/blob/e788c772b12eea5ced4ce46619e13acf0e0eb6ba/ssl/record/methods/tls13_meth.c#L167-L171

What is the notion behind using non-tls cipher path for TLSv1.3? Is it for handling TLSv1.3 IV update with backward compatibility?

There are two options for pipeline in TLSv1.3,

  1. Incorporate pipeline (using mix of EVP_CTX_ctrl functions and EVP_CipherUpdate) into non-tls cipher path. This will define a new standard for engine/providers to implement TLSv1.3 pipeline. The engine/providers will have support two different styles of pipeline usage (for <=TLSv1.2 and TLSv1.3).
  2. Change tls13_cipher() to tls path as used by tls1_cipher(). This breaks TLSv1.3 compatibility with existing cipher implementations and calls for update in IV handling by them.

What do you think would be the best considering all the implications?

ramenhost avatar Feb 01 '23 07:02 ramenhost

The tls1_cipher() way of doing things is really quite horrible and requires the cipher implementation to have TLS specific knowledge. However the EVP_CTRL_AEAD_GET_TAG/EVP_CTRL_AEAD_SET_TAG interface is our standard documented approach for using AEAD ciphers. This is the interface you are supposed to use to do AEAD if your application is doing crypto directly (i.e. not using TLS). It was a deliberate choice for the TLSv1.3 code to work differently and to use the standard interface.

In principle there is nothing TLS specific about doing pipelining. There should be an easy well defined API for performing pipelining with provided ciphers whether or not TLS is in use. The TLSv1.3 code should then be updated to use that API if it detects that pipelining support is available. As far as the provider author is concerned they just need to provide ciphers that support pipelining. They should not need to know anything TLS specific.

As far as tls1_cipher() is concerned I think, eventually, it would be good if we can transition it to use the new APIs for pipelining if it detects that it has a provided cipher that can support it. We should not add pipelining support to providers for the old ctrls. Provider authors should only have to worry about one way of doing pipelining.

mattcaswell avatar Feb 01 '23 11:02 mattcaswell

Understood. Working on a new pipelining approach based on standard AEAD interface for provided ciphers. This will be non-TLS specific.

What do you think should be the pipelining interface used by TLSv1.3 when a capable legacy engine in loaded? Existing engines (including proprietary) will be expecting legacy pipeline interface. TLSv1.3 will not be backward compatible with them if new pipeline interface is used (without differentiating b/w provided or legacy) on seeing EVP_CIPH_FLAG_PIPELINE.

ramenhost avatar Feb 02 '23 13:02 ramenhost

We should not implement legacy pipelining support for TLSv1.3. Engines are deprecated so we should not be implementing new functionality based on them.

mattcaswell avatar Feb 03 '23 08:02 mattcaswell

Leaving out the legacy pipeline support for TLSv1.3, we would need to add pipeline support into provider framework first. Then a separate PR for TLSv1.3 to make use of it. Among the below two, which style of pipelining would you prefer? Design 1 Design 2

EVP_CipherFinal doesn't really fit into the pipelining and need not be called in pipeline case. The EVP_CipherUpdate acts like a do_cipher in the case of pipeline.

ramenhost avatar Feb 14 '23 10:02 ramenhost

Hmmm. I'm not really sure I like either model. In any case for providers (if we need to), we should use OSSL_PARAM instead of ctrls. However I'm wondering whether a whole new API makes sense for this rather than trying to squeeze the pipelining use case into the old API.

So something like:

EVP_CipherPipelineInit(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher, const unsigned char *key, size_t numpipes, const unsigned char **iv, int enc);
EVP_CipherPipelineUpdate(EVP_CIPHER_CTX *ctx, size_t numpipes, unsigned char **out, int **outl, const unsigned char **in, int *inl);
EVP_CipherPipelineFinal(EVP_CIPHER_CTX *ctx, size_t numpipes, unsigned char **outm, int **outl);

Probably this needs more input than just me though...

mattcaswell avatar Feb 15 '23 04:02 mattcaswell

I understand that input/output buffers of a pipeline request are not really a CIPHER CTX PARAM. So as per your suggestion, do you mean creating a new pipeline version of these APIs?

    OSSL_FUNC_cipher_encrypt_init_fn *einit;
    OSSL_FUNC_cipher_decrypt_init_fn *dinit;
    OSSL_FUNC_cipher_update_fn *cupdate;
    OSSL_FUNC_cipher_final_fn *cfinal;
    OSSL_FUNC_cipher_cipher_fn *ccipher;

Also, do you think EVP_CipherPipelineUpdate can be called multiple times before EVP_CipherPipelineFinal? I'm failing to see how multiple update can work with pipeline.

ramenhost avatar Feb 15 '23 07:02 ramenhost

So as per your suggestion, do you mean creating a new pipeline version of these APIs?

Yes - that is my thinking. Although probably this needs more input than just me. I'm not sure I would both with ccipher from the above list.

Also, do you think EVP_CipherPipelineUpdate can be called multiple times before EVP_CipherPipelineFinal? I'm failing to see how multiple update can work with pipeline.

I was thinking that yes. Why do you think this is problematic?

mattcaswell avatar Feb 15 '23 22:02 mattcaswell

Why do you think this is problematic?

I was thinking about TLS case. There is no problem with supporting multiple EVP_CipherPipelineUpdate in the EVP interface, it'll be up to the applications and provider implementations.

ramenhost avatar Feb 16 '23 11:02 ramenhost

For the TLSv1.3 implementation we want to just use the standard EVP interface, i.e. it should not be necessary to have a special TLS aware cipher.

mattcaswell avatar Feb 16 '23 22:02 mattcaswell

@mattcaswell your inputs on draft PR will be helpful.

ramenhost avatar Jun 15 '24 06:06 ramenhost