[Kde-pim] Review Request 113947: Manual revert of d8eeb4c - automatic fallback for SSL/TLS protocol versions
Christian Mollekopf
chrigi_1 at fastmail.fm
Wed Nov 27 12:50:58 GMT 2013
> On Nov. 19, 2013, 5:27 p.m., Stefan Brüns wrote:
> > NACK!
> >
> > This code is there for a reason. See the original review request: https://git.reviewboard.kde.org/r/107099/
>
> Christian Mollekopf wrote:
> I know that it is there for a reason, but as it currently stands we have a non-functional (for all I know) workaround for a problem one imap server out there has. And this exact code was responsible for two hard to track down bugs and countless hours of debugging from multiple persons.
>
> If there is a very good reason to keep this code, I guess I can fix it, but there better be one.
>
> Can you please explain how this code is supposed to work? From what I can tell SessionThread::doStartSsl is only called once, so I don't get how this is supposed to work.
>
> Stefan Brüns wrote:
> It does work.
>
> The session restarts the negotiation after a disconnect. A disconnect happens after a failed negotiation.
>
> Stefan Brüns wrote:
> Try this one:
> diff --git a/kimap/sessionthread.cpp b/kimap/sessionthread.cpp
> index 42a1f62..112a5ae 100644
> --- a/kimap/sessionthread.cpp
> +++ b/kimap/sessionthread.cpp
> @@ -274,7 +274,6 @@ void SessionThread::socketError(KTcpSocket::Error error)
> // Called in secondary thread
> void SessionThread::sslConnected()
> {
> - doSslFallback = false;
> Q_ASSERT( QThread::currentThread() == thread() );
> if ( !m_socket )
> return;
> @@ -291,6 +290,7 @@ void SessionThread::sslConnected()
> KSslErrorUiData errorData( m_socket );
> emit sslError( errorData );
> } else {
> + doSslFallback = false;
> kDebug() << "TLS negotiation done.";
> m_encryptedMode = true;
> emit encryptionNegotiationResult( true, m_socket->negotiatedSslVersion() );
> @@ -309,6 +309,7 @@ void SessionThread::doSslErrorHandlerResponse(bool response)
> if ( !m_socket )
> return;
> if ( response ) {
> + doSslFallback = false;
> m_encryptedMode = true;
> emit encryptionNegotiationResult( true, m_socket->negotiatedSslVersion() );
> } else {
>
>
> Christian Mollekopf wrote:
> We have (after having an unencrypted connection:
> LoginJob::doStart
> Session::startSsl
> SessionThread::doStartSsl
> SessionThread::socketError //because it failed
> SessionThread::slotSocketDisconnected //because we aborted
> SessionThread::reconnect //creates a new unencrypted connection
>
> I don't see SessionThread::doStartSsl, neither in Session nor in LoginJob, so what is supposed to happen exactly?
>
> In any case I really don't like the code. It introduces an error prone new state into the SessionThread, and seems to depend on some callback sequence outside of SessionThread even. If we only require this for a buggy server, I really don't see the point. A much more feasible workaround would be to i.e. allow to force the used encryption version from a config file.
>
> Christian Mollekopf wrote:
> * I don't see SessionThread::doStartSsl being called again, ...
>
> Dan Vrátil wrote:
> I can confirm that the inline patch above fixes the reconnect-loop with both Zimbra and Cyrus IMAP servers for me (so does Christian's patch of course :-))
>
> Christian Mollekopf wrote:
> Yes, but that problem has already been fixed in 4b8adab, and I don't see how this patch does it better. It might even introduce the problem again in the case the errors are ignored.
>
> Anyways, if the code doesn't work, as I suspect, it can be removed for obvious reasons. If it indeed works and is still required I suppose we can replace it by something simpler like just allowing to force the used ssl version. But in a first step I'd like to learn how the code is supposed to work, everything else we can discuss afterwards.
>
> Stefan Brüns wrote:
> in sessionthread.cpp:
> SessionThread::reconnect() calls
> m_socket->connectToHost( m_hostName, m_port );
>
> m_socket::connected() is connected to SessionThread::socketConnected()
> connect( m_socket, SIGNAL(connected()),
> this, SIGNAL(socketConnected()) );
>
> in session.cpp:
> SessionPrivate::thread is connected to SessionPrivate::socketConnected()
> connect( d->thread, SIGNAL(socketConnected()),
> d, SLOT(socketConnected()) );
>
> socketConnected() starts a new LoginJob.
>
> And let me repeat: The code does work. It did not handle connection errors caused by bad certificates, but thats it. As Dan confirmed, the patch above fixes it.
>
> You are deliberately breaking working code, you pushed a change to git regressing the current state, without waiting for review.
Sorry, I missed the response as I somehow didn't get the notification email.
Regarding your explanation, socketConnected simply calls startNext, starting the next job. However, since the initial LoginJob has already been dequeued, I still don't understand how this is supposed to reinitiate the SSL handshake.
Anyways, my point is that this code is very fragile and complex, relying on a somwhat arcane combination of signals in order to work, while introducing a new state into the sessionthread. Last but not least, the code is completely untested. This is precisely the reason we have bugs that were rather difficult to track down, and why I accidentally (which is quite the opposite of deliberately) broke this codepath fixing another bug.
These are the reason why I don't really want to maintain that code and think it should be removed.
So, if this workaround is still required, why doesn't a configuration option that allows to force the encryption used solve your problem?
- Christian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113947/#review43971
-----------------------------------------------------------
On Nov. 19, 2013, 4:06 p.m., Christian Mollekopf wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113947/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 4:06 p.m.)
>
>
> Review request for KDEPIM-Libraries, Stefan Brüns, Dan Vrátil, and Kevin Ottens.
>
>
> Bugs: 316840
> http://bugs.kde.org/show_bug.cgi?id=316840
>
>
> Repository: kdepimlibs
>
>
> Description
> -------
>
> Manual revert of d8eeb4c - automatic fallback for SSL/TLS protocol versions
>
> This code caused a lot of problems and nobody seems to be using it (it was non functional),
> it may as well just die.
>
>
> Diffs
> -----
>
> kimap/sessionthread_p.h c0819ddb862a3d5671d5e1b1a9e3a49c614d7778
> kimap/sessionthread.cpp e70e3175cc0439805cd2d5c4e75ad1c79ef3dc4e
>
> Diff: http://git.reviewboard.kde.org/r/113947/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Christian Mollekopf
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list