Include use of loaded 'libctx' context in KEM and SIG procedures (#557)

[RodriM11]

Inclusion of use of OSSL_LIB_CTX * context in which the oqsprovider is loaded to KeyGen, Encaps, Decaps (KEM) and KeyGen, Sign, Verify (SIG) operations.

This PR is associated with issue #557. I apologize for the delay in the commit.

[baentsch]

Thanks very much for the contribution @RodriM11 ! Just wondering while CI tests this: Would it be feasible to add a test validating this improvement?

[RodriM11]

I wondered that myself when coding it and here is what I came up with. I tested it executing the available test (introducing minor variations to the DRBG procedure, for example) and analyzing the code executed to check that indeed was performing as I expected (but this is clearly not a way to test this). Since the modifications are pertaining to internal structures and not ones directly accessible in a test, it gets harder to do so. The modifications amount to use the OSSL_LIB_CTX * in which the provider is loaded (or, more accurately, a child copy of it) as the context under which the relevant cryptographic operations are performed, instead of the NULL default context. There can be a number of relevant feats one can exploit from this behavior, but perhaps the more visible (useful) is the context from which pseudorandom elements are generated. As such, a possible way to test this modification would be: to create a test that loads a purely deterministic DRBG procedure, and do some 'KAT' like approach in which the test verifies that it always generates the same values (as this would mean that, on every cryptographic procedure with pseudo randomness generation, the modification is working as expected, as I am able to directly set whatever DRBG I wish to). However, this has two inconveniences, IMO:

1. It requires to generate, store and (possibly) updates these values, for every combination and cryptographic element possible.
2. Since this behavior only affects classic elements (the PQ pseudo randomness is managed by liboqs own DRBG), it would be required to separate the classic from the PQ elements and check accordingly. This is no problem for cryptographic keys, but encapsulations, shared secrets and signatures would be much more tedious.
3. It might not capture other uses of the library context that are not directly tied to DRBG procedures.

To have a test of this behavior without having to incur on these problems, I would propose to construct the test in the following way:

1. As a solution to 1., the actual value that is generated is not actually needed, as the test only needs to check that the generation is deterministic (i.e., two executions generate the same result). Therefore the test could be to generate each cryptographic element twice (under a deterministic pseudorandom generation) and check they match.
2. As a solution to 2., the test could also load a deterministic pseudorandom generator to liboqs. This way, it would avoid the problem of having to separate between classic and PQ elements. This approach, however, has a minor caveat IMO: while the test could load a custom DRBG and then, at the end of it, load the default DRBG of OQS (which should be an appropriate behavior for a standard use of liboqs), it could mean that if somebody were to load another custom DRBG before executing the tests, this DRBG would be 'unloaded'. It also has the inconvenience of having to directly access liboqs on the test, which is something that migth be desired to be avoided.

[baentsch]

Thanks for your thoughts pertaining as to how to test this @RodriM11 . I agree that "toggling" DRBG in specific libctxs (other than default) is a good idea how to check this.

Pertaining to the 3 initial "inconveniences" you see, allow me to ask whether they are really problematic, or rather, can be simplified:

1. This would only be necessary if you'd want to test all algorithms; but this is arguably not necessary as oqsprovider is completely algorithm agnostic; so a single (algorithm) test would suffice -- and also there, arguably only OK/NOK with different DRBGs would be sufficient, no? That would do away with the need to maintain lots of KATs; a single one would suffice.
2. Again, as per the above wouldn't it be sufficient to select one algorithm utilizing a DRBG that is easiest to toggle (arguably the one of openssl, probably speaking for a hybrid alg to the chosen, say X25519MLKEM768)?
3. Agreed, but again, this should be a mechanism test, not a complete "all provider functionalities" test: For the latter, I'd trust the openssl logic to do the right thing: If it were not, that'd be a bigger problem than one of oqsprovider :-/

Would these thoughts simplify things? I'd much rather have 1 "basic" test than none at all -- although I'd still argue it be pretty complete considering the design of the system within which it runs.

[RodriM11]

Thank you for your insight @baentsch! I agree with you on the DRBG part, in fact I think an instantiation of a libctx context with openssl's TEST-RAND would be enough to test the functionality.
My comment regarding the amount of KATs to be stored was related to the fact that I didn't want to assume that, say ML-KEM-768, was available, and therefore, had to create the KATs for every possible combination (I would argue also that, at least, I would like the test to "test" both a KEM and a SIG, both with NIST's P and X25519 or X448, for completeness). But as I said, I think KATs are not even necessary, so that part is not a problem IMO, since I don't "care" for the specific value, I only would only want to test that, if I infuse a deterministic pseudorandom generator, the cryptographic operations are deterministic (and, therefore, two consecutive executions yield the same result, for example).
The part that I am less sure about is 2. If it's okey for the test to toggle with liboqs DRBG, then the test can be done easily. If not, either the test needs to separate, for every single KEM or SIG available, the classical part from the PQ, and check for the deterministic behavior in it. Or, construct the test only for two (one KEM, one SIG) easy-to-toggle primitives and only be executed if they are available.

