Make ssh-host-key-policy configurable
This patch adds configuration options for ssh_hosts_key_file and strict_ssh_host_key_policy. You can set strict_ssh_host_key_policy to 'True' or 'False'. If set to false the first connection of a host will cause it to be added to the known_hosts file. Subsequent connections will be enforced against the existing key. Changes in the key are assumed to be a Man-in-the-Middle attack so the connection is rejected. If strict_ssh_host_key_policy is 'True' the key for the host that is being connected to must be in the hosts_key_file. No first connection assumptions are made. strict_ssh_host_key_policy is set to 'False' to keep functionality similar to the existing functionality. With this patch, a default of $state_path/ssh_known_hosts is used for the known_hosts file. Unlike the previous approach, this now requires the user to have a known_hosts file that is writable, somewhere. The option is configurable if they don't want to use $state_path/ssh_known_hosts DocImpact: Need to document the new strict_ssh_host_key_policy as well as the ssh_hosts_key_file. A note should be made for drivers that may pass a hosts_key_file via kwargs when creating an ssh pool: their file will be loaded along with the file configured via /etc/cinder.conf. Also worth noting, for development environments, an ssh_hosts_key_file of /dev/null and a strict_ssh_host_key_policy setting of 'False' may be used. Using those setting will ignore these changes. Change-Id: Ia7a747224dbb08528f3c7a51da4cd4a6bf0eac1c Implements-blueprint: configurable-ssh-host-key-policy
This commit is contained in:
Jay S. Bryant
parent
33b6c68706
commit
8cd11d8a57
|
|
@ -18,9 +18,11 @@
|
|||
|
||||
"""Utilities related to SSH connection management."""
|
||||
|
||||
import os.path
|
||||
import os
|
||||
import string
|
||||
|
||||
from eventlet import pools
|
||||
from oslo.config import cfg
|
||||
import paramiko
|
||||
|
||||
from cinder import exception
|
||||
|
|
@ -29,6 +31,25 @@ from cinder.openstack.common import log as logging
|
|||
|
||||
LOG = logging.getLogger(__name__)
|
||||
|
||||
ssh_opts = [
|
||||
cfg.BoolOpt('strict_ssh_host_key_policy',
|
||||
default=False,
|
||||
help='Option to enable strict host key checking. When '
|
||||
'set to "True" Cinder will only connect to systems '
|
||||
'with a host key present in the configured '
|
||||
'"ssh_hosts_key_file". When set to "False" the host key '
|
||||
'will be saved upon first connection and used for '
|
||||
'subsequent connections. Default=False'),
|
||||
cfg.StrOpt('ssh_hosts_key_file',
|
||||
default='$state_path/ssh_known_hosts',
|
||||
help='File containing SSH host keys for the systems with which '
|
||||
'Cinder needs to communicate. OPTIONAL: '
|
||||
'Default=$state_path/known_hosts'),
|
||||
]
|
||||
|
||||
CONF = cfg.CONF
|
||||
CONF.register_opts(ssh_opts)
|
||||
|
||||
|
||||
class SSHPool(pools.Pool):
|
||||
"""A simple eventlet pool to hold ssh connections."""
|
||||
|
|
@ -41,24 +62,61 @@ class SSHPool(pools.Pool):
|
|||
self.password = password
|
||||
self.conn_timeout = conn_timeout if conn_timeout else None
|
||||
self.privatekey = privatekey
|
||||
if 'missing_key_policy' in kwargs.keys():
|
||||
self.missing_key_policy = kwargs.pop('missing_key_policy')
|
||||
else:
|
||||
self.missing_key_policy = paramiko.AutoAddPolicy()
|
||||
self.hosts_key_file = None
|
||||
|
||||
# Validate good config setting here.
|
||||
# Paramiko handles the case where the file is inaccessible.
|
||||
if not CONF.ssh_hosts_key_file:
|
||||
raise exception.ParameterNotFound(param='ssh_hosts_key_file')
|
||||
elif not os.path.isfile(CONF.ssh_hosts_key_file):
|
||||
# If using the default path, just create the file.
|
||||
if CONF.state_path in CONF.ssh_hosts_key_file:
|
||||
open(CONF.ssh_hosts_key_file, 'a').close()
|
||||
else:
|
||||
msg = (_("Unable to find ssh_hosts_key_file: %s") %
|
||||
CONF.ssh_hosts_key_file)
|
||||
raise exception.InvalidInput(reason=msg)
|
||||
|
||||
if 'hosts_key_file' in kwargs.keys():
|
||||
self.hosts_key_file = kwargs.pop('hosts_key_file')
|
||||
LOG.info(_("Secondary ssh hosts key file %(kwargs)s will be "
|
||||
"loaded along with %(conf)s from /etc/cinder.conf.") %
|
||||
{'kwargs': self.hosts_key_file,
|
||||
'conf': CONF.ssh_hosts_key_file})
|
||||
|
||||
LOG.debug("Setting strict_ssh_host_key_policy to '%(policy)s' "
|
||||
"using ssh_hosts_key_file '%(key_file)s'." %
|
||||
{'policy': CONF.strict_ssh_host_key_policy,
|
||||
'key_file': CONF.ssh_hosts_key_file})
|
||||
|
||||
self.strict_ssh_host_key_policy = CONF.strict_ssh_host_key_policy
|
||||
|
||||
if not self.hosts_key_file:
|
||||
self.hosts_key_file = CONF.ssh_hosts_key_file
|
||||
else:
|
||||
self.hosts_key_file = None
|
||||
self.hosts_key_file += ',' + CONF.ssh_hosts_key_file
|
||||
|
||||
super(SSHPool, self).__init__(*args, **kwargs)
|
||||
|
||||
def create(self):
|
||||
try:
|
||||
ssh = paramiko.SSHClient()
|
||||
ssh.set_missing_host_key_policy(self.missing_key_policy)
|
||||
if not self.hosts_key_file:
|
||||
ssh.load_system_host_keys()
|
||||
if ',' in self.hosts_key_file:
|
||||
files = string.split(self.hosts_key_file, ',')
|
||||
for f in files:
|
||||
ssh.load_host_keys(f)
|
||||
else:
|
||||
ssh.load_host_keys(self.hosts_key_file)
|
||||
# If strict_ssh_host_key_policy is set we want to reject, by
|
||||
# default if there is not entry in the known_hosts file.
|
||||
# Otherwise we use AutoAddPolicy which accepts on the first
|
||||
# Connect but fails if the keys change. load_host_keys can
|
||||
# handle hashed known_host entries.
|
||||
if self.strict_ssh_host_key_policy:
|
||||
ssh.set_missing_host_key_policy(paramiko.RejectPolicy())
|
||||
else:
|
||||
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
|
||||
|
||||
if self.password:
|
||||
ssh.connect(self.ip,
|
||||
port=self.port,
|
||||
|
|
|
|||
|
|
@ -839,6 +839,9 @@ class FakeSSHClient(object):
|
|||
def get_policy(self):
|
||||
return self.policy
|
||||
|
||||
def get_host_keys(self):
|
||||
return '127.0.0.1 ssh-rsa deadbeef'
|
||||
|
||||
def close(self):
|
||||
pass
|
||||
|
||||
|
|
@ -866,9 +869,35 @@ class FakeTransport(object):
|
|||
|
||||
class SSHPoolTestCase(test.TestCase):
|
||||
"""Unit test for SSH Connection Pool."""
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
def test_ssh_key_policy(self, mock_sshclient):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
@mock.patch('os.path.isfile', return_value=True)
|
||||
def test_ssh_default_hosts_key_file(self, mock_isfile, mock_sshclient,
|
||||
mock_open):
|
||||
mock_ssh = mock.MagicMock()
|
||||
mock_sshclient.return_value = mock_ssh
|
||||
|
||||
# create with customized setting
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
host_key_files = sshpool.hosts_key_file
|
||||
|
||||
self.assertEqual('/var/lib/cinder/ssh_known_hosts', host_key_files)
|
||||
|
||||
mock_ssh.load_host_keys.assert_called_once_with(
|
||||
'/var/lib/cinder/ssh_known_hosts')
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
@mock.patch('os.path.isfile', return_value=True)
|
||||
def test_ssh_host_key_file_kwargs(self, mock_isfile, mock_sshclient,
|
||||
mock_open):
|
||||
mock_ssh = mock.MagicMock()
|
||||
mock_sshclient.return_value = mock_ssh
|
||||
|
||||
# create with customized setting
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
|
|
@ -876,29 +905,29 @@ class SSHPoolTestCase(test.TestCase):
|
|||
password="test",
|
||||
min_size=1,
|
||||
max_size=1,
|
||||
missing_key_policy=paramiko.RejectPolicy(),
|
||||
hosts_key_file='dummy_host_keyfile')
|
||||
with sshpool.item() as ssh:
|
||||
self.assertTrue(isinstance(ssh.get_policy(),
|
||||
paramiko.RejectPolicy))
|
||||
self.assertEqual(ssh.hosts_key_file, 'dummy_host_keyfile')
|
||||
|
||||
# create with default setting
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
with sshpool.item() as ssh:
|
||||
self.assertTrue(isinstance(ssh.get_policy(),
|
||||
paramiko.AutoAddPolicy))
|
||||
self.assertEqual(ssh.system_host_keys, 'system_host_keys')
|
||||
host_key_files = sshpool.hosts_key_file
|
||||
|
||||
self.assertIn('dummy_host_keyfile', host_key_files)
|
||||
self.assertIn('/var/lib/cinder/ssh_known_hosts', host_key_files)
|
||||
|
||||
expected = [
|
||||
mock.call.load_host_keys('dummy_host_keyfile'),
|
||||
mock.call.load_host_keys('/var/lib/cinder/ssh_known_hosts')]
|
||||
|
||||
mock_ssh.assert_has_calls(expected, any_order=True)
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('os.path.isfile', return_value=True)
|
||||
@mock.patch('paramiko.RSAKey.from_private_key_file')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
def test_single_ssh_connect(self, mock_sshclient, mock_pkey):
|
||||
def test_single_ssh_connect(self, mock_sshclient, mock_pkey, mock_isfile,
|
||||
mock_open):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
|
||||
CONF.ssh_hosts_key_file = '/var/lib/cinder/ssh_known_hosts'
|
||||
|
||||
# create with password
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
|
|
@ -930,8 +959,9 @@ class SSHPoolTestCase(test.TestCase):
|
|||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
def test_closed_reopend_ssh_connections(self, mock_sshclient):
|
||||
def test_closed_reopened_ssh_connections(self, mock_sshclient, mock_open):
|
||||
mock_sshclient.return_value = eval('FakeSSHClient')()
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
|
|
@ -956,6 +986,97 @@ class SSHPoolTestCase(test.TestCase):
|
|||
|
||||
self.assertNotEqual(first_id, third_id)
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
def test_missing_ssh_hosts_key_config(self, mock_sshclient, mock_open):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
|
||||
CONF.ssh_hosts_key_file = None
|
||||
# create with password
|
||||
self.assertRaises(exception.ParameterNotFound,
|
||||
ssh_utils.SSHPool,
|
||||
"127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
def test_create_default_known_hosts_file(self, mock_sshclient,
|
||||
mock_open):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
|
||||
CONF.state_path = '/var/lib/cinder'
|
||||
CONF.ssh_hosts_key_file = '/var/lib/cinder/ssh_known_hosts'
|
||||
|
||||
default_file = '/var/lib/cinder/ssh_known_hosts'
|
||||
|
||||
ssh_pool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
with ssh_pool.item() as ssh:
|
||||
mock_open.assert_called_once_with(default_file, 'a')
|
||||
ssh_pool.remove(ssh)
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
def test_ssh_missing_hosts_key_file(self, mock_sshclient, mock_open):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
|
||||
CONF.ssh_hosts_key_file = '/tmp/blah'
|
||||
|
||||
self.assertRaises(exception.InvalidInput,
|
||||
ssh_utils.SSHPool,
|
||||
"127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
@mock.patch('os.path.isfile', return_value=True)
|
||||
def test_ssh_strict_host_key_policy(self, mock_isfile, mock_sshclient,
|
||||
mock_open):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
|
||||
CONF.strict_ssh_host_key_policy = True
|
||||
|
||||
# create with customized setting
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
with sshpool.item() as ssh:
|
||||
self.assertTrue(isinstance(ssh.get_policy(),
|
||||
paramiko.RejectPolicy))
|
||||
|
||||
@mock.patch('__builtin__.open')
|
||||
@mock.patch('paramiko.SSHClient')
|
||||
@mock.patch('os.path.isfile', return_value=True)
|
||||
def test_ssh_not_strict_host_key_policy(self, mock_isfile, mock_sshclient,
|
||||
mock_open):
|
||||
mock_sshclient.return_value = FakeSSHClient()
|
||||
|
||||
CONF.strict_ssh_host_key_policy = False
|
||||
|
||||
# create with customized setting
|
||||
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
|
||||
"test",
|
||||
password="test",
|
||||
min_size=1,
|
||||
max_size=1)
|
||||
|
||||
with sshpool.item() as ssh:
|
||||
self.assertTrue(isinstance(ssh.get_policy(),
|
||||
paramiko.AutoAddPolicy))
|
||||
|
||||
|
||||
class BrickUtils(test.TestCase):
|
||||
"""Unit test to test the brick utility
|
||||
|
|
|
|||
|
|
@ -282,6 +282,24 @@
|
|||
#osapi_volume_workers=<None>
|
||||
|
||||
|
||||
#
|
||||
# Options defined in cinder.ssh_utils
|
||||
#
|
||||
|
||||
# Option to enable strict host key checking. When set to
|
||||
# "True" Cinder will only connect to systems with a host key
|
||||
# present in the configured "ssh_hosts_key_file". When set to
|
||||
# "False" the host key will be saved upon first connection and
|
||||
# used for subsequent connections. Default=False (boolean
|
||||
# value)
|
||||
#strict_ssh_host_key_policy=false
|
||||
|
||||
# File containing SSH host keys for the systems with which
|
||||
# Cinder needs to communicate. OPTIONAL:
|
||||
# Default=$state_path/known_hosts (string value)
|
||||
#ssh_hosts_key_file=$state_path/ssh_known_hosts
|
||||
|
||||
|
||||
#
|
||||
# Options defined in cinder.test
|
||||
#
|
||||
|
|
|
|||
Loading…
Reference in New Issue