From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 4A8C9A0471 for ; Sun, 16 Jun 2019 23:39:49 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6EEE81BDC8; Sun, 16 Jun 2019 23:39:48 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id D91BB1BDC7 for ; Sun, 16 Jun 2019 23:39:46 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jun 2019 14:39:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,383,1557212400"; d="scan'208";a="185541721" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 16 Jun 2019 14:39:45 -0700 Received: from hasmsx112.ger.corp.intel.com (10.184.198.40) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 16 Jun 2019 14:39:44 -0700 Received: from HASMSX109.ger.corp.intel.com ([169.254.3.132]) by HASMSX112.ger.corp.intel.com ([169.254.11.200]) with mapi id 14.03.0439.000; Mon, 17 Jun 2019 00:39:41 +0300 From: "Kusztal, ArkadiuszX" To: Shally Verma , "dev@dpdk.org" CC: "akhil.goyal@nxp.com" , "Trahe, Fiona" , "shally.verma@caviumnetworks.com" Thread-Topic: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm Thread-Index: AQHVF7hPBSYpaBP6DEaZdtQZFTJH1KaMziUAgBH4NjA= Date: Sun, 16 Jun 2019 21:39:40 +0000 Message-ID: <06EE24DD0B19E248B53F6DC8657831551B26AC6B@hasmsx109.ger.corp.intel.com> References: <20190531135231.10836-1-arkadiuszx.kusztal@intel.com> <20190531135231.10836-2-arkadiuszx.kusztal@intel.com> In-Reply-To: Accept-Language: pl-PL, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.184.70.10] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Shally, Thanks a lot for your detailed feedback. Sorry for delayed answer. My comments with [AK] > -----Original Message----- > From: Shally Verma [mailto:shallyv@marvell.com] > Sent: Wednesday, June 5, 2019 2:12 PM > To: Kusztal, ArkadiuszX ; dev@dpdk.org > Cc: akhil.goyal@nxp.com; Trahe, Fiona ; > shally.verma@caviumnetworks.com > Subject: RE: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm >=20 > Hi Arek, >=20 >=20 > > -----Original Message----- > > From: Arek Kusztal > > 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 > > > > 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 > > --- > > 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 =3D 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. >=20 > [Shally] This is not clear to me. OP_VERIFY is public key decryption, the= n 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 mentio= n > 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 me= an that driver should not be responsible for padding, and not care what is in there= as long as parameters are correct. >=20 > > 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. >=20 > [Shally] Though I understand PKCS1_V1.5 dictates the use of specific BT f= or > pub/private key encryption/decryption, So, seems we are making that as an > implicit decision by removing individual control of BT by app. However, o= nly > 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. >=20 > > + * > > + * 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. >=20 > [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? >=20 > > */ > > RTE_CRYPTO_RSA_PADDING_OAEP, > > - /**< RSA PKCS#1 OAEP padding scheme */ > > + /**< RSA PKCS#1 OAEP padding scheme, can be used only for > > encryption/ > > + * decryption. >=20 > [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", >=20 > > + */ >=20 > > RTE_CRYPTO_RSA_PADDING_PSS, > > - /**< RSA PKCS#1 PSS padding scheme */ > > + /**< RSA PKCS#1 PSS padding scheme, can be used only for > > signatures. >=20 > [Shally] It is enough to mention till this here. following about op statu= s > should better go to an op_type description than here [AK] Same as above, could you send your proposal please? >=20 > > + * > > + * 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. >=20 > [Shally] Since spec use terminology RSA Decryption for private key > decryption, so , it is enough to mention : "pointer to data to be decrypt= ed." > And same is applicable to other as well. [AK] Ok. >=20 > > + * > > + * 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. >=20 > [Shally] Since it is meant as an input for decryption, reference to op_ty= pe > ENCRYPT look redundant here. It can simply stated as, " length of cipher = data > should be equal to RSA modulus length." >=20 > >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. >=20 > [Shally] Do we really need to mention it? [AK] I have not pronounced myself very clear here, and I forgot to add it t= o the message data. For example we have 256 bytes allocated for RSA-2048 let say decryption. Bu= t 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]=3D0. One leading zero is a good example of PKCS1 padding. =20 =20 >=20 > > + */ > > + > > 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. > > + */ >=20 > [Shally] Again, this looks redundant info here. >=20 > > + > > + 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. >=20 > > + 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. >=20 > [Shally] This whole description is already in RFC. It looks like an overl= oad to > add all such details here. If really intended it is better to give rfc PK= CS1 V2 > reference here. [AK] Ok. >=20 > > + * 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. >=20 > [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. >=20 > > + * > > + * - 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. >=20 > [Shally] Similar feedback here. >=20 > > + */ > > + 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 =3D length of output ot used > > hash function > > + * -2 : seed len is maximized > > + */ > > + } PSS; > > + }; > > + } padding; >=20 > [Shally] having additional struct padding within op_param looks unnecessa= ry. > Why do we need to rearrange it like this? It can simply be: [AK] - its proposal, to discuss I think. >=20 > struct rte_crypto_rsa_op_param { > enum rte_crypto_asym_op_type op_type; > /**< Type of RSA operation for transform */; >=20 > rte_crypto_param cipher; > /**< > * Pointer to data to be decrypted with length of data equal to RSA > modulus length > */ >=20 > rte_crypto_param message; > /**< > * Pointer to data > * - to be encrypted. > * - to be signed. > * - to be verified. > */ >=20 > 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. > */ >=20 > enum rte_crypto_rsa_padding_type pad; > /**< RSA padding scheme to be used for transform */ >=20 > > + 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 =3D length of output ot used > hash function > > + * -2 : seed len is maximized > > + */ > > + } PSS; > }; > } >=20 > > /**< > > - * 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 >=20 > [Shally] Again, probably this reference is unnecessary here. >=20 > > */ > > }; > > > > -- > > 2.1.0