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

Handle PermissionError when importing crypt when FIPS is enabled. #62587

Merged
merged 4 commits into from
Nov 1, 2022

Conversation

takeda
Copy link
Contributor

@takeda takeda commented Aug 31, 2022

What does this PR do?

When salt runs on a machine where FIPS is enabled, it produces:

PermissionError: [Errno 1] Operation not permitted

on import.

This error shows up in certain circumstances, for example when trying to get documentation from a module.

salt-call saltutil

What issues does this PR fix or reference?

Fixes: not created

Merge requirements satisfied?

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

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@takeda takeda requested a review from a team as a code owner August 31, 2022 00:48
@takeda takeda requested review from whytewolf and removed request for a team August 31, 2022 00:48
@welcome
Copy link

welcome bot commented Aug 31, 2022

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@whytewolf
Copy link
Collaborator

Thank you for the PR. Is there anyway we can get some tests to make sure the functionality doesn't break in the future. as well as a changelog file.

@garethgreenaway garethgreenaway added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 30, 2022
@whytewolf
Copy link
Collaborator

@takeda can we get a test case please? I know the fix seems simple but it dos still need a test case just to make sure this doesn't break again in the future.

@takeda
Copy link
Contributor Author

takeda commented Oct 7, 2022

@whytewolf I'm not sure how this could be tested. To enable FIPS you switch a sysctl in the kernel crypto.fips_enabled = 1. I don't see how this could be incorporated in an unit test.

@whytewolf
Copy link
Collaborator

@takeda sorry. I missed this was only catching on import. in theory you could mock the __import__ function but that is tricky business that can cause all sorts of nightmares. I"ll go ahead and see if we can bypass the test requirement.

@whytewolf
Copy link
Collaborator

it does still need a changelog though

@whytewolf whytewolf added needs-changelog-file and removed Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels Oct 7, 2022
s0undt3ch
s0undt3ch previously approved these changes Nov 1, 2022
whytewolf
whytewolf previously approved these changes Nov 1, 2022
@Ch3LL Ch3LL dismissed stale reviews from whytewolf and s0undt3ch via 5b9a502 November 1, 2022 19:59
@Ch3LL Ch3LL added Sulfur v3006.0 release code name and version and removed needs-changelog-file labels Nov 1, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 1, 2022

Merging as tests passed previously before the changelog was added.

@Ch3LL Ch3LL merged commit 2befb46 into saltstack:master Nov 1, 2022
@welcome
Copy link

welcome bot commented Nov 1, 2022

Congratulations on your first PR being merged! 🎉

@takeda takeda deleted the patch-1 branch June 14, 2023 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants