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

Refactor module_utils/crypto.py #27

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Expands the large monolithic plugins/module_utils/crypto.py into a Python package plugins/module_utils/crypto/ with multiple smaller files.

This should not change functionality at all (except fix a tiny bug in x509_crl.py I found while changing this). CI should check essentially everything, except ecs_certificate (and the ECS part of x509_certificate) - @ctrufan can you test it?

I've used pylint and flake8 to check for errors (and clean up unnecessary imports), so the change should be pretty safe.

Fixes #16.

CC @MarkusTeufelberger @puiterwijk @Spredzy @Shaps @ctrufan

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

plugins/module_utils/crypto.py

@felixfontein
Copy link
Contributor Author

(Unstable CI is caused by #23 and unrelated to this PR.)

@felixfontein felixfontein force-pushed the refactor-module_utils-crypto branch from 70304f0 to 1bdfbaa Compare April 13, 2020 11:52
@felixfontein
Copy link
Contributor Author

I moved some generic file I/O methods (load_file_if_exists and write_file) into a new Python module plugins/module_utils/io.py. This makes them easier to use without also having to include the pyOpenSSL / cryptography feature detection.

I also added a new support function for obtaining the serial number of a cryptography certificate, since that changed in cryptography 1.4. Instead of repeating the same try/except in different modules, there's now one common utility function for that.

@felixfontein
Copy link
Contributor Author

I've added some compatibility code to module_utils/crypto/__init__.py. With this, even existing modules and existing code depending on module_utils/crypto.py should work without change - well, should, assuming ansible/ansible#68872 wouldn't be there. But once that's fixed, it will work :)

@felixfontein
Copy link
Contributor Author

(The current error in macOS is caused by pyca/cryptography#5215)

@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger @puiterwijk @Spredzy @Shaps @ctrufan I would be really happy if you could review this PR :)

@ctrufan
Copy link
Collaborator

ctrufan commented Apr 23, 2020

Apologies, I've been super busy - I should have a chance to retest the ecs_certificate module some time this week. I'll see if I can find time this weekend to review the PR as well.

@felixfontein
Copy link
Contributor Author

The problem with cryptography on old macOS versions has been resolved with the release of cryptography 2.9.2. Now it's back to "unstable" as before.

@MarkusTeufelberger
Copy link
Contributor

Looks good to me, let's move this forward. Thanks for the work, Felix!

@felixfontein
Copy link
Contributor Author

I'll merge this now, so work can continue ;) @MarkusTeufelberger thanks for checking this! @ctrufan the change should be pretty safe for the ecs_* modules, but it probably won't hurt when you test it at some point. You should definitely test it before we release 1.0.0 of the collection anyway ;)

@felixfontein felixfontein merged commit 9a096dd into ansible-collections:master May 12, 2020
@felixfontein felixfontein deleted the refactor-module_utils-crypto branch May 12, 2020 09:19
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.

TODO: Refactor module_utils/crypto.py
3 participants