Skip to content

Comments

remove engine usage from our unit tests#28525

Closed
nhorman wants to merge 9 commits intoopenssl:feature/engineremovalfrom
nhorman:1338
Closed

remove engine usage from our unit tests#28525
nhorman wants to merge 9 commits intoopenssl:feature/engineremovalfrom
nhorman:1338

Conversation

@nhorman
Copy link
Contributor

@nhorman nhorman commented Sep 11, 2025

This PR series removes engine usage from our unit tests as part of our overall engine removal effort

A few notes:

  1. evp_kdf_tests.c still references OSSL_ALG_PARAM_ENGINE which remains in place until remove OSSL_ALG_PARAM_ENGINE parameter project#1427 is complete, which will remove this

  2. trace_api_test.c still references the ENGINE_TABLE and ENGINE_REF_COUNT tracing categories, that will be removed as part of remove ENGINE trace category project#1426

  3. This PR will be broken until such time as it is rebased after PR's Convert ossltest engine to a provider #28461 and Remove OPENSSL_NO_ENGINE guarded code #28384 are merged. As such this will remain in draft state until that is complete. Its just here for anyone that wants a preview, for now.

  4. There are several tests (the tls test, test_dgst and the remaining test_rand tests) that continue to use engines. Those tests are not getting removed, but being converted to using a new provider in Convert ossltest engine to a provider #28461

@nhorman nhorman self-assigned this Sep 11, 2025
@nhorman nhorman added the approval: review pending This pull request needs review by a committer label Sep 11, 2025
@nhorman nhorman linked an issue Sep 11, 2025 that may be closed by this pull request
@t8m t8m added triaged: feature The issue/pr requests/adds a feature tests: exempted The PR is exempt from requirements for testing labels Sep 12, 2025
@nhorman nhorman marked this pull request as draft September 12, 2025 12:57
Copy link
Contributor

@jogme jogme left a comment

Choose a reason for hiding this comment

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

looking good, thank you!

@andrewkdinh
Copy link
Contributor

Can you rebase this PR?

@nhorman
Copy link
Contributor Author

nhorman commented Sep 18, 2025

yeah, as soon as I finish fixing the feature branch rebase

The evp_extra_test code makes use of the dasync engine to ensure that we
can do evp operations (signatures and ciphers) with an engine.

The dasync engine is used for this purpose, but it does not exercize any
specific pipeline functionality.

Given that engines are getting removed, the engine tests here I think
can just be removed.
With the impending engine removal, we don't have a need to test engine
functionality in these tests anymore, so remove the test cases that make
use of the dasync engine here.
It seems like it wasn't ever needed before, so with the removal of
engines, just get rid of it.
We have a specific test suite that exercizes the afalg engine, that is
becoming useless with engine removal.

I had considered that we should perhaps convert this into a provider,
but having looked at the engine itself, it only offers implementations
for AES-128, AES-192 and AES-256.  Given that the default provider
offers these algorithms with hardware acceleration via the aesni
instruction set (or comparable instructions on non-x86 arches), it seems
like the only advantage the afalg engine offers is acceleration of these
ciphers on platforms that have off-cpu accelerators and no cpu based
acceleration support.

given that:
a) Most cpus have instruction based acceleration
b) We don't test with any platforms that use external accelerators

It seems like alot of investment to get no real advantage, so just
remove the test, allowing us to delete the engine entirely in another
PR.
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
We're removing the engine, so we don't need to test this anymore.

NOTE: This also removes the engine skip check from the test, and this
breaks testing until such time as PR openssl#28461 is merged (which replaces
the remaining engine test with a provider).

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
Spotted by @andrewkdinh, some extra notes about/useages of engines that
are now vestigial.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
The evp_extra_test code makes use of the dasync engine to ensure that we
can do evp operations (signatures and ciphers) with an engine.

The dasync engine is used for this purpose, but it does not exercize any
specific pipeline functionality.

Given that engines are getting removed, the engine tests here I think
can just be removed.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
With the impending engine removal, we don't have a need to test engine
functionality in these tests anymore, so remove the test cases that make
use of the dasync engine here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
It seems like it wasn't ever needed before, so with the removal of
engines, just get rid of it.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
We have a specific test suite that exercizes the afalg engine, that is
becoming useless with engine removal.

I had considered that we should perhaps convert this into a provider,
but having looked at the engine itself, it only offers implementations
for AES-128, AES-192 and AES-256.  Given that the default provider
offers these algorithms with hardware acceleration via the aesni
instruction set (or comparable instructions on non-x86 arches), it seems
like the only advantage the afalg engine offers is acceleration of these
ciphers on platforms that have off-cpu accelerators and no cpu based
acceleration support.

given that:
a) Most cpus have instruction based acceleration
b) We don't test with any platforms that use external accelerators

It seems like alot of investment to get no real advantage, so just
remove the test, allowing us to delete the engine entirely in another
PR.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
With engine removal, we expect that init flag to disappear, so stop
using it here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
We're removing the engine, so we don't need to test this anymore.

