Re: [dpdk-dev] [PATCH 1/4] lib/crypto: add support for ECDSA

[Anoob Joseph <anoobj@marvell.com>]

Hi Arek,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Sent: Friday, December 20, 2019 9:36 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; De
> Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Ayuj Verma <ayverma@marvell.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Narayana Prasad Raju Athreya <pathreya@marvell.com>; Shally Verma
> <shallyv@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Sunila
> Sahu <ssahu@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH 1/4] lib/crypto: add support for ECDSA
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> Few suggestions inline.
> 
> >  ;
> >  [Asymmetric]
> > -RSA =
> > -DSA =
> > -Modular Exponentiation =
> > -Modular Inversion =
> > -Diffie-hellman =
> > \ No newline at end of file
> > +RSA                     =
> > +DSA                     =
> > +Modular Exponentiation  =
> > +Modular Inversion       =
> > +Diffie-hellman          =
> > +ECDSA                   =
> > diff --git a/lib/librte_cryptodev/rte_crypto_asym.h
> > b/lib/librte_cryptodev/rte_crypto_asym.h
> > index 0d34ce8..dd5e6e3 100644
> > --- a/lib/librte_cryptodev/rte_crypto_asym.h
> > +++ b/lib/librte_cryptodev/rte_crypto_asym.h
> > @@ -81,6 +81,10 @@ enum rte_crypto_asym_xform_type {
> >  	/**< Modular Exponentiation
> >  	 * Perform Modular Exponentiation b^e mod n
> >  	 */
> > +	RTE_CRYPTO_ASYM_XFORM_ECDSA,
> > +	/**< Elliptic Curve Digital Signature Algorithm
> > +	 * Perform Signature Generation and Verification.
> > +	 */
> >  	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >  	/**< End of list */
> >  };
> > @@ -319,6 +323,46 @@ struct rte_crypto_dsa_xform {  };
> >
> >  /**
> > + * TLS named curves
> > + *
> > +https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.iana.org_ass
> > +ignments_tls-
> 2Dparameters_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8r
> > +wwviRSxyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=nYBsHHO1NEdu-
> JmNULDH0kx7auwQd
> >
> +SbI0VYNVNxmm1w&s=RKZgomFiHpm9Yp8AYEn4FjlPpzbdavkMv5cRUggVid
> A&e=
> > + * tls-parameters.xhtml#tls-parameters-8
> > + * secp192r1 = 19,
> > + * secp224r1 = 21,
> > + * secp256r1 = 23,
> > + * secp384r1 = 24,
> > + * secp521r1 = 25,
> > + */
> > +enum rte_crypto_ec_group {
> > +	RTE_CRYPTO_EC_GROUP_UNKNOWN  = 0,
> > +	RTE_CRYPTO_EC_GROUP_NISTP192 = 19,
> > +	RTE_CRYPTO_EC_GROUP_NISTP224 = 21,
> > +	RTE_CRYPTO_EC_GROUP_NISTP256 = 23,
> > +	RTE_CRYPTO_EC_GROUP_NISTP384 = 24,
> > +	RTE_CRYPTO_EC_GROUP_NISTP521 = 25,
> > +};
> 
> [Arek] Since in comment we use SECG naming maybe enum should follow to
> avoid confusion?

[Anoob] How about RTE_CRYPTO_EC_GROUP_SECP521R1 and so on?