[baentsch]

was related to the fact that I didn't want to assume that, say ML-KEM-768, was available

You're right: That's a very fair point/assumption I didn't consider.

only want to test that, if I infuse a deterministic pseudorandom generator, the cryptographic operations are deterministic (and, therefore, two consecutive executions yield the same result, for example)

Sounds like a good plan.

If it's okey for the test to toggle with liboqs DRBG

Well, is this sufficient (for testing the new code)? I'm a bit unsure as to whether there is a relation between the liboqs DRBG and the openssl libctx,,, Isn't liboqs always using the DRBG of the default context?

[RodriM11]

Yes, you are absolutely right. The improvement of adding the libctx context in oqsprovider is only meant to influence the classical parts of the hybrid generation, the PQ, as you noted, are dependent upon a completely different and unrelated context (in fact, the NULL default context).
What I meant by toggling with liboqs DRBG in the test is to use the procedure OQS_randombytes_custom_algorithm to establish a deterministic pseudorandom generator in the PQ part as well, so the test generates fully deterministic outputs in both the classical (by the new improvement + establishing a deterministic DRBG for the test's sake) and the PQ, so the test can avoid the need for separating the classical and PQ parts and checking that the classical (the ones the improvement is actually about) are indeed the same, as the output as a whole would be deterministic (to then later establish the default DRBG). But I understand that this might be somewhat undesired, as it would mean toggling with liboqs DRBG just to deeply simplify the test, and also the inconvenience of plausible custom DRBGs loaded previously (which should not be the case in standard constructions of liboqs, but nonetheless)

[beldmit]

Will this change also affect the fetching properties (e.g. fips=yes)?

[RodriM11]

No, it will not. The inclusion made is pertaining to cases in which cryptographic operations are to be performed, impacting elements like the DRBG to consume and so on, and no fetching property was used in those (and none is established now). It is true though that, with these changes, the API's used to use the libctx context always allow to use property queries within these procedures (instead of the NULL value established in this commit), in case future developments would need to use those for any reason.

[baentsch]

so the test can avoid the need for separating the classical and PQ parts

ahh -- then I get it. Sensible approach to make things easier. Thanks for the idea and explanation. Looking forward to seeing the test then!

Completely independent of this, of course, is the (at least to me only now obvious) shortcoming of liboqs in terms of (not) honoring a libctx. But that's a discussion for another day maybe: It surely serves to highlight again the benefit of "natively" integrating crypto into openssl proper/default provider and not via a "loosely coupled" external library, i.e., where openssl is just but one of several crypto backends.

[RodriM11]

I have added the test, along with the correct LLVM formatting (I apologize for my first commit).
I have run every test and have encountered a 'weird' behavior: I executed every test, modifying the DRBG of the libctx context to use, and checked that it was used by every procedure. This was indeed what happened, except for a subroutine of one of the tests (oqs_test_endecode). After checking in more detail, I was able to tie the behavior to the function PKCS8_encrypt_ex of p8info_to_encp8. After running some more tests with additional debug info, I saw that the libctx context the process is using (which should be tied to the libctx of the provider itself) did not have the DRBG I had instantiated in the test (but did not understand why). I am sure that this is due to a lack of understanding of some part of the procedure on my behalf, but wanted to note it just in case (and is not actually related to the changes of this issue, I believe).

Regarding liboqs DRBG consumption (openssl), yes I agree (I have been thinking about it for the last couple days as well). I could open an issue there to discuss it, if you want, but as a fleeting thought: regarding the use of a libctx instead of the standard NULL library context, this on itself could also be added without major problems (I wouldn't mind taking a swing at adding a PR of it). The "tricky" part would be how to be able to personalize it (if desired): on oqsprovider, that is not necessary since the provider is being loaded onto a library context, and it should be expected that this context would have been personalized by the code loading the provider. On liboqs it would be different, and it might be of interest to add certain personalization capacities. We can discuss them in more detail there, if desired.

[baentsch]

Please see the single comment; more problematic is that this code base does not permit a clean build as per this error message:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
LIBOQS_CRYPTO_LIBRARY
    linked by target "oqs_test_libctx" in directory /home/mib/git/rodrim11/oqs-provider/test

--> Is something not checked in? To reproduce, I cloned your repo clean and ran scripts/fullbuild.sh there...

[RodriM11]

I was checking the construction too. The problem is that I was both making a poor choice as to how to link liboqs in the test, and incurred in a deeper problem that I hadn't tested: the test modifies liboqs behavior in terms of DRBG, but in the case that the provider is built as a shared module, but liboqs is built statically, since I cannot link against oqsprovider in the test (it is a Module), there was a second liboqs copy in place and the modification did not carry to one of them. I have modified the test construction to only be added if either the provider is static, or liboqs is dynamic, but I would gladly take suggestions if there is a better alternative (I apologize).

The OQS_USE_OPENSSL is the variable from liboqs, I am using it to establish the expected default DRBG back (which is OpenSSL's if OQS_USE_OPENSSL is ON, and System otherwise).

[RodriM11]

I apologize for the amount of commits (I can close this PR and create a "clean" one if desired). Turns out that the way I constructed the test was not "random" enough for RSA procedures, as I was always relying on a fixed, constant string of all equal bytes (spent more time that I should have today realizing it haha). I modified the test to "reset" on each iteration, and using the fact that TEST-RAND, if uninitialized, lets you start from the same entropy values, was enough to work on RSA too (my liboqs construction did not have all algorithms activated and was not activating any with and RSA hybrid procedure).

[baentsch]

Thanks for these improvements! The PR branch now builds for me in the default config again.

I have modified the test construction to only be added if either the provider is static, or liboqs is dynamic, but I would gladly take suggestions if there is a better alternative

Ah, then I understand why the test is not run when doing a default build (shared lib provider, statically linked liboqs symbols). Now, I'm not sure as part of which CI test the new test would be executed: just triggered a full run so you can check and point it out if/where run-- if not, please consider adding a (github-based CI) build config that causes the test to be run. Or if you're unfamiliar with GH CI, just let me know how the build should be done so I can run it locally and add it to CI.

I apologize for the amount of commits (I can close this PR and create a "clean" one if desired)

No reason to apologize: Things improve over time and that's fair to be visible (and I'd squash-merge eventually anyway). The one reason why a new "single-commit" PR might be good were if you wanted to get the DCO "test" to pass (see open-quantum-safe/liboqs#1760 for info; I keep apologizing for this and don't think this is sensible, but LinuxFoundation demanded use of this procedure (in essence, always adding "-s" to each commit) as part of their take-over of this project.

[RodriM11]

I have checked the CI tests and, except from the first CircleCI tests (in which it was being run), no other CI I saw actually compiled the test (since, as you pointed out, the default construction, shared osprovider + static liboqs, is precisely the exception to the test construction). I was not comfortable with the fact that certain constructions would not test a behavior that would later appear in their code, and didn't want to force you to modify the present CI configuration just for the sake of my changes, so I have gone through the process of avoiding the need to toggle liboqs DRBG in the test, and have added it to the branch (once the tests are run, if you want, I can proceed with a clean PR, to account for the DCO checks). The test I made required of certain code snippets from another test (oqs_test_evp_pkey_params), so in order to avoid code duplication, I have taken the liberty of grouping common code in test_common, I hope that is alright. The new test is more lengthy that what I initially desired, but it is required to be able to check that EVP procedures inherit the properties the library context in which the provider is loaded, with regards to the DRBG at least.

[baentsch]

I can proceed with a clean PR

That indeed would be good. I basically now can follow the jist of the new test and agree that it tests what it should. Also, it now builds by default, and it shows your new libctx improvement passes (and fails on the original code). Good work & Thanks again!

I have some comments on the code that I'll still add to this PR so you could address them before proceeding to a "clean, ready-to-merge PR": OK?

[RodriM11]

Yes, sounds good.

[baentsch]

Many nits, conceptually this all looks good. Looking forward to the "final PR". Thanks again, @RodriM11 and have a good start into 2025!

[RodriM11]

I have added the requested changes in this PR in case any further revision is desired. I will proceed to close this PR and open a new, clean one if no further changes are requested. Thanks for your help, and happy holidays!

[baentsch]

I have added the requested changes in this PR in case any further revision is desired. I will proceed to close this PR and open a new, clean one if no further changes are requested. Thanks for your help, and happy holidays!

Thanks @RodriM11 -- not all my PR comments have been addressed, so you may want to take another look before doing a "clean" PR. Happy New Year also to you!

[RodriM11]

I think that the comments you highlighted were actually addressed on my last commit (nit typos, the ret_rev variable was deleted and so on). Please do correct me if I am wrong, and otherwise I will submit the clean PR (thank you for your help and comments!)

[baentsch]

My question pertains to why this is required to be statically allocated and not passed as parameter?

[baentsch]

Only one question remaining pertaining to "libctx" being a (file-)global static variable: Why can't it be a stack-only parameter passed between (test) functions? Otherwise, all good for me: Please do a final PR for this now. Accordingly closing this PR for now.