Prefer the stdlib SSLContext over urllib3 context #32053

abadger · 2017-10-23T20:29:55Z

We do not go through the effort of finding the right PROTOCOL setting if
we have SSLContext in the stdlib. So we do not want to hit the code
that uses PROTOCOL to set the urllib3-provided ssl context when
SSLContext is available. Also, the urllib3 implementation appears to
have a bug in some recent versions. Preferring the stdlib version will
work around that for those with Python-2.7.9+ as well.

Fixes #26235
Fixes #25402
Fixes #31998

@sivel, if you have time, please review this.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

lib/ansible/module_utils/urls.py

ANSIBLE VERSION

devel, 2.4, 2.3 (but I don't know if we'lll backport to 2.3.x)

We do not go through the effort of finding the right PROTOCOL setting if we have SSLContext in the stdlib. So we do not want to hit the code that uses PROTOCOL to set the urllib3-provided ssl context when SSLContext is available. Also, the urllib3 implementation appears to have a bug in some recent versions. Preferring the stdlib version will work around that for those with Python-2.7.9+ as well. Fixes ansible#26235 Fixes ansible#25402 Fixes ansible#31998

sivel · 2017-10-23T21:08:28Z

lib/ansible/module_utils/urls.py

-        if HAS_URLLIB3_PYOPENSSLCONTEXT:
+        if HAS_SSLCONTEXT:
+            context = create_default_context()
+        elif HAS_URLLIB3_PYOPENSSLCONTEXT:


I like this. It should have been done this way to start with.

sivel

Looks good. The only thing I wonder is whether we could do a try/except around importing tlsv1_2 to use it if available, failing back to v1

alikins · 2017-10-23T21:17:02Z

