Initial fetching of MD and Cipher objects from OpenSSL(3)

[beldmit]

We need init them and free them in one place to avoid threading issues.

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• Does this PR change the the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

[beldmit]

Related: #1427

[baentsch]

We need init them and free them in one place to avoid threading issues.

What about using

liboqs/src/common/common.c

Line 197 in b1d42d6

OQS_API void OQS_init(void) {

for the former and adding a function for the latter? Probably easier than the alternative of adding init and close to all OQS operations.

[beldmit]

Thanks! It's quite reasonable

[baentsch]

Thanks! It's quite reasonable

But then again -- is this really sufficient? Are these EVP objects thread-safe? What if someone calls in different threads different OpenSSL APIs, all reaching into oqsprovider which in turn calls back into OpenSSL in this way?

The documentation states

However most OpenSSL data structures are not thread-safe.

[beldmit]

EVP_MD and EVP_CIPHER objects should be thread-safe - they are just sets of callbacks and flags. The problem may happen if the provider providing them is unloaded.

[baentsch]

The problem may happen if the provider providing them is unloaded.

Good. I don't see how this should be facilitated by an application that is using liboqs directly: liboqs doesn't make available (doesn't know about :) OSSL_PROVIDER or OSSL_LIB_CTX, so how could an unload happen?

If oqsprovider in turn triggers usage of liboqs what about checking there that a suitable provider for SHA/AES is loaded/available (in the same OSSL_LIB_CTX)? But yikes, then the proposed implementation passing NULL to the EVP_fetch routines doesn't cut it.... :-(

[beldmit]

I groomed the code according my understanding of how it goes.
The tests work for me.

Consequence: OQS_init call becomes mandatory

[beldmit]

At this point I'm waiting for your feedback whether we should follow this direction as a whole or not

[baentsch]

At this point I'm waiting for your feedback whether we should follow this direction as a whole or not

As far as I'm concerned, it does. The one thing not looking good (also as per CI) is that the ossl_helper functions should be namespaced: Proposal would be "oqs_ossl_xyz" (instead of just "_xyz")

[beldmit]

I will adjust namespace, but also init and free functions must be documented as mandatory...

[baentsch]

Thank you very much again for your contribution: Very much appreciated! Don't feel stopped to address/comment on the single comments I just made -- but if you have more urgent things to do, we'll follow up accordingly.

[beldmit]

I'm going to add OQS_destroy function and call it in the tests. I'd like to finalize this issue if we have an agreement what should be done

[baentsch]

I'm going to add OQS_destroy function and call it in the tests. I'd like to finalize this issue if we have an agreement what should be done

Good for me -- provided the comment here, gets updated (making the invocation of OQS_init a requirement for correct use of the library). Thanks again.

[beldmit]

So. I finished what I understood was worth doing. Could you please specify what are the next steps to deal with it? Is there anything to be done by me just now or it can be merged as is and we could move forward?

[baentsch]

nit: typo. Can deal with when doing next doc review. Would also recommend phrasing more strongly along the lines of "Needs to be invoked to ensure proper operation of library."

[baentsch]

Is there anything to be done by me just now

Not really. Thanks again for the contribution.

or it can be merged as is and we could move forward?

Yes from my perspective but I leave the merge to another team member (@dstebila : Back again?)

[sahanaprasad07]

Hi @beldmit , would it make sense to replace EVP_aes_256_ecb() in src/common/rand/rand_nist.c?

[beldmit]

Probably yes, if this code is used multiple times.

[dstebila]

Hi @beldmit , would it make sense to replace EVP_aes_256_ecb() in src/common/rand/rand_nist.c?

Probably yes, if this code is used multiple times.

Okay, done in 87780d2. Will merge once CI passes.