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

Kusztal, ArkadiuszX arkadiuszx.kusztal at intel.com
Sun Jun 16 23:39:40 CEST 2019
Previous message: [dpdk-dev] [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
Next message: [dpdk-dev] [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Hi Shally,

Thanks a lot for your detailed feedback. Sorry for delayed answer.
My comments with [AK]

> -----Original Message-----
> From: Shally Verma [mailto:shallyv at marvell.com]
> Sent: Wednesday, June 5, 2019 2:12 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal at intel.com>; dev at dpdk.org
> Cc: akhil.goyal at nxp.com; Trahe, Fiona <fiona.trahe at intel.com>;
> shally.verma at caviumnetworks.com
> Subject: RE: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
> 
> Hi Arek,
> 
> 
> > -----Original Message-----
> > From: Arek Kusztal <arkadiuszx.kusztal at intel.com>
> > Sent: Friday, May 31, 2019 7:22 PM
> > To: dev at dpdk.org
> > Cc: akhil.goyal at nxp.com; fiona.trahe at intel.com;
> > shally.verma at caviumnetworks.com; Arek Kusztal
> > <arkadiuszx.kusztal at 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 at 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?

[AK] - yes, you right 'decryption', public key but technically decryption.

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

[AK] Yes. It may be common situation that HW drivers wont natively be doing any padding,
Especially PSS and OAEP which expects very strong TRNG. NO_PADDING would mean that
driver should not be responsible for padding, and not care what is in there as long
as parameters are correct.

> 
> > But it is USER
> > RESPONSABILITY to
> > +	 * remove padding and verify signature.

[AK] - Same as above.

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

[AK] About BT 00 it is nothing else than zero padding, plaintext RSA, more or less the same thing as NO_PADDING. It has been silently abandoned
long time ago. Only RFC 2313 mentions it. No 2437,3447 and 8017.

> 
> > +	 *
> > +	 * 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
[AK] Could you send your proposal please?

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

[AK] Usually V2 is not added to it. Example from openssl " RSA_padding_add_PKCS1_OAEP",

> 
> > +	 */
> 
> >  	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
[AK] Same as above, could you send your proposal please?

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

[AK] Ok.
> 
> > +	 *
> > +	 * 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?

[AK] I have not pronounced myself very clear here, and I forgot to add it to the message data.
For example we have 256 bytes allocated for RSA-2048 let say decryption. But we decrypted only 255 bytes (one leading zero).
So should we trim this zero or not? If we trim, size is 255 and data[0] is some non zero byte. If we don't, size if 256 and data[0]=0.
One leading zero is a good example of PKCS1 padding.
 
 
> 
> > +	 */
> > +
> >  	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?

[AK] - this would be part of v2. Thanks.
> 
> > +		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.

[AK] Ok.
> 
> > +		 * 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.
[AK] - provided message fits into key size.
> 
> [Shally] who will set it? PMD or application? Its bit unclear here where and
> when it will  be set to INVALID / NONE?

[AK] PMD according to its default. Still only a proposal.
> 
> > +		 *
> > +		 * -	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:

[AK] - its proposal, to discuss I think.

> 
> 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
Previous message: [dpdk-dev] [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
Next message: [dpdk-dev] [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list