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.

Sign up for GitHub

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

Jump to bottom

Support cryptography 35.0.0 for all modules except openssl_pkcs12 #294

Merged
merged 9 commits into from
Oct 3, 2021
Merged

Support cryptography 35.0.0 for all modules except openssl_pkcs12 #294

merged 9 commits into from
Oct 3, 2021

Conversation

felixfontein
Copy link
Contributor

SUMMARY

The latest cryptography release, 35.0.0 (released today), uses a lot more Rust code than previous relesaes. Unfortunately this breaks some of our 'workarounds' which access some internals to gather some information that cryptography unfortunately does not provide. These are in particular:

  1. Getting hold of raw extension data (DER encoded bytes) for all CSR and X.509 certificate extensions (needed for the openssl_csr_info, x509_certificate_info, and get_certificate modules);
  2. Getting hold of the 'friendly name' of the 'main' certificate in a PKCS#12 file (needed for the openssl_pkcs12 module).

I've been able to work around 1 in some ugly fashion, but didn't find a working solution for 2 yet. It seems that the friendly name is somehow stripped from the certificate.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cryptography support

ADDITIONAL INFORMATION

@felixfontein felixfontein changed the title [WIP] Support cryptography 35.0.0 Support cryptography 35.0.0 for all modules except openssl_pkcs12 Sep 30, 2021
@felixfontein
Copy link
Contributor Author

Ok, as the succeeding CI run shows when the openssl_pkcs12 tests were disabled, this fixes 'everything' except the problem with extracting the friendly name for PKCS#12 archives. My suggestion would be to merge this despite failing CI, and try to work on a fix for openssl_pkcs12 in a separate PR.

@felixfontein felixfontein mentioned this pull request Sep 30, 2021
Closed
@felixfontein
Copy link
Contributor Author

Ok, so the latest changes still work everywhere (except for openssl_pkcs12, as expected).

@felixfontein felixfontein mentioned this pull request Oct 3, 2021
Merged
@felixfontein
Copy link
Contributor Author

The follow-up #296 will fix the remaining PKCS#12 issues (once rebased after this has been merged).

briantist
briantist approved these changes Oct 3, 2021
View reviewed changes
Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

unfortunate to have to resort to such workarounds, but these look like solid fixes to me

@felixfontein
Copy link
Contributor Author

@briantist I'm really looking forward to get rid of them (resp. only have to use them for older cryptography versions)! I've started another push for that in cryptography, but it probably has to wait a bit longer (pyca/cryptography#6346 (comment))... Let's see how many ugly workarounds similar to the ones here or in #296 we need until then...

@felixfontein felixfontein merged commit a2a7d94 into ansible-collections:main Oct 3, 2021
@felixfontein felixfontein deleted the fix-cryptography branch October 3, 2021 14:53
@patchback
Copy link

patchback bot commented Oct 3, 2021 •
edited
Loading

Backport to stable-1: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-1/a2a7d940559cc1eac7457ffc0a0dcf0af08baa60/pr-294

Backported as #297

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Oct 3, 2021
@felixfontein
Support cryptography 35.0.0 for all modules except openssl_pkcs12 (#294)
70c86ee 
* Add some workarounds for cryptography 35.0.0.

* Make fix work with very old cryptography versions as well (which supported multiple backends).

* [TEMP] Disable openssl_pkcs12 tests to see whether everything else works.

* Revert "[TEMP] Disable openssl_pkcs12 tests to see whether everything else works."

This reverts commit 3f905bc.

* Add changelog fragment.

* Remove unnecessary assignment.

* Simplify code change.

* [TEMP] Disable openssl_pkcs12 tests to see whether everything else works.

* Revert "[TEMP] Disable openssl_pkcs12 tests to see whether everything else works."

This reverts commit fdb2105.

(cherry picked from commit a2a7d94)
@patchback patchback bot mentioned this pull request Oct 3, 2021
Merged
@felixfontein
Copy link
Contributor Author

@webknjaz @briantist thanks a lot for reviewing this!

felixfontein added a commit that referenced this pull request Oct 3, 2021
@patchback @felixfontein
Support cryptography 35.0.0 for all modules except openssl_pkcs12 (#294…
44b6df0 
…) (#297)

* Add some workarounds for cryptography 35.0.0.

* Make fix work with very old cryptography versions as well (which supported multiple backends).

* [TEMP] Disable openssl_pkcs12 tests to see whether everything else works.

* Revert "[TEMP] Disable openssl_pkcs12 tests to see whether everything else works."

This reverts commit 3f905bc.

* Add changelog fragment.

* Remove unnecessary assignment.

* Simplify code change.

* [TEMP] Disable openssl_pkcs12 tests to see whether everything else works.

* Revert "[TEMP] Disable openssl_pkcs12 tests to see whether everything else works."

This reverts commit fdb2105.

(cherry picked from commit a2a7d94)

Co-authored-by: Felix Fontein <felix@fontein.de>
@patchback patchback bot mentioned this pull request Oct 3, 2021
Merged
@renovate renovate bot mentioned this pull request Nov 1, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz left review comments

@briantist briantist briantist approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felixfontein @webknjaz @briantist