From deb60fd1987ee8bf709baf22147f08d7c1f0c503 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Fri, 2 Jan 2015 16:07:38 -0800 Subject: [PATCH 1/4] Make AES key never hit disk on the master In the past we've been doing coordination of the aes key using dropfiles etc. Not only is this significantly more costly (at least 1 stat per reqserver request) it also means that the symmetric key we use to pub/req messages has been on disk. This re-works the aes key to be a multiprocessing.Array() (char array) which is shared amongst the processes. Now the dropfile is just a request for the master to rotate the key, and this means that *all* key rotation on the master will generate the appropriate event (instead of just ones who passed in sock_dir to dropfile()) --- salt/config.py | 2 -- salt/crypt.py | 68 ++++++++++++++--------------------------------- salt/master.py | 71 +++++++++++++++++++++++++------------------------- 3 files changed, 55 insertions(+), 86 deletions(-) diff --git a/salt/config.py b/salt/config.py index 25cf23c4e7..d54935b3d6 100644 --- a/salt/config.py +++ b/salt/config.py @@ -2056,8 +2056,6 @@ def apply_master_config(overrides=None, defaults=None): if len(opts['sock_dir']) > len(opts['cachedir']) + 10: opts['sock_dir'] = os.path.join(opts['cachedir'], '.salt-unix') - opts['aes'] = salt.crypt.Crypticle.generate_key_string() - opts['extension_modules'] = ( opts.get('extension_modules') or os.path.join(opts['cachedir'], 'extmods') diff --git a/salt/crypt.py b/salt/crypt.py index a1eb20b744..a6289ffc25 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -42,59 +42,28 @@ log = logging.getLogger(__name__) def dropfile(cachedir, user=None, sock_dir=None): ''' - Set an AES dropfile to update the publish session key - - A dropfile is checked periodically by master workers to determine - if AES key rotation has occurred. + Set an AES dropfile to request the master update the publish session key ''' - dfnt = os.path.join(cachedir, '.dfnt') dfn = os.path.join(cachedir, '.dfn') - - def ready(): - ''' - Because MWorker._update_aes uses second-precision mtime - to detect changes to the file, we must avoid writing two - versions with the same mtime. - - Note that this only makes rapid updates in serial safe: concurrent - updates could still both pass this check and then write two different - keys with the same mtime. - ''' - try: - stats = os.stat(dfn) - except os.error: - # Not there, go ahead and write it - return True - else: - if stats.st_mtime == time.time(): - # The mtime is the current time, we must - # wait until time has moved on. - return False - else: - return True - - while not ready(): - log.warning('Waiting before writing {0}'.format(dfn)) - time.sleep(1) - - log.info('Rotating AES key') - aes = Crypticle.generate_key_string() + # set a mask (to avoid a race condition on file creation) and store original. mask = os.umask(191) - with salt.utils.fopen(dfnt, 'w+') as fp_: - fp_.write(aes) - if user: - try: - import pwd - uid = pwd.getpwnam(user).pw_uid - os.chown(dfnt, uid, -1) - except (KeyError, ImportError, OSError, IOError): - pass + try: + log.info('Rotating AES key') - shutil.move(dfnt, dfn) - os.umask(mask) + with salt.utils.fopen(dfn, 'w+') as fp_: + fp_.write('') + if user: + try: + import pwd + uid = pwd.getpwnam(user).pw_uid + os.chown(dfn, uid, -1) + except (KeyError, ImportError, OSError, IOError): + pass + finally: + os.umask(mask) # restore original umask if sock_dir: - event = salt.utils.event.SaltEvent('master', sock_dir) - event.fire_event({'rotate_aes_key': True}, tag='key') + # TODO: deprecation warning + pass def gen_keys(keydir, keyname, keysize, user=None): @@ -745,7 +714,8 @@ class Crypticle(object): SIG_SIZE = hashlib.sha256().digest_size def __init__(self, opts, key_string, key_size=192): - self.keys = self.extract_keys(key_string, key_size) + self.key_string = key_string + self.keys = self.extract_keys(self.key_string, key_size) self.key_size = key_size self.serial = salt.payload.Serial(opts) diff --git a/salt/master.py b/salt/master.py index bdfbfc083f..965e9e1755 100644 --- a/salt/master.py +++ b/salt/master.py @@ -7,6 +7,7 @@ involves preparing the three listeners and the workers needed by the master. from __future__ import absolute_import # Import python libs +import ctypes import os import re import time @@ -77,6 +78,7 @@ class SMaster(object): :param dict opts: The salt options dictionary ''' self.opts = opts + self.opts['aes'] = multiprocessing.Array(ctypes.c_char, salt.crypt.Crypticle.generate_key_string()) self.master_key = salt.crypt.MasterKeys(self.opts) self.key = self.__prep_key() self.crypticle = self.__prep_crypticle() @@ -85,7 +87,7 @@ class SMaster(object): ''' Return the crypticle used for AES ''' - return salt.crypt.Crypticle(self.opts, self.opts['aes']) + return salt.crypt.Crypticle(self.opts, self.opts['aes'].value) def __prep_key(self): ''' @@ -225,20 +227,36 @@ class Maintenance(multiprocessing.Process): def handle_key_rotate(self, now): ''' - Rotate the AES key on a schedule + Rotate the AES key rotation ''' + to_rotate = False + dfn = os.path.join(self.opts['cachedir'], '.dfn') + try: + stats = os.stat(dfn) + if stats.st_mode == 0o100400: + to_rotate = True + else: + log.error('Found dropfile with incorrect permissions, ignoring...') + os.remove(dfn) + except os.error: + pass + + if self.opts.get('publish_session'): if now - self.rotate >= self.opts['publish_session']: - salt.crypt.dropfile( - self.opts['cachedir'], - self.opts['user'], - self.opts['sock_dir']) - self.rotate = now - if self.opts.get('ping_on_rotate'): - # Ping all minions to get them to pick up the new key - log.debug('Pinging all connected minions ' - 'due to AES key rotation') - salt.utils.master.ping_all_connected_minions(self.opts) + to_rotate = True + + if to_rotate: + # should be unecessary-- since no one else should be modifying + with self.opts['aes'].get_lock(): + self.opts['aes'].value = salt.crypt.Crypticle.generate_key_string() + self.event.fire_event({'rotate_aes_key': True}, tag='key') + self.rotate = now + if self.opts.get('ping_on_rotate'): + # Ping all minions to get them to pick up the new key + log.debug('Pinging all connected minions ' + 'due to AES key rotation') + salt.utils.master.ping_all_connected_minions(self.opts) def handle_pillargit(self): ''' @@ -808,27 +826,10 @@ class MWorker(multiprocessing.Process): Check to see if a fresh AES key is available and update the components of the worker ''' - dfn = os.path.join(self.opts['cachedir'], '.dfn') - try: - stats = os.stat(dfn) - except os.error: - return - if stats.st_mode != 0o100400: - # Invalid dfn, return - return - if stats.st_mtime > self.k_mtime: - # new key, refresh crypticle - with salt.utils.fopen(dfn) as fp_: - aes = fp_.read() - if len(aes) != 76: - return - log.debug('New master AES key found by pid {0}'.format(os.getpid())) - self.crypticle = salt.crypt.Crypticle(self.opts, aes) + if self.opts['aes'].value != self.crypticle.key_string: + self.crypticle = salt.crypt.Crypticle(self.opts, self.opts['aes'].value) self.clear_funcs.crypticle = self.crypticle - self.clear_funcs.opts['aes'] = aes self.aes_funcs.crypticle = self.crypticle - self.aes_funcs.opts['aes'] = aes - self.k_mtime = stats.st_mtime def run(self): ''' @@ -1832,13 +1833,13 @@ class ClearFuncs(object): if 'token' in load: try: mtoken = self.master_key.key.private_decrypt(load['token'], 4) - aes = '{0}_|-{1}'.format(self.opts['aes'], mtoken) + aes = '{0}_|-{1}'.format(self.opts['aes'].value, mtoken) except Exception: # Token failed to decrypt, send back the salty bacon to # support older minions pass else: - aes = self.opts['aes'] + aes = self.opts['aes'].value ret['aes'] = pub.public_encrypt(aes, 4) else: @@ -1853,8 +1854,8 @@ class ClearFuncs(object): # support older minions pass - aes = self.opts['aes'] - ret['aes'] = pub.public_encrypt(self.opts['aes'], 4) + aes = self.opts['aes'].value + ret['aes'] = pub.public_encrypt(self.opts['aes'].value, 4) # Be aggressive about the signature digest = hashlib.sha256(aes).hexdigest() ret['sig'] = self.master_key.key.private_encrypt(digest, 5) From e37353e50cbe33571951377bd8df83ab6ae86790 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Fri, 2 Jan 2015 16:14:19 -0800 Subject: [PATCH 2/4] Remove sock_dir from crypt.dropfile() --- salt/crypt.py | 5 +---- salt/key.py | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index a6289ffc25..8628de9669 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -40,7 +40,7 @@ from salt.exceptions import ( log = logging.getLogger(__name__) -def dropfile(cachedir, user=None, sock_dir=None): +def dropfile(cachedir, user=None): ''' Set an AES dropfile to request the master update the publish session key ''' @@ -61,9 +61,6 @@ def dropfile(cachedir, user=None, sock_dir=None): pass finally: os.umask(mask) # restore original umask - if sock_dir: - # TODO: deprecation warning - pass def gen_keys(keydir, keyname, keysize, user=None): diff --git a/salt/key.py b/salt/key.py index f0b7579b23..ab79adea1d 100644 --- a/salt/key.py +++ b/salt/key.py @@ -752,7 +752,7 @@ class Key(object): pass self.check_minion_cache(preserve_minions=matches.get('minions', [])) if self.opts.get('rotate_aes_key'): - salt.crypt.dropfile(self.opts['cachedir'], self.opts['user'], self.opts['sock_dir']) + salt.crypt.dropfile(self.opts['cachedir'], self.opts['user']) return ( self.name_match(match) if match is not None else self.dict_match(matches) @@ -774,7 +774,7 @@ class Key(object): pass self.check_minion_cache() if self.opts.get('rotate_aes_key'): - salt.crypt.dropfile(self.opts['cachedir'], self.opts['user'], self.opts['sock_dir']) + salt.crypt.dropfile(self.opts['cachedir'], self.opts['user']) return self.list_keys() def reject(self, match=None, match_dict=None, include_accepted=False): @@ -812,7 +812,7 @@ class Key(object): pass self.check_minion_cache() if self.opts.get('rotate_aes_key'): - salt.crypt.dropfile(self.opts['cachedir'], self.opts['user'], self.opts['sock_dir']) + salt.crypt.dropfile(self.opts['cachedir'], self.opts['user']) return ( self.name_match(match) if match is not None else self.dict_match(matches) @@ -843,7 +843,7 @@ class Key(object): pass self.check_minion_cache() if self.opts.get('rotate_aes_key'): - salt.crypt.dropfile(self.opts['cachedir'], self.opts['user'], self.opts['sock_dir']) + salt.crypt.dropfile(self.opts['cachedir'], self.opts['user']) return self.list_keys() def finger(self, match): From d226ebbf4361c8288fbcd9ac139e53b6b21d1c3a Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Mon, 5 Jan 2015 15:46:50 -0800 Subject: [PATCH 3/4] Pylint cleanup --- salt/crypt.py | 1 - salt/master.py | 1 - 2 files changed, 2 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index 8628de9669..64c966d71b 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -12,7 +12,6 @@ import os import sys import time import hmac -import shutil import hashlib import logging import traceback diff --git a/salt/master.py b/salt/master.py index 965e9e1755..ab5a1bd8bf 100644 --- a/salt/master.py +++ b/salt/master.py @@ -241,7 +241,6 @@ class Maintenance(multiprocessing.Process): except os.error: pass - if self.opts.get('publish_session'): if now - self.rotate >= self.opts['publish_session']: to_rotate = True From 687012b6c564f4917926e9f1ceac106d3b7b9142 Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Mon, 5 Jan 2015 17:21:54 -0800 Subject: [PATCH 4/4] Pop "aes" from the opts before passing to ext_pillar modules We should probably not put "aes" in the global opts dict, but rather as a class attribute of auth/crypticle or something --- salt/pillar/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 6df4e2504c..98a3895d45 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -134,6 +134,9 @@ class Pillar(object): # location of file_roots. Issue 5951 ext_pillar_opts = dict(self.opts) ext_pillar_opts['file_roots'] = self.actual_file_roots + # TODO: consolidate into "sanitize opts" + if 'aes' in ext_pillar_opts: + ext_pillar_opts.pop('aes') self.merge_strategy = 'smart' if opts.get('pillar_source_merging_strategy'): self.merge_strategy = opts['pillar_source_merging_strategy'] @@ -591,6 +594,7 @@ class Pillar(object): errors.extend(terrors) if self.opts.get('pillar_opts', True): mopts = dict(self.opts) + # TODO: consolidate into sanitize function if 'grains' in mopts: mopts.pop('grains') if 'aes' in mopts: