From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 48121A046B for ; Fri, 28 Jun 2019 18:55:33 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E490537A2; Fri, 28 Jun 2019 18:55:32 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 5F5DB37A2 for ; Fri, 28 Jun 2019 18:55:31 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Jun 2019 09:55:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,428,1557212400"; d="scan'208";a="153421581" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by orsmga007.jf.intel.com with ESMTP; 28 Jun 2019 09:55:27 -0700 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.80]) by IRSMSX102.ger.corp.intel.com ([169.254.2.159]) with mapi id 14.03.0439.000; Fri, 28 Jun 2019 17:55:27 +0100 From: "Trahe, Fiona" To: "Kusztal, ArkadiuszX" , Shally Verma , "dev@dpdk.org" CC: "akhil.goyal@nxp.com" , "shally.verma@caviumnetworks.com" , "Trahe, Fiona" Thread-Topic: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm Thread-Index: AQHVF7hOC4X+olRuMUO4HUFIAGQi/aaM76wAgBHoIACAEoXhkA== Date: Fri, 28 Jun 2019 16:55:26 +0000 Message-ID: <348A99DA5F5B7549AA880327E580B435897A7DF1@IRSMSX101.ger.corp.intel.com> References: <20190531135231.10836-1-arkadiuszx.kusztal@intel.com> <20190531135231.10836-2-arkadiuszx.kusztal@intel.com> <06EE24DD0B19E248B53F6DC8657831551B26AC6B@hasmsx109.ger.corp.intel.com> In-Reply-To: <06EE24DD0B19E248B53F6DC8657831551B26AC6B@hasmsx109.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNmM2ODJlZDItY2IxYy00Y2YzLWI3NGYtNTQ4NzFhNjY5ZDUyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiTEtBengxVGdzNFlRMUQ5QlY0Q2Q4Sk5JWERIcDc4ZFh5aHV5MWxhMVlUVEdGdnQ3M29xdCt6RVUyTmdGWkt6ZSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.182] 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 Arek, Shally, Comments below. @Arek, I'd suggest sending a v2 after this, updated with whatever issues ca= n be closed and a listing of the issues still open. As getting hard to follow the thread.=20 @Shally, can you reply please - just with whatever items below you're ok wi= th, so Arek can do the v2. And we can continue discussion on the v2 on anything still open. > -----Original Message----- > From: Kusztal, ArkadiuszX > Sent: Sunday, June 16, 2019 10:40 PM > To: Shally Verma ; dev@dpdk.org > Cc: akhil.goyal@nxp.com; Trahe, Fiona ; shally.ver= ma@caviumnetworks.com > Subject: RE: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm >=20 > Hi Shally, >=20 > Thanks a lot for your detailed feedback. Sorry for delayed answer. > My comments with [AK] >=20 > > -----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 > > > > Hi Arek, > > > > > > > -----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. [Fiona] Grammar: provision not providing > > > + * 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, t= hen why > > it is mentioned as 'encryption' above? >=20 > [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 ment= ion > > case when app can pass data with padding for decryption with NO_PADDING > > scheme? >=20 > [AK] Yes. It may be common situation that HW drivers wont natively be doi= ng 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 the= re as long > as parameters are correct. [Fiona] just to add to that - PADDING_NONE typically really means PADDING_D= ONE_BY_APPLICATION But using the NONE terminology is consistent with openssl lib, so I think w= e should stick with that.=20 >=20 > > > > > But it is USER > > > RESPONSABILITY to > > > + * remove padding and verify signature. >=20 > [AK] - Same as above. >=20 > > > + */ > > > + 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 th= at? > > what is rationale for removing BT0 here? >=20 > [AK] About BT 00 it is nothing else than zero padding, plaintext RSA, mor= e 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. [Fiona] Actually as PKCS#1 v1.5 is less secure for encryption than OAEP, ar= guably it should be removed too from this enum to discourage its use. However as it's still com= monly used we=20 need to keep it. We should make sure that capability allows a PMD to opt ou= t of supporting it if it chooses to only support PSS and OAEP. =20 =20 >=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. > > > > [Shally] This description should go under OP_TYPE_VERIFICATION than her= e. > > Here it should limit to description about padding scheme > [AK] Could you send your proposal please? [Fiona] I agree with shally that it's not needed here. But think it can be = left out altogether - it's stating the obvious. The PADDING_NONE case is different a= s it's not possible for the PMD to do full signature verification in that case. So= it makes sense to call that out there. >=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. > > > > [Shally] Better to add version V2 here: RSA PKCS#1 V2 OAEP padding sche= me >=20 > [AK] Usually V2 is not added to it. Example from openssl " RSA_padding_ad= d_PKCS1_OAEP", [Fiona] Agree with Arek, better to leave out v2.=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. > > > > [Shally] It is enough to mention till this here. following about op sta= tus > > should better go to an op_type description than here > [AK] Same as above, could you send your proposal please? [Fiona] As above, I'd just leave out following sentence. =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. > > > > [Shally] Since spec use terminology RSA Decryption for private key > > decryption, so , it is enough to mention : "pointer to data to be decry= pted." > > And same is applicable to other as well. >=20 > [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 ciphe= r 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 arr= ay > > > + * (at message.data[message.length - 1]), cipher.length SHALL > > > + * therefore remain unchanged. > > > > [Shally] Do we really need to mention it? >=20 > [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] i= s 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 > > > > > + */ > > > + > > > 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 arr= ay > > > + * (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 no= t have > > any capa struct changes to return this. So do you mean capability or PM= D > > release note here? >=20 > [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 ove= rload to > > add all such details here. If really intended it is better to give rfc = PKCS1 V2 > > reference here. >=20 > [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 wher= e and > > when it will be set to INVALID / NONE? >=20 > [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 =3D length of output ot used > > > hash function > > > + * -2 : seed len is maximized > > > + */ > > > + } PSS; > > > + }; > > > + } padding; > > > > [Shally] having additional struct padding within op_param looks unneces= sary. > > Why do we need to rearrange it like this? It can simply be: >=20 > [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 */; > > > > 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 =3D 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