Excess import/export operations

[paulidale]

#14126 notes that import/export between key managers is happening even when the key managers are the same. This should be addressed.

This is most noticeable when running with the no-cached-fetch option but it will also occur if the fetch cache is flushed.

[levitte]

I did have a thought about methods being able to show "kinship" with each other, but that turned out to be a complex idea when I talked about it, so we ditched it.

I fail to see an operation cache flush as an issue. When the cache is flushed, there's nothing left to compare with. Or are you thinking of selective/partial flushing? Is that something for 3.0?

[paulidale]

fetch cache. If that is flushed, the next fetch will return a different object and the import/export issues appear.

[levitte]

We should probably rethink what we consider to be "the same", in terms of methods (like EVP_KEYMGMT). It's true that we may find ourselves having multiple copies of the same method at times, and our current comparison of direct EVP_KEYMGMT pointers becomes flawed in that situation.

So, another thought we might want to consider is trusting that dlopen() and similar will try very hard not to load a .so / .dll into memory more than once, meaning that its code sections can be seen as fairly unique and constant. So, instead of comparing keymgmt1 against keymgmt2, we might want to consider comparing keymgmt1->free against keymgmt2->free.

In current code, it could look as simple as this:

diff --git a/crypto/evp/keymgmt_lib.c b/crypto/evp/keymgmt_lib.c
index 0c643b3b49..2730caf10b 100644
--- a/crypto/evp/keymgmt_lib.c
+++ b/crypto/evp/keymgmt_lib.c
@@ -217,7 +217,7 @@ size_t evp_keymgmt_util_find_operation_cache_index(EVP_PKEY *pk,
     size_t i, end = OSSL_NELEM(pk->operation_cache);
 
     for (i = 0; i < end && pk->operation_cache[i].keymgmt != NULL; i++) {
-        if (keymgmt == pk->operation_cache[i].keymgmt)
+        if (keymgmt->free == pk->operation_cache[i].keymgmt->free)
             break;
     }
 

[levitte]

(I picked keymgmt->free, because it's singularly required to be present by keymgmt_from_dispatch())

[mattcaswell]

This seems dubious to me. It is entirely possible that different key managers have a common free function. In fact we already have this in our code:

openssl/providers/implementations/keymgmt/ecx_kmgmt.c

Lines 749 to 777 in 13888e7

	#define MAKE_KEYMGMT_FUNCTIONS(alg) \
	const OSSL_DISPATCH ossl_##alg##_keymgmt_functions[] = { \
	{ OSSL_FUNC_KEYMGMT_NEW, (void (*)(void))alg##_new_key }, \
	{ OSSL_FUNC_KEYMGMT_FREE, (void (*)(void))ecx_key_free }, \
	{ OSSL_FUNC_KEYMGMT_GET_PARAMS, (void (*) (void))alg##_get_params }, \
	{ OSSL_FUNC_KEYMGMT_GETTABLE_PARAMS, (void (*) (void))alg##_gettable_params }, \
	{ OSSL_FUNC_KEYMGMT_SET_PARAMS, (void (*) (void))alg##_set_params }, \
	{ OSSL_FUNC_KEYMGMT_SETTABLE_PARAMS, (void (*) (void))alg##_settable_params }, \
	{ OSSL_FUNC_KEYMGMT_HAS, (void (*)(void))ecx_has }, \
	{ OSSL_FUNC_KEYMGMT_MATCH, (void (*)(void))ecx_match }, \
	{ OSSL_FUNC_KEYMGMT_VALIDATE, (void (*)(void))alg##_validate }, \
	{ OSSL_FUNC_KEYMGMT_IMPORT, (void (*)(void))ecx_import }, \
	{ OSSL_FUNC_KEYMGMT_IMPORT_TYPES, (void (*)(void))ecx_imexport_types }, \
	{ OSSL_FUNC_KEYMGMT_EXPORT, (void (*)(void))ecx_export }, \
	{ OSSL_FUNC_KEYMGMT_EXPORT_TYPES, (void (*)(void))ecx_imexport_types }, \
	{ OSSL_FUNC_KEYMGMT_GEN_INIT, (void (*)(void))alg##_gen_init }, \
	{ OSSL_FUNC_KEYMGMT_GEN_SET_PARAMS, (void (*)(void))ecx_gen_set_params }, \
	{ OSSL_FUNC_KEYMGMT_GEN_SETTABLE_PARAMS, \
	(void (*)(void))ecx_gen_settable_params }, \
	{ OSSL_FUNC_KEYMGMT_GEN, (void (*)(void))alg##_gen }, \
	{ OSSL_FUNC_KEYMGMT_GEN_CLEANUP, (void (*)(void))ecx_gen_cleanup }, \
	{ OSSL_FUNC_KEYMGMT_LOAD, (void (*)(void))ecx_load }, \
	{ 0, NULL } \
	};
	MAKE_KEYMGMT_FUNCTIONS(x25519)
	MAKE_KEYMGMT_FUNCTIONS(x448)
	MAKE_KEYMGMT_FUNCTIONS(ed25519)
	MAKE_KEYMGMT_FUNCTIONS(ed448)

[paulidale]

Checking all of the function pointers might work better, what we really want is some kind of UID.

[levitte]

We could have something else that must be a unique address... could be a function that does nothing, but just sits for the sake of uniqueness.

What I'd like to avoid is to push the whole UID concept on provider authors other than that. We've already had fantasy discussions on UID coordination between providers that consider each other "kin"... let's avoid that rabbit hole for now, yeah?

[mattcaswell]

We already have a name_id...won't the combination of keymgmt->name_id and keymgmt->prov be sufficient to check equality?

[paulidale]

We already allocate a numeric ID for each thing a provider presents. Couldn't that, along with the library context, act as the UUID?

[mattcaswell]

We already allocate a numeric ID for each thing a provider presents. Couldn't that, along with the library context, act as the UUID?

Isn't that what I just said :-)....except I said name_id and prov. Technically a name_id is only unique within the scope of a libctx. But since we will also need to check the prov and the prov ptr will always be unique even for the same provider in different libctxs, it is sufficient just to check the name_id and prov.

[paulidale]

Yeah it is. I should refresh and read after an interruption :)

[levitte]

Yes! I'm on board for the name_id + prov check.