On Thu, Dec 19, 2013 at 4:23 PM, Hrvoje Ribicic <[email protected]> wrote:
> > > On Thu, Dec 19, 2013 at 3:49 PM, Helga Velroyen <[email protected]> wrote: > >> This patch adds a couple of utility functions to manipulate >> the map of master candidate SSL certificate digests. >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> Makefile.am | 2 + >> lib/utils/security.py | 76 >> +++++++++++++++++++++++++++++++ >> test/py/ganeti.utils.security_unittest.py | 74 >> ++++++++++++++++++++++++++++++ >> 3 files changed, 152 insertions(+) >> create mode 100644 lib/utils/security.py >> create mode 100755 test/py/ganeti.utils.security_unittest.py >> >> diff --git a/Makefile.am b/Makefile.am >> index e74f58b..96e1284 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -490,6 +490,7 @@ utils_PYTHON = \ >> lib/utils/nodesetup.py \ >> lib/utils/process.py \ >> lib/utils/retry.py \ >> + lib/utils/security.py \ >> lib/utils/storage.py \ >> lib/utils/text.py \ >> lib/utils/version.py \ >> @@ -1473,6 +1474,7 @@ python_tests = \ >> test/py/ganeti.utils.nodesetup_unittest.py \ >> test/py/ganeti.utils.process_unittest.py \ >> test/py/ganeti.utils.retry_unittest.py \ >> + test/py/ganeti.utils.security_unittest.py \ >> test/py/ganeti.utils.storage_unittest.py \ >> test/py/ganeti.utils.text_unittest.py \ >> test/py/ganeti.utils.version_unittest.py \ >> diff --git a/lib/utils/security.py b/lib/utils/security.py >> new file mode 100644 >> index 0000000..221b587 >> --- /dev/null >> +++ b/lib/utils/security.py >> @@ -0,0 +1,76 @@ >> +# >> +# >> + >> +# Copyright (C) 2013 Google Inc. >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, but >> +# WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> +# General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program; if not, write to the Free Software >> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> +# 02110-1301, USA. >> + >> +"""Utility functions for security features of Ganeti. >> + >> +""" >> + >> +import logging >> + >> + >> +def AddNodeToCandidateCerts(node_uuid, cert_digest, candidate_certs, >> + info_fn=logging.info, warn_fn=logging.warn): >> + """Adds an entry to the candidate certificate map. >> + >> + @type node_uuid: string >> + @param node_uuid: the node's UUID >> + @type cert_digest: string >> + @param cert_digest: the digest of the node's client SSL certificate >> + @type candidate_certs: dict of strings to strings >> + @param candidate_certs: map of node UUIDs to the digests of their >> client >> + SSL certificates, will be manipulated in this function >> + @type info_fn: function >> + @param info_fn: logging function for information messages >> + @type warn_fn: function >> + @param warn_fn: logging function for warning messages >> + >> + """ >> + assert candidate_certs is not None >> + >> + if node_uuid in candidate_certs: >> + old_cert_digest = candidate_certs[node_uuid] >> + if old_cert_digest == cert_digest: >> + info_fn("Certificate digest for node %s already in config." >> + "Not doing anything.") >> > > The node identifier is not injected into the logging message here. > Right, thanks for noticing. > > >> + return >> + else: >> + warn_fn("Overriding differing certificate digest for node %s" >> + % node_uuid) >> > > Regarding the node_uuid, a more general question: > Do we have some standards regarding what goes into a log? > A node uuid will identify the node, but I guess a node name is more useful > to the user. > If this is what we do elsewhere, then let it be :) > I don't know what the standard is in general, but while working on it, I found it highly practical to have the node UUIDs instead of the name, because when debugging this it is kind of crucial to distinguish two nodes that have the same host name at some point in time, for example a node can be removed and a different node can be added using the same name and there it should actually be the case that the UUID differs. Hence related to this SSL certificate handling, I found it highly beneficial to have the UUID, because otherwise you don't know which host of that name has the certificate. > > >> + candidate_certs[node_uuid] = cert_digest >> + >> + >> +def RemoveNodeFromCandidateCerts(node_uuid, candidate_certs, >> + warn_fn=logging.warn): >> + """Removes the entry of the given node in the certificate map. >> + >> + @type node_uuid: string >> + @param node_uuid: the node's UUID >> + @type candidate_certs: dict of strings to strings >> + @param candidate_certs: map of node UUIDs to the digests of their >> client >> + SSL certificates, will be manipulated in this function >> + @type warn_fn: function >> + @param warn_fn: logging function for warning messages >> + >> + """ >> + if node_uuid not in candidate_certs: >> + warn_fn("Cannot remove certifcate for node %s, because it's not in >> the" >> + "candidate map.") >> > > Again, missing identifier, and a typo: s/certifcate/certificate/ > Thx, will send an interdiff. > > >> + return >> + del candidate_certs[node_uuid] >> diff --git a/test/py/ganeti.utils.security_unittest.py b/test/py/ >> ganeti.utils.security_unittest.py >> new file mode 100755 >> index 0000000..08c6b58 >> --- /dev/null >> +++ b/test/py/ganeti.utils.security_unittest.py >> @@ -0,0 +1,74 @@ >> +#!/usr/bin/python >> +# >> + >> +# Copyright (C) 2013 Google Inc. >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, but >> +# WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> +# General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program; if not, write to the Free Software >> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> +# 02110-1301, USA. >> + >> + >> +"""Script for unittesting the ganeti.utils.storage module""" >> + >> +import mock >> +import unittest >> + >> +from ganeti.utils import security >> + >> +import testutils >> + >> + >> +class TestCandidateCerts(unittest.TestCase): >> + >> + def setUp(self): >> + self._warn_fn = mock.Mock() >> + self._info_fn = mock.Mock() >> + self._candidate_certs = {} >> + >> + def testAddAndRemoveCerts(self): >> + self.assertEqual(0, len(self._candidate_certs)) >> + >> + node_uuid = "1234" >> + cert_digest = "foobar" >> + security.AddNodeToCandidateCerts(node_uuid, cert_digest, >> + self._candidate_certs, warn_fn=self._warn_fn, >> info_fn=self._info_fn) >> + self.assertEqual(1, len(self._candidate_certs)) >> + >> + # Try adding the same cert again >> + security.AddNodeToCandidateCerts(node_uuid, cert_digest, >> + self._candidate_certs, warn_fn=self._warn_fn, >> info_fn=self._info_fn) >> + self.assertEqual(1, len(self._candidate_certs)) >> + self.assertTrue(self._candidate_certs[node_uuid] == cert_digest) >> + >> + # Overriding cert >> + other_digest = "barfoo" >> + security.AddNodeToCandidateCerts(node_uuid, other_digest, >> + self._candidate_certs, warn_fn=self._warn_fn, >> info_fn=self._info_fn) >> + self.assertEqual(1, len(self._candidate_certs)) >> + self.assertTrue(self._candidate_certs[node_uuid] == other_digest) >> + >> + # Try removing a certificate from a node that is not in the list >> + other_node_uuid = "5678" >> + security.RemoveNodeFromCandidateCerts( >> + other_node_uuid, self._candidate_certs, warn_fn=self._warn_fn) >> + self.assertEqual(1, len(self._candidate_certs)) >> + >> + # Remove a certificate from a node that is in the list >> + security.RemoveNodeFromCandidateCerts( >> + node_uuid, self._candidate_certs, warn_fn=self._warn_fn) >> + self.assertEqual(0, len(self._candidate_certs)) >> + >> + >> +if __name__ == "__main__": >> + testutils.GanetiTestProgram() >> -- >> 1.8.5.1 >> >> > -- -- Helga Velroyen | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
