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

[Shally Verma <shallyv@marvell.com>]

Hi Arek,


> -----Original Message-----
> From: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> Sent: Friday, May 31, 2019 7:22 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; fiona.trahe@intel.com;
> shally.verma@caviumnetworks.com; Arek Kusztal
> <arkadiuszx.kusztal@intel.com>
> Subject: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
> 
> External Email
> 
> ----------------------------------------------------------------------
> This patch reworks API of RSA algorithm.
> Major changes:
> - Cipher field was introduced
> - Padding struct was created
> - PKCS1-v1_5 Block type 0 was removed
> - Fixed comments about prime numbers
> 
> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> ---
>  lib/librte_cryptodev/rte_crypto_asym.h | 149
> +++++++++++++++++++++++++--------
>  1 file changed, 114 insertions(+), 35 deletions(-)
> 
> diff --git a/lib/librte_cryptodev/rte_crypto_asym.h
> b/lib/librte_cryptodev/rte_crypto_asym.h
> index 8672f21..bb94fa5 100644
> --- a/lib/librte_cryptodev/rte_crypto_asym.h
> +++ b/lib/librte_cryptodev/rte_crypto_asym.h
> @@ -111,23 +111,33 @@ 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
> -	 */
> -	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
> +	/**< RSA no padding scheme.
> +	 * In this case user is responsible for providing and verification
> +	 * of padding.
> +	 * In case RTE_CRYPTO_ASYM_OP_VERIFY op type is used if no
> +	 * problems in public key 'encryption' detected driver SHALL return
> +	 * RTE_CRYPTO_OP_STATUS_SUCCESS.

[Shally] This is not clear to me. OP_VERIFY is public key decryption, then why it is mentioned as 'encryption' above?
Secondly,  Any public/private key encryption with NO_PADDING scheme, would result in encryption data without any padding string. 
And, if same is passed to PMD for decryption with PADDING_NONE, then PMD would assume encryption block is without any padding string. 
So, which case are we trying to clarify here? Or, do you intend to mention case when app can pass data with padding for decryption with NO_PADDING scheme?

> But it is USER
> RESPONSABILITY to
> +	 * remove padding and verify signature.
> +	 */
> +	RTE_CRYPTO_RSA_PADDING_PKCS1,
> +	/**< RSA PKCS#1 PKCS1-v1_5 padding scheme. For signatures block
> type 01,
> +	 * for encryption block type 02 are used.

[Shally] Though I understand PKCS1_V1.5 dictates the use of specific BT for pub/private key encryption/decryption,
So, seems we are making that as an implicit decision by removing individual control of BT by app. However, only point is then it states only about BT1 and BT2 not BT0.
What if , an app want to use BT0 for padding, then how do we support that? what is rationale for removing BT0 here?

> +	 *
> +	 * In case RTE_CRYPTO_ASYM_OP_VERIFY op type is used crypto op
> status
> +	 * is set to RTE_CRYPTO_OP_STATUS_SUCCESS when signature is
> properly
> +	 * verified, RTE_CRYPTO_OP_STATUS_ERROR when it failed.

[Shally] This description should go under OP_TYPE_VERIFICATION than here. Here it should limit to description about padding scheme

>  	 */
>  	RTE_CRYPTO_RSA_PADDING_OAEP,
> -	/**< RSA PKCS#1 OAEP padding scheme */
> +	/**< RSA PKCS#1 OAEP padding scheme, can be used only for
> encryption/
> +	 * decryption.

[Shally] Better to add version V2 here: RSA PKCS#1 V2 OAEP padding scheme

> +	 */

