-
Notifications
You must be signed in to change notification settings - Fork 86
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_csr: handle missing basic constraint #180
Conversation
Thanks for the PR! I'll take a look tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a changelog fragment?
As suggested by felixfontein
I have made the suggest modifications. I frankly find the logic in this method confusing. Initially I rewrote the whole method but decided on a more minimal change. The logic seems to be get the fields from both objects, giving them non-matching defaults if missing and then compare the fields one at a time. I would have tested that both objects existed before doing so which would caused this bug to never happen. I also tend to check each field for equality and then return true after all checks letting the optimizer convert the return true to the result of the last comparison because it makes it far easier to modify in the future. This is mostly taste and style which is why I went with the minimal change. Hopefully the change log fragment is sufficient. There is only one other in this repo for an example;) |
Thanks!
I agree, I cannot remember why the function is as it is now, and I don't think there's a reason.
What do you mean by "non-matching defaults"?
For every release, all fragments are incorporated in changelogs/changelog.yaml, so you can find more examples there: https://github.com/ansible-collections/community.crypto/blob/main/changelogs/changelog.yaml#L289 |
Oops, I meant in the general case not the specific case. Eg: class has fields field1 and field2, if object_a.field1 != object_b.field1 return false. If object_a.field2 != object.field2 return false. Finally, return true;) |
@schallee thanks for fixing this! |
…2.9.19 Alexander Sowitzki (1): [stable-2.9] Bump azure-pipelines-test-container to version 1.8.0 (#73550) Ashwini Mhatre (1): [stable-2.9]:Fix IOSXR integration test (#73607) Felix Fontein (4): Backport of ansible-collections/community.docker@8702713 (#73638) Backport of bugfix parts of ansible-collections/community.docker#87 to stable-2.9. (#73817) Backport of ansible-collections/community.docker#88 to stable-1. (#73816) Backport of ansible-collections/community.crypto#180 to stable-2.9. (#73815) Gonéri Le Bouder (2): ansible-test: yamllint, check the assigment (#73584) [ansible-test] attempt to work around podman (#72096) (#73570) Mark Chappell (2): New AWS module mod_defaults - ec2_vpc_endpoint_service_info (#73670) New AWS module mod_defaults - iam_saml_federation (#73669) Matt Martz (2): [stable-2.9] Normalize ConfigParser between Python2 and Python3 (#73715) (#73724) [stable-2.9] Don't treat host_pinned as lockstep (#73484) (#73513) Rhys (1): Update mongodb replicaset check_compatibility function (#72299) Rick Elrod (4): Update Ansible release version to v2.9.18.post0. New release v2.9.19rc1 Update Ansible release version to v2.9.19rc1.post0. New release v2.9.19 Sam Doran (5): Update signing key used in incidental_setup_flatpak_remote tests Rebalance Windows test groups to avoid timeouts Move win_uri to a group that does not reboot the test instance [stable-2.9] hostname - add Almalinux support (#73619) (#73649) [stable-2.9] Add AlmaLinux to the family of Red Hat-like operating systems (#73541) (#73544) Zadkiel (1): terraform: Remove line that is suppressing output being shown (#66322) (#73803)
SUMMARY
Fixes #179 by verifying the basic constraint exists before checking it is critical.
ISSUE TYPE
COMPONENT NAME
openssl_csr
ADDITIONAL INFORMATION