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

Make x509 module compatible with M2Crypto 0.44.0 #67782

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vzhestkov
Copy link
Contributor

What does this PR do?

Even if the module is specified as deprecated, we need to keep it working until it gets removed with 3009.
With the most recent M2Crypto version there are differences in the exported objects, so it's better to align import to make it compatible.

Previous Behavior

Exceptions on loading the module with new M2Crypto version due to the differences in exports

New Behavior

Now is compatible with new and the previous versions

Merge requirements satisfied?

No need to adjust the tests as they were failing with the recent M2Crypto version.

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes/No

@@ -41,7 +41,7 @@
from salt.utils.odict import OrderedDict

try:
import M2Crypto
from M2Crypto import ASN1, BIO, EVP, RSA, X509, m2
Copy link
Contributor

@meaksh meaksh Feb 28, 2025

Choose a reason for hiding this comment

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

Nitpick: not sure if I'm missing something here, but if we do like this:

Suggested change
from M2Crypto import ASN1, BIO, EVP, RSA, X509, m2
import M2Crypto
from M2Crypto import ASN1, BIO, EVP, RSA, X509, m2

This would also keep the compatiblity between 0.44.0 and lower versions, and will prevent changing the rest of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How could it help? M2Crypto with this change is not used anymore in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this is just cosmetic.

With the suggested changes, we don't need to change the rest of the file and we can just keep the calls as they are right now, i.a. M2Crypto.X509.X509_Extension (keeping the M2Crypto prefix), instead of X509.X509_Extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not work that way. In case of using import M2Crypto it can use M2Crypto.X509.X509_Extension only if they are defined inside M2Crypto/__init__.py, but with the recent version of M2Crypto they are not defined there, but from M2Crypto import ASN1, BIO, EVP, RSA, X509, m2 is not allowing to use M2Crypto.X509.X509_Extension as it loads X509 as standalone definition which should be used as X509.X509_Extension, but not as M2Crypto.X509...

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested with both older than 0.44.0 and with 0.44.0 and this works if we keep both imports:

import M2Crypto
from M2Crypto import ASN1, BIO, EVP, RSA, X509, m2

Then, with 0.44.0 (in Tumbleweed):

e374c2a931b3:/ # python3
Python 3.11.11 (main, Dec 04 2024, 21:44:34) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import M2Crypto     
>>> from M2Crypto import ASN1, BIO, EVP, RSA, X509, m2
>>> M2Crypto.m2.x509v3_ext_conf
<built-in function x509v3_ext_conf>
>>> M2Crypto.X509.X509_Extension
<class 'M2Crypto.X509.X509_Extension'>
>>> RSA.load_key_string
<function load_key_string at 0x7f29f77f0ae0>

Copy link
Contributor Author

@vzhestkov vzhestkov Feb 28, 2025

Choose a reason for hiding this comment

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

With this change both will work, but the key point here that import M2Crypto became useless as not used at all in the code. And BTW, if you've tested it with the bundle in the next there is a roll back patch for it applied for M2Crypto that's why it's not failing with old way of using M2Crypto. Because of this: https://build.opensuse.org/projects/home:vizhestkov:new-salt-deps/packages/saltbundlepy-m2crypto/files/fix-imcompatibility-with-salt.patch?expand=1

@whytewolf
Copy link
Collaborator

i thought the x509 is being dropped in favor of x509_v2 soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants