RE: [RFC PATCH 2/3] security: add TLS record processing

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Wednesday, September 20, 2023 12:52 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; dev@dpdk.org; Matz,
> Olivier <olivier.matz@6wind.com>; Vidya Sagar Velumuri
> <vvelumuri@marvell.com>; Thomas Monjalon <thomas@monjalon.net>; Akhil
> Goyal <gakhil@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev@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.haaren@intel.com>
> > Sent: Wednesday, September 20, 2023 2:53 PM
> > To: Anoob Joseph <anoobj@marvell.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Akhil Goyal <gakhil@marvell.com>; Jerin Jacob
> > Kollanukkaran <jerinj@marvell.com>; Konstantin Ananyev
> > <konstantin.v.ananyev@yandex.ru>
> > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; dev@dpdk.org; Matz,
> > Olivier <olivier.matz@6wind.com>; Vidya Sagar Velumuri
> > <vvelumuri@marvell.com>
> > Subject: [EXT] RE: [RFC PATCH 2/3] security: add TLS record processing
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > > -----Original Message-----
> > > From: Anoob Joseph <anoobj@marvell.com>
> > > Sent: Friday, August 11, 2023 8:17 AM
> > > To: Thomas Monjalon <thomas@monjalon.net>; Akhil Goyal
> > > <gakhil@marvell.com>; Jerin Jacob <jerinj@marvell.com>; Konstantin
> > > Ananyev <konstantin.v.ananyev@yandex.ru>
> > > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; dev@dpdk.org; Matz,
> > > Olivier <olivier.matz@6wind.com>; Vidya Sagar Velumuri
> > > <vvelumuri@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 <gakhil@marvell.com>
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > Signed-off-by: Vidya Sagar Velumuri <vvelumuri@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.


> > > +             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://elixir.bootlin.com/dpdk/latest/source/lib/cryptodev/rte_crypto.h#L67

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.

> 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?

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".

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?

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)
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)

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?

> >
> > > +			 * transformations.
> > > +			 */
> > > +			uint32_t min_payload_len;
> > > +		} tls_1_3;
> > > +
> > > +		/** DTLS 1.2 parameters */
> > > +		struct {
> > > +			/** Epoch value to be used. */
> > > +			uint16_t epoch;
> > > +			/** 6B starting sequence number to be used. */
> > > +			uint64_t seq_no;
> > > +			/** Salt to be used for AEAD algos. */
> > > +			uint8_t salt[RTE_SECURITY_DTLS_1_2_SALT_LEN];
> > > +			/** Anti replay window size to enable sequence
> > replay
> > > attack handling.
> > > +			 * Anti replay check is disabled if the window size is 0.
> > > +			 */
> > > +			uint32_t ar_win_sz;
> > > +		} dtls_1_2;
> > > +	};
> > > +};
> > > +
> > >  /**
> > >   * Security session action type.
> > >   */
> > > @@ -654,6 +747,8 @@ enum rte_security_session_protocol {
> > >  	/**< PDCP Protocol */
> > >  	RTE_SECURITY_PROTOCOL_DOCSIS,
> > >  	/**< DOCSIS Protocol */
> > > +	RTE_SECURITY_PROTOCOL_TLS_RECORD,
> > > +	/**< TLS Record Protocol */
> > >  };
> > >
> > >  /**
> > > @@ -670,6 +765,7 @@ struct rte_security_session_conf {
> > >  		struct rte_security_macsec_xform macsec;
> > >  		struct rte_security_pdcp_xform pdcp;
> > >  		struct rte_security_docsis_xform docsis;
> > > +		struct rte_security_tls_record_xform tls;
> >
> > Do we see TLS handshake xform being added in future? If so, is 'tls' a good
> > name, perhaps 'tls_record'?
> > That would allow 'tls_handshake' in future, with consistent naming scheme
> > without API/ABI break.
> 
> [Anoob] In the future, I would like to see TLS handshake also offloaded in a
> similar manner. But that would need some fundamental changes in security
> library. Like, current security library is pretty much tied to symmetric
> operations but a handshake would involve both symmetric & asymmetric
> operations.
> 
> Said that, I agree with your suggestion to rename the field. Will change it to
> 'tls_record' in next version.

Agreed, that handshake is significantly more complex, thanks for rename to "tls_record".


> 
> >
> >
> > >  	};
> > >  	/**< Configuration parameters for security session */
> > >  	struct rte_crypto_sym_xform *crypto_xform; @@ -1190,6 +1286,16
> > @@
> > > struct rte_security_capability {
> > >  			/**< DOCSIS direction */
> > >  		} docsis;
> > >  		/**< DOCSIS capability */
> > > +		struct {
> > > +			enum rte_security_tls_version ver;
> > > +			/**< TLS record version. */
> > > +			enum rte_security_tls_sess_type type;
> > > +			/**< TLS record session type. */
> > > +			uint32_t ar_win_size;
> > > +			/**< Maximum anti replay window size supported
> > for
> > > DTLS 1.2 record read
> > > +			 * operation. Value of 0 means anti replay check is
> > not
> > > supported.
> > > +			 */
> > > +		} tls_record;
> >
> > Missing      /**< TLS Record Capability */     docstring here.
> 
> [Anoob] Agreed. Will add in next version.

Thanks!

<snip>