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.

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

test: make unit tests FIPS provider version aware #19201

Closed
wants to merge 7 commits into from

Conversation

paulidale
Copy link
Contributor

This PR is to address the CI failures when running 3.0.x against the validated 3.0.0 FIPS provider.
Adding additional test cases is separate.

Fixes #19171

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Sep 12, 2022
@paulidale paulidale self-assigned this Sep 12, 2022
@paulidale
Copy link
Contributor Author

An alternative would be to version check all of the currently loaded providers against the testable version rather than just the FIPS provider.

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Should this be discussed in OTC?

@slontis
Copy link
Member

slontis commented Sep 12, 2022

Looks ok to me Pauli

@slontis
Copy link
Member

slontis commented Sep 13, 2022

@paulidale

As @t8m suggested changing the code in EVP_PKEY_eq() to this seems to work

    if (a->keymgmt != NULL || b->keymgmt != NULL) {
        int selection = SELECT_PARAMETERS;

        if (evp_keymgmt_util_has((EVP_PKEY *)a, OSSL_KEYMGMT_SELECT_PUBLIC_KEY)
             && evp_keymgmt_util_has((EVP_PKEY *)b, OSSL_KEYMGMT_SELECT_PUBLIC_KEY))
            selection |= OSSL_KEYMGMT_SELECT_PUBLIC_KEY;
        else
            selection |= OSSL_KEYMGMT_SELECT_KEYPAIR;
        return evp_pkey_cmp_any(a, b, selection);
    }

30-test_evp_extra.t passes if you also check b

@paulidale
Copy link
Contributor Author

With that last change the test log is now:

04-test_encoder_decoder.t        (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
25-test_verify.t                 (Wstat: 512 Tests: 182 Failed: 2)
  Failed tests:  134-135
  Non-zero exit status: 2

@levitte
Copy link
Member

levitte commented Sep 13, 2022

@paulidale

As @t8m suggested changing the code in EVP_PKEY_eq() to this seems to work

    if (a->keymgmt != NULL || b->keymgmt != NULL) {
        int selection = SELECT_PARAMETERS;

        if (evp_keymgmt_util_has((EVP_PKEY *)a, OSSL_KEYMGMT_SELECT_PUBLIC_KEY)
             && evp_keymgmt_util_has((EVP_PKEY *)b, OSSL_KEYMGMT_SELECT_PUBLIC_KEY))
            selection |= OSSL_KEYMGMT_SELECT_PUBLIC_KEY;
        else
            selection |= OSSL_KEYMGMT_SELECT_KEYPAIR;
        return evp_pkey_cmp_any(a, b, selection);
    }

30-test_evp_extra.t passes if you also check b

The way I'm reading this is that with this change, all the stuff done in test/ in this PR can be dropped. Did I read that wrong?

@slontis
Copy link
Member

slontis commented Sep 13, 2022

@paulidale
As @t8m suggested changing the code in EVP_PKEY_eq() to this seems to work

    if (a->keymgmt != NULL || b->keymgmt != NULL) {
        int selection = SELECT_PARAMETERS;

        if (evp_keymgmt_util_has((EVP_PKEY *)a, OSSL_KEYMGMT_SELECT_PUBLIC_KEY)
             && evp_keymgmt_util_has((EVP_PKEY *)b, OSSL_KEYMGMT_SELECT_PUBLIC_KEY))
            selection |= OSSL_KEYMGMT_SELECT_PUBLIC_KEY;
        else
            selection |= OSSL_KEYMGMT_SELECT_KEYPAIR;
        return evp_pkey_cmp_any(a, b, selection);
    }

30-test_evp_extra.t passes if you also check b

The way I'm reading this is that with this change, all the stuff done in test/ in this PR can be dropped. Did I read that wrong?

Yes you read it wrong.. There are a group of tests that access EVP_PKEY_eq() internally that currently fail.. such as the ssl related tests and cms. This change fixes that issue.. There are numerous other issues because we added tests to check new behaviour, or we broke tests because we changed behaviour (such as not allowing Explicit EC curves).

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Sep 13, 2022
@paulidale
Copy link
Contributor Author

As @slontis wrote, that fixes the pkey eq problems. There are plenty of other bug fixes.

@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Sep 13, 2022
@paulidale
Copy link
Contributor Author

Yeah, I pushed the authorship change to the wrong repo. FFS.
Fixed now I hope.

@paulidale
Copy link
Contributor Author

I'll be diverting from the two remaining fixes (EC explicit curve related I suspect) to working on adding the additional required tests tomorrow.

@slontis
Copy link
Member

slontis commented Sep 13, 2022

Do you mean the CI workflow?

test/testutil.h Outdated Show resolved Hide resolved
test/testutil.h Outdated Show resolved Hide resolved
test/testutil/provider.c Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor Author

Yes, we need additional CI workflows to validate future fixes.

test/testutil/provider.c Outdated Show resolved Hide resolved
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Also fix a number of regressions when run against the 3.0.0 FIPS provider
that result from bug fixes.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19201)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19201)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)
@paulidale
Copy link
Contributor Author

Merged to master and 3.0. 3.0 required dropping the KDF_CTX_dup part of evptest but was otherwise clean enough.

@paulidale paulidale closed this Sep 15, 2022
@paulidale paulidale deleted the fips300 branch September 15, 2022 22:42
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Fixes #19171

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)

(cherry picked from commit eaac058)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Also fix a number of regressions when run against the 3.0.0 FIPS provider
that result from bug fixes.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)

(cherry picked from commit 54a7bbe)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)

(cherry picked from commit 4d0249c)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)

(cherry picked from commit 9684335)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19201)

(cherry picked from commit c342004)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19201)

(cherry picked from commit e1289d9)
openssl-machine pushed a commit that referenced this pull request Sep 15, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #19201)

(cherry picked from commit 65080a3)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Fixes openssl#19171

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Also fix a number of regressions when run against the 3.0.0 FIPS provider
that result from bug fixes.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19201)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19201)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Fixes openssl#19171

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Also fix a number of regressions when run against the 3.0.0 FIPS provider
that result from bug fixes.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19201)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#19201)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.0 FIPS provider is not compatible with tests in 3.0.5
6 participants