Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: rework api of rsa algorithm

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Shally,

Do you have further comments on this series?

If yes, we may need to defer this to next release. As cryptodev change should not be there beyond RC2. In fact all of them should be closed by RC1.

Regards,
Akhil

> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Sent: Tuesday, July 9, 2019 3:33 PM
> To: Shally Verma <shallyv@marvell.com>; dev@dpdk.org
> Cc: Akhil Goyal <akhil.goyal@nxp.com>; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v2 1/3] cryptodev: rework api of rsa algorithm
> 
> To clarify bit more
> With [AK2]
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Kusztal, ArkadiuszX
> > Sent: Monday, July 8, 2019 7:44 PM
> > To: Shally Verma <shallyv@marvell.com>; dev@dpdk.org
> > Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>;
> > shally.verma@caviumnetworks.com
> > Subject: Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: rework api of rsa
> > algorithm
> >
> > Hi Shally,
> >
> > With [AK]
> >
> > > -----Original Message-----
> > > From: Shally Verma [mailto:shallyv@marvell.com]
> > > Sent: Saturday, July 6, 2019 3:14 PM
> > > To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> > > Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>;
> > > shally.verma@caviumnetworks.com
> > > Subject: RE: [PATCH v2 1/3] cryptodev: rework api of rsa algorithm
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> > > > Sent: Thursday, July 4, 2019 6:10 PM
> > > > To: dev@dpdk.org
> > > > Cc: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>;
> > > > shally.verma@caviumnetworks.com; Shally Verma
> > <shallyv@marvell.com>
> > > > Subject: [EXT] RE: [PATCH v2 1/3] cryptodev: rework api of rsa
> > > > algorithm
> > > >
> > > > External Email
> > > >
> > > ..
> > >
> > > > > diff --git a/lib/librte_cryptodev/rte_crypto_asym.h
> > > > > b/lib/librte_cryptodev/rte_crypto_asym.h
> > > > > index 8672f21..486399c 100644
> > > > > --- a/lib/librte_cryptodev/rte_crypto_asym.h
> > > > > +++ b/lib/librte_cryptodev/rte_crypto_asym.h
> > > > > @@ -111,23 +111,21 @@ enum rte_crypto_asym_op_type {
> > > > >   */
> > > > >  enum rte_crypto_rsa_padding_type {
> > > > >  	RTE_CRYPTO_RSA_PADDING_NONE = 0,
> > > > > -	/**< RSA no padding scheme */
> > > > > -	RTE_CRYPTO_RSA_PKCS1_V1_5_BT0,
> > > > > -	/**< RSA PKCS#1 V1.5 Block Type 0 padding scheme
> > > > > -	 * as described in rfc2313
> > > > > +	/**< RSA no padding scheme.
> > > > > +	 * In this case user is responsible for provision and verification
> > > > > +	 * of padding.
> > > > >  	 */
> > > > > -	RTE_CRYPTO_RSA_PKCS1_V1_5_BT1,
> > > > > -	/**< RSA PKCS#1 V1.5 Block Type 01 padding scheme
> > > > > -	 * as described in rfc2313
> > > > > -	 */
> > > > > -	RTE_CRYPTO_RSA_PKCS1_V1_5_BT2,
> > > > > -	/**< RSA PKCS#1 V1.5 Block Type 02 padding scheme
> > > > > -	 * as described in rfc2313
> > > > > +	RTE_CRYPTO_RSA_PADDING_PKCS1,
> > > [Shally] My preference would still be to rename as PKCS1_V1.5 to align
> > > more to standard
> > [AK] - Agree.
> >
> > >
> > > > > +	/**< RSA PKCS#1 PKCS1-v1_5 padding scheme. For signatures
> block
> > > > > type 01,
> > > > > +	 * for encryption block type 02 are used.
> > > > >  	 */
> > > > >  	RTE_CRYPTO_RSA_PADDING_OAEP,
> > > > > -	/**< RSA PKCS#1 OAEP padding scheme */
> > > > > +	/**< RSA PKCS#1 OAEP padding scheme, can be used only for
> > > > > encryption/
> > > > > +	 * decryption.
> > > > > +	 */
> > > > >  	RTE_CRYPTO_RSA_PADDING_PSS,
> > > > > -	/**< RSA PKCS#1 PSS padding scheme */
> > > > > +	/**< RSA PKCS#1 PSS padding scheme, can be used only for
> > > > > signatures.
> > > > > +	 */
> > > > >  	RTE_CRYPTO_RSA_PADDING_TYPE_LIST_END
> > > > >  };
> > > > >
> > > ...
> > >
> > > > >  struct rte_crypto_rsa_xform {
> > > > >  	rte_crypto_param n;
> > > > > -	/**< n - Prime modulus
> > > > > -	 * Prime modulus data of RSA operation in Octet-string network
> > > > > +	/**< n - Modulus
> > > > > +	 * Modulus data of RSA operation in Octet-string network
> > > > >  	 * byte order format.
> > > > >  	 */
> > > > >
> > > > > @@ -397,9 +395,36 @@ struct rte_crypto_rsa_op_param {
> > > > >  	/**<
> > > > >  	 * Pointer to data
> > > > >  	 * - to be encrypted for RSA public encrypt.
> > > > > -	 * - to be decrypted for RSA private decrypt.
> > > > >  	 * - to be signed for RSA sign generation.
> > > > >  	 * - to be authenticated for RSA sign verification.
> > > > > +	 *
> > > > > +	 * Octet-string network byte order format.
> > > > > +	 *
> > > > > +	 * This field is an input to RTE_CRYPTO_ASYM_OP_ENCRYPT
> > > > > +	 * operation, and output to RTE_CRYPTO_ASYM_OP_DECRYPT
> > > > > operation.
> > > > > +	 *
> > > > > +	 * When RTE_CRYPTO_ASYM_OP_DECRYPT op_type used
> length in
> > > > > bytes
> > > > > +	 * of this field needs to be greater or equal to the length of
> > > > > +	 * corresponding RSA key in bytes.
> > > [Shally] better rephrased " When an op_type ASYM_OP_DECRYPT used,
> > > length of output buffer should be greater than  or equal to RSA key
> > > modulus length
> > [AK] - RSA key size is a RSA modulus size, but can be changed.
> > >
> > > > > +	 *
> > > > > +	 * When padding field is set to
> RTE_CRYPTO_RSA_PADDING_NONE
> > > > > +	 * returned data size will be equal to the size of RSA key
> > > > > +	 * in bytes. All leading zeroes will be preserved.
> > > > > +	 */
> > > [Shally] Is it in context of OP_TYPE_DECRYPT? Even if it is
> > > PADDING_NONE, whether it is encrypted/decrypted, o/p length can vary
> > > between 0 .. RSA modulus length - 1 as perf rfc8017
> > [AK]
> > example. 20 bytes was encrypted with 2048bit key PKCS_1.5 1. Padding
> > PKCS_1.5 set - Upon decryption we return 20 bytes of recovered message.
> > 2. Padding NONE set (padding done by user) - we return 236 bytes of padding
> > (one leading zero) | 20 bytes of message = 256 bytes.
> > (like in example test case I have added) 3. Padding NONE set (textbook rsa) -
> > we return 236 bytes of zeroes | 20 bytes of message = 256 bytes.
> > >
> > > > > +
> > > > > +	rte_crypto_param cipher;
> > > > > +	/**<
> > > > > +	 * Pointer to data
> > > > > +	 * - to be decrypted for RSA private decrypt.
> > > > > +	 *
> > > > > +	 * Octet-string network byte order format.
> > > > > +	 *
> > > > > +	 * This field is an input to RTE_CRYPTO_ASYM_OP_DECRYPT
> > > > > +	 * operation, and output to RTE_CRYPTO_ASYM_OP_ENCRYPT
> > > > > operation.
> > > > > +	 *
> > > > > +	 * When RTE_CRYPTO_ASYM_OP_ENCRYPT op_type used
> length in
> > > > > bytes
> > > > > +	 * of this field needs to be greater or equal to the length of
> > > > > +	 * corresponding RSA key in bytes.
> > > > >  	 */
> > > [Shally] recommend rephrasing as above
> > >
> > > > >
> > > > >  	rte_crypto_param sign;
> > > > > @@ -408,27 +433,88 @@ struct rte_crypto_rsa_op_param {
> > > > >  	 * sign @ref RTE_CRYPTO_ASYM_OP_SIGN, buffer will be
> > > > >  	 * over-written with generated signature.
> > > > >  	 *
> > > > > -	 * Length of the signature data will be equal to the
> > > > > -	 * RSA prime modulus length.
> > > > > +	 * Octet-string network byte order format.
> > > > > +	 *
> > > > > +	 * When RTE_CRYPTO_ASYM_OP_SIGN op_type used length in
> bytes
> > > > > +	 * of this field needs to be greater or equal to the length of
> > > > > +	 * corresponding RSA key in bytes.
> > > [Shally] field ---> buffer
> > [AK] - Agreed
> > >
> > > > >  	 */
> > > > >
> > > > > -	enum rte_crypto_rsa_padding_type pad;
> > > > > -	/**< RSA padding scheme to be used for transform */
> > > > > -
> > > > > -	enum rte_crypto_auth_algorithm md;
> > > > > -	/**< Hash algorithm to be used for data hash if padding
> > > > > -	 * scheme is either OAEP or PSS. Valid hash algorithms
> > > > > -	 * are:
> > > > > -	 * MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> > > > > +	rte_crypto_param message_to_verify;
> > > > > +	/**<
> > > > > +	 * Pointer to the message 'm' that was signed with
> > > > > +	 * RSASP1 in RFC8017.
> > > >> It is the result of operation RSAVP1
> > > > > +	 * defined in RFC8017, where field `sign` is the input
> > > > > +	 * parameter `s`.
> > > > > +	 *
> > > [Shally] This is confusing. Are you trying to say "this is output to VERIFY_OP
> > ?
> > > where output should be same as message buffer provided above?
> > > If yes, then why not just use that message buffer as an output of
> > > VERIFY_OP than adding a new one?
> > [AK] - it is output of signature verify (in openssl Public_Decrypt) for
> > PADDING_NONE case. It will be used to check if signature is correct.
> > In `message` then there could be original message.
> > But yes, we can use message as an output buffer, I just though it would be
> > more transparent.
> > Since user need to verify it by himself it is not that important to keep original
> > message in `message` field, both ways will do.
> > Waiting for opinions.
> > >
> > > > > +	 * Used only when padding type is set to
> > > > > RTE_CRYPTO_RSA_PADDING_NONE
> > > [Shally] I think regardless of padding, we can provide it as output to
> > > sign operation
> > [AK] - `Sign` is the place to put signature into.
> > >
> > > > > +	 * and `op_type` is set to RTE_CRYPTO_ASYM_OP_VERIFY.
> > > > > +	 *
> > > > > +	 * Returned data size will be equal to the size of RSA key
> > > > > +	 * in bytes. All leading zeroes will be preserved.
> > > > > +	 *
> > > [Shally] Again, I think it should instead be mentioned as return size
> > > can be 0 ... modulus_len - 1
> > >
> > > > > +	 * When RTE_CRYPTO_ASYM_OP_VERIFY op_type used length
> in
> > > > > bytes
> > > > > +	 * of this field needs to be greater or equal to the length of
> > > > > +	 * corresponding RSA key in bytes.
> > > > >  	 */
> > > [Shally] There're multiple statements starting with when op_type =
> > > VERIFY, can we club them and make description shorter?
> > >
> > > > >
> > > > > -	enum rte_crypto_auth_algorithm mgf1md;
> > > > > +	enum rte_crypto_rsa_padding_type padding;
> > > > > +	/**<
> > > > > +	 * In case RTE_CRYPTO_RSA_PADDING_PKCS1 is selected,
> > > > > +	 * driver will distinguish between block type basing
> > > > > +	 * on rte_crypto_asym_op_type of the operation.
> > > > > +	 *
> > > > > +	 * Which padding type is supported by the driver can be
> > > > > +	 * found in in specific driver guide.
> > > [Shally] better rephrase " PMD expose padding type support in its
> > capability.
> > > Refer to <struct / API definition>
> > >
> > > > > +	 */
> > > > > +	enum rte_crypto_auth_algorithm padding_hash;
> > > > > +	/**<
> > > > > +	 * -	For PKCS1-v1_5 signature (Block type 01) this field
> > > > > +	 * represents hash function that will be used to create
> > > > > +	 * message hash.
> > > [Shally] Currently PMD input pre-computed hash atleast for PKCSV_1.5 ..
> > > does your hw support this offload for RSA?
> > > So far , in our testing we observe openssl does it already before
> > > calling private/public_key_enc so we should not be mentioning it in
> > > spec unless any hw provide that combined offload hash + rsa
> >
> > [AK] - Openssl for PKCS1_5 signature use RSA_private_encrypt which does
> > not handle algorithmIdentifier.
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.op
> enssl.org%2Fdocs%2Fman1.0.2%2Fman3%2FRSA_private_encrypt.html&amp;da
> ta=02%7C01%7Cakhil.goyal%40nxp.com%7Cdb7fd9c8893144df755808d704549
> 746%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636982633668853
> 558&amp;sdata=FGOMjneOQsB7DRg8fKYHWhn5TKH3eO8QKA1bGbbt25w%3D&
> amp;reserved=0
> > And it is not said what data should be provided for signing. To be properly
> > signed input buffer should be DER encoded concatenation of
> > algorithmIdentifier and message digest.
> > There would be much work to be done by user then: adding OID, computing
> > Hash of message, ASN.1 call. Waiting for comments on that.
> 
> [AK2] - we do not specify what data should user provide to be signed.
> 
> >
> > >
> > > > > +	 *
> > > > > +	 * -	For OAEP this field represents hash function that will
> > > > > +	 * be used to produce hash of the optional label.
> > > > > +	 *
> > > > > +	 * -	For PSS this field represents hash function that will be
> used
> > > > > +	 * to produce hash (mHash) of message M and of M' (padding1 |
> > > > > mHash | salt)
> > > > > +	 *
> > > > > +	 * If not set driver will use default value.
> > > > > +	 */
> > > > > +	union {
> > > > > +		struct {
> > > > > +			enum rte_crypto_auth_algorithm mgf;
> > > > > +			/**<
> > > > > +			 * Mask genereation function hash algorithm.
> > > > > +			 *
> > > > > +			 * If not set driver will use default value.
> > > > > +			 */
> > > > > +			rte_crypto_param label;
> > > > > +			/**<
> > > > > +			 * Optional label, if driver does not support
> > > > > +			 * this option, optional label is just an empty
> string.
> > > > > +			 */
> > > > > +		} OAEP;
> > > > > +		struct {
> > > > > +			enum rte_crypto_auth_algorithm mgf;
> > > [Shally] Though  it is mentioned in current spec, but I have similar
> > > doubt here do we need to provide this offload in spec? I will use
> > > terms from rfc8017 for further discussion.
> > >   if we have any PMD whose HW provide full RSAES-OAEP offload i.e.
> > > doing EME-OAEP followed by RSAEP, then it make sense to have it in
> > > spec. But if we don't have any PMD example which support that full
> > > offload, then we can redefine spec only to support RSAEP and omit md and
> > mgf from spec.
> > >
> > [AK] - I thought to add PSS/OAEP to Openssl. Though OAEP and PSS for sure
> > will be implemented having custom MGF is not that necessary. Waiting for
> > opinions.
> 
> >
> > >
> > >
> > > > > +			/**<
> > > > > +			 * Mask genereation function hash algorithm.
> > > > > +			 *
> > > > > +			 * If not set driver will use default value.
> > > > > +			 */
> > > > > +			int seed_len;
> > > > > +			/**<
> > > > > +			 * Intended seed length. Nagative number has
> > > > special
> > > > > +			 * value as follows:
> > > > > +			 * -1 : seed len = length of output ot used hash
> > > > > function
> > > > > +			 * -2 : seed len is maximized
> > > > > +			 */
> > > > > +		} PSS;
> > > > > +	};
> > > > >  	/**<
> > > > > -	 * Hash algorithm to be used for mask generation if
> > > > > -	 * padding scheme is either OAEP or PSS. If padding
> > > > > -	 * scheme is unspecified data hash algorithm is used
> > > > > -	 * for mask generation. Valid hash algorithms are:
> > > > > -	 * MD5, SHA1, SHA224, SHA256, SHA384, SHA512
> > > > > +	 * Padding type of RSA crypto operation.
> > > > > +	 * What are random number generator requirements and
> prequisites
> > > > > +	 * can be found specific driver guide.
> > > [Shally] I would suggest it to rephrase again " app should refer to
> > > PMD guide to check for RNG requirement and other pre-requisites used
> > > in hash generation.
> > > However , this feedback is relevant if we are retaining full OAEP offload.
> > >
> > > > >  	 */
> > > > >  };
> > > > >
> > > > > --
> > > > > 2.1.0