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

openssl_pkcs12: add cryptography backend #234

Merged

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 18, 2021

SUMMARY

This requires cryptography 3.0 and @Ajpantuso's hack, so we can't deprecate pyOpenSSL anytime soon.

Also this is at the moment pretty ugly, I'll have to do a lot more cleanup...

Finally, iter_size and maciter_size are not supported. Also, the cryptography docs say PKCS12 encryption is not secure and should not be used as a security mechanism. Wrap a PKCS12 blob in a more secure envelope if you need to store or send it safely. Encryption is provided for compatibility reasons only..

Fixes #21.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

openssl_pkcs12

@felixfontein felixfontein changed the title [WIP] openssl_pkcs12: add cryptography backend openssl_pkcs12: add cryptography backend May 19, 2021
@felixfontein felixfontein mentioned this pull request May 19, 2021
11 tasks
@felixfontein
Copy link
Contributor Author

ready_for_review

Copy link
Collaborator

@Ajpantuso Ajpantuso left a comment

Choose a reason for hiding this comment

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

Nothing major to address. Only functional concern is handling empty passwords with cryptography which is an edge case.

plugins/modules/openssl_pkcs12.py Outdated Show resolved Hide resolved
'''
Load list of concatenated PEM files, and return a list of parsed certificates.
'''
with open(filename, 'rb') as f:
data = f.read().decode('utf-8')
return [load_certificate(None, content=cert) for cert in split_pem_list(data)]
return [load_certificate(None, content=cert.encode('utf-8'), backend=backend) for cert in split_pem_list(data)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, but how was PyOpenSSL handling the text certificate data previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PyOpenSSL has some magic to automatically encode: https://github.com/pyca/pyopenssl/blob/main/src/OpenSSL/crypto.py#L2898

tests/integration/targets/openssl_pkcs12/tasks/main.yml Outdated Show resolved Hide resolved
tests/integration/targets/openssl_pkcs12/tasks/main.yml Outdated Show resolved Hide resolved
felixfontein and others added 3 commits May 19, 2021 22:36
Co-authored-by: Ajpantuso <ajpantuso@gmail.com>
Co-authored-by: Ajpantuso <ajpantuso@gmail.com>
Copy link
Collaborator

@Ajpantuso Ajpantuso left a comment

Choose a reason for hiding this comment

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

LGTM, with the exception of CI for 2.9 which I broke by not remembering range() must be typecasted to list for compatibility. "{{ range(1, 4) | list }}" should clear the failures.

@felixfontein felixfontein merged commit e9bc7c7 into ansible-collections:main May 20, 2021
@felixfontein felixfontein deleted the pkcs12-cryptography branch May 20, 2021 17:36
@felixfontein
Copy link
Contributor Author

@Ajpantuso thanks a lot for reviewing this!

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.

openssl_pkcs12: add cryptography backend
2 participants