[2/4] cryptodev: promote asym APIs to stable

Message ID 20210731181327.660296-3-gakhil@marvell.com (mailing list archive)
State Rejected, archived
Delegated to: akhil goyal
Headers
Series cryptodev and security ABI improvements |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Akhil Goyal July 31, 2021, 6:13 p.m. UTC
  Asymmetric crypto APIs have been stable from
quite some time, hence moving them from experimental
to stable in DPDK 21.11

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
 lib/cryptodev/rte_cryptodev.h | 10 ----------
 lib/cryptodev/version.map     | 24 +++++++++++++-----------
 2 files changed, 13 insertions(+), 21 deletions(-)
  

Comments

Arkadiusz Kusztal Aug. 30, 2021, 3:49 p.m. UTC | #1
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Akhil Goyal
> Sent: Saturday, July 31, 2021 8:13 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; anoobj@marvell.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Doherty, Declan <declan.doherty@intel.com>; matan@nvidia.com;
> g.singh@nxp.com; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> jianjay.zhou@huawei.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Akhil Goyal <gakhil@marvell.com>
> Subject: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable
> 

Hi Akhil,

I am not sure if this API is ready to be stable so I will add few comments here:

RSA:
	rte_crypto_param message;
	...
	 * - to be signed for RSA sign generation.

If this message is plaintext, then in case of:
1) PKCS1_1.5 padding:
Standard defines data to be signed as DER encoded struct of digestAlgorithm + digest
(few exceptions I am aware of were TLS prior to 1.2 or IKE version 1)
- There is no field to specify that, even if PMD would be correctly implemented it still would lack information about hash aglorithm.
- Currently what openssl pmd for example is doing is RSA_private_encrypt which omits this step (https://www.openssl.org/docs/man1.1.1/man3/RSA_private_encrypt.html - mentions this).
2) PADDING_NONE:
I cannot find what user is supposed to do in this case, and I think it may be quite common option for hw due to reliance on strong CSPRNG for PSS or OAEP.

