[ovs-dev] [PATCH] Don't raise an Exception on failure to connect via SSL
twilson at redhat.com
Tue Sep 15 21:00:25 UTC 2020
On Tue, Sep 15, 2020 at 2:55 PM Ilya Maximets <i.maximets at ovn.org> wrote:
> On 9/15/20 7:17 PM, Terry Wilson wrote:
> > With other socket types, trying to connect and failing will return
> > an error code, but if an SSL Stream is used, then when
> > check_connection_completion(sock) is called, SSL will raise an
> > exception that doesn't derive from socket.error which is handled.
> > This adds handling for SSL.error, which get_exception_errno
> > will simply return errno.EPROTO for.
> > Signed-off-by: Terry Wilson <twilson at redhat.com>
> > ---
> Hi, Terry. Thanks for working on this!
Thanks for the review!
> > - except socket.error as e:
> > + except (socket.error, SSL.Error) if SSL else socket.error
> as e:
> This doesn't look right because this breaks the stream abstraction.
> I think, SSL.Error should not go outside of SSLStream class in the first
If that's true, doesn't having the staticmethod Stream.open() call
check_connection_completion(sock) break the abstraction by operating
directly on the sock object and not the Stream object?
> Looking at methods of SSLStream class, I see that it catches only few of
> errors and translates them to errno, but it must catch all possible SSL
Right, it already catches the errors we need in the SSLStream.send(), it's
just that check_connection_completion() is in socket_util.py which doesn't
operate on streams, so calls socket.send(). Stream.open() is a
staticmethod, so it's not going to be class-aware, so also kludgy to add
the SSL-specific stuff there. But basically it seems like either wrap
everywhere that calls check_connection_completion(), come up with some kind
of wrapper for check_connection_completion in stream.py classes that is at
least a classmethod that , or just handle it in socket_util. I'm not sure
there is an easy solution that I'd find 'pretty', but I'm willing to do
whatever is least ugly to everyone. ;)
See interpret_ssl_error() of the C implementation in lib/stream-ssl.c.
> I think, we need to implement similar handling in python code to unify
> What do you think?
I think most of that already exists with the SSLStream class. I remember
using it for 5fe179987d to do the same thing stream-ssl.c did for closing
an SSL connection. If Stream.open() wasn't called as a staticmethod/factory
function, then it'd be a lot easier to make a cleaner implementation.The
cleanest thing I can think of is to define a staticmethod
Stream.check_connection_completion(sock) that returns the sock_util
version, then override with a staticmethod in SSLStream that try's calling
Stream.check_connection_completion but catches SSL.Error and returns the
errno. And replace calls to the sock_util.check_connection_completion with
the class-based ones. I'll send a patch shortly with that implementation to
see if looks better. Thanks again for the review!
More information about the dev