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

[Shally Verma <shallyv@marvell.com>]

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

> > +	/**< 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 

> > +	 *
> > +	 * 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

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

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

> > +	 * 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 

> > +	 * 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

> > +	 *
> > +	 * -	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.



> > +			/**<
> > +			 * 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