From d97244b09ae2dbcc60b8e170667bfdc81d94a860 Mon Sep 17 00:00:00 2001 From: Chris MacNaughton Date: Wed, 27 Feb 2019 15:44:52 +0100 Subject: [PATCH] Add security-checklist audit Change-Id: I575fab32eb04efe28522b1005fb392e44c4247b4 --- actions.yaml | 2 + actions/security-checklist | 1 + actions/security_checklist.py | 67 +++++++ .../contrib/openstack/audits/__init__.py | 102 ++++++++-- hooks/charmhelpers/core/hookenv.py | 74 +++++++ hooks/charmhelpers/fetch/ubuntu.py | 185 ++++++++++++++---- tests/basic_deployment.py | 11 ++ 7 files changed, 394 insertions(+), 48 deletions(-) create mode 120000 actions/security-checklist create mode 100755 actions/security_checklist.py diff --git a/actions.yaml b/actions.yaml index c04d8e0f..892bd15a 100644 --- a/actions.yaml +++ b/actions.yaml @@ -11,3 +11,5 @@ resume: Resume neutron-api services. If the neutron-api deployment is clustered using the hacluster charm, the corresponding hacluster unit on the node must be resumed as well. +security-checklist: + description: Validate the running configuration against the OpenStack security guides checklist diff --git a/actions/security-checklist b/actions/security-checklist new file mode 120000 index 00000000..47464970 --- /dev/null +++ b/actions/security-checklist @@ -0,0 +1 @@ +security_checklist.py \ No newline at end of file diff --git a/actions/security_checklist.py b/actions/security_checklist.py new file mode 100755 index 00000000..0e955bd4 --- /dev/null +++ b/actions/security_checklist.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +# +# Copyright 2019 Canonical Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import configparser +import sys + +sys.path.append('hooks') + +import charmhelpers.contrib.openstack.audits as audits +from charmhelpers.contrib.openstack.audits import ( + openstack_security_guide, +) + + +# Via the openstack_security_guide above, we are running the following +# security assertions automatically: +# +# - Check-Neutron-01 - validate-file-ownership +# - Check-Neutron-02 - validate-file-permissions +# - Check-Neutron-03 - validate-uses-keystone +# - Check-Neutron-04 - validate-uses-tls-for-keystone + +@audits.audit(audits.is_audit_type(audits.AuditType.OpenStackSecurityGuide)) +def validate_enables_tls(audit_options): + """Verify that TLS is enabled on Neutron. + + Security Guide Check Name: Check-Neutron-05 + + :param audit_options: Dictionary of options for audit configuration + :type audit_options: Dict + :raises: AssertionError if the assertion fails. + """ + section = audit_options['neutron_config']['DEFAULT'] + assert section.get('use_ssl') == "True", \ + "SSL should be enabled on neutron-api" + + +def main(): + config = { + 'config_path': '/etc/neutron', + 'config_file': 'neutron.conf', + 'audit_type': audits.AuditType.OpenStackSecurityGuide, + 'files': openstack_security_guide.FILE_ASSERTIONS['neutron-api'], + 'excludes': [ + 'validate-uses-tls-for-glance', + ], + } + conf = configparser.ConfigParser() + conf.read("/etc/neutron/neutron.conf") + config['neutron_config'] = dict(conf) + return audits.action_parse_results(audits.run(config)) + +if __name__ == "__main__": + sys.exit(main()) diff --git a/hooks/charmhelpers/contrib/openstack/audits/__init__.py b/hooks/charmhelpers/contrib/openstack/audits/__init__.py index 9fde7b26..7f7e5f79 100644 --- a/hooks/charmhelpers/contrib/openstack/audits/__init__.py +++ b/hooks/charmhelpers/contrib/openstack/audits/__init__.py @@ -19,7 +19,7 @@ from enum import Enum import traceback from charmhelpers.core.host import cmp_pkgrevno - +import charmhelpers.contrib.openstack.utils as openstack_utils import charmhelpers.core.hookenv as hookenv @@ -39,7 +39,7 @@ def audit(*args): deployed system that matches the given configuration :param args: List of functions to filter tests against - :type args: List[Callable(Config)] + :type args: List[Callable[Dict]] """ def wrapper(f): test_name = f.__name__ @@ -58,28 +58,92 @@ def audit(*args): def is_audit_type(*args): - """This audit is included in the specified kinds of audits.""" - def should_run(audit_options): + """This audit is included in the specified kinds of audits. + + :param *args: List of AuditTypes to include this audit in + :type args: List[AuditType] + :rtype: Callable[Dict] + """ + def _is_audit_type(audit_options): if audit_options.get('audit_type') in args: return True else: return False - return should_run + return _is_audit_type def since_package(pkg, pkg_version): - """This audit should be run after the specified package version (incl).""" - return lambda audit_options=None: cmp_pkgrevno(pkg, pkg_version) >= 0 + """This audit should be run after the specified package version (incl). + + :param pkg: Package name to compare + :type pkg: str + :param release: The package version + :type release: str + :rtype: Callable[Dict] + """ + def _since_package(audit_options=None): + return cmp_pkgrevno(pkg, pkg_version) >= 0 + + return _since_package def before_package(pkg, pkg_version): - """This audit should be run before the specified package version (excl).""" - return lambda audit_options=None: not since_package(pkg, pkg_version)() + """This audit should be run before the specified package version (excl). + + :param pkg: Package name to compare + :type pkg: str + :param release: The package version + :type release: str + :rtype: Callable[Dict] + """ + def _before_package(audit_options=None): + return not since_package(pkg, pkg_version)() + + return _before_package + + +def since_openstack_release(pkg, release): + """This audit should run after the specified OpenStack version (incl). + + :param pkg: Package name to compare + :type pkg: str + :param release: The OpenStack release codename + :type release: str + :rtype: Callable[Dict] + """ + def _since_openstack_release(audit_options=None): + _release = openstack_utils.get_os_codename_package(pkg) + return openstack_utils.CompareOpenStackReleases(_release) >= release + + return _since_openstack_release + + +def before_openstack_release(pkg, release): + """This audit should run before the specified OpenStack version (excl). + + :param pkg: Package name to compare + :type pkg: str + :param release: The OpenStack release codename + :type release: str + :rtype: Callable[Dict] + """ + def _before_openstack_release(audit_options=None): + return not since_openstack_release(pkg, release)() + + return _before_openstack_release def it_has_config(config_key): - """This audit should be run based on specified config keys.""" - return lambda audit_options: audit_options.get(config_key) is not None + """This audit should be run based on specified config keys. + + :param config_key: Config key to look for + :type config_key: str + :rtype: Callable[Dict] + """ + def _it_has_config(audit_options): + return audit_options.get(config_key) is not None + + return _it_has_config def run(audit_options): @@ -87,11 +151,19 @@ def run(audit_options): :param audit_options: Configuration for the audit :type audit_options: Config + + :rtype: Dict[str, str] """ errors = {} results = {} for name, audit in sorted(_audits.items()): result_name = name.replace('_', '-') + if result_name in audit_options.get('excludes', []): + print( + "Skipping {} because it is" + "excluded in audit config" + .format(result_name)) + continue if all(p(audit_options) for p in audit.filters): try: audit.func(audit_options) @@ -121,7 +193,13 @@ def run(audit_options): def action_parse_results(result): - """Parse the result of `run` in the context of an action.""" + """Parse the result of `run` in the context of an action. + + :param result: The result of running the security-checklist + action on a unit + :type result: Dict[str, Dict[str, str]] + :rtype: int + """ passed = True for test, result in result.items(): if result['success']: diff --git a/hooks/charmhelpers/core/hookenv.py b/hooks/charmhelpers/core/hookenv.py index 2e287659..4744eb43 100644 --- a/hooks/charmhelpers/core/hookenv.py +++ b/hooks/charmhelpers/core/hookenv.py @@ -50,6 +50,11 @@ TRACE = "TRACE" MARKER = object() SH_MAX_ARG = 131071 + +RANGE_WARNING = ('Passing NO_PROXY string that includes a cidr. ' + 'This may not be compatible with software you are ' + 'running in your shell.') + cache = {} @@ -1414,3 +1419,72 @@ def unit_doomed(unit=None): # I don't think 'dead' units ever show up in the goal-state, but # check anyway in addition to 'dying'. return units[unit]['status'] in ('dying', 'dead') + + +def env_proxy_settings(selected_settings=None): + """Get proxy settings from process environment variables. + + Get charm proxy settings from environment variables that correspond to + juju-http-proxy, juju-https-proxy and juju-no-proxy (available as of 2.4.2, + see lp:1782236) in a format suitable for passing to an application that + reacts to proxy settings passed as environment variables. Some applications + support lowercase or uppercase notation (e.g. curl), some support only + lowercase (e.g. wget), there are also subjectively rare cases of only + uppercase notation support. no_proxy CIDR and wildcard support also varies + between runtimes and applications as there is no enforced standard. + + Some applications may connect to multiple destinations and expose config + options that would affect only proxy settings for a specific destination + these should be handled in charms in an application-specific manner. + + :param selected_settings: format only a subset of possible settings + :type selected_settings: list + :rtype: Option(None, dict[str, str]) + """ + SUPPORTED_SETTINGS = { + 'http': 'HTTP_PROXY', + 'https': 'HTTPS_PROXY', + 'no_proxy': 'NO_PROXY', + 'ftp': 'FTP_PROXY' + } + if selected_settings is None: + selected_settings = SUPPORTED_SETTINGS + + selected_vars = [v for k, v in SUPPORTED_SETTINGS.items() + if k in selected_settings] + proxy_settings = {} + for var in selected_vars: + var_val = os.getenv(var) + if var_val: + proxy_settings[var] = var_val + proxy_settings[var.lower()] = var_val + # Now handle juju-prefixed environment variables. The legacy vs new + # environment variable usage is mutually exclusive + charm_var_val = os.getenv('JUJU_CHARM_{}'.format(var)) + if charm_var_val: + proxy_settings[var] = charm_var_val + proxy_settings[var.lower()] = charm_var_val + if 'no_proxy' in proxy_settings: + if _contains_range(proxy_settings['no_proxy']): + log(RANGE_WARNING, level=WARNING) + return proxy_settings if proxy_settings else None + + +def _contains_range(addresses): + """Check for cidr or wildcard domain in a string. + + Given a string comprising a comma seperated list of ip addresses + and domain names, determine whether the string contains IP ranges + or wildcard domains. + + :param addresses: comma seperated list of domains and ip addresses. + :type addresses: str + """ + return ( + # Test for cidr (e.g. 10.20.20.0/24) + "/" in addresses or + # Test for wildcard domains (*.foo.com or .foo.com) + "*" in addresses or + addresses.startswith(".") or + ",." in addresses or + " ." in addresses) diff --git a/hooks/charmhelpers/fetch/ubuntu.py b/hooks/charmhelpers/fetch/ubuntu.py index 8a5cadf1..2394caf3 100644 --- a/hooks/charmhelpers/fetch/ubuntu.py +++ b/hooks/charmhelpers/fetch/ubuntu.py @@ -19,15 +19,16 @@ import re import six import time import subprocess -from tempfile import NamedTemporaryFile from charmhelpers.core.host import ( - lsb_release + get_distrib_codename, + CompareHostReleases, ) from charmhelpers.core.hookenv import ( log, DEBUG, WARNING, + env_proxy_settings, ) from charmhelpers.fetch import SourceConfigError, GPGKeyError @@ -303,12 +304,17 @@ def import_key(key): """Import an ASCII Armor key. A Radix64 format keyid is also supported for backwards - compatibility, but should never be used; the key retrieval - mechanism is insecure and subject to man-in-the-middle attacks - voiding all signature checks using that key. + compatibility. In this case Ubuntu keyserver will be + queried for a key via HTTPS by its keyid. This method + is less preferrable because https proxy servers may + require traffic decryption which is equivalent to a + man-in-the-middle attack (a proxy server impersonates + keyserver TLS certificates and has to be explicitly + trusted by the system). - :param keyid: The key in ASCII armor format, - including BEGIN and END markers. + :param key: A GPG key in ASCII armor format, + including BEGIN and END markers or a keyid. + :type key: (bytes, str) :raises: GPGKeyError if the key could not be imported """ key = key.strip() @@ -319,35 +325,137 @@ def import_key(key): log("PGP key found (looks like ASCII Armor format)", level=DEBUG) if ('-----BEGIN PGP PUBLIC KEY BLOCK-----' in key and '-----END PGP PUBLIC KEY BLOCK-----' in key): - log("Importing ASCII Armor PGP key", level=DEBUG) - with NamedTemporaryFile() as keyfile: - with open(keyfile.name, 'w') as fd: - fd.write(key) - fd.write("\n") - cmd = ['apt-key', 'add', keyfile.name] - try: - subprocess.check_call(cmd) - except subprocess.CalledProcessError: - error = "Error importing PGP key '{}'".format(key) - log(error) - raise GPGKeyError(error) + log("Writing provided PGP key in the binary format", level=DEBUG) + if six.PY3: + key_bytes = key.encode('utf-8') + else: + key_bytes = key + key_name = _get_keyid_by_gpg_key(key_bytes) + key_gpg = _dearmor_gpg_key(key_bytes) + _write_apt_gpg_keyfile(key_name=key_name, key_material=key_gpg) else: raise GPGKeyError("ASCII armor markers missing from GPG key") else: - # We should only send things obviously not a keyid offsite - # via this unsecured protocol, as it may be a secret or part - # of one. log("PGP key found (looks like Radix64 format)", level=WARNING) - log("INSECURLY importing PGP key from keyserver; " + log("SECURELY importing PGP key from keyserver; " "full key not provided.", level=WARNING) - cmd = ['apt-key', 'adv', '--keyserver', - 'hkp://keyserver.ubuntu.com:80', '--recv-keys', key] - try: - _run_with_retries(cmd) - except subprocess.CalledProcessError: - error = "Error importing PGP key '{}'".format(key) - log(error) - raise GPGKeyError(error) + # as of bionic add-apt-repository uses curl with an HTTPS keyserver URL + # to retrieve GPG keys. `apt-key adv` command is deprecated as is + # apt-key in general as noted in its manpage. See lp:1433761 for more + # history. Instead, /etc/apt/trusted.gpg.d is used directly to drop + # gpg + key_asc = _get_key_by_keyid(key) + # write the key in GPG format so that apt-key list shows it + key_gpg = _dearmor_gpg_key(key_asc) + _write_apt_gpg_keyfile(key_name=key, key_material=key_gpg) + + +def _get_keyid_by_gpg_key(key_material): + """Get a GPG key fingerprint by GPG key material. + Gets a GPG key fingerprint (40-digit, 160-bit) by the ASCII armor-encoded + or binary GPG key material. Can be used, for example, to generate file + names for keys passed via charm options. + + :param key_material: ASCII armor-encoded or binary GPG key material + :type key_material: bytes + :raises: GPGKeyError if invalid key material has been provided + :returns: A GPG key fingerprint + :rtype: str + """ + # trusty, xenial and bionic handling differs due to gpg 1.x to 2.x change + release = get_distrib_codename() + is_gpgv2_distro = CompareHostReleases(release) >= "bionic" + if is_gpgv2_distro: + # --import is mandatory, otherwise fingerprint is not printed + cmd = 'gpg --with-colons --import-options show-only --import --dry-run' + else: + cmd = 'gpg --with-colons --with-fingerprint' + ps = subprocess.Popen(cmd.split(), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.PIPE) + out, err = ps.communicate(input=key_material) + if six.PY3: + out = out.decode('utf-8') + err = err.decode('utf-8') + if 'gpg: no valid OpenPGP data found.' in err: + raise GPGKeyError('Invalid GPG key material provided') + # from gnupg2 docs: fpr :: Fingerprint (fingerprint is in field 10) + return re.search(r"^fpr:{9}([0-9A-F]{40}):$", out, re.MULTILINE).group(1) + + +def _get_key_by_keyid(keyid): + """Get a key via HTTPS from the Ubuntu keyserver. + Different key ID formats are supported by SKS keyservers (the longer ones + are more secure, see "dead beef attack" and https://evil32.com/). Since + HTTPS is used, if SSLBump-like HTTPS proxies are in place, they will + impersonate keyserver.ubuntu.com and generate a certificate with + keyserver.ubuntu.com in the CN field or in SubjAltName fields of a + certificate. If such proxy behavior is expected it is necessary to add the + CA certificate chain containing the intermediate CA of the SSLBump proxy to + every machine that this code runs on via ca-certs cloud-init directive (via + cloudinit-userdata model-config) or via other means (such as through a + custom charm option). Also note that DNS resolution for the hostname in a + URL is done at a proxy server - not at the client side. + + 8-digit (32 bit) key ID + https://keyserver.ubuntu.com/pks/lookup?search=0x4652B4E6 + 16-digit (64 bit) key ID + https://keyserver.ubuntu.com/pks/lookup?search=0x6E85A86E4652B4E6 + 40-digit key ID: + https://keyserver.ubuntu.com/pks/lookup?search=0x35F77D63B5CEC106C577ED856E85A86E4652B4E6 + + :param keyid: An 8, 16 or 40 hex digit keyid to find a key for + :type keyid: (bytes, str) + :returns: A key material for the specified GPG key id + :rtype: (str, bytes) + :raises: subprocess.CalledProcessError + """ + # options=mr - machine-readable output (disables html wrappers) + keyserver_url = ('https://keyserver.ubuntu.com' + '/pks/lookup?op=get&options=mr&exact=on&search=0x{}') + curl_cmd = ['curl', keyserver_url.format(keyid)] + # use proxy server settings in order to retrieve the key + return subprocess.check_output(curl_cmd, + env=env_proxy_settings(['https'])) + + +def _dearmor_gpg_key(key_asc): + """Converts a GPG key in the ASCII armor format to the binary format. + + :param key_asc: A GPG key in ASCII armor format. + :type key_asc: (str, bytes) + :returns: A GPG key in binary format + :rtype: (str, bytes) + :raises: GPGKeyError + """ + ps = subprocess.Popen(['gpg', '--dearmor'], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.PIPE) + out, err = ps.communicate(input=key_asc) + # no need to decode output as it is binary (invalid utf-8), only error + if six.PY3: + err = err.decode('utf-8') + if 'gpg: no valid OpenPGP data found.' in err: + raise GPGKeyError('Invalid GPG key material. Check your network setup' + ' (MTU, routing, DNS) and/or proxy server settings' + ' as well as destination keyserver status.') + else: + return out + + +def _write_apt_gpg_keyfile(key_name, key_material): + """Writes GPG key material into a file at a provided path. + + :param key_name: A key name to use for a key file (could be a fingerprint) + :type key_name: str + :param key_material: A GPG key material (binary) + :type key_material: (str, bytes) + """ + with open('/etc/apt/trusted.gpg.d/{}.gpg'.format(key_name), + 'wb') as keyf: + keyf.write(key_material) def add_source(source, key=None, fail_invalid=False): @@ -442,13 +550,13 @@ def add_source(source, key=None, fail_invalid=False): def _add_proposed(): """Add the PROPOSED_POCKET as /etc/apt/source.list.d/proposed.list - Uses lsb_release()['DISTRIB_CODENAME'] to determine the correct staza for + Uses get_distrib_codename to determine the correct stanza for the deb line. For intel architecutres PROPOSED_POCKET is used for the release, but for other architectures PROPOSED_PORTS_POCKET is used for the release. """ - release = lsb_release()['DISTRIB_CODENAME'] + release = get_distrib_codename() arch = platform.machine() if arch not in six.iterkeys(ARCH_TO_PROPOSED_POCKET): raise SourceConfigError("Arch {} not supported for (distro-)proposed" @@ -461,11 +569,16 @@ def _add_apt_repository(spec): """Add the spec using add_apt_repository :param spec: the parameter to pass to add_apt_repository + :type spec: str """ if '{series}' in spec: - series = lsb_release()['DISTRIB_CODENAME'] + series = get_distrib_codename() spec = spec.replace('{series}', series) - _run_with_retries(['add-apt-repository', '--yes', spec]) + # software-properties package for bionic properly reacts to proxy settings + # passed as environment variables (See lp:1433761). This is not the case + # LTS and non-LTS releases below bionic. + _run_with_retries(['add-apt-repository', '--yes', spec], + cmd_env=env_proxy_settings(['https'])) def _add_cloud_pocket(pocket): @@ -534,7 +647,7 @@ def _verify_is_ubuntu_rel(release, os_release): :raises: SourceConfigError if the release is not the same as the ubuntu release. """ - ubuntu_rel = lsb_release()['DISTRIB_CODENAME'] + ubuntu_rel = get_distrib_codename() if release != ubuntu_rel: raise SourceConfigError( 'Invalid Cloud Archive release specified: {}-{} on this Ubuntu' diff --git a/tests/basic_deployment.py b/tests/basic_deployment.py index 7b88a692..2f691784 100644 --- a/tests/basic_deployment.py +++ b/tests/basic_deployment.py @@ -545,6 +545,17 @@ class NeutronAPIBasicDeployment(OpenStackAmuletDeployment): self.d.configure('neutron-api', set_default) u.log.debug('OK') + def test_501_security_checklist_action(self): + """Verify expected result on a default install""" + u.log.debug("Testing security-checklist") + sentry_unit = self.neutron_api_sentry + + action_id = u.run_action(sentry_unit, "security-checklist") + u.wait_on_action(action_id) + data = amulet.actions.get_action_output(action_id, full_output=True) + assert data.get(u"status") == "failed", \ + "Security check is expected to not pass by default" + def test_900_restart_on_config_change(self): """Verify that the specified services are restarted when the config is changed."""