On Fri, 3 Jul 2015 at 11:26 Petr Pudlak <[email protected]> wrote: > On Wed, Jul 01, 2015 at 03:02:00PM +0000, 'Helga Velroyen' via > ganeti-devel wrote: > >Hi, > > > >I forgot the tests, so please consider this interdiff: > >diff --git a/test/py/ganeti.ssh_unittest.py b/test/py/ > ganeti.ssh_unittest.py > >index b520a97..a121d42 100755 > >--- a/test/py/ganeti.ssh_unittest.py > >+++ b/test/py/ganeti.ssh_unittest.py > >@@ -147,6 +147,9 @@ class TestGetUserFiles(unittest.TestCase): > > constants.SSHK_DSA: > > (os.path.join(self.tmpdir, ".ssh", "id_dsa"), > > os.path.join(self.tmpdir, ".ssh", "id_dsa.pub")), > >+ constants.SSHK_ECDSA: > >+ (os.path.join(self.tmpdir, ".ssh", "id_ecdsa"), > >+ os.path.join(self.tmpdir, ".ssh", "id_ecdsa.pub")), > > })) > > self.assertEqual(os.listdir(self.tmpdir), []) > > > >diff --git a/test/py/ganeti.tools.prepare_node_join_unittest.py > b/test/py/ > >ganeti.tools.prepare_node_join_unittest.py > >index e0c60a4..409b7bb 100755 > >--- a/test/py/ganeti.tools.prepare_node_join_unittest.py > >+++ b/test/py/ganeti.tools.prepare_node_join_unittest.py > >@@ -143,6 +143,9 @@ class TestUpdateSshDaemon(unittest.TestCase): > > constants.SSHK_DSA: > > (utils.PathJoin(self.tmpdir, "dsa.private"), > > utils.PathJoin(self.tmpdir, "dsa.public")), > >+ constants.SSHK_ECDSA: > >+ (utils.PathJoin(self.tmpdir, "ecdsa.private"), > >+ utils.PathJoin(self.tmpdir, "ecdsa.public")), > > } > > > > def tearDown(self): > > Would it make sense to also add some more tests to the file, like a new > method 'testDryRunEcdsa' and to extend _TestUpdate? >
Indeed, good point. I extended the tests and pasted an additional interdiff below. > > Rest (with the interdiff) LGTM, thanks > PS: Some interesting reading about ECDSA: > http://security.stackexchange.com/a/46781/12485 Yeah, I found that too. I guess we should still support ECDSA as it is default in some distros. :/ > > > > > > > > > >On Wed, 1 Jul 2015 at 14:26 Helga Velroyen <[email protected]> wrote: > > > >> So far, Ganeti did only care about DSA and RSA host > >> keys. With the rising popularity of ECDSA, we should > >> support this key type as well, as it is already > >> enabled by default in many common distributions. > >> > >> This fixes Issue 1098. > >> > >> Signed-off-by: Helga Velroyen <[email protected]> > >> --- > >> lib/ssh.py | 2 ++ > >> src/Ganeti/Constants.hs | 12 +++++++++++- > >> 2 files changed, 13 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/ssh.py b/lib/ssh.py > >> index 599b7cc..06003a5 100644 > >> --- a/lib/ssh.py > >> +++ b/lib/ssh.py > >> @@ -79,6 +79,8 @@ def GetUserFiles(user, mkdir=False, dircheck=True, > >> kind=constants.SSHK_DSA, > >> suffix = "dsa" > >> elif kind == constants.SSHK_RSA: > >> suffix = "rsa" > >> + elif kind == constants.SSHK_ECDSA: > >> + suffix = "ecdsa" > >> else: > >> raise errors.ProgrammerError("Unknown SSH key kind '%s'" % kind) > >> > >> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > >> index 97093df..106e4d4 100644 > >> --- a/src/Ganeti/Constants.hs > >> +++ b/src/Ganeti/Constants.hs > >> @@ -4399,11 +4399,14 @@ cryptoOptionSerialNo = "serial_no" > >> sshkDsa :: String > >> sshkDsa = "dsa" > >> > >> +sshkEcdsa :: String > >> +sshkEcdsa = "ecdsa" > >> + > >> sshkRsa :: String > >> sshkRsa = "rsa" > >> > >> sshkAll :: FrozenSet String > >> -sshkAll = ConstantUtils.mkSet [sshkRsa, sshkDsa] > >> +sshkAll = ConstantUtils.mkSet [sshkRsa, sshkDsa, sshkEcdsa] > >> > >> -- * SSH authorized key types > >> > >> @@ -4438,6 +4441,12 @@ sshHostDsaPriv = sshConfigDir ++ > "/ssh_host_dsa_key" > >> sshHostDsaPub :: String > >> sshHostDsaPub = sshHostDsaPriv ++ ".pub" > >> > >> +sshHostEcdsaPriv :: String > >> +sshHostEcdsaPriv = sshConfigDir ++ "/ssh_host_ecdsa_key" > >> + > >> +sshHostEcdsaPub :: String > >> +sshHostEcdsaPub = sshHostEcdsaPriv ++ ".pub" > >> + > >> sshHostRsaPriv :: String > >> sshHostRsaPriv = sshConfigDir ++ "/ssh_host_rsa_key" > >> > >> @@ -4448,6 +4457,7 @@ sshDaemonKeyfiles :: Map String (String, String) > >> sshDaemonKeyfiles = > >> Map.fromList [ (sshkRsa, (sshHostRsaPriv, sshHostRsaPub)) > >> , (sshkDsa, (sshHostDsaPriv, sshHostDsaPub)) > >> + , (sshkEcdsa, (sshHostEcdsaPriv, sshHostEcdsaPub)) > >> ] > >> > >> -- * Node daemon setup > >> -- > >> 2.4.3.573.g4eafbef > >> > >> > The interdiff: diff --git a/test/py/ganeti.tools.prepare_node_join_unittest.py b/test/py/ ganeti.tools.prepare_node_join_unittest.py index 409b7bb..09f8750 100755 --- a/test/py/ganeti.tools.prepare_node_join_unittest.py +++ b/test/py/ganeti.tools.prepare_node_join_unittest.py @@ -183,6 +183,13 @@ class TestUpdateSshDaemon(unittest.TestCase): ], }) + def testDryRunEcdsa(self): + self._TestDryRun({ + constants.SSHS_SSH_HOST_KEY: [ + (constants.SSHK_ECDSA, "ecdsapriv", "ecdsapub"), + ], + }) + def _RunCmd(self, fail, cmd, interactive=NotImplemented): self.assertTrue(interactive) self.assertEqual(cmd, [pathutils.DAEMON_UTIL, "reload-ssh-keys"]) @@ -198,6 +205,7 @@ class TestUpdateSshDaemon(unittest.TestCase): data = { constants.SSHS_SSH_HOST_KEY: [ (constants.SSHK_DSA, "dsapriv", "dsapub"), + (constants.SSHK_ECDSA, "ecdsapriv", "ecdsapub"), (constants.SSHK_RSA, "rsapriv", "rsapub"), ], } @@ -212,6 +220,7 @@ class TestUpdateSshDaemon(unittest.TestCase): self.assertEqual(sorted(os.listdir(self.tmpdir)), sorted([ "rsa.public", "rsa.private", "dsa.public", "dsa.private", + "ecdsa.public", "ecdsa.private", ])) self.assertEqual(utils.ReadFile(utils.PathJoin(self.tmpdir, "rsa.public")), "rsapub") @@ -221,6 +230,10 @@ class TestUpdateSshDaemon(unittest.TestCase): "dsapub") self.assertEqual(utils.ReadFile(utils.PathJoin(self.tmpdir, "dsa.private")), "dsapriv") + self.assertEqual(utils.ReadFile(utils.PathJoin( + self.tmpdir, "ecdsa.public")), "ecdsapub") + self.assertEqual(utils.ReadFile(utils.PathJoin( + self.tmpdir, "ecdsa.private")), "ecdsapriv") def testSuccess(self): self._TestUpdate(False) (END)
