Skip to content

Commit

Permalink
Vendored Docker SDK for Python updates (#434)
Browse files Browse the repository at this point in the history
* utils: fix IPv6 address w/ port parsing

This was using a deprecated function (`urllib.splitnport`),
ostensibly to work around issues with brackets on IPv6 addresses.

Ironically, its usage was broken, and would result in mangled IPv6
addresses if they had a port specified in some instances.

Usage of the deprecated function has been eliminated and extra test
cases added where missing. All existing cases pass as-is. (The only
other change to the test was to improve assertion messages.)

Cherry-picked from
docker/docker-py@f16c4e1

Co-authored-by: Milas Bowman <milas.bowman@docker.com>

* client: fix exception semantics in _raise_for_status

We want "The above exception was the direct cause of the following exception:" instead of "During handling of the above exception, another exception occurred:"

Cherry-picked from
docker/docker-py@bb11197

Co-authored-by: Maor Kleinberger <kmaork@gmail.com>

* tls: use auto-negotiated highest version

Specific TLS versions are deprecated in latest Python, which
causes test failures due to treating deprecation errors as
warnings.

Luckily, the fix here is straightforward: we can eliminate some
custom version selection logic by using `PROTOCOL_TLS_CLIENT`,
which is the recommended method and will select the highest TLS
version supported by both client and server.

Cherry-picked from
docker/docker-py@56dd6de

Co-authored-by: Milas Bowman <milas.bowman@docker.com>

* transport: fix ProxyCommand for SSH conn

Cherry-picked from
docker/docker-py@4e19cc4

Co-authored-by: Guy Lichtman <glicht@users.noreply.github.com>

* ssh: do not create unnecessary subshell on exec

Cherry-picked from
docker/docker-py@bb40ba0

Co-authored-by: liubo <liubo@uniontech.com>

* ssh: reject unknown host keys when using Python SSH impl

In the Secure Shell (SSH) protocol, host keys are used to verify the identity of remote hosts. Accepting unknown host keys may leave the connection open to man-in-the-middle attacks.

Do not accept unknown host keys. In particular, do not set the default missing host key policy for the Paramiko library to either AutoAddPolicy or WarningPolicy. Both of these policies continue even when the host key is unknown. The default setting of RejectPolicy is secure because it throws an exception when it encounters an unknown host key.

Reference: https://cwe.mitre.org/data/definitions/295.html

NOTE: This only affects SSH connections using the native Python SSH implementation (Paramiko), when `use_ssh_client=False` (default). If using the system SSH client (`use_ssh_client=True`), the host configuration
(e.g. `~/.ssh/config`) will apply.

Cherry-picked from
docker/docker-py@d929864

Co-authored-by: Audun Nes <audun.nes@gmail.com>

* lint: fix deprecation warnings from threading package

Set `daemon` attribute instead of using `setDaemon` method that
was deprecated in Python 3.10.

Cherry-picked from
docker/docker-py@adf5a97

Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

* api: preserve cause when re-raising error

Use `from e` to ensure that the error context is propagated
correctly.

Cherry-picked from
docker/docker-py@05e1434

Co-authored-by: Milas Bowman <milas.bowman@docker.com>

* build: trim trailing whitespace from dockerignore entries

Cherry-picked from
docker/docker-py@3ee3a24

Co-authored-by: Clément Loiselet <clement.loiselet@capgemini.com>

* Improve formulation, also mention the security change as a breaking change.

Co-authored-by: Milas Bowman <milas.bowman@docker.com>
Co-authored-by: Maor Kleinberger <kmaork@gmail.com>
Co-authored-by: Guy Lichtman <glicht@users.noreply.github.com>
Co-authored-by: liubo <liubo@uniontech.com>
Co-authored-by: Audun Nes <audun.nes@gmail.com>
Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
Co-authored-by: Clément Loiselet <clement.loiselet@capgemini.com>
  • Loading branch information
8 people authored Jul 31, 2022
1 parent a33e51e commit ae708a7
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 33 deletions.
11 changes: 11 additions & 0 deletions changelogs/fragments/docker-py-changes-1.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
bugfixes:
- "modules and plugins communicating directly with the Docker daemon - fix parsing of IPv6 addresses with a port in ``docker_host``. This is only a change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
- "modules and plugins communicating directly with the Docker daemon - fix ``ProxyCommand`` handling for SSH connections when not using ``use_ssh_client=true``. This is only a change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
- "modules and plugins communicating directly with the Docker daemon - do not create a subshell for SSH connections when using ``use_ssh_client=true``. This is only a change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
- "docker_image - when composing the build context, trim trailing whitespace from ``.dockerignore`` entries. This is only a change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
minor_changes:
- "modules and plugins communicating directly with the Docker daemon - improve default TLS version selection for Python 3.6 and newer. This is only a change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
security_fixes:
- "modules and plugins communicating directly with the Docker daemon - when connecting by SSH and not using ``use_ssh_client=true``, reject unknown host keys instead of accepting them. This is only a change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
breaking_changes:
- "modules and plugins communicating directly with the Docker daemon - when connecting by SSH and not using ``use_ssh_client=true``, reject unknown host keys instead of accepting them. This is only a breaking change relative to older community.docker 3.0.0 pre-releases or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
4 changes: 2 additions & 2 deletions plugins/module_utils/_api/api/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import struct
from functools import partial

from ansible.module_utils.six import PY3, binary_type, iteritems, string_types
from ansible.module_utils.six import PY3, binary_type, iteritems, string_types, raise_from
from ansible.module_utils.six.moves.urllib.parse import quote

from .. import auth
Expand Down Expand Up @@ -258,7 +258,7 @@ def _raise_for_status(self, response):
try:
response.raise_for_status()
except _HTTPError as e:
raise create_api_error_from_http_exception(e)
raise_from(create_api_error_from_http_exception(e), e)

def _result(self, response, json=False, binary=False):
if json and binary:
Expand Down
4 changes: 3 additions & 1 deletion plugins/module_utils/_api/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

from ._import_helper import HTTPError as _HTTPError

from ansible.module_utils.six import raise_from


class DockerException(Exception):
"""
Expand Down Expand Up @@ -40,7 +42,7 @@ def create_api_error_from_http_exception(e):
cls = ImageNotFound
else:
cls = NotFound
raise cls(e, response=response, explanation=explanation)
raise_from(cls(e, response=response, explanation=explanation), e)


class APIError(_HTTPError, DockerException):
Expand Down
10 changes: 4 additions & 6 deletions plugins/module_utils/_api/tls.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import os
import ssl
import sys

from . import errors
from .transport.ssladapter import SSLHTTPAdapter
Expand Down Expand Up @@ -49,15 +50,10 @@ def __init__(self, client_cert=None, ca_cert=None, verify=None,
self.assert_hostname = assert_hostname
self.assert_fingerprint = assert_fingerprint

# TODO(dperny): according to the python docs, PROTOCOL_TLSvWhatever is
# depcreated, and it's recommended to use OPT_NO_TLSvWhatever instead
# to exclude versions. But I think that might require a bigger
# architectural change, so I've opted not to pursue it at this time

# If the user provides an SSL version, we should use their preference
if ssl_version:
self.ssl_version = ssl_version
else:
elif (sys.version_info.major, sys.version_info.minor) < (3, 6):
# If the user provides no ssl version, we should default to
# TLSv1_2. This option is the most secure, and will work for the
# majority of users with reasonably up-to-date software. However,
Expand All @@ -73,6 +69,8 @@ def __init__(self, client_cert=None, ca_cert=None, verify=None,
# SSLv23 fails in mysterious ways:
# https://github.com/docker/docker-py/issues/963
self.ssl_version = ssl.PROTOCOL_TLSv1
else:
self.ssl_version = ssl.PROTOCOL_TLS_CLIENT

# "client_cert" must have both or neither cert/key files. In
# either case, Alert the user when both are expected, but any are
Expand Down
7 changes: 3 additions & 4 deletions plugins/module_utils/_api/transport/sshconn.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,8 @@ def f():
env.pop('SSL_CERT_FILE', None)

self.proc = subprocess.Popen(
' '.join(args),
args,
env=env,
shell=True,
stdout=subprocess.PIPE,
stdin=subprocess.PIPE,
preexec_fn=preexec_func)
Expand Down Expand Up @@ -225,7 +224,7 @@ def _create_paramiko_client(self, base_url):
host_config = conf.lookup(base_url.hostname)
if 'proxycommand' in host_config:
self.ssh_params["sock"] = paramiko.ProxyCommand(
self.ssh_conf['proxycommand']
host_config['proxycommand']
)
if 'hostname' in host_config:
self.ssh_params['hostname'] = host_config['hostname']
Expand All @@ -237,7 +236,7 @@ def _create_paramiko_client(self, base_url):
self.ssh_params['key_filename'] = host_config['identityfile']

self.ssh_client.load_system_host_keys()
self.ssh_client.set_missing_host_key_policy(paramiko.WarningPolicy())
self.ssh_client.set_missing_host_key_policy(paramiko.RejectPolicy())

def _connect(self):
if self.ssh_client:
Expand Down
3 changes: 3 additions & 0 deletions plugins/module_utils/_api/utils/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ def __init__(self, pattern_str):
@classmethod
def normalize(cls, p):

# Remove trailing spaces
p = p.strip()

# Leading and trailing slashes are not relevant. Yes,
# "foo.py/" must exclude the "foo.py" regular file. "."
# components are not relevant either, even if the whole
Expand Down
41 changes: 25 additions & 16 deletions plugins/module_utils/_api/utils/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
__metaclass__ = type

import base64
import collections
import json
import os
import os.path
Expand All @@ -22,17 +23,22 @@
from ansible.module_utils.six import PY2, PY3, binary_type, integer_types, iteritems, string_types, text_type

from .. import errors
from .. import tls
from ..constants import DEFAULT_HTTP_HOST
from ..constants import DEFAULT_UNIX_SOCKET
from ..constants import DEFAULT_NPIPE
from ..constants import BYTE_UNITS
from ..tls import TLSConfig

if PY2:
from urllib import splitnport
from urlparse import urlparse
from urlparse import urlparse, urlunparse
else:
from urllib.parse import splitnport, urlparse
from urllib.parse import urlparse, urlunparse


URLComponents = collections.namedtuple(
'URLComponents',
'scheme netloc url params query fragment',
)


def create_ipam_pool(*args, **kwargs):
Expand Down Expand Up @@ -220,10 +226,6 @@ def parse_repository_tag(repo_name):


def parse_host(addr, is_win32=False, tls=False):
path = ''
port = None
host = None

# Sensible defaults
if not addr and is_win32:
return DEFAULT_NPIPE
Expand Down Expand Up @@ -282,20 +284,20 @@ def parse_host(addr, is_win32=False, tls=False):
# to be valid and equivalent to unix:///path
path = '/'.join((parsed_url.hostname, path))

netloc = parsed_url.netloc
if proto in ('tcp', 'ssh'):
# parsed_url.hostname strips brackets from IPv6 addresses,
# which can be problematic hence our use of splitnport() instead.
host, port = splitnport(parsed_url.netloc)
if port is None or port < 0:
port = parsed_url.port or 0
if port <= 0:
if proto != 'ssh':
raise errors.DockerException(
'Invalid bind address format: port is required:'
' {0}'.format(addr)
)
port = 22
netloc = '{0}:{1}'.format(parsed_url.netloc, port)

if not host:
host = DEFAULT_HTTP_HOST
if not parsed_url.hostname:
netloc = '{0}:{1}'.format(DEFAULT_HTTP_HOST, port)

# Rewrite schemes to fit library internals (requests adapters)
if proto == 'tcp':
Expand All @@ -305,7 +307,14 @@ def parse_host(addr, is_win32=False, tls=False):

if proto in ('http+unix', 'npipe'):
return "{0}://{1}".format(proto, path).rstrip('/')
return '{0}://{1}:{2}{3}'.format(proto, host, port, path).rstrip('/')
return urlunparse(URLComponents(
scheme=proto,
netloc=netloc,
url=path,
params='',
query='',
fragment='',
)).rstrip('/')


def parse_devices(devices):
Expand Down Expand Up @@ -370,7 +379,7 @@ def kwargs_from_env(ssl_version=None, assert_hostname=None, environment=None):
# so if it's not set already then set it to false.
assert_hostname = False

params['tls'] = tls.TLSConfig(
params['tls'] = TLSConfig(
client_cert=(os.path.join(cert_path, 'cert.pem'),
os.path.join(cert_path, 'key.pem')),
ca_cert=os.path.join(cert_path, 'ca.pem'),
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/plugins/module_utils/_api/api/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ def setUp(self):
self.server_socket = self._setup_socket()
self.stop_server = False
server_thread = threading.Thread(target=self.run_server)
server_thread.setDaemon(True)
server_thread.daemon = True
server_thread.start()
self.response = None
self.request_handler = None
Expand Down Expand Up @@ -519,7 +519,7 @@ def setup_class(cls):
cls.server = six.moves.socketserver.ThreadingTCPServer(
('', 0), cls.get_handler_class())
cls.thread = threading.Thread(target=cls.server.serve_forever)
cls.thread.setDaemon(True)
cls.thread.daemon = True
cls.thread.start()
cls.address = 'http://{0}:{1}'.format(
socket.gethostname(), cls.server.server_address[1])
Expand Down
11 changes: 9 additions & 2 deletions tests/unit/plugins/module_utils/_api/utils/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,17 +285,24 @@ def test_parse_host(self):
'[fd12::82d1]:2375/docker/engine': (
'http://[fd12::82d1]:2375/docker/engine'
),
'ssh://[fd12::82d1]': 'ssh://[fd12::82d1]:22',
'ssh://user@[fd12::82d1]:8765': 'ssh://user@[fd12::82d1]:8765',
'ssh://': 'ssh://127.0.0.1:22',
'ssh://user@localhost:22': 'ssh://user@localhost:22',
'ssh://user@remote': 'ssh://user@remote:22',
}

for host in invalid_hosts:
with pytest.raises(DockerException):
msg = 'Should have failed to parse invalid host: {0}'.format(host)
with self.assertRaises(DockerException, msg=msg):
parse_host(host, None)

for host, expected in valid_hosts.items():
assert parse_host(host, None) == expected
self.assertEqual(
parse_host(host, None),
expected,
msg='Failed to parse valid host: {0}'.format(host),
)

def test_parse_host_empty_value(self):
unix_socket = 'http+unix:///var/run/docker.sock'
Expand Down

0 comments on commit ae708a7

Please sign in to comment.