Re: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto

["Verma, Shally" <Shally.Verma@cavium.com>]

Hi Fiona

Thanks for feedback. PSB.

>-----Original Message-----
>From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>Sent: 14 March 2018 17:42
>To: Verma, Shally <Shally.Verma@cavium.com>; dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>;
>Murthy, Nidadavolu <Nidadavolu.Murthy@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish
><Ashish.Gupta@cavium.com>; Doherty, Declan <declan.doherty@intel.com>; Keating, Brian A <brian.a.keating@intel.com>; Griffin,
>John <john.griffin@intel.com>; Tadepalli, Hari K <hari.k.tadepalli@intel.com>
>Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>
>Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
>

//snip

>> >> +enum rte_crypto_asym_op_type {
>> >> +	RTE_CRYPTO_ASYM_OP_NOT_SPECIFIED = 1,
>> >[Fiona] Why does this start at 1?
>> >And is it necessary?
>> >
>> [Shally]  We need to indicate list of supported op in xform capability structure. Because an implementation
>> may support RSA encrypt and decrypt but not RSA Sign and verify. Or, Can support DSA Sign compute but
>> not verify.
>> So, it was added to indicate end-of-array marker (though doesn’t need to be 1 for that reason). but now
>> when I think to re-design its support, then it won't be needed.
>> So, I thought rather than carrying op_type array, I can add an op_type bitmask in xform capability to show
>> supported ops.
>[Fiona] I think a bitmask is ok. Would only need an array if there were other params whose capabilities would
> vary depending on the op_type. E.g. like range of digest_size in sym depends on the algo. But does code below
>not need a xform_type input? Or is capa the capability for one specific xform_type?

[Shally] Yes. capability here is of one xform. I will change variable name to rte_crypto_asym_xform_capabilities to be more clear here.

>
>> Example capability check code then would look like:
>> int rte_crypto_asym_check_op_type ( const rte_crypto_asym_capabilties *capa, int op_type) {
>> 	If(capa->op_types & (1 << op_type))
>> 		return 0;
>> 	return -1;
>> }

//snip

>> >> +	/**< Signature verification operation */
>> >> +	RTE_CRYPTO_ASYM_OP_KEY_PAIR_GENERATION,
>> >> +	/**< Public/Private key pair generation operation */
>> >[Fiona] In the comment, clarify that this is for DH and ECDH, and for the
>> > generation of the public key (and optionally the private key?)
>> >
>>
>> [Shally] so far, I was assuming it will generate both but when you say private key optional, where you
>> expect it to be coming from? - from app or generated internally? Is their hw variant which may not
>> generate private key?
>>
>[Fiona] In our native driver the private key, which is usually just a random number conforming to
>0 < private_key < (primeP - 1), is passed in by the application and only the public key is generated.
>Some hw accelerators may have RNG capabilities, others may not or the application may prefer to generate
>its own RN.
>

[Shally] Ok. I will work around to add this support.

//snip

>>
>> [Shally] I would take this question in-parts:
>>
>> " Also do we want to list all "published" curves, or allow customers to specify their own curve parameters,"
>> - Currently specification allow app to do both i.e. it can either setup these published curve ids or specify its
>> own domain params to all elliptic curve based xforms (ecdh, ecdsa, and fecc).
>>   If input curve has curve_type = UNSPECIFIED, PMD uses domain parameters else it uses curveid given by
>> app from this published list.
>>   So, is this missing any requirement that need to be supported?!
>[Fiona] So you mean the params in ECDH xform (a,b,G,n,h) are only specified if curve_type = UNSPECIFIED,
>else not needed? And in FECC the params (order,G,a,b,h) ?
>Ecdsa xform not yet specified, but it will have a similar set?
>Then I would suggest creating a struct to hold this param set and using same struct in all 3 xforms.
>
[Shally] Ok. Sounds better. See new proposed struct below.

//snip 

>> "do we have to publish curves < 224 bits"
>> - We just listed all based on previous feedback to include non-nist curve id. But agree it can be narrowed
>> down to few (may be to one used by ssl)
>[Fiona] I'm looking for feedback internally on this - will get back to you later.
>But I think it could be trimmed to at least removing those curves < 224 bits.
>The xform params can be used to cover unlikely curves trimmed from the list.
>
[Shally] I look forward to it.

//snip

>> >> +
>> >> +/**
>> >> + * Elliptic curve id
>> >> + */
>> >> +struct rte_crypto_ec_curve_id {
>> >> +	RTE_STD_C11
>> >> +	union {
>> >> +		enum rte_crypto_ec_prime_curve pcurve;
>> >> +		enum rte_crypto_ec_binary_curve bcurve;
>> >> +	};
>> >> +};
>> >[Fiona] Because the values of these two enums overlap, if you specify the curve type incorrectly, you'll still
>> >have a potentially valid curve enum specified. I suggest it's safer to include the type here with the union,
>> >rather than in the wrapper xform struct, i.e.
>> >struct rte_crypto_ec_curve {
>> >	enum rte_crypto_ec_curve_type curve_type;
>> >	/**< curve type: Prime vs Binary */
>> >	union {
>> >	enum rte_crypto_ec_prime_curve pcurve_id;
>> >	enum rte_crypto_ec_binary_curve bcurve_id;
>> >	};
>> >};
>> [Shally] We would need curve type if we need to define a new curve based on domain params. Let's cover
>> it later once we clarify above feedback.
>[Fiona] So how about adding the struct with curve params I mentioned above as a 3rd element in the union?
>

[Shally] Yes agreed. So new structs looks like:

struct rte_crypto_ec_domain_params { g, a , b, n ,h };

struct rte_crypto_ec_curve {
	enum rte_crypto_ec_curve_type curve_type;
	/**< curve type: Prime vs Binary */
	union {
	enum rte_crypto_ec_prime_curve pcurve_id;
	enum rte_crypto_ec_binary_curve bcurve_id;
	struct rte_crypto_ec_domain_params dparams;
	};
};
//snip

>> >>  	cryptodev_sym_configure_session_t session_configure;
>> >>  	/**< Configure a Crypto session. */
>> >[Fiona] This should really be renamed sym_session_configure, same for session_clear,
>> >qp_attach_session and qp_detach_session
>> >
>>
>> [Shally] I thought to keep it backward compatible for now. I think we can take this change later as it need
>> announcement first
>[Fiona] As these are on the internal API between PMDs and API layer, rather than the application<->cryptodev
>interface I think they don't need an announcement.
>But would need to be done in a standalone patch, with all PMDs changed
>

[Shally] ok.

So, I will cover elliptic curve and session_configure change in separate patches and send 1st patch with features covered and agreed.


//snip

Thanks
Shally