NOTE: This also removes the engine skip check from the test, and this
breaks testing until such time as PR openssl#28461 is merged (which replaces
the remaining engine test with a provider).

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 1, 2025
Spotted by @andrewkdinh, some extra notes about/useages of engines that
are now vestigial.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
The evp_extra_test code makes use of the dasync engine to ensure that we
can do evp operations (signatures and ciphers) with an engine.

The dasync engine is used for this purpose, but it does not exercize any
specific pipeline functionality.

Given that engines are getting removed, the engine tests here I think
can just be removed.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
With the impending engine removal, we don't have a need to test engine
functionality in these tests anymore, so remove the test cases that make
use of the dasync engine here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
It seems like it wasn't ever needed before, so with the removal of
engines, just get rid of it.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
We have a specific test suite that exercizes the afalg engine, that is
becoming useless with engine removal.

I had considered that we should perhaps convert this into a provider,
but having looked at the engine itself, it only offers implementations
for AES-128, AES-192 and AES-256.  Given that the default provider
offers these algorithms with hardware acceleration via the aesni
instruction set (or comparable instructions on non-x86 arches), it seems
like the only advantage the afalg engine offers is acceleration of these
ciphers on platforms that have off-cpu accelerators and no cpu based
acceleration support.

given that:
a) Most cpus have instruction based acceleration
b) We don't test with any platforms that use external accelerators

It seems like alot of investment to get no real advantage, so just
remove the test, allowing us to delete the engine entirely in another
PR.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
With engine removal, we expect that init flag to disappear, so stop
using it here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
We're removing the engine, so we don't need to test this anymore.

NOTE: This also removes the engine skip check from the test, and this
breaks testing until such time as PR openssl#28461 is merged (which replaces
the remaining engine test with a provider).

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
Spotted by @andrewkdinh, some extra notes about/useages of engines that
are now vestigial.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
The evp_extra_test code makes use of the dasync engine to ensure that we
can do evp operations (signatures and ciphers) with an engine.

The dasync engine is used for this purpose, but it does not exercize any
specific pipeline functionality.

Given that engines are getting removed, the engine tests here I think
can just be removed.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
With the impending engine removal, we don't have a need to test engine
functionality in these tests anymore, so remove the test cases that make
use of the dasync engine here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
It seems like it wasn't ever needed before, so with the removal of
engines, just get rid of it.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
We have a specific test suite that exercizes the afalg engine, that is
becoming useless with engine removal.

I had considered that we should perhaps convert this into a provider,
but having looked at the engine itself, it only offers implementations
for AES-128, AES-192 and AES-256.  Given that the default provider
offers these algorithms with hardware acceleration via the aesni
instruction set (or comparable instructions on non-x86 arches), it seems
like the only advantage the afalg engine offers is acceleration of these
ciphers on platforms that have off-cpu accelerators and no cpu based
acceleration support.

given that:
a) Most cpus have instruction based acceleration
b) We don't test with any platforms that use external accelerators

It seems like alot of investment to get no real advantage, so just
remove the test, allowing us to delete the engine entirely in another
PR.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
With engine removal, we expect that init flag to disappear, so stop
using it here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
We're removing the engine, so we don't need to test this anymore.

NOTE: This also removes the engine skip check from the test, and this
breaks testing until such time as PR openssl#28461 is merged (which replaces
the remaining engine test with a provider).

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
nhorman added a commit to nhorman/openssl that referenced this pull request Dec 4, 2025
Spotted by @andrewkdinh, some extra notes about/useages of engines that
are now vestigial.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
The evp_extra_test code makes use of the dasync engine to ensure that we
can do evp operations (signatures and ciphers) with an engine.

The dasync engine is used for this purpose, but it does not exercize any
specific pipeline functionality.

Given that engines are getting removed, the engine tests here I think
can just be removed.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
With the impending engine removal, we don't have a need to test engine
functionality in these tests anymore, so remove the test cases that make
use of the dasync engine here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
It seems like it wasn't ever needed before, so with the removal of
engines, just get rid of it.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
We have a specific test suite that exercizes the afalg engine, that is
becoming useless with engine removal.

I had considered that we should perhaps convert this into a provider,
but having looked at the engine itself, it only offers implementations
for AES-128, AES-192 and AES-256.  Given that the default provider
offers these algorithms with hardware acceleration via the aesni
instruction set (or comparable instructions on non-x86 arches), it seems
like the only advantage the afalg engine offers is acceleration of these
ciphers on platforms that have off-cpu accelerators and no cpu based
acceleration support.

given that:
a) Most cpus have instruction based acceleration
b) We don't test with any platforms that use external accelerators

It seems like alot of investment to get no real advantage, so just
remove the test, allowing us to delete the engine entirely in another
PR.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
With engine removal, we expect that init flag to disappear, so stop
using it here.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
We're removing the engine, so we don't need to test this anymore.

NOTE: This also removes the engine skip check from the test, and this
breaks testing until such time as PR openssl#28461 is merged (which replaces
the remaining engine test with a provider).

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
mbroz pushed a commit to mbroz/openssl that referenced this pull request Dec 4, 2025
Spotted by @andrewkdinh, some extra notes about/useages of engines that
are now vestigial.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#28525)
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: feature The issue or PR is relevant only to one of the feature branches. tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix up tests that use engines

6 participants