From c5adc0c126e593d12c9b18bcf60f96336c75e4a8 Mon Sep 17 00:00:00 2001 From: Florian Bergmann Date: Fri, 14 Sep 2018 10:30:39 +0200 Subject: [PATCH] X509 fixes (#111) * Return proper content type for the x509 certificate * Remove parenthesis * Remove extra-variables during the import * Comment fix * Remove double returns * Change log level from trace to debug * Remove 'pass' and add logging instead * Remove unnecessary wrapping Remove wrapping * PEP 8: line too long PEP8: line too long * PEP8: Redefine RSAError variable in except clause * Do not return None if name was not found * Do not return None if no matched minions found * Fix unit tests --- salt/modules/publish.py | 8 +-- salt/modules/x509.py | 132 ++++++++++++++++------------------------ salt/states/x509.py | 22 ++++--- 3 files changed, 69 insertions(+), 93 deletions(-) diff --git a/salt/modules/publish.py b/salt/modules/publish.py index 62e3e98f2f..fda848d1ec 100644 --- a/salt/modules/publish.py +++ b/salt/modules/publish.py @@ -82,10 +82,8 @@ def _publish( in minion configuration but `via_master` was specified.') else: # Find the master in the list of master_uris generated by the minion base class - matching_master_uris = [master for master - in __opts__['master_uri_list'] - if '//{0}:'.format(via_master) - in master] + matching_master_uris = [master for master in __opts__['master_uri_list'] + if '//{0}:'.format(via_master) in master] if not matching_master_uris: raise SaltInvocationError('Could not find match for {0} in \ @@ -178,6 +176,8 @@ def _publish( finally: channel.close() + return {} + def publish(tgt, fun, diff --git a/salt/modules/x509.py b/salt/modules/x509.py index 8689bfad35..4126f34960 100644 --- a/salt/modules/x509.py +++ b/salt/modules/x509.py @@ -38,14 +38,13 @@ from salt.state import STATE_INTERNAL_KEYWORDS as _STATE_INTERNAL_KEYWORDS # Import 3rd Party Libs try: import M2Crypto - HAS_M2 = True except ImportError: - HAS_M2 = False + M2Crypto = None + try: import OpenSSL - HAS_OPENSSL = True except ImportError: - HAS_OPENSSL = False + OpenSSL = None __virtualname__ = 'x509' @@ -83,10 +82,7 @@ def __virtual__(): ''' only load this module if m2crypto is available ''' - if HAS_M2: - return __virtualname__ - else: - return (False, 'Could not load x509 module, m2crypto unavailable') + return __virtualname__ if M2Crypto is not None else False, 'Could not load x509 module, m2crypto unavailable' class _Ctx(ctypes.Structure): @@ -129,10 +125,8 @@ def _new_extension(name, value, critical=0, issuer=None, _pyfree=1): doesn't support getting the publickeyidentifier from the issuer to create the authoritykeyidentifier extension. ''' - if name == 'subjectKeyIdentifier' and \ - value.strip('0123456789abcdefABCDEF:') is not '': - raise salt.exceptions.SaltInvocationError( - 'value must be precomputed hash') + if name == 'subjectKeyIdentifier' and value.strip('0123456789abcdefABCDEF:') is not '': + raise salt.exceptions.SaltInvocationError('value must be precomputed hash') # ensure name and value are bytes name = salt.utils.stringutils.to_str(name) @@ -147,7 +141,7 @@ def _new_extension(name, value, critical=0, issuer=None, _pyfree=1): x509_ext_ptr = M2Crypto.m2.x509v3_ext_conf(None, ctx, name, value) lhash = None except AttributeError: - lhash = M2Crypto.m2.x509v3_lhash() # pylint: disable=no-member + lhash = M2Crypto.m2.x509v3_lhash() # pylint: disable=no-member ctx = M2Crypto.m2.x509v3_set_conf_lhash( lhash) # pylint: disable=no-member # ctx not zeroed @@ -198,10 +192,8 @@ def _get_csr_extensions(csr): csrtempfile.flush() csryaml = _parse_openssl_req(csrtempfile.name) csrtempfile.close() - if csryaml and 'Requested Extensions' in \ - csryaml['Certificate Request']['Data']: - csrexts = \ - csryaml['Certificate Request']['Data']['Requested Extensions'] + if csryaml and 'Requested Extensions' in csryaml['Certificate Request']['Data']: + csrexts = csryaml['Certificate Request']['Data']['Requested Extensions'] if not csrexts: return ret @@ -296,7 +288,7 @@ def _get_signing_policy(name): signing_policy = policies.get(name) if signing_policy: return signing_policy - return __salt__['config.get']('x509_signing_policies', {}).get(name) + return __salt__['config.get']('x509_signing_policies', {}).get(name) or {} def _pretty_hex(hex_str): @@ -335,9 +327,11 @@ def _text_or_file(input_): ''' if _isfile(input_): with salt.utils.files.fopen(input_) as fp_: - return salt.utils.stringutils.to_str(fp_.read()) + out = salt.utils.stringutils.to_str(fp_.read()) else: - return salt.utils.stringutils.to_str(input_) + out = salt.utils.stringutils.to_str(input_) + + return out def _parse_subject(subject): @@ -355,7 +349,7 @@ def _parse_subject(subject): ret[nid_name] = val nids.append(nid_num) except TypeError as err: - log.trace("Missing attribute '%s'. Error: %s", nid_name, err) + log.debug("Missing attribute '%s'. Error: %s", nid_name, err) return ret @@ -533,8 +527,8 @@ def get_pem_entries(glob_path): if os.path.isfile(path): try: ret[path] = get_pem_entry(text=path) - except ValueError: - pass + except ValueError as err: + log.debug('Unable to get PEM entries from %s: %s', path, err) return ret @@ -612,8 +606,8 @@ def read_certificates(glob_path): if os.path.isfile(path): try: ret[path] = read_certificate(certificate=path) - except ValueError: - pass + except ValueError as err: + log.debug('Unable to read certificate %s: %s', path, err) return ret @@ -642,12 +636,10 @@ def read_csr(csr): # Get size returns in bytes. The world thinks of key sizes in bits. 'Subject': _parse_subject(csr.get_subject()), 'Subject Hash': _dec2hex(csr.get_subject().as_hash()), - 'Public Key Hash': hashlib.sha1(csr.get_pubkey().get_modulus())\ - .hexdigest() + 'Public Key Hash': hashlib.sha1(csr.get_pubkey().get_modulus()).hexdigest(), + 'X509v3 Extensions': _get_csr_extensions(csr), } - ret['X509v3 Extensions'] = _get_csr_extensions(csr) - return ret @@ -943,7 +935,7 @@ def create_crl( # pylint: disable=too-many-arguments,too-many-locals # pyOpenSSL Note due to current limitations in pyOpenSSL it is impossible # to specify a digest For signing the CRL. This will hopefully be fixed # soon: https://github.com/pyca/pyopenssl/pull/161 - if not HAS_OPENSSL: + if OpenSSL is None: raise salt.exceptions.SaltInvocationError( 'Could not load OpenSSL module, OpenSSL unavailable' ) @@ -969,8 +961,7 @@ def create_crl( # pylint: disable=too-many-arguments,too-many-locals continue if 'revocation_date' not in rev_item: - rev_item['revocation_date'] = datetime.datetime\ - .now().strftime('%Y-%m-%d %H:%M:%S') + rev_item['revocation_date'] = datetime.datetime.now().strftime('%Y-%m-%d %H:%M:%S') rev_date = datetime.datetime.strptime( rev_item['revocation_date'], '%Y-%m-%d %H:%M:%S') @@ -1011,8 +1002,9 @@ def create_crl( # pylint: disable=too-many-arguments,too-many-locals try: crltext = crl.export(**export_kwargs) except (TypeError, ValueError): - log.warning( - 'Error signing crl with specified digest. Are you using pyopenssl 0.15 or newer? The default md5 digest will be used.') + log.warning('Error signing crl with specified digest. ' + 'Are you using pyopenssl 0.15 or newer? ' + 'The default md5 digest will be used.') export_kwargs.pop('digest', None) crltext = crl.export(**export_kwargs) @@ -1050,8 +1042,7 @@ def sign_remote_certificate(argdic, **kwargs): if 'signing_policy' in argdic: signing_policy = _get_signing_policy(argdic['signing_policy']) if not signing_policy: - return 'Signing policy {0} does not exist.'.format( - argdic['signing_policy']) + return 'Signing policy {0} does not exist.'.format(argdic['signing_policy']) if isinstance(signing_policy, list): dict_ = {} @@ -1091,6 +1082,7 @@ def get_signing_policy(signing_policy_name): signing_policy = _get_signing_policy(signing_policy_name) if not signing_policy: return 'Signing policy {0} does not exist.'.format(signing_policy_name) + if isinstance(signing_policy, list): dict_ = {} for item in signing_policy: @@ -1103,10 +1095,9 @@ def get_signing_policy(signing_policy_name): pass try: - signing_policy['signing_cert'] = get_pem_entry( - signing_policy['signing_cert'], 'CERTIFICATE') + signing_policy['signing_cert'] = get_pem_entry(signing_policy['signing_cert'], 'CERTIFICATE') except KeyError: - pass + log.debug('Unable to get "certificate" PEM entry') return signing_policy @@ -1356,8 +1347,7 @@ def create_certificate( salt '*' x509.create_certificate path=/etc/pki/myca.crt signing_private_key='/etc/pki/myca.key' csr='/etc/pki/myca.csr'} ''' - if not path and not text and \ - ('testrun' not in kwargs or kwargs['testrun'] is False): + if not path and not text and ('testrun' not in kwargs or kwargs['testrun'] is False): raise salt.exceptions.SaltInvocationError( 'Either path or text must be specified.') if path and text: @@ -1386,8 +1376,7 @@ def create_certificate( # Including listen_in and preqreuired because they are not included # in STATE_INTERNAL_KEYWORDS # for salt 2014.7.2 - for ignore in list(_STATE_INTERNAL_KEYWORDS) + \ - ['listen_in', 'preqrequired', '__prerequired__']: + for ignore in list(_STATE_INTERNAL_KEYWORDS) + ['listen_in', 'preqrequired', '__prerequired__']: kwargs.pop(ignore, None) certs = __salt__['publish.publish']( @@ -1500,8 +1489,7 @@ def create_certificate( continue # Use explicitly set values first, fall back to CSR values. - extval = kwargs.get(extname) or kwargs.get(extlongname) or \ - csrexts.get(extname) or csrexts.get(extlongname) + extval = kwargs.get(extname) or kwargs.get(extlongname) or csrexts.get(extname) or csrexts.get(extlongname) critical = False if extval.startswith('critical '): @@ -1623,8 +1611,8 @@ def create_csr(path=None, text=False, **kwargs): if 'private_key' not in kwargs and 'public_key' in kwargs: kwargs['private_key'] = kwargs['public_key'] - log.warning( - "OpenSSL no longer allows working with non-signed CSRs. A private_key must be specified. Attempting to use public_key as private_key") + log.warning("OpenSSL no longer allows working with non-signed CSRs. " + "A private_key must be specified. Attempting to use public_key as private_key") if 'private_key' not in kwargs: raise salt.exceptions.SaltInvocationError('private_key is required') @@ -1636,11 +1624,9 @@ def create_csr(path=None, text=False, **kwargs): kwargs['private_key_passphrase'] = None if 'public_key_passphrase' not in kwargs: kwargs['public_key_passphrase'] = None - if kwargs['public_key_passphrase'] and not kwargs[ - 'private_key_passphrase']: + if kwargs['public_key_passphrase'] and not kwargs['private_key_passphrase']: kwargs['private_key_passphrase'] = kwargs['public_key_passphrase'] - if kwargs['private_key_passphrase'] and not kwargs[ - 'public_key_passphrase']: + if kwargs['private_key_passphrase'] and not kwargs['public_key_passphrase']: kwargs['public_key_passphrase'] = kwargs['private_key_passphrase'] csr.set_pubkey(get_public_key(kwargs['public_key'], @@ -1684,18 +1670,10 @@ def create_csr(path=None, text=False, **kwargs): extstack.push(ext) csr.add_extensions(extstack) - csr.sign(_get_private_key_obj(kwargs['private_key'], passphrase=kwargs['private_key_passphrase']), kwargs['algorithm']) - if path: - return write_pem( - text=csr.as_pem(), - path=path, - pem_type='CERTIFICATE REQUEST' - ) - else: - return csr.as_pem() + return write_pem(text=csr.as_pem(), path=path, pem_type='CERTIFICATE REQUEST') if path else csr.as_pem() def verify_private_key(private_key, public_key, passphrase=None): @@ -1720,8 +1698,7 @@ def verify_private_key(private_key, public_key, passphrase=None): salt '*' x509.verify_private_key private_key=/etc/pki/myca.key \\ public_key=/etc/pki/myca.crt ''' - return bool(get_public_key(private_key, passphrase) - == get_public_key(public_key)) + return get_public_key(private_key, passphrase) == get_public_key(public_key) def verify_signature(certificate, signing_pub_key=None, @@ -1775,9 +1752,8 @@ def verify_crl(crl, cert): salt '*' x509.verify_crl crl=/etc/pki/myca.crl cert=/etc/pki/myca.crt ''' if not salt.utils.path.which('openssl'): - raise salt.exceptions.SaltInvocationError( - 'openssl binary not found in path' - ) + raise salt.exceptions.SaltInvocationError('External command "openssl" not found') + crltext = _text_or_file(crl) crltext = get_pem_entry(crltext, pem_type='X509 CRL') crltempfile = tempfile.NamedTemporaryFile() @@ -1798,10 +1774,7 @@ def verify_crl(crl, cert): crltempfile.close() certtempfile.close() - if 'verify OK' in output: - return True - else: - return False + return 'verify OK' in output def expired(certificate): @@ -1838,8 +1811,9 @@ def expired(certificate): ret['expired'] = True else: ret['expired'] = False - except ValueError: - pass + except ValueError as err: + log.debug('Failed to get data of expired certificate: %s', err) + log.trace(err, exc_info=True) return ret @@ -1862,6 +1836,7 @@ def will_expire(certificate, days): salt '*' x509.will_expire "/etc/pki/mycert.crt" days=30 ''' + ts_pt = "%Y-%m-%d %H:%M:%S" ret = {} if os.path.isfile(certificate): @@ -1871,18 +1846,13 @@ def will_expire(certificate, days): cert = _get_certificate_obj(certificate) - _check_time = datetime.datetime.utcnow() + \ - datetime.timedelta(days=days) + _check_time = datetime.datetime.utcnow() + datetime.timedelta(days=days) _expiration_date = cert.get_not_after().get_datetime() ret['cn'] = _parse_subject(cert.get_subject())['CN'] - - if _expiration_date.strftime("%Y-%m-%d %H:%M:%S") <= \ - _check_time.strftime("%Y-%m-%d %H:%M:%S"): - ret['will_expire'] = True - else: - ret['will_expire'] = False - except ValueError: - pass + ret['will_expire'] = _expiration_date.strftime(ts_pt) <= _check_time.strftime(ts_pt) + except ValueError as err: + log.debug('Unable to return details of a sertificate expiration: %s', err) + log.trace(err, exc_info=True) return ret diff --git a/salt/states/x509.py b/salt/states/x509.py index 209cbc6738..8c79c6d034 100644 --- a/salt/states/x509.py +++ b/salt/states/x509.py @@ -163,6 +163,7 @@ import copy # Import Salt Libs import salt.exceptions +import salt.utils.stringutils # Import 3rd-party libs from salt.ext import six @@ -170,7 +171,7 @@ from salt.ext import six try: from M2Crypto.RSA import RSAError except ImportError: - pass + RSAError = Exception('RSA Error') def __virtual__(): @@ -180,7 +181,7 @@ def __virtual__(): if 'x509.get_pem_entry' in __salt__: return 'x509' else: - return (False, 'Could not load x509 state: m2crypto unavailable') + return False, 'Could not load x509 state: the x509 is not available' def _revoked_to_list(revs): @@ -267,7 +268,8 @@ def private_key_managed(name, new: Always create a new key. Defaults to False. - Combining new with :mod:`prereq `, or when used as part of a `managed_private_key` can allow key rotation whenever a new certificiate is generated. + Combining new with :mod:`prereq `, or when used as part of a + `managed_private_key` can allow key rotation whenever a new certificiate is generated. overwrite: Overwrite an existing private key if the provided passphrase cannot decrypt it. @@ -459,8 +461,10 @@ def certificate_managed(name, private_key_args['name'], pem_type='RSA PRIVATE KEY') else: new_private_key = True - private_key = __salt__['x509.create_private_key'](text=True, bits=private_key_args['bits'], passphrase=private_key_args[ - 'passphrase'], cipher=private_key_args['cipher'], verbose=private_key_args['verbose']) + private_key = __salt__['x509.create_private_key'](text=True, bits=private_key_args['bits'], + passphrase=private_key_args['passphrase'], + cipher=private_key_args['cipher'], + verbose=private_key_args['verbose']) kwargs['public_key'] = private_key @@ -671,8 +675,10 @@ def crl_managed(name, else: current = '{0} does not exist.'.format(name) - new_crl = __salt__['x509.create_crl'](text=True, signing_private_key=signing_private_key, signing_private_key_passphrase=signing_private_key_passphrase, - signing_cert=signing_cert, revoked=revoked, days_valid=days_valid, digest=digest, include_expired=include_expired) + new_crl = __salt__['x509.create_crl'](text=True, signing_private_key=signing_private_key, + signing_private_key_passphrase=signing_private_key_passphrase, + signing_cert=signing_cert, revoked=revoked, days_valid=days_valid, + digest=digest, include_expired=include_expired) new = __salt__['x509.read_crl'](crl=new_crl) new_comp = new.copy() @@ -714,6 +720,6 @@ def pem_managed(name, Any arguments supported by :py:func:`file.managed ` are supported. ''' file_args, kwargs = _get_file_args(name, **kwargs) - file_args['contents'] = __salt__['x509.get_pem_entry'](text=text) + file_args['contents'] = salt.utils.stringutils.to_str(__salt__['x509.get_pem_entry'](text=text)) return __states__['file.managed'](**file_args) -- 2.17.1