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_csr cryptography backend, try II #50894

Merged
merged 5 commits into from
Jan 21, 2019
Merged

openssl_csr cryptography backend, try II #50894

merged 5 commits into from
Jan 21, 2019

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jan 14, 2019 •
edited

SUMMARY

The new cryptography backend for openssl_csr (#50324) failed for some integration tests of other modules; see here.

This PR is for debugging that issue and fixing it. I've added a test commit (ffb07c6d4a004ad42acb95671834d5823bf392e4) which touches all tests using openssl_csr so that CI should reproduce the failure.

This PR also contains a fix (in form of a new feature) for #36690.

CC @mattclay

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

openssl_csr

@ansibot
Copy link
Contributor

ansibot commented Jan 14, 2019

cc @MarkusTeufelberger @Spredzy @gdelpierre @resmo
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. 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 Jan 14, 2019
MarkusTeufelberger
MarkusTeufelberger reviewed Jan 14, 2019
View reviewed changes
lib/ansible/modules/crypto/openssl_csr.py Outdated
can_use_pyopenssl = PYOPENSSL_FOUND and PYOPENSSL_VERSION >= LooseVersion(MINIMAL_PYOPENSSL_VERSION)

# Decision
if module.params['cipher'] and module.params['passphrase'] and module.params['cipher'] != 'auto':
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to be copy-pasted from openssl_privatekey - but this module here doesn't have these parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I know, I already prepared a commit, but wanted to wait until the first testing round finishes... which took like forever :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Those lemurs at shippable sure need to work harder! ;-)

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 14, 2019
@felixfontein
Copy link
Contributor Author

cryptography doesn't seem to like arbitrary strings for cryptography.x509.NameAttribute(cryptography.x509.oid.NameOID.COMMON_NAME, string) (see here with cryptography 1.7.2 on FreeBSD 11.1 and here with cryptography 1.3.1 on OpenSuSE 42.3).

@MarkusTeufelberger
Copy link
Contributor

Feels to me like an issue with something missing at the beginning of SubjectAlternativeNames (see around line 400 in your code how it should be prefixed with "DNS:").

Alternatively you can patch out automatically adding SANs to CSRs (I think there's an open bug about this).

@felixfontein
Copy link
Contributor Author

felixfontein commented Jan 14, 2019 •
edited

FAILED
freebsd11.1 1.0.2k-freebsd 1.7.2
opensuse42.3 1.0.2j-fips 1.3.1

SUCCESS
osx10.11 0.9.8zg 2.4.2
rhel7.6 1.0.2k-fips 1.7.2
rhel8.0 1.1.1 2.3
centos6 1.0.1e-fips 1.9
centos7 1.0.2k-fips 1.7.2
fedora28 1.1.0i-fips 2.3
fedora29 1.1.1 2.3
ubuntu1404 1.0.1f 2.2.2
ubuntu1604 1.0.2g 1.2.3 (The cryptography backend tests aren't run for version 1.2.3.)
ubuntu1604py3 1.0.2g 1.2.3

(CI image / OpenSSL version / cryptography version)

@felixfontein
Copy link
Contributor Author

The strange thing is that with older, newer and the same versions of both OpenSSL and cryptography, it works fine.

@MarkusTeufelberger
Copy link
Contributor

The bug about automatic SANs is #36690 btw.

@felixfontein
Copy link
Contributor Author

If this new test (which tests arbitrary commonName) fails everywhere where the cryptography backend tests are run, then I'll fix #36690 by adding a option to disable this behavior next ;-)

@felixfontein
Copy link
Contributor Author

All tests pass now (except a sanity one which I'll fix), see here for shippable output. I'll now rebase and remove that commit which forced all tests to run (and include the version_added which makes the sanity test fail).

@felixfontein felixfontein changed the title [WIP] openssl_csr cryptography backend, try II openssl_csr cryptography backend, try II Jan 14, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jan 14, 2019
MarkusTeufelberger
MarkusTeufelberger reviewed Jan 15, 2019
View reviewed changes
lib/ansible/modules/crypto/openssl_csr.py
backend = 'pyopenssl'

# Success?
if backend == 'auto':
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this just be the else of lines 985-988?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could. I would still prefer this not to be an else, so that the failure code is somewhat separated from the detection code.

@felixfontein
Copy link
Contributor Author

Does anyone mind if this gets merged?

@MarkusTeufelberger
Copy link
Contributor

Not me...

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 Jan 20, 2019
@gundalow gundalow merged commit 345011e into ansible:devel Jan 21, 2019
@gundalow
Copy link
Contributor

@felixfontein Thanks for the PR
@MarkusTeufelberger Thanks for the review.

Merged into devel for release in Ansible 2.8

@felixfontein felixfontein deleted the openssl_csr-crypto-revert branch January 21, 2019 18:15
@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger thanks for reviewing!
@gundalow thanks for merging!

knumskull pushed a commit to knumskull/ansible that referenced this pull request Jan 21, 2019
@felixfontein @knumskull
openssl_csr cryptography backend, try II (ansible#50894)
540abe5 
* Revert "Revert "openssl_csr: Allow to use cryptography as backend (ansible#50324)""

This reverts commit bbd2e31.

* Remove more complicated selection copy'n'pasted from openssl_privatekey.

* Add tests for backend selection.

* Add openssl_csr test for arbitrary string commonName.

* Allow to disable commonName -> SAN copying (fixes ansible#36690).
@dagwieers dagwieers added the crypto Crypto community (ACME, openssl, letsencrypt) label Feb 7, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MarkusTeufelberger MarkusTeufelberger MarkusTeufelberger left review comments

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.

None yet
5 participants
@felixfontein @ansibot @MarkusTeufelberger @gundalow @dagwieers