> -----Original Message-----
> From: Anoob Joseph <ano...@marvell.com>
> Sent: Thursday, September 21, 2023 11:55 AM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>
> Cc: Hemant Agrawal <hemant.agra...@nxp.com>; dev@dpdk.org; Matz, Olivier
> <olivier.m...@6wind.com>; Vidya Sagar Velumuri <vvelum...@marvell.com>;
> Thomas Monjalon <tho...@monjalon.net>; Akhil Goyal <gak...@marvell.com>;
> Jerin Jacob Kollanukkaran <jer...@marvell.com>; Konstantin Ananyev
> <konstantin.v.anan...@yandex.ru>
> Subject: RE: [RFC PATCH 2/3] security: add TLS record processing
>
> Hi Harry,
>
> Please see inline.
Read all comments inline below, agree that improving docs is the only action.
I wasn't aware a unit-test suite for the TLS record work was in progress, glad
to see that.
All looks good, happy to Ack next version with doc updates.
> Thanks,
> Anoob
>
> > -----Original Message-----
> > From: Van Haaren, Harry <harry.van.haa...@intel.com>
> > Sent: Thursday, September 21, 2023 2:09 PM
> > To: Anoob Joseph <ano...@marvell.com>
> > Cc: Hemant Agrawal <hemant.agra...@nxp.com>; dev@dpdk.org; Matz,
> > Olivier <olivier.m...@6wind.com>; Vidya Sagar Velumuri
> > <vvelum...@marvell.com>; Thomas Monjalon <tho...@monjalon.net>;
> > Akhil Goyal <gak...@marvell.com>; Jerin Jacob Kollanukkaran
> > <jer...@marvell.com>; Konstantin Ananyev
> > <konstantin.v.anan...@yandex.ru>
> > Subject: [EXT] RE: [RFC PATCH 2/3] security: add TLS record processing
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > > -----Original Message-----
> > > From: Anoob Joseph <ano...@marvell.com>
> > > Sent: Wednesday, September 20, 2023 12:52 PM
> > > To: Van Haaren, Harry <harry.van.haa...@intel.com>
> > > Cc: Hemant Agrawal <hemant.agra...@nxp.com>; dev@dpdk.org; Matz,
> > > Olivier <olivier.m...@6wind.com>; Vidya Sagar Velumuri
> > > <vvelum...@marvell.com>; Thomas Monjalon <tho...@monjalon.net>;
> > Akhil
> > > Goyal <gak...@marvell.com>; Jerin Jacob Kollanukkaran
> > > <jer...@marvell.com>; Konstantin Ananyev
> > > <konstantin.v.anan...@yandex.ru>
> > > Subject: RE: [RFC PATCH 2/3] security: add TLS record processing
> > >
> > > Hi Harry,
> > >
> > > Thanks for the review. Please see inline.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > > -----Original Message-----
> > > > From: Van Haaren, Harry <harry.van.haa...@intel.com>
> > > > Sent: Wednesday, September 20, 2023 2:53 PM
> > > > To: Anoob Joseph <ano...@marvell.com>; Thomas Monjalon
> > > > <tho...@monjalon.net>; Akhil Goyal <gak...@marvell.com>; Jerin Jacob
> > > > Kollanukkaran <jer...@marvell.com>; Konstantin Ananyev
> > > > <konstantin.v.anan...@yandex.ru>
> > > > Cc: Hemant Agrawal <hemant.agra...@nxp.com>; dev@dpdk.org; Matz,
> > > > Olivier <olivier.m...@6wind.com>; Vidya Sagar Velumuri
> > > > <vvelum...@marvell.com>
> > > > Subject: [EXT] RE: [RFC PATCH 2/3] security: add TLS record
> > > > processing
> > > >
> > > > External Email
> > > >
> > > > --------------------------------------------------------------------
> > > > --
> > > > > -----Original Message-----
> > > > > From: Anoob Joseph <ano...@marvell.com>
> > > > > Sent: Friday, August 11, 2023 8:17 AM
> > > > > To: Thomas Monjalon <tho...@monjalon.net>; Akhil Goyal
> > > > > <gak...@marvell.com>; Jerin Jacob <jer...@marvell.com>; Konstantin
> > > > > Ananyev <konstantin.v.anan...@yandex.ru>
> > > > > Cc: Hemant Agrawal <hemant.agra...@nxp.com>; dev@dpdk.org;
> > Matz,
> > > > > Olivier <olivier.m...@6wind.com>; Vidya Sagar Velumuri
> > > > > <vvelum...@marvell.com>
> > > > > Subject: [RFC PATCH 2/3] security: add TLS record processing
> > > > >
> > > > > Add Transport Layer Security (TLS) and Datagram Transport Layer
> > > > > Security (DTLS). The protocols provide communications privacy for
> > > > > L4 protocols such as TCP & UDP.
> > > > >
> > > > > TLS (and DTLS) protocol is composed of two layers, 1. TLS Record
> > > > > Protocol 2. TLS Handshake Protocol
> > > > >
> > > > > While TLS Handshake Protocol helps in establishing security
> > > > > parameters by which client and server can communicate, TLS Record
> > > > > Protocol provides the connection security. TLS Record Protocol
> > > > > leverages symmetric cryptographic operations such as data
> > > > > encryption and authentication for providing security to the
> > communications.
> > > > >
> > > > > Cryptodevs that are capable of offloading TLS Record Protocol may
> > > > > perform other operations like IV generation, header insertion,
> > > > > atomic sequence number updates and anti-replay window check in
> > > > > addition to cryptographic transformations.
> > > > >
> > > > > The support is added for TLS 1.2, TLS 1.3 and DTLS 1.2.
> > > >
> > > > From the code below, my understanding is that *ONLY* the record
> > > > layer is being added/supported? The difference is described well
> > > > above, but the intended support added is not clearly defined.
> > > >
> > > > Suggest reword the last line to clarify:
> > > > "Support for TLS record protocol is added for TLS 1.2, TLS 1.3 and DTLS
> > 1.2."
> > >
> > > [Anoob] Indeed. Will reword as suggested.
> >
> > Thanks.
> >
> > > > > Signed-off-by: Akhil Goyal <gak...@marvell.com>
> > > > > Signed-off-by: Anoob Joseph <ano...@marvell.com>
> > > > > Signed-off-by: Vidya Sagar Velumuri <vvelum...@marvell.com>
> > > > > ---
> > > > > doc/guides/prog_guide/rte_security.rst | 58 +++++++++++++
> > > > > lib/security/rte_security.c | 4 +
> > > > > lib/security/rte_security.h | 110
> > > > > +++++++++++++++++++++++++
> > > > > 3 files changed, 172 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/prog_guide/rte_security.rst
> > > > > b/doc/guides/prog_guide/rte_security.rst
> > > > > index 7418e35c1b..7716d7239f 100644
> > > > > --- a/doc/guides/prog_guide/rte_security.rst
> > > > > +++ b/doc/guides/prog_guide/rte_security.rst
> > > > > @@ -399,6 +399,64 @@ The API ``rte_security_macsec_sc_create``
> > > > > returns a handle for SC, and this handle is set in
> > > > > ``rte_security_macsec_xform`` to create a MACsec session using
> > > > > ``rte_security_session_create``.
> > > > >
> > > > > +TLS-Record Protocol
> > > > > +~~~~~~~~~~~~~~~~~~~
> > > > > +
> > > > > +The Transport Layer Protocol provides communications security
> > > > > +over the
> > > > > Internet. The protocol
> > > > > +allows client/server applications to communicate in a way that is
> > > > > +designed to
> > > > > prevent eavesdropping,
> > > > > +tampering, or message forgery.
> > > > > +
> > > > > +TLS protocol is composed of two layers: the TLS Record Protocol
> > > > > +and the TLS
> > > > > Handshake Protocol. At
> > > > > +the lowest level, layered on top of some reliable transport
> > > > > +protocol (e.g., TCP),
> > > > > is the TLS Record
> > > > > +Protocol. The TLS Record Protocol provides connection security
> > > > > +that has two
> > > > > basic properties:
> > > > > +
> > > > > + - The connection is private. Symmetric cryptography is used for
> > > > > data
> > > > > + encryption (e.g., AES, DES, etc.). The keys for this
> > > > > + symmetric
> > > > encryption
> > > > > + are generated uniquely for each connection and are based on a
> > secret
> > > > > + negotiated by another protocol (such as the TLS Handshake
> > Protocol).
> > > > The
> > > > > + Record Protocol can also be used without encryption.
> > > > > +
> > > > > + - The connection is reliable. Message transport includes a
> > > > > message
> > > > > + integrity check using a keyed MAC. Secure hash functions
> > > > > (e.g.,
> > > > > + SHA-1, etc.) are used for MAC computations. The Record
> > > > > Protocol
> > > > > + can operate without a MAC, but is generally only used in this
> > > > > mode
> > > > > + while another protocol is using the Record Protocol as a
> > > > > transport
> > > > > + for negotiating security parameters.
> > > > > +
> > > > > +.. code-block:: c
> > > >
> > > > The code block below isn't C? Is there a better code block type for
> > > > a text diagram?
> > >
> > > [Anoob] Valid point. I was just following the general scheme followed in
> > this file.
> > > May be, I'll introduce a .svg image for newly added code.
> >
> > This was a nit-pick that perhaps "code-block:: text-diagram" or so exists.
> > No need to make a .svg in my opinion, the text-diagrams are clear.
>
> [Anoob] Thanks for the confirmation. I do not think "code-block::
> text-diagram"
> exists. Anyway, I'll improve the diagrams to make the padding etc more clear.
>
> >
> >
> > > > > + Record Write Record Read
> > > > > + ------------ -----------
> > > > > +
> > > > > + TLSPlaintext TLSCiphertext
> > > > > + | |
> > > > > + ~ ~
> > > > > + | |
> > > > > + V V
> > > > > + +---------|----------+ +----------|---------+
> > > > > + | Seq. no generation | | Seq. no generation |
> > > > > + +---------|----------+ +----------|---------+
> > > > > + | |
> > > > > + +---------|----------+ +----------|---------+
> > > > > + | Header insertion | | Decryption & |
> > > > > + +---------|----------+ | MAC verification |
> > > > > + | +----------|---------+
> > > > > + +---------|----------+ |
> > > > > + | MAC generation & | +----------|---------+
> > > > > + | Encryption | | TLS Header removal |
> > > > > + +---------|----------+ +----------|---------+
> > > > > + | |
> > > > > + ~ ~
> > > > > + | |
> > > > > + V V
> > > > > + TLSCiphertext TLSPlaintext
> > > > > +
> > > > > +Supported Versions
> > > > > +^^^^^^^^^^^^^^^^^^
> > > > > +
> > > > > +* TLS 1.2
> > > > > +* TLS 1.3
> > > > > +* DTLS 1.2
> > > > >
> > > > > Device Features and Capabilities
> > > > > --------------------------------- diff --git
> > > > > a/lib/security/rte_security.c b/lib/security/rte_security.c index
> > > > > c4d64bb8e9..bd7b026547 100644
> > > > > --- a/lib/security/rte_security.c
> > > > > +++ b/lib/security/rte_security.c
> > > > > @@ -282,6 +282,10 @@ rte_security_capability_get(struct
> > > > > rte_security_ctx *instance,
> > > > > if (capability->docsis.direction ==
> > > > > idx-
> >docsis.direction)
> > > > > return capability;
> > > > > + } else if (idx->protocol ==
> > > > > RTE_SECURITY_PROTOCOL_TLS_RECORD) {
> > > > > + if (capability->tls_record.ver == idx-
> > > > > >tls_record.ver &&
> > > > > + capability->tls_record.type == idx-
> > > > > >tls_record.type)
> > > > > + return capability;
> > > > > }
> > > > > }
> > > > > }
> > > > > diff --git a/lib/security/rte_security.h
> > > > > b/lib/security/rte_security.h index 3b2df526ba..b9d064ed84 100644
> > > > > --- a/lib/security/rte_security.h
> > > > > +++ b/lib/security/rte_security.h
> > > > > @@ -620,6 +620,99 @@ struct rte_security_docsis_xform {
> > > > > /**< DOCSIS direction */
> > > > > };
> > > > >
> > > > > +/** Salt len to be used with AEAD algos in TLS 1.2 */ #define
> > > > > +RTE_SECURITY_TLS_1_2_SALT_LEN 4
> > > > > +/** Salt len to be used with AEAD algos in TLS 1.3 */ #define
> > > > > +RTE_SECURITY_TLS_1_3_SALT_LEN 12
> > > > > +/** Salt len to be used with AEAD algos in DTLS 1.2 */ #define
> > > > > +RTE_SECURITY_DTLS_1_2_SALT_LEN 4
> > > > > +
> > > > > +/** TLS version */
> > > > > +enum rte_security_tls_version {
> > > > > + RTE_SECURITY_VERSION_TLS_1_2, /**< TLS 1.2 */
> > > > > + RTE_SECURITY_VERSION_TLS_1_3, /**< TLS 1.3 */
> > > > > + RTE_SECURITY_VERSION_DTLS_1_2, /**< DTLS 1.2 */
> > > > > +};
> > > > > +
> > > > > +/** TLS session type */
> > > > > +enum rte_security_tls_sess_type {
> > > > > + /** Record read session
> > > > > + * - Decrypt & digest verification.
> > > > > + */
> > > > > + RTE_SECURITY_TLS_SESS_TYPE_READ,
> > > > > + /** Record write session
> > > > > + * - Encrypt & digest generation.
> > > > > + */
> > > > > + RTE_SECURITY_TLS_SESS_TYPE_WRITE, };
> > > > > +
> > > > > +/**
> > > > > + * Configure soft and hard lifetime of a TLS record session
> > > > > + *
> > > > > + * Lifetime of a TLS record session would specify the maximum
> > > > > +number of
> > > > > packets that can be
> > > > > + * processed. TLS record processing operations would start
> > > > > + failing once hard
> > > > > limit is reached.
> > > > > + *
> > > > > + * Soft limits can be specified to generate notification when the
> > > > > + TLS record
> > > > > session is approaching
> > > > > + * hard limits for lifetime. This would result in a warning
> > > > > + returned in
> > > > > ``rte_crypto_op.aux_flags``.
> > > >
> > > > Can we define "a warning" better? Perhaps an example of a soft-limit
> > > > and hard-limit, what the user can check aux_flags for, to identify?
> > > > Or link to the appropriate part of the crypto_op.aux_flags documentation
> > to help the user.
> > > >
> > >
> > > [Anoob] The concept of lifetime is present in most protocols. Idea is
> > > to limit the max number of operations performed with a session. Soft
> > > expiry notification is to help application prepare for an expiry and
> > > setup a new session before the current one expires.
> >
> > Understood, yes.
> >
> > > The idea was borrowed from IPsec which has the
> > > 'RTE_CRYPTO_OP_AUX_FLAGS_IPSEC_SOFT_EXPIRY' flag defined. But I
> > > realize, it should be better defined. I can rename the flag to
> > > 'RTE_CRYPTO_OP_AUX_FLAGS_SEC_SOFT_EXPIRY' to avoid redefining
> > same
> > > flag for each security offload. Do you agree to this suggestion?
> > >
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.co
> > > m_dpdk_latest_source_lib_cryptodev_rte-5Fcrypto.h-
> > 23L67&d=DwIFAg&c=nKj
> > > Wec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > WYLn1v9SyTMrT5EQqh2TU&m=F6
> > >
> > WuwwuvXG0eJ1IWAMFgcXlMdQwKFDx6C1qsQgSaDa2XGr3PZ6LEDtSw0OeC
> > DstG&s=0iO8Y
> > > sr0f4cnE8ihg40sZfEEZfPzEXvJBvcNcyOdS98&e=
> >
> > So we cannot "just rename" the flag, its an API break. It likely is
> > possible to
> > add an additional #define with the same value as IPSEC_SOFT_EXPIRY, and
> > call it SEC_SOFT_EXPIRY.
> > Is that the best/most descriptive name? SYM_ALG_SOFT_EXPIRY? I'm not
> > sure here, input welcomed.
> >
> > Perhaps we can improve the doc-string there, to explain what it means a bit
> > more verbosely.
>
> [Anoob] Agreed. Let me try to improve these aspects in next version. We can
> revisit this topic after that and see if we are able to reach a conclusion.
>
> >
> > > Do note that once hard expiry is hit, the operation would fail.
> > > Expectation is, cryptodev would return 'RTE_CRYPTO_OP_STATUS_ERROR'
> > in case of errors.
> >
> > That is good.
> >
> >
> > > > > + */
> > > > > +struct rte_security_tls_record_lifetime {
> > > > > + /** Soft expiry limit in number of packets */
> > > > > + uint64_t packets_soft_limit;
> > > > > + /** Hard expiry limit in number of packets */
> > > > > + uint64_t packets_hard_limit;
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * TLS record protocol session configuration.
> > > > > + *
> > > > > + * This structure contains data required to create a TLS record
> > > > > +security
> > > > session.
> > > > > + */
> > > > > +struct rte_security_tls_record_xform {
> > > > > + /** TLS record version. */
> > > > > + enum rte_security_tls_version ver;
> > > > > + /** TLS record session type. */
> > > > > + enum rte_security_tls_sess_type type;
> > > > > + /** TLS record session lifetime. */
> > > > > + struct rte_security_tls_record_lifetime life;
> > > > > + union {
> > > > > + /** TLS 1.2 parameters. */
> > > > > + struct {
> > > > > + /** Starting sequence number. */
> > > > > + uint64_t seq_no;
> > > > > + /** Salt to be used for AEAD algos. */
> > > > > + uint8_t salt[RTE_SECURITY_TLS_1_2_SALT_LEN];
> > > > > + } tls_1_2;
> > > > > +
> > > > > + /** TLS 1.3 parameters. */
> > > > > + struct {
> > > > > + /** Starting sequence number. */
> > > > > + uint64_t seq_no;
> > > > > + /** Salt to be used for AEAD algos. */
> > > > > + uint8_t salt[RTE_SECURITY_TLS_1_3_SALT_LEN];
> > > > > + /**
> > > > > + * Minimum payload length (in case of write
> > > > sessions).
> > > > > For shorter inputs,
> > > > > + * the payload would be padded appropriately
> before
> > > > > performing crypto
> > > >
> > > > Replace "would be" with "must be"? And who must do this step, is it
> > > > the application?
> > >
> > > [Anoob] Padding is performed by the PMD/cryptodev device. I'll change
> > > "would be" to "will be". Would that address your concern?
> >
> > I suppose my concern is "is it clear to PMD authors that they must implement
> > X in their PMD", and to ensure we (DPDK community) do our best to clarify
> > API demands, and to ensure future contributions are of high quality too.
> >
> > For example, could we have a security library unit-test that checks the
> > padding case, to ensure correct & consistent behaviour across different
> > crypto PMDs?
>
> [Anoob] Glad that you brought up that point. For IPsec, we have rather
> extensive
> framework which covers all the features that are supported today in
> rte_security.
> Features like soft expiry & hard expiry are tested on platforms that support
> the
> same. As for TLS, we are working on adding unit test framework. It would be
> part
> of the next version.
Perfect, I wasn't aware of plans to add it - great!
> > Same thoughts for SOFT and HARD error variants, (although they might
> > take... hours?) to execute.
> > But it is nice to automate-and-test these "corner cases".
>
> [Anoob] It won't. Soft expiry can be very low value as well. It is already
> covered.
> Please check.
> https://elixir.bootlin.com/dpdk/latest/source/app/test/test_cryptodev.c#L10272
Ah great, good to hear.
> > It's not about wording, its about clarity between PMD devs, security library
> > devs, and application facing APIs, to ensure that DPDK provides the
> > most/best help it can to provide correctness. Does that clarify what I'd
> > like to
> > see?
>
> [Anoob] I understand your concern. My understanding is that for security
> offloads
> (rte_security lib), documentation has complete details of application & PMD
> expectations. If we have missed some features, we can take a re-look at the
> features and add as required. Other than documentation, do you have any other
> suggestions in mind?
Not right now - with additional unit tests (like IPsec has) all seems well in
hand.
> > For this patchset, could we document a list of "caveats" when implementing
> > PMD functionality for TLS-record security offload, and indicate that:
> > 1) Padding must be added by the PMD based on security library flags& algo in
> > use, not application layer (I know this is demanded by the sym algos anyway,
> > but let's make it explicit)
>
> [Anoob] Agreed. Will update documentation as required.
>
> > 2) It is strongly recommended to build unit-tests for _SOFT and _HARD error
> > cases (potentially by "fast forwarding" the internal counters via private
> > APIs
> > to avoid hours of enc/decryption)
>
> [Anoob] For IPsec, it is already added. For TLS, we have plans to add the
> same.
>
> >
> > I think that limits scope-impact to this patchset, but is clear for PMD
> > implementations in future what expectations are.
> > Thoughts, is that a good way forward?
>
> [Anoob] Indeed. Having a very well defined API interface for applications is
> a very
> positive change. I'm open to more ideas than just updating the documentation.
>
> Appreciate your feedback.
>
> <snip>