> > +
> > +/**
> > + * Structure for elliptic curve point  */ struct rte_crypto_ec_point
> > +{
> > +	rte_crypto_param x;
> > +	/**< X coordinate */
> > +	rte_crypto_param y;
> > +	/**< Y coordinate */
> > +};
> > +
> > +/**
> > + * Asymmetric elliptic curve transform data
> > + *
> > + * Structure describing all EC based xform params
> > + *
> > + */
> > +struct rte_crypto_ec_xform {
> > +	enum rte_crypto_ec_group curve_id;
> > +	/**< Pre-defined ec groups */
> > +};
> > +
> > +/**
> >   * Operations params for modular operations:
> >   * exponentiation and multiplicative inverse
> >   *
> > @@ -372,6 +416,11 @@ struct rte_crypto_asym_xform {
> >
> >  		struct rte_crypto_dsa_xform dsa;
> >  		/**< DSA xform parameters */
> > +
> > +		struct rte_crypto_ec_xform ec;
> > +		/**< EC xform parameters, used by elliptic curve based
> > +		 * operations.
> > +		 */
> >  	};
> >  };
> >
> > @@ -516,6 +565,39 @@ struct rte_crypto_dsa_op_param {  };
> >
> >  /**
> > + * ECDSA operation params
> > + */
> > +struct rte_crypto_ecdsa_op_param {
> > +	enum rte_crypto_asym_op_type op_type;
> > +	/**< Signature generation or verification */
> > +
> > +	rte_crypto_param pkey;
> > +	/**< Private key of the signer for signature generation */
> [Arek] - for DSA we have private key in xform, why this inconsistency?

[Anoob] Putting key in the session would limit the number of ops we can perform with one session. In regular use cases, the number of ops done with one key is very minimal. But the number of ops using same EC group is more. So if we define the session to one per EC group, then we will have better usage of session. Otherwise, the session need to be created and destroyed every few operations.
 
> > +
> > +	struct rte_crypto_ec_point q;
> > +	/**< Public key of the signer for verification */
> > +
> > +	rte_crypto_param message;
> > +	/**< Input message to be signed or verified */
> [Arek] - This I expect should be message digest instead of message itself?

[Anoob] Yes. Message digest is what is expected. This is following what is done with DSA & RSA. Do you recommend any changes? Variable name or description. 

> > +
> > +	rte_crypto_param k;
> > +	/**< The ECDSA per-message secret number, which is an integer
> > +	 * in the interval (1, n-1)
> > +	 */
> [Arek] - If pmd can generate 'k' internally we could do something like:
> 'if k.data == NULL => PMD will generate 'k' internally, k.data remains
> untouched.'

[Anoob] So that will be exposed as a new capability, right? Do you suggest any changes here to accommodate that? 

> Another option is to provide user with some callback function to generate
> CSRN which could be useful for RSA PSS, OAEP as well (we already discussed
> that internally in Intel, I will elaborate on this bit more in different thread).

[Anoob] Do you intend to keep the generated CSRN hidden in the PMD? 
 
> 
> > +
> > +	rte_crypto_param r;
> > +	/**< r component of elliptic curve signature
> > +	 *     output : for signature generation
> > +	 *     input  : for signature verification
> > +	 */
> > +	rte_crypto_param s;
> > +	/**< s component of elliptic curve signature
> > +	 *     output : for signature generation
> > +	 *     input  : for signature verification
> > +	 */
> [Arek] - Do we want to add any constraints like 'this field should be big
> enough to hold...'

[Anoob] For every case where rte_crypto_param is used for 'output', application should make sure the buffers are large enough. Do you think we could document it somewhere common instead of adding per operation?
 
> > +};
> > +
> > +/**
> >   * Asymmetric Cryptographic Operation.
> >   *
> >   * Structure describing asymmetric crypto operation params.
> > @@ -537,6 +619,7 @@ struct rte_crypto_asym_op {
> >  		struct rte_crypto_mod_op_param modinv;
> >  		struct rte_crypto_dh_op_param dh;
> >  		struct rte_crypto_dsa_op_param dsa;
> > +		struct rte_crypto_ecdsa_op_param ecdsa;
> >  	};
> >  };
> >
> > diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> > b/lib/librte_cryptodev/rte_cryptodev.c
> > index 89aa2ed..0d6babb 100644
> > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > @@ -173,6 +173,7 @@ const char *rte_crypto_asym_xform_strings[] = {
> >  	[RTE_CRYPTO_ASYM_XFORM_MODINV]	= "modinv",
> >  	[RTE_CRYPTO_ASYM_XFORM_DH]	= "dh",
> >  	[RTE_CRYPTO_ASYM_XFORM_DSA]	= "dsa",
> > +	[RTE_CRYPTO_ASYM_XFORM_ECDSA]	= "ecdsa",
> >  };
> >
> >  /**
> > --
> > 2.7.4
> 
> Regards,
> Arek