>  	RTE_CRYPTO_RSA_PADDING_PSS,
> -	/**< RSA PKCS#1 PSS padding scheme */
> +	/**< RSA PKCS#1 PSS padding scheme, can be used only for
> signatures.

[Shally] It is enough to mention till this here. following about op status should better go to an op_type description than here

> +	 *
> +	 * Crypto op status is set to RTE_CRYPTO_OP_STATUS_SUCCESS
> +	 * when signature is properly verified,
> RTE_CRYPTO_OP_STATUS_ERROR
> +	 * when it failed.
> +	 */
>  	RTE_CRYPTO_RSA_PADDING_TYPE_LIST_END
>  };
> 
> @@ -199,8 +209,8 @@ struct rte_crypto_rsa_priv_key_qt {
>   */
>  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,11 +407,23 @@ 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.
>  	 */
> 
> +	rte_crypto_param cipher;
> +	/**<
> +	 * Pointer to data
> +	 * - to be decrypted for RSA private decrypt.

[Shally] Since spec use terminology RSA Decryption for private key decryption, so , it is enough to mention : "pointer to data to be decrypted." And same is applicable to other as well.

> +	 *
> +	 * When RTE_CRYPTO_ASYM_OP_ENCRYPT op_type used size in
> bytes
> +	 * of this field need to be equal to the size of corresponding
> +	 * RSA key. 

[Shally] Since it is meant as an input for decryption, reference to op_type ENCRYPT look redundant here. It can simply stated as, " length of cipher data should be equal to RSA modulus length."

>Returned data is in Big-Endian format which means
> +	 * that Least-Significant byte will be placed at top byte of an array
> +	 * (at message.data[message.length - 1]), cipher.length SHALL
> +	 * therefore remain unchanged.

[Shally] Do we really need to mention it?

> +	 */
> +
>  	rte_crypto_param sign;
>  	/**<
>  	 * Pointer to RSA signature data. If operation is RSA @@ -410,25
> +432,82 @@ struct rte_crypto_rsa_op_param {
>  	 *
>  	 * Length of the signature data will be equal to the
>  	 * RSA prime modulus length.
> -	 */
> -
> -	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
> -	 */
> -
> -	enum rte_crypto_auth_algorithm mgf1md;
> +	 *
> +	 * Returned data is in Big-Endian format which means
> +	 * that Least-Significant byte will be placed at top byte of an array
> +	 * (at message.data[message.length - 1]), sign.length SHALL
> +	 * therefore remain unchanged.
> +	 */

[Shally] Again, this looks redundant info here.

> +
> +	struct {
> +		enum rte_crypto_rsa_padding_type type;
> +		/**<
> +		 * 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 SHALL be
> +		 * available in in specific driver guide or capabilities.
> +		 */
[Shally] you mentioned it as part of capability, but this patch does not have any capa struct changes to return this. So do you mean capability or PMD release note here?

> +		enum rte_crypto_auth_algorithm hash;
> +		/**<
> +		 * -	For PKCS1-v1_5 signature (Block type 01) this field
> +		 * represents hash function that will be used to create
> +		 * message hash. This hash will be then concatenated with
> +		 * hash algorithm identifier into DER encoded sequence.

[Shally] This whole description is already in RFC. It looks like an overload to add all such details here. If really intended it is better to give rfc PKCS1 V2 reference here.

> +		 * If RTE_CRYPTO_HASH_INVALID is set, driver default will be
> set.
> +		 * If RTE_CRYPTO_HASH_NONE is set, message will be signed
> as it is.

[Shally] who will set it? PMD or application? Its bit unclear here where and when it will  be set to INVALID / NONE?

> +		 *
> +		 * -	For OAEP this field represents hash function that will
> +		 * be used to produce hash of the optional label.
> +		 * If RTE_CRYPTO_HASH_INVALID or
> RTE_CRYPTO_HASH_NONE is set
> +		 * driver will use default value. For OAEP usually it is SHA-1.
> +		 *
> +		 * -	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 RTE_CRYPTO_HASH_INVALID or
> RTE_CRYPTO_HASH_NONE is set
> +		 * driver will use default value.
> +		 *
> +		 * -	If driver supports only one function
> RTE_CRYPTO_HASH_NONE
> +		 * according to aformentioned restrictions should be used or
> +		 * specific function should be set, otherwise on dequeue the
> driver
> +		 * SHALL set crypto_op_status to
> RTE_CRYPTO_OP_STATUS_INVALID_ARGS.

[Shally] Similar  feedback here.

> +		 */
> +		union {
> +			struct {
> +				enum rte_crypto_auth_algorithm mgf;
> +				/**<
> +				 * Mask genereation function hash algorithm.
> +				 * If this field is not supported by the driver,
> +				 * it should be set to
> RTE_CRYPTO_HASH_NONE.
> +				 */
> +				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;
> +				/**<
> +				 * Mask genereation function hash algorithm.
> +				 * If this field is not supported by the driver,
> +				 * it should be set to
> RTE_CRYPTO_HASH_NONE.
> +				 */
> +				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;
> +		};
> +	} padding;

[Shally] having additional struct padding within op_param looks unnecessary. Why do we need to rearrange it like this? It can simply be:

struct rte_crypto_rsa_op_param {
	enum rte_crypto_asym_op_type op_type;
	/**< Type of RSA operation for transform */;

	rte_crypto_param cipher;
	/**<
	 * Pointer to data to be decrypted with length of data equal to RSA modulus length
                */

	rte_crypto_param message;
	/**<
	 * Pointer to data
	 * - to be encrypted.
	 * - to be signed.
	 * - to be verified.
	 */

	rte_crypto_param sign;
	/**<
	 * Pointer to RSA signature data. If operation is RSA
	 * 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.
	 */

	enum rte_crypto_rsa_padding_type pad;
	/**< RSA padding scheme to be used for transform */

> +		union {
> +			struct {
> +				enum rte_crypto_auth_algorithm mgf;
> +				/**<
> +				 * Mask genereation function hash algorithm.
> +				 * If this field is not supported by the driver,
> +				 * it should be set to
> RTE_CRYPTO_HASH_NONE.
> +				 */
> +				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;
> +				/**<
> +				 * Mask genereation function hash algorithm.
> +				 * If this field is not supported by the driver,
> +				 * it should be set to
> RTE_CRYPTO_HASH_NONE.
> +				 */
> +				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
> +	 * SHALL be put in specific driver guide

[Shally] Again, probably this reference is unnecessary here.

>  	 */
>  };
> 
> --
> 2.1.0