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

Extension parsing: add new fallback code which uses the new cryptography API #331

Merged

Conversation

felixfontein
Copy link
Contributor

SUMMARY

This adds a fallback to extension parsing which uses the new API from pyca/cryptography#6346. I've used the new code as a fallback since it is potentially a breaking change (as announced as upcoming eventually in the changelog for 2.0.0). Switching to it as a default should only happen in a new minor release, while this can (and should) go into a bugfix release.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

get_certificate
openssl_csr_info
x509_certificate_info

@felixfontein
Copy link
Contributor Author

ready_for_review

@felixfontein
Copy link
Contributor Author

pyca/cryptography#6346 has been merged. One thing that might get fixed are the differences for KeyUsage that crept into our tests (https:/ansible-collections/community.crypto/pull/331/files#diff-1f795e32ff22e33251b41bd3572823f7677804e01c3fcf8ceb56185c22014537R158, https:/ansible-collections/community.crypto/pull/331/files#diff-1f795e32ff22e33251b41bd3572823f7677804e01c3fcf8ceb56185c22014537R35, https:/ansible-collections/community.crypto/pull/331/files#diff-c809c2bf907465bdbbfa9115178aef8985383112034fa42b1c347eae05bd8c97R24), though we likely have to keep supporting both values since 35.0.0 already introduced this difference. (But maybe we can do something like only accept the other value if cryptography major version == 35. But that's something I would do in a follow-up PR on main only, since the logic would be even more PITA on stable-1...)

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

@felixfontein felixfontein changed the title [WIP] Extension parsing: add new fallback code which uses the new cryptography API Extension parsing: add new fallback code which uses the new cryptography API Nov 22, 2021
@felixfontein felixfontein merged commit 3f40795 into ansible-collections:main Nov 22, 2021
@felixfontein felixfontein deleted the cryptography-serialize-exts branch November 22, 2021 06:42
@felixfontein
Copy link
Contributor Author

@Ajpantuso thanks a lot for also reviewing this one!

@patchback
Copy link

patchback bot commented Nov 22, 2021

Backport to stable-1: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-1/3f40795a98a7fae560ea2edd544276eb83a2cd45/pr-331

Backported as #345

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

patchback bot pushed a commit that referenced this pull request Nov 22, 2021
…phy API (#331)

* Add new code as fallback which re-serializes de-serialized extensions using the new cryptography API.

* Forgot Base64 encoding.

* Add extension by OID tests.

* There's one value which is different with the new code.

* Differences in CI.

* Working around older Jinjas.

* Value depends on which SAN was included.

* Force complete CI run now since cryptography 36.0.0 is out.

ci_complete

(cherry picked from commit 3f40795)
felixfontein added a commit that referenced this pull request Nov 22, 2021
…back code which uses the new cryptography API (#345)

* Extension parsing: add new fallback code which uses the new cryptography API (#331)

* Add new code as fallback which re-serializes de-serialized extensions using the new cryptography API.

* Forgot Base64 encoding.

* Add extension by OID tests.

* There's one value which is different with the new code.

* Differences in CI.

* Working around older Jinjas.

* Value depends on which SAN was included.

* Force complete CI run now since cryptography 36.0.0 is out.

ci_complete

(cherry picked from commit 3f40795)

* Adjust tests.

Co-authored-by: Felix Fontein <[email protected]>
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.

2 participants