From ae708a7333a17bc564f007f237092dce7756494e Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 31 Jul 2022 17:09:18 +0200 Subject: [PATCH] Vendored Docker SDK for Python updates (#434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 https://github.com/docker/docker-py/commit/f16c4e1147c81afd822fe72191f0f720cb0ba637 Co-authored-by: Milas Bowman * 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 https://github.com/docker/docker-py/commit/bb11197ee3407798a53c50e43aa994fe8cd9c8e7 Co-authored-by: Maor Kleinberger * 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 https://github.com/docker/docker-py/commit/56dd6de7dfad9bedc7c8af99308707ecc3fad78e Co-authored-by: Milas Bowman * transport: fix ProxyCommand for SSH conn Cherry-picked from https://github.com/docker/docker-py/commit/4e19cc48dfd88d0a9a8bdbbe4df4357322619d02 Co-authored-by: Guy Lichtman * ssh: do not create unnecessary subshell on exec Cherry-picked from https://github.com/docker/docker-py/commit/bb40ba051fc67605d5c9e7fd1eb5f9aa3e0fb501 Co-authored-by: liubo * 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 https://github.com/docker/docker-py/commit/d9298647d91c52e1ee9ac448e43a7fea1c69bdbe Co-authored-by: Audun Nes * 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 https://github.com/docker/docker-py/commit/adf5a97b1203623ae47bf7aa1367b6bb7c261980 Co-authored-by: Karthikeyan Singaravelan * api: preserve cause when re-raising error Use `from e` to ensure that the error context is propagated correctly. Cherry-picked from https://github.com/docker/docker-py/commit/05e143429e892fb838bbff058391456ba3d0a19c Co-authored-by: Milas Bowman * build: trim trailing whitespace from dockerignore entries Cherry-picked from https://github.com/docker/docker-py/commit/3ee3a2486fe75ed858f8a3defe0fc79b2743d5df Co-authored-by: Clément Loiselet * Improve formulation, also mention the security change as a breaking change. Co-authored-by: Milas Bowman Co-authored-by: Maor Kleinberger Co-authored-by: Guy Lichtman Co-authored-by: liubo Co-authored-by: Audun Nes Co-authored-by: Karthikeyan Singaravelan Co-authored-by: Clément Loiselet --- changelogs/fragments/docker-py-changes-1.yml | 11 +++++ plugins/module_utils/_api/api/client.py | 4 +- plugins/module_utils/_api/errors.py | 4 +- plugins/module_utils/_api/tls.py | 10 ++--- .../module_utils/_api/transport/sshconn.py | 7 ++-- plugins/module_utils/_api/utils/build.py | 3 ++ plugins/module_utils/_api/utils/utils.py | 41 +++++++++++-------- .../module_utils/_api/api/test_client.py | 4 +- .../module_utils/_api/utils/test_utils.py | 11 ++++- 9 files changed, 62 insertions(+), 33 deletions(-) create mode 100644 changelogs/fragments/docker-py-changes-1.yml diff --git a/changelogs/fragments/docker-py-changes-1.yml b/changelogs/fragments/docker-py-changes-1.yml new file mode 100644 index 00000000..3f9f2ba8 --- /dev/null +++ b/changelogs/fragments/docker-py-changes-1.yml @@ -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)." diff --git a/plugins/module_utils/_api/api/client.py b/plugins/module_utils/_api/api/client.py index 45ea7497..faf30cda 100644 --- a/plugins/module_utils/_api/api/client.py +++ b/plugins/module_utils/_api/api/client.py @@ -15,7 +15,7 @@ import logging 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 @@ -258,7 +258,7 @@ class APIClient( 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: diff --git a/plugins/module_utils/_api/errors.py b/plugins/module_utils/_api/errors.py index 03948736..90dd5aad 100644 --- a/plugins/module_utils/_api/errors.py +++ b/plugins/module_utils/_api/errors.py @@ -12,6 +12,8 @@ __metaclass__ = type from ._import_helper import HTTPError as _HTTPError +from ansible.module_utils.six import raise_from + class DockerException(Exception): """ @@ -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): diff --git a/plugins/module_utils/_api/tls.py b/plugins/module_utils/_api/tls.py index 33406b85..ed5416d8 100644 --- a/plugins/module_utils/_api/tls.py +++ b/plugins/module_utils/_api/tls.py @@ -12,6 +12,7 @@ __metaclass__ = type import os import ssl +import sys from . import errors from .transport.ssladapter import SSLHTTPAdapter @@ -49,15 +50,10 @@ class TLSConfig(object): 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, @@ -73,6 +69,8 @@ class TLSConfig(object): # 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 diff --git a/plugins/module_utils/_api/transport/sshconn.py b/plugins/module_utils/_api/transport/sshconn.py index 47dc35db..063c2882 100644 --- a/plugins/module_utils/_api/transport/sshconn.py +++ b/plugins/module_utils/_api/transport/sshconn.py @@ -78,9 +78,8 @@ class SSHSocket(socket.socket): 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) @@ -225,7 +224,7 @@ class SSHHTTPAdapter(BaseHTTPAdapter): 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'] @@ -237,7 +236,7 @@ class SSHHTTPAdapter(BaseHTTPAdapter): 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: diff --git a/plugins/module_utils/_api/utils/build.py b/plugins/module_utils/_api/utils/build.py index 9b5d1874..85704f94 100644 --- a/plugins/module_utils/_api/utils/build.py +++ b/plugins/module_utils/_api/utils/build.py @@ -244,6 +244,9 @@ class Pattern(object): @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 diff --git a/plugins/module_utils/_api/utils/utils.py b/plugins/module_utils/_api/utils/utils.py index 6401fbab..910b0dc3 100644 --- a/plugins/module_utils/_api/utils/utils.py +++ b/plugins/module_utils/_api/utils/utils.py @@ -11,6 +11,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type import base64 +import collections import json import os import os.path @@ -22,17 +23,22 @@ from ansible_collections.community.docker.plugins.module_utils.version import St 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): @@ -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 @@ -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': @@ -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): @@ -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'), diff --git a/tests/unit/plugins/module_utils/_api/api/test_client.py b/tests/unit/plugins/module_utils/_api/api/test_client.py index c0a8ed39..ea003565 100644 --- a/tests/unit/plugins/module_utils/_api/api/test_client.py +++ b/tests/unit/plugins/module_utils/_api/api/test_client.py @@ -393,7 +393,7 @@ class UnixSocketStreamTest(unittest.TestCase): 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 @@ -519,7 +519,7 @@ class TCPSocketStreamTest(unittest.TestCase): 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]) diff --git a/tests/unit/plugins/module_utils/_api/utils/test_utils.py b/tests/unit/plugins/module_utils/_api/utils/test_utils.py index 7679ba43..cc0dc695 100644 --- a/tests/unit/plugins/module_utils/_api/utils/test_utils.py +++ b/tests/unit/plugins/module_utils/_api/utils/test_utils.py @@ -285,17 +285,24 @@ class ParseHostTest(unittest.TestCase): '[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'