DSA:
	struct rte_crypto_dsa_op_param {
	...
There is no 'k' parameter? I though I have added it, how hw with no CSRNG should work with DSA?

For ECDSA private key is in Op, for DSA is in xform. Where this inconsistency comes from?

	/**< x: Private key of the signer in octet-string network
	 * byte order format.
	 * Used when app has pre-defined private key.
	 * Valid only when xform chain is DSA ONLY.
	 * if xform chain is DH private key generate + DSA, then DSA sign
	 * compute will use internally generated key.

And this one I cannot understand, there is DH and DSA in one line plus seems that private dsa key would be generated and used in the same operation.
We want to create self-signed certificate here on the fly or something?

	RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
	/**< DH Private Key generation operation */

This is another interesting part (similar to 'k' in (EC)DSA, PSS, QAEO in RSA), there was no any type of hw random number generation concept for symmetric crypto (i.e. salt, IV, nonce) and here we have
standalone Diffie Hellman private key generator.
And since it is no crypto computation but random number generation, maybe there should be another module to handle CSRNG or we could register randomness
source into cryptodev, like callback? Another option would be to predefine randomness source per device like (i.e. x86 RDRAND, /dev/random) for user to decide.

Additionally there is DH op but there is no ECDH (I know there is ECPM, but the same way there is MODEXP which creates another inconsistency). Optionally we can extend DH API to work with EC?
EDDSA, EDDH needs to be implemented soon too.

Regards,
Arek
  
Akhil Goyal Sept. 3, 2021, 3:17 p.m. UTC | #2
Hi Arek,

Do you think all the asym APIs are not eligible for promoting it to stable APIs?
I haven't seen any changes for quite some time and we cannot have it experimental
Forever.
The APIs which you think are expected to change, we can leave them as experimental
And mark the others as stable.

Can you post a patch for it? I will drop it from my series.

Also, could you review the other patches in the series as well.

Regards,
Akhil

> Hi Akhil,
> 
> I am not sure if this API is ready to be stable so I will add few comments here:
> 
> RSA:
> 	rte_crypto_param message;
> 	...
> 	 * - to be signed for RSA sign generation.
> 
> If this message is plaintext, then in case of:
> 1) PKCS1_1.5 padding:
> Standard defines data to be signed as DER encoded struct of digestAlgorithm
> + digest
> (few exceptions I am aware of were TLS prior to 1.2 or IKE version 1)
> - There is no field to specify that, even if PMD would be correctly
> implemented it still would lack information about hash aglorithm.
> - Currently what openssl pmd for example is doing is RSA_private_encrypt
> which omits this step (https://www.openssl.org/docs/man1.1.1/man3/RSA_private_encrypt.html  - mentions this).
> 2) PADDING_NONE:
> I cannot find what user is supposed to do in this case, and I think it may be
> quite common option for hw due to reliance on strong CSPRNG for PSS or
> OAEP.
> 
> DSA:
> 	struct rte_crypto_dsa_op_param {
> 	...
> There is no 'k' parameter? I though I have added it, how hw with no CSRNG
> should work with DSA?
> 
> For ECDSA private key is in Op, for DSA is in xform. Where this inconsistency
> comes from?
> 
> 	/**< x: Private key of the signer in octet-string network
> 	 * byte order format.
> 	 * Used when app has pre-defined private key.
> 	 * Valid only when xform chain is DSA ONLY.
> 	 * if xform chain is DH private key generate + DSA, then DSA sign
> 	 * compute will use internally generated key.
> 
> And this one I cannot understand, there is DH and DSA in one line plus seems
> that private dsa key would be generated and used in the same operation.
> We want to create self-signed certificate here on the fly or something?
> 
> 	RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> 	/**< DH Private Key generation operation */
> 
> This is another interesting part (similar to 'k' in (EC)DSA, PSS, QAEO in RSA),
> there was no any type of hw random number generation concept for
> symmetric crypto (i.e. salt, IV, nonce) and here we have
> standalone Diffie Hellman private key generator.
> And since it is no crypto computation but random number generation,
> maybe there should be another module to handle CSRNG or we could
> register randomness
> source into cryptodev, like callback? Another option would be to predefine
> randomness source per device like (i.e. x86 RDRAND, /dev/random) for user
> to decide.
> 
> Additionally there is DH op but there is no ECDH (I know there is ECPM, but
> the same way there is MODEXP which creates another inconsistency).
> Optionally we can extend DH API to work with EC?
> EDDSA, EDDH needs to be implemented soon too.
> 
> Regards,
> Arek
  
Arkadiusz Kusztal Sept. 7, 2021, 11:42 a.m. UTC | #3
> Do you think all the asym APIs are not eligible for promoting it to stable APIs?
> I haven't seen any changes for quite some time and we cannot have it
> experimental Forever.
> The APIs which you think are expected to change, we can leave them as
> experimental And mark the others as stable.
We could potentially make capability related functions stable but for others there are still some many uncertainties, another example:
Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even have function that accepts 'k' (although optionally inverse of k yes), what should user put then here?

This API needs more attention I believe, I can send patches for it after 21.11 release. 
My opinion is that we should push all this by another year.

> 
> Can you post a patch for it? I will drop it from my series.
> 
> Also, could you review the other patches in the series as well.
> 
> Regards,
> Akhil
> 
> > Hi Akhil,
> >
> > I am not sure if this API is ready to be stable so I will add few comments here:
> >
> > RSA:
> > 	rte_crypto_param message;
> > 	...
> > 	 * - to be signed for RSA sign generation.
> >
> > If this message is plaintext, then in case of:
> > 1) PKCS1_1.5 padding:
> > Standard defines data to be signed as DER encoded struct of
> > digestAlgorithm
> > + digest
> > (few exceptions I am aware of were TLS prior to 1.2 or IKE version 1)
> > - There is no field to specify that, even if PMD would be correctly
> > implemented it still would lack information about hash aglorithm.
> > - Currently what openssl pmd for example is doing is
> > RSA_private_encrypt which omits this step
> (https://www.openssl.org/docs/man1.1.1/man3/RSA_private_encrypt.html  -
> mentions this).
> > 2) PADDING_NONE:
> > I cannot find what user is supposed to do in this case, and I think it
> > may be quite common option for hw due to reliance on strong CSPRNG for
> > PSS or OAEP.
> >
> > DSA:
> > 	struct rte_crypto_dsa_op_param {
> > 	...
> > There is no 'k' parameter? I though I have added it, how hw with no
> > CSRNG should work with DSA?
> >
> > For ECDSA private key is in Op, for DSA is in xform. Where this
> > inconsistency comes from?
> >
> > 	/**< x: Private key of the signer in octet-string network
> > 	 * byte order format.
> > 	 * Used when app has pre-defined private key.
> > 	 * Valid only when xform chain is DSA ONLY.
> > 	 * if xform chain is DH private key generate + DSA, then DSA sign
> > 	 * compute will use internally generated key.
> >
> > And this one I cannot understand, there is DH and DSA in one line plus
> > seems that private dsa key would be generated and used in the same
> operation.
> > We want to create self-signed certificate here on the fly or something?
> >
> > 	RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > 	/**< DH Private Key generation operation */
> >
> > This is another interesting part (similar to 'k' in (EC)DSA, PSS, QAEO
> > in RSA), there was no any type of hw random number generation concept
> > for symmetric crypto (i.e. salt, IV, nonce) and here we have
> > standalone Diffie Hellman private key generator.
> > And since it is no crypto computation but random number generation,
> > maybe there should be another module to handle CSRNG or we could
> > register randomness source into cryptodev, like callback? Another
> > option would be to predefine randomness source per device like (i.e.
> > x86 RDRAND, /dev/random) for user to decide.
> >
> > Additionally there is DH op but there is no ECDH (I know there is
> > ECPM, but the same way there is MODEXP which creates another
> inconsistency).
> > Optionally we can extend DH API to work with EC?
> > EDDSA, EDDH needs to be implemented soon too.
> >
> > Regards,
> > Arek
  
Akhil Goyal Sept. 7, 2021, 11:45 a.m. UTC | #4
> > Do you think all the asym APIs are not eligible for promoting it to stable
> APIs?
> > I haven't seen any changes for quite some time and we cannot have it
> > experimental Forever.
> > The APIs which you think are expected to change, we can leave them as
> > experimental And mark the others as stable.
> We could potentially make capability related functions stable but for others
> there are still some many uncertainties, another example:
> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even
> have function that accepts 'k' (although optionally inverse of k yes), what
> should user put then here?
> 
> This API needs more attention I believe, I can send patches for it after 21.11
> release.
> My opinion is that we should push all this by another year.
> 
Ok will drop this patch for now.
  
Ray Kinsella Sept. 8, 2021, 12:37 p.m. UTC | #5
Folks,

On 07/09/2021 12:45, Akhil Goyal wrote:
>>> Do you think all the asym APIs are not eligible for promoting it to stable
>> APIs?
>>> I haven't seen any changes for quite some time and we cannot have it
>>> experimental Forever.
>>> The APIs which you think are expected to change, we can leave them as
>>> experimental And mark the others as stable.
>> We could potentially make capability related functions stable but for others
>> there are still some many uncertainties, another example:
>> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even
>> have function that accepts 'k' (although optionally inverse of k yes), what
>> should user put then here?
>>
>> This API needs more attention I believe, I can send patches for it after 21.11
>> release.
>> My opinion is that we should push all this by another year.
>>
> Ok will drop this patch for now.
> 

Look since everyone is in alignment here, I am going to ask the symbol bot to ignore
the asym crypto APIs for the next year. Thanks for the diligence on this, and thanks to 
Fan for sending me an FYI.

Ray K
  
Akhil Goyal Feb. 2, 2023, 10:49 a.m. UTC | #6
Folks,

Can we promote the Asym APIs to stable now?

Regards,
Akhil

> 
> On 07/09/2021 12:45, Akhil Goyal wrote:
> >>> Do you think all the asym APIs are not eligible for promoting it to stable
> >> APIs?
> >>> I haven't seen any changes for quite some time and we cannot have it
> >>> experimental Forever.
> >>> The APIs which you think are expected to change, we can leave them as
> >>> experimental And mark the others as stable.
> >> We could potentially make capability related functions stable but for others
> >> there are still some many uncertainties, another example:
> >> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even
> >> have function that accepts 'k' (although optionally inverse of k yes), what
> >> should user put then here?
> >>
> >> This API needs more attention I believe, I can send patches for it after 21.11
> >> release.
> >> My opinion is that we should push all this by another year.
> >>
> > Ok will drop this patch for now.
> >
> 
> Look since everyone is in alignment here, I am going to ask the symbol bot to
> ignore
> the asym crypto APIs for the next year. Thanks for the diligence on this, and
> thanks to
> Fan for sending me an FYI.
> 
> Ray K
  
Hemant Agrawal Feb. 2, 2023, 11:02 a.m. UTC | #7
Hi Akhil

> 
> Can we promote the Asym APIs to stable now?

+1 for it


> 
> Regards,
> Akhil
> 
> >
> > On 07/09/2021 12:45, Akhil Goyal wrote:
> > >>> Do you think all the asym APIs are not eligible for promoting it
> > >>> to stable
> > >> APIs?
> > >>> I haven't seen any changes for quite some time and we cannot have
> > >>> it experimental Forever.
> > >>> The APIs which you think are expected to change, we can leave them
> > >>> as experimental And mark the others as stable.
> > >> We could potentially make capability related functions stable but
> > >> for others there are still some many uncertainties, another example:
> > >> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd
> > >> will not even have function that accepts 'k' (although optionally
> > >> inverse of k yes), what should user put then here?
> > >>
> > >> This API needs more attention I believe, I can send patches for it
> > >> after 21.11 release.
> > >> My opinion is that we should push all this by another year.
> > >>
> > > Ok will drop this patch for now.
> > >
> >
> > Look since everyone is in alignment here, I am going to ask the symbol
> > bot to ignore the asym crypto APIs for the next year. Thanks for the
> > diligence on this, and thanks to Fan for sending me an FYI.
> >
> > Ray K
  
Arkadiusz Kusztal Feb. 14, 2023, 6:05 p.m. UTC | #8
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, February 2, 2023 11:49 AM
> To: Kinsella, Ray <mdr@ashroe.eu>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>; De Lara
> Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> matan@nvidia.com; g.singh@nxp.com; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; David Marchand <david.marchand@redhat.com>
> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to
> stable
> 
>  Folks,
> 
> Can we promote the Asym APIs to stable now?

Yes, I think we cannot keep it as experimental forever. Although there is still much work to be done starting with capabilities and tests, I will describe it in separate threads.

> 
> Regards,
> Akhil
> 
> >
> > On 07/09/2021 12:45, Akhil Goyal wrote:
> > >>> Do you think all the asym APIs are not eligible for promoting it
> > >>> to stable
> > >> APIs?
> > >>> I haven't seen any changes for quite some time and we cannot have
> > >>> it experimental Forever.
> > >>> The APIs which you think are expected to change, we can leave them
> > >>> as experimental And mark the others as stable.
> > >> We could potentially make capability related functions stable but
> > >> for others there are still some many uncertainties, another example:
> > >> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd
> > >> will not even have function that accepts 'k' (although optionally
> > >> inverse of k yes), what should user put then here?
> > >>
> > >> This API needs more attention I believe, I can send patches for it
> > >> after 21.11 release.
> > >> My opinion is that we should push all this by another year.
> > >>
> > > Ok will drop this patch for now.
> > >
> >
> > Look since everyone is in alignment here, I am going to ask the symbol
> > bot to ignore the asym crypto APIs for the next year. Thanks for the
> > diligence on this, and thanks to Fan for sending me an FYI.
> >
> > Ray K
  

Patch

diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 11f4e6fdbf..425f459143 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -247,7 +247,6 @@  rte_cryptodev_sym_capability_get(uint8_t dev_id,
  *   - Return description of the asymmetric crypto capability if exist.
  *   - Return NULL if the capability not exist.
  */
-__rte_experimental
 const struct rte_cryptodev_asymmetric_xform_capability *
 rte_cryptodev_asym_capability_get(uint8_t dev_id,
 		const struct rte_cryptodev_asym_capability_idx *idx);
@@ -317,7 +316,6 @@  rte_cryptodev_sym_capability_check_aead(
  *   - Return 1 if the op type is supported
  *   - Return 0 if unsupported
  */
-__rte_experimental
 int
 rte_cryptodev_asym_xform_capability_check_optype(
 	const struct rte_cryptodev_asymmetric_xform_capability *capability,
@@ -333,7 +331,6 @@  rte_cryptodev_asym_xform_capability_check_optype(
  *   - Return 0 if the parameters are in range of the capability.
  *   - Return -1 if the parameters are out of range of the capability.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_xform_capability_check_modlen(
 	const struct rte_cryptodev_asymmetric_xform_capability *capability,
@@ -395,7 +392,6 @@  rte_cryptodev_get_aead_algo_enum(enum rte_crypto_aead_algorithm *algo_enum,
  * - Return -1 if string is not valid
  * - Return 0 if the string is valid
  */
-__rte_experimental
 int
 rte_cryptodev_asym_get_xform_enum(enum rte_crypto_asym_xform_type *xform_enum,
 		const char *xform_string);
@@ -1192,7 +1188,6 @@  rte_cryptodev_sym_session_create(struct rte_mempool *mempool);
  *  - On success return pointer to asym-session
  *  - On failure returns NULL
  */
-__rte_experimental
 struct rte_cryptodev_asym_session *
 rte_cryptodev_asym_session_create(struct rte_mempool *mempool);
 
@@ -1223,7 +1218,6 @@  rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess);
  *  - -EINVAL if session is NULL.
  *  - -EBUSY if not all device private data has been freed.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess);
 
@@ -1264,7 +1258,6 @@  rte_cryptodev_sym_session_init(uint8_t dev_id,
  *  - -ENOTSUP if crypto device does not support the crypto transform.
  *  - -ENOMEM if the private session could not be allocated.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_session_init(uint8_t dev_id,
 			struct rte_cryptodev_asym_session *sess,
@@ -1299,7 +1292,6 @@  rte_cryptodev_sym_session_clear(uint8_t dev_id,
  *  - 0 if successful.
  *  - -EINVAL if device is invalid or session is NULL.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_session_clear(uint8_t dev_id,
 			struct rte_cryptodev_asym_session *sess);
@@ -1336,7 +1328,6 @@  rte_cryptodev_sym_get_existing_header_session_size(
  * @return
  *   Size of the asymmetric header session.
  */
-__rte_experimental
 unsigned int
 rte_cryptodev_asym_get_header_session_size(void);
 
@@ -1364,7 +1355,6 @@  rte_cryptodev_sym_get_private_session_size(uint8_t dev_id);
  *   - Size of the asymmetric private data, if successful
  *   - 0 if device is invalid or does not have private session
  */
-__rte_experimental
 unsigned int
 rte_cryptodev_asym_get_private_session_size(uint8_t dev_id);
 
diff --git a/lib/cryptodev/version.map b/lib/cryptodev/version.map
index 9f04737aed..707a2e32d3 100644
--- a/lib/cryptodev/version.map
+++ b/lib/cryptodev/version.map
@@ -1,4 +1,4 @@ 
-DPDK_21 {
+DPDK_22 {
 	global:
 
 	rte_crypto_aead_algorithm_strings;
@@ -9,6 +9,18 @@  DPDK_21 {
 	rte_crypto_cipher_operation_strings;
 	rte_crypto_op_pool_create;
 	rte_cryptodev_allocate_driver;
+
+	rte_cryptodev_asym_capability_get;
+	rte_cryptodev_asym_get_header_session_size;
+	rte_cryptodev_asym_get_private_session_size;
+	rte_cryptodev_asym_get_xform_enum;
+	rte_cryptodev_asym_session_clear;
+	rte_cryptodev_asym_session_create;
+	rte_cryptodev_asym_session_free;
+	rte_cryptodev_asym_session_init;
+	rte_cryptodev_asym_xform_capability_check_modlen;
+	rte_cryptodev_asym_xform_capability_check_optype;
+
 	rte_cryptodev_callback_register;
 	rte_cryptodev_callback_unregister;
 	rte_cryptodev_close;
@@ -61,16 +73,6 @@  DPDK_21 {
 EXPERIMENTAL {
 	global:
 
-	rte_cryptodev_asym_capability_get;
-	rte_cryptodev_asym_get_header_session_size;
-	rte_cryptodev_asym_get_private_session_size;
-	rte_cryptodev_asym_get_xform_enum;
-	rte_cryptodev_asym_session_clear;
-	rte_cryptodev_asym_session_create;
-	rte_cryptodev_asym_session_free;
-	rte_cryptodev_asym_session_init;
-	rte_cryptodev_asym_xform_capability_check_modlen;
-	rte_cryptodev_asym_xform_capability_check_optype;
 	rte_cryptodev_sym_cpu_crypto_process;
 	rte_cryptodev_sym_get_existing_header_session_size;
 	rte_cryptodev_sym_session_get_user_data;