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

[Shally Verma <shallyv@marvell.com>]

> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: Friday, June 28, 2019 10:25 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Shally Verma
> <shallyv@marvell.com>; dev@dpdk.org
> Cc: akhil.goyal@nxp.com; shally.verma@caviumnetworks.com; Trahe, Fiona
> <fiona.trahe@intel.com>
> Subject: RE: [EXT] [PATCH 1/3] cryptodev: rework api of rsa algorithm
> 
> Hi Arek, Shally,
> 
> Comments below.
> @Arek, I'd suggest sending a v2 after this, updated with whatever issues can
> be closed and a listing of the issues still open.
> As getting hard to follow the thread.
> @Shally, can you reply please - just with whatever items below you're ok
> with, so Arek can do the v2.
> And we can continue discussion on the v2 on anything still open.
> 
...

> > > > --- 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.
> [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, 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.
> [Fiona] just to add to that - PADDING_NONE typically really means
> PADDING_DONE_BY_APPLICATION But using the NONE terminology is
> consistent with openssl lib, so I think we should stick with that.
> 
[Shally] I look forward to v2 to comment further on this.

...

> >
> > [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.
> [Fiona] Actually as PKCS#1 v1.5 is less secure for encryption than OAEP,
> arguably it should be removed too from this enum to discourage its use.
> However as it's still commonly used we need to keep it. We should make
> sure that capability allows a PMD to opt out of supporting it if it chooses to
> only support PSS and OAEP.
> 
[Shally] We have been practically using it , so I can say it is still very much in use.
we can update capability to check for supported padding type if PMD does not support all. 
Regarding BT0 did we conclude to remove BT0 as practically not in use and then we're using PKCS _V1.5 only.
If yes, am fine with this.

> >
....

> > > [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",
> [Fiona] Agree with Arek, better to leave out v2.
> 
[Shally] Okay. Agree to both Arek, Fiona here. 

...

> > >
> > > >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.
> >
[Shally] Output of RSA public/private key encryption operation is length of modulus.
So it should return 256 length output here as is and let app use that.


> > >
> > > > +
> > > > +	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.
[Shally] Okay.

....

> > > > +		 * 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.
[Shally] Okay. I look forward further discussion on this in v2.

...
> > > [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.
[Shally] I believe not needed though but look forward to v2 for further discussion.

...

Thanks
Shally
> > > > 2.1.0