Change in vdsm[master]: Implement SSL session cache

Alon Bar-Lev alonbl at redhat.com
Tue Nov 13 17:15:23 UTC 2012


Alon Bar-Lev has posted comments on this change.

Change subject: Implement SSL session cache
......................................................................


Patch Set 9: (6 inline comments)

Nice!

I think the test is missing.

....................................................
File vdsm/SecureXMLRPCServer.py
Line 49:     The rest of the methods are just delegated to the SSL connection
Line 50:     class.
Line 51:     """
Line 52: 
Line 53:     def __init__(self, raw, certfile=None, keyfile=None, ca_certs=None,
Can we have the option for PKCS#12? :)

For future, so I can bootstrap with standard single PKCS#12 Key+chain.
Line 54:                  session_id="vdsm"):
Line 55:         # Create the SSL context:
Line 56:         self.context = SSL.Context(protocol="sslv23")
Line 57:         self.context.set_session_id_ctx(session_id)


Line 52: 
Line 53:     def __init__(self, raw, certfile=None, keyfile=None, ca_certs=None,
Line 54:                  session_id="vdsm"):
Line 55:         # Create the SSL context:
Line 56:         self.context = SSL.Context(protocol="sslv23")
Can we have this as parameter?

And start use tlsv1?

So this will be auto nego?

I don't see any reason to use sslv2, choice should be between sslv3 and tlsv1.

We should fix the engine to use tlsv1 as well.
Line 57:         self.context.set_session_id_ctx(session_id)
Line 58: 
Line 59:         # Load the server certificate and key files:
Line 60:         if certfile and keyfile:


Line 57:         self.context.set_session_id_ctx(session_id)
Line 58: 
Line 59:         # Load the server certificate and key files:
Line 60:         if certfile and keyfile:
Line 61:             self.context.load_cert(certfile, keyfile)
Can we support certificate chain and not just end-certificate?
Line 62: 
Line 63:         def verify(context, certificate, error, depth, result):
Line 64:             # The validation of the client certificate has already been
Line 65:             # performed by the OpenSSL library and the handhake already aborted


Line 66:             # if it fails as we use the verify_fail_if_no_peer_cert mode. We
Line 67:             # are not doing any additional validation, so we just need to log
Line 68:             # it and return the same result.
Line 69:             if not result:
Line 70:                  certificate = X509.X509(certificate)
I think you should free the certificate.
Line 71:                  logging.error(
Line 72:                      "invalid client certificate with subject \"%s\"",
Line 73:                      certificate.get_subject())
Line 74:             return result


Line 101:         # SSL connection:
Line 102:         return getattr(self.connection, name)
Line 103: 
Line 104: 
Line 105: class SSLClientSocket(object):
I am not sure the client is the term...

There are active and passive sockets.

There are connected and passive sockets.

There are connected and accepted sockets.

There are client and server sockets, but these are by terms of application, I don't think vdsm has the nature of client... no?
Line 106:     """SSL decorator for client sockets.
Line 107: 
Line 108:     This class wraps a client socket returned by the accept method of a
Line 109:     server socket providing the SSL socket methods that are missing in


....................................................
File vdsm.spec.in
Line 40: # BuildRequires needed by the tests during the build
Line 41: BuildRequires: python-ethtool
Line 42: BuildRequires: libvirt-python
Line 43: BuildRequires: genisoimage
Line 44: BuildRequires: openssl
Why do we need openssl on build?
Line 45: %if 0%{?rhel}
Line 46: BuildRequires: python-ordereddict
Line 47: %endif
Line 48: 


--
To view, visit http://gerrit.ovirt.org/8123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic75adee4070b415b8855af1f2ea289825496fbc1
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernandez at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernandez at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list