Hi Shally, Thanks a lot for your detailed feedback. Sorry for delayed answer. My comments with [AK]
> -----Original Message----- > From: Shally Verma [mailto:shal...@marvell.com] > Sent: Wednesday, June 5, 2019 2:12 PM > To: Kusztal, ArkadiuszX <arkadiuszx.kusz...@intel.com>; dev@dpdk.org > Cc: akhil.go...@nxp.com; Trahe, Fiona <fiona.tr...@intel.com>; > shally.ve...@caviumnetworks.com > Subject: RE: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm > > Hi Arek, > > > > -----Original Message----- > > From: Arek Kusztal <arkadiuszx.kusz...@intel.com> > > Sent: Friday, May 31, 2019 7:22 PM > > To: dev@dpdk.org > > Cc: akhil.go...@nxp.com; fiona.tr...@intel.com; > > shally.ve...@caviumnetworks.com; Arek Kusztal > > <arkadiuszx.kusz...@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.kusz...@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
