Add security-checklist audit

Change-Id: I575fab32eb04efe28522b1005fb392e44c4247b4

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

actions/security-checklist

@@ -0,0 +1 @@
+security_checklist.py

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())

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."""
+    """This audit is included in the specified kinds of audits.
-    def should_run(audit_options):
+
+    :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)."""
+    """This audit should be run after the specified package version (incl).
-    return lambda audit_options=None: cmp_pkgrevno(pkg, pkg_version) >= 0
+
+    :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)."""
+    """This audit should be run before the specified package version (excl).
-    return lambda audit_options=None: not since_package(pkg, pkg_version)()
+
+    :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."""
+    """This audit should be run based on specified config keys.
-    return lambda audit_options: audit_options.get(config_key) is not None
+
+    :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']:

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)

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
+    compatibility. In this case Ubuntu keyserver will be
-    mechanism is insecure and subject to man-in-the-middle attacks
+    queried for a key via HTTPS by its keyid. This method
-    voiding all signature checks using that key.
+    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,
+    :param key: A GPG key in ASCII armor format,
-                  including BEGIN and END markers.
+                  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)
+            log("Writing provided PGP key in the binary format", level=DEBUG)
-            with NamedTemporaryFile() as keyfile:
+            if six.PY3:
-                with open(keyfile.name, 'w') as fd:
+                key_bytes = key.encode('utf-8')
-                    fd.write(key)
+            else:
-                    fd.write("\n")
+                key_bytes = key
-                cmd = ['apt-key', 'add', keyfile.name]
+            key_name = _get_keyid_by_gpg_key(key_bytes)
-                try:
+            key_gpg = _dearmor_gpg_key(key_bytes)
-                    subprocess.check_call(cmd)
+            _write_apt_gpg_keyfile(key_name=key_name, key_material=key_gpg)
-                except subprocess.CalledProcessError:
-                    error = "Error importing PGP key '{}'".format(key)
-                    log(error)
-                    raise GPGKeyError(error)
         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',
+        # as of bionic add-apt-repository uses curl with an HTTPS keyserver URL
-               'hkp://keyserver.ubuntu.com:80', '--recv-keys', key]
+        # to retrieve GPG keys. `apt-key adv` command is deprecated as is
-        try:
+        # apt-key in general as noted in its manpage. See lp:1433761 for more
-            _run_with_retries(cmd)
+        # history. Instead, /etc/apt/trusted.gpg.d is used directly to drop
-        except subprocess.CalledProcessError:
+        # gpg
-            error = "Error importing PGP key '{}'".format(key)
+        key_asc = _get_key_by_keyid(key)
-            log(error)
+        # write the key in GPG format so that apt-key list shows it
-            raise GPGKeyError(error)
+        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'

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."""