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

openssl_certificate, fixed has_expired to check the cert expiration date #53168

Merged
merged 9 commits into from
Mar 1, 2019
Merged

openssl_certificate, fixed has_expired to check the cert expiration date #53168

merged 9 commits into from
Mar 1, 2019

Conversation

Shaps
Copy link
Contributor

@Shaps Shaps commented Mar 1, 2019

SUMMARY

has_expired was not actually checking the certificate expiration, and had an issue with older versions of the pyOpenSSL package which was incorrectly verifying the expiry date.
Also adds tests for this case

Fixes #51267

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

openssl_certificate

ADDITIONAL INFORMATION

N/A

@Shaps
has_expired now does what's supposed to
8ba57b7 
When set to true will ensure the certificate is expired; when set to
false ensures the certificate is not expired

Also fixes an issue with older versions of pyOpenSSL which prevented the
certificate expiry date to be validated correctly.
@Shaps
Added tests for expired certificate
9be4df6
@Shaps
Added changelog fragment
4da13c9
@Shaps Shaps mentioned this pull request Mar 1, 2019
Closed
@ansibot
Copy link
Contributor

ansibot commented Mar 1, 2019

cc @MarkusTeufelberger @Spredzy @Xyon @felixfontein @puiterwijk @resmo
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. crypto Crypto community (ACME, openssl, letsencrypt) module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Mar 1, 2019
felixfontein
felixfontein approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Just some nits :)

changelogs/fragments/openssl_certificate_fix_has_expired.yml Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py
self.message.append(
'Certificate expiration check failed (certificate expiration is %s, expected %s)' % (self.cert.has_expired(), self.has_expired)
)
time_string = to_native(self.cert.get_notAfter())
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment here that this is similar to the PyOpenSSL code for cert.has_expired(), but that older versions have a buggy implementation and we thus do it manually here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments, hopefully makes sense :)

test/integration/targets/openssl_certificate/tasks/expired.yml Outdated Show resolved Hide resolved
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 1, 2019
@felixfontein @Shaps
clarify which PyOpenSSL version we're talking about
765bb34 
Co-Authored-By: Shaps <andrea@braingap.uk>
felixfontein
felixfontein approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 1, 2019
resmo
resmo approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

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

shipit

@resmo
Copy link
Contributor

resmo commented Mar 1, 2019

bot_status

@ansibot
Copy link
Contributor

ansibot commented Mar 1, 2019

Components

changelogs/fragments/openssl_certificate_fix_has_expired.yml
support: community
maintainers:

lib/ansible/modules/crypto/openssl_certificate.py
support: community
maintainers: MarkusTeufelberger Shaps Spredzy Xyon felixfontein puiterwijk resmo

test/integration/targets/openssl_certificate/tasks/expired.yml
support: community
maintainers: MarkusTeufelberger Shaps Spredzy Xyon felixfontein puiterwijk resmo

test/integration/targets/openssl_certificate/tasks/main.yml
support: community
maintainers: MarkusTeufelberger Shaps Spredzy Xyon felixfontein puiterwijk resmo

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 3
shipit_actors (maintainers or core team members): felixfontein resmo Shaps
shipit_actors_other: []
automerge: automerge !module file(s) test failed

click here for bot help

@resmo resmo merged commit d5d92e4 into ansible:devel Mar 1, 2019
@felixfontein
Copy link
Contributor

@resmo I think automerge didn't work because of the changelog fragment... (The usual fun...)

@Shaps thanks a lot for fixing this!
@resmo thanks for reviewing and merging!

@felixfontein felixfontein mentioned this pull request Mar 2, 2019
Closed
@felixfontein felixfontein mentioned this pull request Mar 2, 2019
Merged
felixfontein pushed a commit to felixfontein/ansible that referenced this pull request Mar 2, 2019
@Shaps @felixfontein
openssl_certificate, fixed has_expired to check the cert expiration d…
cefad5b 
…ate (ansible#53168)

(cherry picked from commit d5d92e4)
abadger pushed a commit that referenced this pull request Mar 4, 2019
@felixfontein @abadger
[2.7] openssl_certificate, fixed has_expired to check the cert expira…
597db1d 
…tion date (#53203)

* Type error in openssl_certificate (#47508)

* Fixed #47505: Type error in openssl_certificate
* Use to_bytes instead of str.encode in SelfSignedCertificate. Updates #47508
* Use to_bytes instead of str.encode in OwnCACertificate
* Added integration tests for openssl_certificate: selfsigned_not_before/after and ownca_not_before/after

(cherry picked from commit 5b1c685)

* openssl_certificate, fixed has_expired to check the cert expiration date (#53168)

(cherry picked from commit d5d92e4)

* Use fixed timestamp in past instead of relative time (relative times are a feature of devel).

* Add changelog for #47508.
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@resmo resmo resmo approved these changes

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. crypto Crypto community (ACME, openssl, letsencrypt) module This issue/PR relates to a module. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

openssl_certificate: has_expired=no does nothing
4 participants
@Shaps @ansibot @resmo @felixfontein