Skip to content

bpo-31904: Add _crypt module support for VxWorks RTOS #12321

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

Closed
wants to merge 4 commits into from

Conversation

pxinwr
Copy link
Contributor

@pxinwr pxinwr commented Mar 14, 2019

This is the successive PR after #11968. This PR adds _crypt module support for VxWorks RTOS.
VxWorks has no APIs crypt() and crypt_r() provided. Instead, DES_crypt() is defined in openssl/des.h to replace the crypt(). It has the same prototype as crypt(). This extension module _crypt will not be built if VxWorks users don't enable the OPENSSL features. See

cpython/setup.py

Lines 976 to 989 in f2f55e7

def detect_crypt(self):
# crypt module.
if self.compiler.find_library_file(self.lib_dirs, 'crypt'):
libs = ['crypt']
else:
libs = []
if not VXWORKS:
self.add(Extension('_crypt', ['_cryptmodule.c'],
libraries=libs))
elif self.compiler.find_library_file(self.lib_dirs, 'OPENSSL'):
libs = ['OPENSSL']
self.add(Extension('_crypt', ['_cryptmodule.c'],
libraries=libs))

More and full support on modules for VxWorks will continuously be added by the coming PRs.
VxWorks is a product developed and owned by Wind River. For VxWorks introduction or more details, go to https://www.windriver.com/products/vxworks/
Wind River will have a dedicated engineering team to contribute to the support as maintainers.
We already have a working buildbot worker internally, but has not bound to master. We will check the process for the buildbot, then add it.

https://bugs.python.org/issue31904

@pxinwr
Copy link
Contributor Author

pxinwr commented Mar 26, 2019

@serhiy-storchaka I've fixed the problem you mentioned. Could you help to review again and do the merge if the code is OK?

@vstinner
Copy link
Member

@tiran: Do you think that using OpenSSL DES_crypt() to implement crypt.crypt() is acceptable?

@vstinner
Copy link
Member

Oh, the commit 32f5fdd already added OpenSSL dependency to _crypt Python module in setup.py. IMHO it would be better to not add it before this change, but well, since the commit is already pushed, it's too late and it's ok :-)

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

-1 for implementing crypt as DES.

Do you really need the crypt module or are you simply implementing it for the sake of having a complete Python stdlib?

I'm planning to deprecate and drop the crypt module. It's not very useful these days and most hashing algorithms are weak. Especially DES is super weak.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pxinwr
Copy link
Contributor Author

pxinwr commented Mar 29, 2019

@tiran Thanks for your comments. I am pinging my PM for the necessity of crypt module. Before I get the decision, could you tell what modules can be used to replace the crypt module?

@tiran
Copy link
Member

tiran commented Mar 29, 2019

It depends on your use case. For passwords the hashlib module provides PBKDF2-HMAC and scrypt. Both algorithms are much better than crypt. There are also external packages like bcrypt, cryptography, and passlib, that provide secure password derivation algorithms.

The crypt module is really only useful for legacy systems or for use with the spwd module. The spwd module must not be used on any modern Unix system that uses PAM.

@serhiy-storchaka
Copy link
Member

Agree. If VxWorks RTOS for some (likely good) reasons does not provide the system crypt() function, it may be better to not provide it in Python. The crypt module is optional.

@pxinwr
Copy link
Contributor Author

pxinwr commented Apr 1, 2019

@tiran @serhiy-storchaka Thanks for your comments. My PM has confirmed that VxWorks can drop the crypt module support. So as you required, I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@serhiy-storchaka, @tiran: please review the changes made to this pull request.

@pxinwr
Copy link
Contributor Author

pxinwr commented Apr 1, 2019

Already checked the CI failures. That should be caused by other PRs other than this one.

@pxinwr
Copy link
Contributor Author

pxinwr commented Apr 3, 2019

Closing it and reopening it is to rerun the CI to update the regression result.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I would prefer to get a new PR. It's surprising that a PR to add VxWorks support to crypt became "remove crypt". For example, you didn't update the PR title.

Please open a new PR with requested changes.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pxinwr
Copy link
Contributor Author

pxinwr commented Apr 15, 2019

As @vstinner requested, a new PR #12833 has been created to replace this one. And all the issues raised here have been fixed in the new PR #12833

@kuhlenough kuhlenough mannequin mentioned this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants