Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restrict libctx test to oqsprovider #620

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

restrict libctx test to oqsprovider #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32de7e2
limit libctx test to oqsprovider
baentsch Jan 9, 2025
f62d9a5
format fixup
baentsch Jan 9, 2025
4ca62ac
streamline test-propq
baentsch Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions test/oqs_test_endecode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
static OSSL_LIB_CTX *libctx = NULL;
static char *modulename = NULL;
static char *configfile = NULL;
// as different providers may support different key formats, limit testing to
// oqsprovider
static char *testpropq = "provider=oqsprovider";
static OSSL_LIB_CTX *keyctx = NULL;
static OSSL_LIB_CTX *testctx = NULL;

Expand Down Expand Up @@ -64,8 +61,8 @@ static EVP_PKEY *oqstest_make_key(const char *type, EVP_PKEY *template,
}

ctx = (template != NULL)
? EVP_PKEY_CTX_new_from_pkey(keyctx, template, testpropq)
: EVP_PKEY_CTX_new_from_name(keyctx, type, testpropq);
? EVP_PKEY_CTX_new_from_pkey(keyctx, template, OQSPROV_PROPQ)
: EVP_PKEY_CTX_new_from_name(keyctx, type, OQSPROV_PROPQ);

/*
* No real need to check the errors other than for the cascade
Expand Down
4 changes: 1 addition & 3 deletions test/oqs_test_evp_pkey_params.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,7 @@ static OSSL_LIB_CTX *init_openssl(void) {
static EVP_PKEY_CTX *init_EVP_PKEY_CTX(OSSL_LIB_CTX *libctx, const char *alg) {
EVP_PKEY_CTX *ctx;

// make sure we only test oqsprovider
if (!(ctx = EVP_PKEY_CTX_new_from_name(libctx, alg,
"provider=oqsprovider"))) {
if (!(ctx = EVP_PKEY_CTX_new_from_name(libctx, alg, OQSPROV_PROPQ))) {
fprintf(stderr,
cRED "`EVP_PKEY_CTX_new_from_name` failed with algorithm %s: ",
alg);
Expand Down
10 changes: 5 additions & 5 deletions test/oqs_test_kems.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,18 @@ static int test_oqs_kems(const char *kemalg_name) {
// limit testing to oqsprovider as other implementations may support
// different key formats than what is defined by NIST
if (OSSL_PROVIDER_available(libctx, "default")) {
testresult &=
(ctx = EVP_PKEY_CTX_new_from_name(
libctx, kemalg_name, "provider=oqsprovider")) != NULL &&
EVP_PKEY_keygen_init(ctx) && EVP_PKEY_generate(ctx, &key);
testresult &= (ctx = EVP_PKEY_CTX_new_from_name(
libctx, kemalg_name, OQSPROV_PROPQ)) != NULL &&
EVP_PKEY_keygen_init(ctx) && EVP_PKEY_generate(ctx, &key);

if (!testresult)
goto err;
EVP_PKEY_CTX_free(ctx);
ctx = NULL;

testresult &=
(ctx = EVP_PKEY_CTX_new_from_pkey(libctx, key, NULL)) != NULL &&
(ctx = EVP_PKEY_CTX_new_from_pkey(libctx, key, OQSPROV_PROPQ)) !=
NULL &&
EVP_PKEY_encapsulate_init(ctx, NULL) &&
EVP_PKEY_encapsulate(ctx, NULL, &outlen, NULL, &seclen) &&
(out = OPENSSL_malloc(outlen)) != NULL &&
Expand Down
11 changes: 6 additions & 5 deletions test/oqs_test_libctx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ static int oqs_generate_kem_elems(const char *kemalg_name, EVP_PKEY **key,
}

if (OSSL_PROVIDER_available(libctx, "default")) {
testresult = (ctx = EVP_PKEY_CTX_new_from_name(libctx, kemalg_name,
NULL)) != NULL &&
testresult = (ctx = EVP_PKEY_CTX_new_from_name(
libctx, kemalg_name, OQSPROV_PROPQ)) != NULL &&
EVP_PKEY_keygen_init(ctx) && EVP_PKEY_generate(ctx, key);

if (!testresult)
Expand All @@ -168,7 +168,8 @@ static int oqs_generate_kem_elems(const char *kemalg_name, EVP_PKEY **key,
ctx = NULL;

testresult &=
(ctx = EVP_PKEY_CTX_new_from_pkey(libctx, *key, NULL)) != NULL &&
(ctx = EVP_PKEY_CTX_new_from_pkey(libctx, *key, OQSPROV_PROPQ)) !=
NULL &&
EVP_PKEY_encapsulate_init(ctx, NULL) &&
EVP_PKEY_encapsulate(ctx, NULL, outlen, NULL, seclen) &&
(*out = OPENSSL_malloc(*outlen)) != NULL &&
Expand Down Expand Up @@ -209,8 +210,8 @@ static int oqs_generate_sig_elems(const char *sigalg_name, const char *msg,
}

if (OSSL_PROVIDER_available(libctx, "default")) {
testresult = (ctx = EVP_PKEY_CTX_new_from_name(libctx, sigalg_name,
NULL)) != NULL &&
testresult = (ctx = EVP_PKEY_CTX_new_from_name(
libctx, sigalg_name, OQSPROV_PROPQ)) != NULL &&
EVP_PKEY_keygen_init(ctx) && EVP_PKEY_generate(ctx, key) &&
(mdctx = EVP_MD_CTX_new()) != NULL &&
EVP_DigestSignInit_ex(mdctx, NULL, "SHA512", libctx, NULL,
Copy link
Contributor

@RodriM11 RodriM11 Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the OQSPROV_PROPQ should also be present on the EVP_DigestSignInit_ex/EVP_DigestVerifyInit_ex, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No: oqsprovider does not contain SHA implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was my doubt as well, because documentation clearly states that "If the provider supports fetching the digest then it may use the props argument for the properties to be used during the fetch". But then I saw that the property query is also being used in here, which is an operation where I would assume the property query provider=oqsprovider would be desired (as it is done in the KEM tests, for example). But, if I understand correctly, oqsprovider will later fetch the digest to use without that property query (here), although is true that openssl will at first try to use the property query to fetch the digest (here) and that could pose problematic, specially in the future. I'm sure I am missing something, so I apologize for the confusion I might be creating.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No confusion nor need to apologize -- I'm in fact very glad that someone else but just me checks these code paths! In this case, though, I do not see a path leading to something other than the loading of a digest. And that must be fulfilled by something other than oqsprovider (default or fips provider, notably -- whatever is loaded into the NULL/default libctx). If you see a path to something different, please let me know (where): Thanks in advance!

Copy link
Contributor

@RodriM11 RodriM11 Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that the property query passed to the EVP_DigestSignInit_ex (or analogue for Verify) is not only used for the query properties of the digest (which, of course, should not be fetched from oqsprovider), it is also employed in the underlying EVP_PKEY_CTX *of the EVP_MD_CTX * to use (see here), and if the property query provider=oqsprovider is desired on that EVP_PKEY_CTX * (like, for example, is employed in the "analogue" for Encapsulation/Decapsulation in KEMs (e.g. here), then there would indeed be a "problem". But I'm sure there is a reason why on that particular EVP_PKEY_CTX * the requirement of provider=oqsprovider is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure there is a reason why on that particular EVP_PKEY_CTX * the requirement of provider=oqsprovider is not needed.

Well, I'm not so sure now as your argument looks plausible. The only "reason" I see is that tests of this logic within the feature branch with standardized PQC algs passed that didn't pass before. Unfortunately, due to another problem with ML-DSA all tests are breaking now and I think it is best to wait with further work until the feature/ml-dsa branch is functionally far enough to serve as guidance for an update of oqsprovider with regard to the openssl naming.

Copy link
Contributor

@RodriM11 RodriM11 Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I imagine is not a problem now, but it could be once the default provider has an implementation of the same signature algorithm as the one someone would be working with on oqsprovider (like it will soon happen).
Additionally, on a (quick) look, if you interpret the underlying OpenSSL procedure do_sigver_init , it seems like it does not account for the hypothetical of having different providers for the pkey operation and the digest to employ (if different providers have both implementations of one of them), since are both being impacted from the same property query, and this could potentially be used to restrict from which to take each implementation.
And I imagine there is little oqsprovider could do on that regard, as you cannot avoid the prop query received (since it might be any other thing like fips, etc...). The only possibility I see would maybe be to have an OSSL_PARAM for this fact, but even with that, do_sigver_init is using the same propq for two different purposes (which now, since they are maintaining some legacy code, is not a failure (the hash will be fetch with a digestByName approach) but it might be in the future.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"?provider=oqsprovider" query could be used in this case to prefer the algorithm implementation from oqsprovider if available there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this scenario I agree that this would solve the problem: since oqsprovider does not have any digest implementation, the digest fetch would be done via the default provider, while the pkey operation would be given priority to oqsprovider.
What I was pointing out is that openssl code would not account for the existance of two providers both of which have an implementation of the same digest and pkey operation to be used since they are both affected from the same property queries, but (in the case that is of interest) it is not a matter of oqsprovider in any case.

Expand Down
6 changes: 3 additions & 3 deletions test/oqs_test_signatures.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ static int test_oqs_signatures(const char *sigalg_name) {
// provider
if (OSSL_PROVIDER_available(libctx, "default")) {
testresult &=
(ctx = EVP_PKEY_CTX_new_from_name(
libctx, sigalg_name, "provider=oqsprovider")) != NULL &&
(ctx = EVP_PKEY_CTX_new_from_name(libctx, sigalg_name,
OQSPROV_PROPQ)) != NULL &&
EVP_PKEY_keygen_init(ctx) && EVP_PKEY_generate(ctx, &key) &&
(mdctx = EVP_MD_CTX_new()) != NULL &&
EVP_DigestSignInit_ex(mdctx, NULL, "SHA512", libctx, NULL, key,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here :-) No, Thanks.

Expand Down Expand Up @@ -66,7 +66,7 @@ static int test_oqs_signatures(const char *sigalg_name) {
// this test must work also with default provider inactive:
testresult &=
(ctx = EVP_PKEY_CTX_new_from_name(libctx, sigalg_name,
"provider=oqsprovider")) != NULL &&
OQSPROV_PROPQ)) != NULL &&
EVP_PKEY_keygen_init(ctx) && EVP_PKEY_generate(ctx, &key) &&
(mdctx = EVP_MD_CTX_new()) != NULL &&
EVP_DigestSignInit_ex(mdctx, NULL, NULL, libctx, NULL, key, NULL) &&
Expand Down
3 changes: 3 additions & 0 deletions test/test_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include <openssl/provider.h>
#include <stdio.h>

/* limit testing to just this guy */
#define OQSPROV_PROPQ "provider=oqsprovider"

/* For controlled success */
#define T(e) \
if (!(e)) { \
Expand Down
4 changes: 3 additions & 1 deletion test/tlstest_helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
#include <openssl/err.h>
#include <openssl/ssl.h>

#include "test_common.h"

#define MAXLOOPS 1000000

/* Stolen from openssl/tests/sslapitest.c: */
int create_cert_key(OSSL_LIB_CTX *libctx, char *algname, char *certfilename,
char *privkeyfilename) {
EVP_PKEY_CTX *evpctx =
EVP_PKEY_CTX_new_from_name(libctx, algname, "provider=oqsprovider");
EVP_PKEY_CTX_new_from_name(libctx, algname, OQSPROV_PROPQ);
EVP_PKEY *pkey = NULL;
X509 *x509 = X509_new();
X509_NAME *name = NULL;
Expand Down
Loading