Interdiff LGTM, but I guess more interdiffs will have to come as a result of the constant renaming.
Maybe make a separate patch for that at the end of the patch series? Either works for me, pick whatever you like. On Fri, Dec 20, 2013 at 10:25 AM, Helga Velroyen <[email protected]> wrote: > > > > On Thu, Dec 19, 2013 at 4:59 PM, Hrvoje Ribicic <[email protected]> wrote: > >> >> >> >> On Thu, Dec 19, 2013 at 3:49 PM, Helga Velroyen <[email protected]>wrote: >> >>> In various cluster operations, the master node need to >>> >> s/need/needs/ or s/need/may need/ >> > > Changed it to "needs". > > >> retrieve the digest of a node's SSL certificate. For this >>> purpose, we add an RPC call to retrieve the digest. The >>> function is designed in a general way to make it possible >>> to retrieve other (public) cryptographic tokens of a node >>> in the future as well (for example an SSH key). >>> >>> Signed-off-by: Helga Velroyen <[email protected]> >>> --- >>> lib/backend.py | 24 ++++++++++++++++++++++++ >>> lib/rpc_defs.py | 3 +++ >>> lib/server/noded.py | 8 ++++++++ >>> lib/utils/__init__.py | 1 + >>> lib/utils/security.py | 18 ++++++++++++++++++ >>> src/Ganeti/Constants.hs | 15 +++++++++++++++ >>> test/py/ganeti.backend_unittest.py | 20 ++++++++++++++++++++ >>> test/py/ganeti.utils.security_unittest.py | 18 ++++++++++++++++++ >>> 8 files changed, 107 insertions(+) >>> >>> diff --git a/lib/backend.py b/lib/backend.py >>> index b329864..e2e3e28 100644 >>> --- a/lib/backend.py >>> +++ b/lib/backend.py >>> @@ -1162,6 +1162,30 @@ def VerifyNode(what, cluster_name, all_hvparams, >>> node_groups, groups_cfg): >>> return result >>> >>> >>> +def GetCryptoTokens(token_types): >>> + """Get the node's public cryptographic tokens. >>> + >>> + This can be the public ssh key of the node or the certifciate digest >>> of >>> >> s/certifciate/certificate/ >> > > ACK > > >> + the node's public client SSL certificate. >>> + >>> + Note: so far, only retrieval of the SSL digest is implemented >>> + >>> + @type token_types: list of strings in constants.CRYPTO_TYPES >>> + @param token_types: list of types of requested cryptographic tokens >>> + @rtype: list of tuples (string, string) >>> + @return: list of tuples of the token type and the public crypto token >>> + >>> + """ >>> + tokens = [] >>> + for token_type in token_types: >>> + if token_type not in constants.CRYPTO_TYPES: >>> + raise errors.ProgrammerError("Token type %s not supported." % >>> + token_type) >>> + if token_type == constants.CRYPTO_TYPE_SSL: >>> >> >> This is quite a generic name; given that this call can retrieve any sort >> of crypto token, >> there might be confusion. >> >> Consider making it more descriptive, maybe by adding a _DIGEST to the >> name? >> >> >>> + tokens.append((token_type, utils.GetClientCertificateDigest())) >>> + return tokens >>> + >>> + >>> def GetBlockDevSizes(devices): >>> """Return the size of the given block devices >>> >>> diff --git a/lib/rpc_defs.py b/lib/rpc_defs.py >>> index b9b1905..b92f720 100644 >>> --- a/lib/rpc_defs.py >>> +++ b/lib/rpc_defs.py >>> @@ -505,6 +505,9 @@ _NODE_CALLS = [ >>> ("ovs_name", None, "Name of the OpenvSwitch to create"), >>> ("ovs_link", None, "Link of the OpenvSwitch to the outside"), >>> ], None, None, "This will create and setup the OpenvSwitch"), >>> + ("node_crypto_tokens", SINGLE, None, constants.RPC_TMO_NORMAL, [ >>> + ("token_types", None, "List of requested crypto token types"), >>> + ], None, None, "Retrieve public crypto tokes from the node."), >>> >> s/tokes/tokens/ >> > > ACK > > >> ] >>> >>> _MISC_CALLS = [ >>> diff --git a/lib/server/noded.py b/lib/server/noded.py >>> index 74f300d..e5bf330 100644 >>> --- a/lib/server/noded.py >>> +++ b/lib/server/noded.py >>> @@ -864,6 +864,14 @@ class >>> NodeRequestHandler(http.server.HttpServerHandler): >>> (ovs_name, ovs_link) = params >>> return backend.ConfigureOVS(ovs_name, ovs_link) >>> >>> + @staticmethod >>> + def perspective_node_crypto_tokens(params): >>> + """Gets the node's public crypto tokens. >>> + >>> + """ >>> + token_types = params[0] >>> + return backend.GetCryptoTokens(token_types) >>> + >>> # cluster -------------------------- >>> >>> @staticmethod >>> diff --git a/lib/utils/__init__.py b/lib/utils/__init__.py >>> index 23650ca..50d88c0 100644 >>> --- a/lib/utils/__init__.py >>> +++ b/lib/utils/__init__.py >>> @@ -53,6 +53,7 @@ from ganeti.utils.mlock import * >>> from ganeti.utils.nodesetup import * >>> from ganeti.utils.process import * >>> from ganeti.utils.retry import * >>> +from ganeti.utils.security import * >>> from ganeti.utils.storage import * >>> from ganeti.utils.text import * >>> from ganeti.utils.wrapper import * >>> diff --git a/lib/utils/security.py b/lib/utils/security.py >>> index 221b587..fe1f21c 100644 >>> --- a/lib/utils/security.py >>> +++ b/lib/utils/security.py >>> @@ -23,6 +23,10 @@ >>> """ >>> >>> import logging >>> +import OpenSSL >>> + >>> +from ganeti.utils import io >>> +from ganeti import pathutils >>> >>> >>> def AddNodeToCandidateCerts(node_uuid, cert_digest, candidate_certs, >>> @@ -74,3 +78,17 @@ def RemoveNodeFromCandidateCerts(node_uuid, >>> candidate_certs, >>> "candidate map.") >>> return >>> del candidate_certs[node_uuid] >>> + >>> + >>> +def GetClientCertificateDigest(cert_filename=pathutils.NODED_CERT_FILE): >>> + """Reads the SSL certificate and returns the sha1 digest. >>> + >>> + """ >>> + # FIXME: This is supposed to read the client certificate, but >>> + # in this stage of the patch series there is no client certificate >>> + # yet, so we return the digest of the server certificate to get >>> + # the rest of the key management infrastructure running. >>> + cert_plain = io.ReadFile(cert_filename) >>> + cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM, >>> + cert_plain) >>> + return cert.digest("sha1") >>> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs >>> index b27657a..09ee7b8 100644 >>> --- a/src/Ganeti/Constants.hs >>> +++ b/src/Ganeti/Constants.hs >>> @@ -4140,6 +4140,21 @@ fakeOpMasterTurndown = "OP_CLUSTER_IP_TURNDOWN" >>> fakeOpMasterTurnup :: String >>> fakeOpMasterTurnup = "OP_CLUSTER_IP_TURNUP" >>> >>> + >>> +-- * Crypto Types >>> +-- Types of cryptographic tokens used in node communication >>> + >>> +cryptoTypeSsl :: String >>> +cryptoTypeSsl = "ssl" >>> + >>> +cryptoTypeSsh :: String >>> +cryptoTypeSsh = "ssh" >>> >> >> A quick search of this patch series did not show the constant being used >> afterwards. >> Was it left behind during testing or do you want it to remain as a >> reminder? >> >> >>> + >>> +-- So far only ssl keys are used in the context of this constant >>> +cryptoTypes :: FrozenSet String >>> +cryptoTypes = ConstantUtils.mkSet [cryptoTypeSsl] >>> + >>> + >>> -- * SSH key types >>> >>> sshkDsa :: String >>> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ >>> ganeti.backend_unittest.py >>> index 033419e..5c30bc2 100755 >>> --- a/test/py/ganeti.backend_unittest.py >>> +++ b/test/py/ganeti.backend_unittest.py >>> @@ -73,6 +73,26 @@ class TestX509Certificates(unittest.TestCase): >>> self.assertEqual(utils.ListVisibleFiles(self.tmpdir), [name]) >>> >>> >>> +class TestGetCryptoTokens(testutils.GanetiTestCase): >>> + >>> + def setUp(self): >>> + self._digest_fn_orig = utils.GetClientCertificateDigest >>> + self._ssl_digest = "12345" >>> + utils.GetClientCertificateDigest = mock.Mock( >>> + return_value=self._ssl_digest) >>> + >>> + def tearDown(self): >>> + utils.GetClientCertificateDigest = self._digest_fn_orig >>> + >>> + def testSslToken(self): >>> + result = backend.GetCryptoTokens([constants.CRYPTO_TYPE_SSL]) >>> + self.assertTrue((constants.CRYPTO_TYPE_SSL, self._ssl_digest) in >>> result) >>> + >>> + def testUnknownToken(self): >>> + self.assertRaises(errors.ProgrammerError, >>> + backend.GetCryptoTokens, ["pink_bunny"]) >>> + >>> + >>> class TestNodeVerify(testutils.GanetiTestCase): >>> >>> def setUp(self): >>> diff --git a/test/py/ganeti.utils.security_unittest.py b/test/py/ >>> ganeti.utils.security_unittest.py >>> index 08c6b58..f42dd22 100755 >>> --- a/test/py/ganeti.utils.security_unittest.py >>> +++ b/test/py/ganeti.utils.security_unittest.py >>> @@ -70,5 +70,23 @@ class TestCandidateCerts(unittest.TestCase): >>> self.assertEqual(0, len(self._candidate_certs)) >>> >>> >>> +class TestGetCertificateDigest(testutils.GanetiTestCase): >>> + >>> + def setUp(self): >>> + testutils.GanetiTestCase.setUp(self) >>> + # certificate file that contains the certificate only >>> + self._certfilename1 = testutils.TestDataFilename("cert1.pem") >>> + # (different) certificate file that contains both, certificate >>> + # and private key >>> + self._certfilename2 = testutils.TestDataFilename("cert2.pem") >>> + >>> + def testGetCertificateDigest(self): >>> + digest1 = security.GetClientCertificateDigest( >>> + cert_filename=self._certfilename1) >>> + digest2 = security.GetClientCertificateDigest( >>> + cert_filename=self._certfilename2) >>> + self.assertFalse(digest1 == digest2) >>> + >>> + >>> if __name__ == "__main__": >>> testutils.GanetiTestProgram() >>> -- >>> 1.8.5.1 >>> >>> >> > Interdiff: > > diff --git a/lib/backend.py b/lib/backend.py > index e2e3e28..da556fd 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -1165,7 +1165,7 @@ def VerifyNode(what, cluster_name, all_hvparams, > node_groups, groups_cfg): > def GetCryptoTokens(token_types): > """Get the node's public cryptographic tokens. > > - This can be the public ssh key of the node or the certifciate digest of > + This can be the public ssh key of the node or the certificate digest of > the node's public client SSL certificate. > > Note: so far, only retrieval of the SSL digest is implemented > @@ -1181,7 +1181,7 @@ def GetCryptoTokens(token_types): > if token_type not in constants.CRYPTO_TYPES: > raise errors.ProgrammerError("Token type %s not supported." % > token_type) > - if token_type == constants.CRYPTO_TYPE_SSL: > + if token_type == constants.CRYPTO_TYPE_SSL_DIGEST: > tokens.append((token_type, utils.GetClientCertificateDigest())) > return tokens > > > -cryptoTypeSsl :: String > -cryptoTypeSsl = "ssl" > +cryptoTypeSslDigest :: String > +cryptoTypeSslDigest = "ssl" > > cryptoTypeSsh :: String > cryptoTypeSsh = "ssh" > > -- So far only ssl keys are used in the context of this constant > cryptoTypes :: FrozenSet String > -cryptoTypes = ConstantUtils.mkSet [cryptoTypeSsl] > +cryptoTypes = ConstantUtils.mkSet [cryptoTypeSslDigest] > > > -- * SSH key types > diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ > ganeti.backend_unittest.py > index 5c30bc2..81f3217 100755 > --- a/test/py/ganeti.backend_unittest.py > +++ b/test/py/ganeti.backend_unittest.py > @@ -85,8 +85,9 @@ class TestGetCryptoTokens(testutils.GanetiTestCase): > utils.GetClientCertificateDigest = self._digest_fn_orig > > def testSslToken(self): > - result = backend.GetCryptoTokens([constants.CRYPTO_TYPE_SSL]) > - self.assertTrue((constants.CRYPTO_TYPE_SSL, self._ssl_digest) in > result) > + result = backend.GetCryptoTokens([constants.CRYPTO_TYPE_SSL_DIGEST]) > + self.assertTrue((constants.CRYPTO_TYPE_SSL_DIGEST, self._ssl_digest) > + in result)diff --git a/lib/backend.py b/lib/backend.py > index e2e3e28..da556fd 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -1165,7 +1165,7 @@ def VerifyNode(what, cluster_name, all_hvparams, > node_groups, groups_cfg): > def GetCryptoTokens(token_types): > """Get the node's public cryptographic tokens. > > - This can be the public ssh key of the node or the certifciate digest of > + This can be the public ssh key of the node or the certificate digest of > the node's public client SSL certificate. > > Note: so far, only retrieval of the SSL digest is implemented > @@ -1181,7 +1181,7 @@ def GetCryptoTokens(token_types): > if token_type not in constants.CRYPTO_TYPES: > raise errors.ProgrammerError("Token type %s not supported." % > token_type) > - if token_type == constants.CRYPTO_TYPE_SSL: > + if token_type == constants.CRYPTO_TYPE_SSL_DIGEST: > tokens.append((token_type, utils.GetClientCertificateDigest())) > return tokens > > > def testUnknownToken(self): > self.assertRaises(errors.ProgrammerError, > > > > Cheers, > Helga > > -- > -- > 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 >