There is similar code around the CustomHTTPSConnection (https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py#L365). Should it be updated as well?

sivel · 2017-10-23T21:22:18Z

There is similar code around the CustomHTTPSConnection (https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py#L365). Should it be updated as well?

That seems to be in the correct order.

abadger · 2017-10-24T14:17:13Z

@sivel I think we could use a try: except in the imports but it would still need the ctypes code in addition. The vanilla stdlib introduces SSLContext in the same version as it introduces PROTOCOL_TLSv1_2 (2.7.9). I know that RHEL7 backported those changes to python-2.7.5 but it backported both changes. So right now the check for if not HAS_SSLCONTEXT is also a hidden (version-based) check for PROTOCOL_TLSv1_2. If we added a try: except to import PROTOCOL_TLSv1_2, it would end up being in addition to the SSLCONTEXT check.

We still end up using ctypes as the next step as that check finds out if the underlying openssl will support TLS-1.1 and TLS-1.2. If it does, then we set the PROTOCOL to ssl.PROTOCOL_SSLv23 which allows the library to negotiate the best version that both the client and server support. If it doesn't, then we leave the protocol setting at ssl.PROTOCOL_TLSv1 which at least prevents ssl2/3 from being used. So a try: except will help us out if there's platforms which have PROTOCOL_TLSv1_2 exposed in the python layer but do not implement SSLContext.

abadger · 2017-10-24T14:22:47Z

@alikins combining the code is not needed because the order is correct in the second instance but it does look tantaliyzingly like duplicate that could be combined. The _make_context() helper is not using self so we could move that out to a toplevel helper function. One is using context.load_verify_locations(to_add_ca_cert_path) while the other is using context.load_cert_chain(self.cert_file, self.key_file) though. I don't know what changing that code would do so I'm loathe to change that in a bugfix. Perhaps that's something someone can research and refactor in devel if they have time.

sivel · 2017-10-24T14:23:02Z

I think that makes sense. We should move forward with this as is.

As for TLS versions, I suppose I speak mostly from experience on Mac. Although SSLContext is available on Mac, PROTOCOL_TLSv1_2 is not, at least not on macOS 10.12 running Python 2.7.10.

abadger · 2017-10-24T14:24:04Z

Interesting. I wonder if that means the underlying openssl on MacOS 10.12 also doesn't support PROTOCOL_TLSv1_2.

abadger · 2017-10-24T14:36:40Z

Merged to devel and cherry-picked to temp-staging-post-2.4.1 This fix will be present in the 2.4.2beta1 release but not in the 2.4.1 release that's due out later this week.

There is an issue with ansible 2.4.0 to validate certs with get_url. The validation fails if liburl3 is present on the server (with pike version, it's functional with newton version) issue: ansible/ansible#32053 The statement could be remove when ansible 2.4.2 will be available on centos 7 Change-Id: I25219b24c85a74672a0c56da58f913d53ffb4241 Depends-on: I33a8536f51078d56427b2bef38dc11a9488390d6

In #414 we tried making this include conditional, however this playbook is still being included when run w/ older versions of ansible (1.9.6 as an example). Instead, we switch the two `Add influxdata apt-keys` tasks to using the shell module. Using ansible's apt_key fails w/ hosts using SNI (as https://repos.influxdata.com/ appears to be using) because: 1. ansible/ansible#32053 (not backported to 2.3.2.0) 2. ansible 1.9.6 doesn't seem to support SNI on Trusty (python < 2.7.9)

In #414 we tried making this include conditional, however this playbook is still being included when run w/ older versions of ansible (1.9.6 as an example). Instead, we update maas_influxdata_key to have an id and keyserver key. This allows apt_key to work since it doesn't need to speak to https://repos.influxdata.com/ directly. Using ansible's apt_key with https://repos.influxdata.com/ fails because of SNI and: 1. ansible/ansible#32053 (not backported to 2.3.2.0) 2. ansible 1.9.6 doesn't seem to support SNI on Trusty (python < 2.7.9)

abadger · 2018-02-23T19:18:44Z

Cherry-picked to stable-2.3 for a 2.3.4 release.

sivel self-requested a review October 23, 2017 20:30

ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request module_utils/urls needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 23, 2017

This was referenced Oct 23, 2017

Fixes #25402 - bad handshake failure #26235

Closed

SSLValidationError in urls.py when urllib3 is installed with ansbile 2.4.0 and pyhon 2.7.13 #31998

Closed

abadger added this to To be cherrypicked (rc only) in 2.4.x Blocker List Oct 23, 2017

abadger moved this from To be cherrypicked (rc only) to TODO: Next release in 2.4.x Blocker List Oct 23, 2017

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 23, 2017

sivel reviewed Oct 23, 2017

View reviewed changes

sivel approved these changes Oct 23, 2017

View reviewed changes

abadger merged commit 725ae96 into ansible:devel Oct 24, 2017

abadger deleted the fix-ssl-context-creation branch October 24, 2017 14:22

abadger moved this from TODO: Next release to Done in 2.4.2 in 2.4.x Blocker List Oct 26, 2017

samdoran removed the needs_triage Needs a first human triage before being processed. label Oct 26, 2017

abadger mentioned this pull request Nov 16, 2017

SSL: SSLV3_ALERT_HANDSHAKE_FAILURE with ansible-playbook 2.4.1.0 #32964

Closed

jacksonp2008 mentioned this pull request Nov 27, 2017

Request failed: <urlopen error [SSL: SSLV3_ALERT_HANDSHAKE_FAILURE] sslv3 alert handshake failure (_ssl.c:590)>" #33312

Closed

mattt416 mentioned this pull request Dec 12, 2017

Update maas_influxdata_key variable rcbops/rpc-maas#415

Merged

andrewkroh mentioned this pull request Jan 9, 2018

TLS v1.2 issues downloading from S3 elastic/beats-tester#60

Closed

sodar mentioned this pull request Feb 27, 2018

ansible: bump to 2.4.2.0 in requirements.txt codilime/contrail-windows-ci#126

Merged

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

dagwieers added the crypto Crypto community (ACME, openssl, letsencrypt) label Feb 7, 2019

wangxin mentioned this pull request Apr 1, 2019

[docker] Use recommended CMD for getting docker GPG key sonic-net/sonic-mgmt#843

Merged

1 task

hectcastro mentioned this pull request Apr 9, 2019

Upgrade to Django 1.11 and Python 3.6 azavea/climate-change-api#882

Merged

2 tasks

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer the stdlib SSLContext over urllib3 context #32053

Prefer the stdlib SSLContext over urllib3 context #32053

abadger commented Oct 23, 2017

sivel Oct 23, 2017

sivel left a comment

alikins commented Oct 23, 2017

sivel commented Oct 23, 2017

abadger commented Oct 24, 2017

abadger commented Oct 24, 2017

sivel commented Oct 24, 2017

abadger commented Oct 24, 2017

abadger commented Oct 24, 2017

abadger commented Feb 23, 2018

Prefer the stdlib SSLContext over urllib3 context #32053

Prefer the stdlib SSLContext over urllib3 context #32053

Conversation

abadger commented Oct 23, 2017

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

sivel Oct 23, 2017

Choose a reason for hiding this comment

sivel left a comment

Choose a reason for hiding this comment

alikins commented Oct 23, 2017

sivel commented Oct 23, 2017

abadger commented Oct 24, 2017

abadger commented Oct 24, 2017

sivel commented Oct 24, 2017

abadger commented Oct 24, 2017

abadger commented Oct 24, 2017

abadger commented Feb 23, 2018