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

["Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com>]

Hi Anoob,

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Thursday, January 2, 2020 8:55 AM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.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: RE: [PATCH 1/4] lib/crypto: add support for ECDSA
> 
> 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?
[Arek] Iam ok with that.
> 
> > > +
> > > +/**
> > > + * 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.
[Arek] I agree with that, so maybe we could change DSA similar way (leave only group params?)
> 
> > > +
> > > +	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.
[Arek] Some information would be good I think.
> 
> > > +
> > > +	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?
[Arek] Or maybe feature flag, it would apply to DSA as well.
> 
> > 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?
[Arek] Openssl PMD does that with DSA but iam not a fan of that. But if some hw can do DSA/ECDSA by generating 'k' by itself it would have to be added anyway. Other option is to add aforementioned callbacks (for HW that not generate 'k' by itself)
> 
> >
> > > +
> > > +	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?
[Arek]
Both options look good to me.
> 
> > > +};
> > > +
> > > +/**
> > >   * 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