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

Swap out legacycrypt for crypt-r for Python 3.13+ #3141

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jeremycline
Copy link
Member

Description

PR #3070 pulled in legacycrypt to replace the removed crypt module. legacycrypt hasn't been updated since it was initially pulled out of Python's stdlib in 2019 (Python 3.7). crypt-r pulls in the module as it was in Python 3.12. While there's been no major developments since 3.7, it's more likely to be kept updated for any breakages in future Python releases.

It's also already packaged for Fedora and means one less package for me to maintain so that would be nice.


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made

Quality of Code and Contribution Guidelines

PR Azure#3070 pulled in legacycrypt to replace the removed crypt module.
legacycrypt hasn't been updated since it was initially pulled out of
Python's stdlib in 2019 (Python 3.7). crypt-r pulls in the module as it
was in Python 3.12. While there's been no major developments since 3.7,
it's more likely to be kept updated for any breakages in future Python
releases.

It's also already packaged for Fedora and means one less package for me
to maintain so that would be nice.
@nagworld9
Copy link
Contributor

nagworld9 commented Jun 17, 2024

@jeremycline Thanks for the change. We implemented temporary workaround to use legacycrypt to fix importing crypt issue on agent start. But the code that's need crypt() function from crypt module is owned by(JIT) team. We started a discussion with them and will let JIT team to handle this.

@nmeyerhans
Copy link

My suggestion would be to avoid handling hashing here at all, and instead leverage chpasswd for password setting. It has a few advantages over the current implementation:

  1. It's a common tool (part of the shadow suite) and well tested
  2. It integrates with pam, so system-wide configuration applies for settings like the hash algorithm
  3. It avoids taking a dependency on legacycrypt or some other crypt frontend

You can look at the cloud-init sources for an example of how to use chpasswd in this scenario.

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