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

OpenSslEnginesCheck: New actor for OpenSSL engines (9->10) #1338

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

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Feb 4, 2025

The OpenSSL engines are deprecated in OpenSSL 3.0 (RHEL9) and in RHEL10 we removed existing pkcs11 engine and do not support building new engines. This check builds on top of existing 8to9 check OpenSslConfigCheck that I wrote years back that was doing some incompatibilities checks when updating from openssl 1 to 3.

Now, we really do not want users to upgrade with existing engines configured, and especially not with the ones that we removed from distribution (pkcs11), which might break the OpenSSL.

Copy link

github-actions bot commented Feb 4, 2025

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
However, here are additional useful commands for packit:

  • /packit test to re-run manually the default tests
  • /packit retest-failed to re-run failed tests manually
  • /packit test oamg/leapp#42 to run tests with leapp builds for the leapp PR#42 (default is latest upstream - main - build)

Note that first time contributors cannot run tests automatically - they need to be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

  • /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
  • /packit test --labels beaker-minimal-8.10to9.4,kernel-rt-8.10to9.4 to schedule kernel-rt and beaker-minimal test sets for 8.10->9.4 upgrade path

See other labels for particular jobs defined in the .packit.yaml file.

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

@Jakuje
Copy link
Contributor Author

Jakuje commented Feb 4, 2025

I see there are some other common openssl actors already in repos/system_upgrade/common/actors/openssl/ so I will probably move the OpenSslConfigScanner in there in the next iteration.

# Iterate over engines directives -- they point to another block
for engine in engines_block.pairs:
name = engine.key
engine_block = _openssl_find_block(config, engine.value)

Choose a reason for hiding this comment

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

Now, technically the name of the engine configuration block is arbitrary, too.
For example, this is a valid configuration that load the pkcs11 engine:

openssl_conf = openssl_init

[openssl_init]
engines = engines_sect

[engines_sect]
cxpf11 = cxpf11_sect

[cxpf11_sect]
engine_id = pkcs11
dynamic_path = /usr/lib64/engines-3/libpkcs11.so
MODULE_PATH = opensc-pkcs11.so
init = 1

I'm not sure whether we actually want to go all the way and detect this, too. It feels like over-complicating things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reading of the manual page was that this is just an "alternative name".

engine_id
This is used to specify an alternate name, overriding the default name specified in the list of engines. If present, it must be first. For example:

You are right it overrides the original id, but the search for the pkcs11 is mostly additional check -- we really care if there are any engines or not and if they are we are going to say the user to fix their config.

I would probably like to avoid going to the engine_id, but it should not be that hard to add.

Choose a reason for hiding this comment

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

Well even if the engine ID is just an alternate name, the dynamic_path is probably what really matters then. But yeah, those will be detected as engines one way or the other, so I don't think we need to cover these special cases.

@Jakuje
Copy link
Contributor Author

Jakuje commented Feb 5, 2025

Ugh. Looking at the other actors, there is #1131 which basically always backs up the modified openssl conf and uses the rpm one on the target system, which means that any changes to the OpenSSL configuration files we made with 8->9 upgrade nor this one nor the one proposed from CRYPTO-15581 would have any effect. So should we just kill these actors and let the actor in handle it all?

https://github.com/oamg/leapp-repository/blob/main/repos/system_upgrade/common/actors/openssl/migrateopensslconf/actor.py

@pirat89 ?

@Jakuje
Copy link
Contributor Author

Jakuje commented Feb 6, 2025

Sounds like this actor checking the configured engines should be still usable. Even though we will get the new configuration automatically, warning the user if they have the engines hardcoded is still useful information.

OpensslConfigScanner: Make linter happy and execute on EL 8+ only

We are not interested to execute these actors on RHEL 7 as they have
not been used there before and no actor will consume the msg for
IPU 7 -> 8 at all.
Copy link
Member

@pirat89 pirat89 left a comment

Choose a reason for hiding this comment

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

discussed with @Jakuje , we need better clarification of the impact and remediation instruction regarding pkcs11-engine. Currently we do not know how to properly instruct users about expected impact and actions that should be done.

so should we just kill these actors and let the actor in handle it all?

@neverpanic it makes sense to me still to report this msg, just not sure about <paragraph-above>. maybe it would make a sense to merge few reports together in future but let's keep it split for this release as time is running.

@pirat89 pirat89 added the enhancement New feature or request label Feb 6, 2025
@pirat89 pirat89 added this to the 8.10/9.6 milestone Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants