On Fri, Dec 20, 2013 at 10:32 AM, Hrvoje Ribicic <[email protected]> wrote:
> 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. > I'm going through the patch series as there are more comments coming and keep that in mind for each patch. > > > 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 >> > > -- -- 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
