[Kde-pim] Review Request 113947: Manual revert of d8eeb4c - automatic fallback for SSL/TLS protocol versions

[Dan Vrátil]

> On Nov. 19, 2013, 6: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, ...

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 :-))


- Dan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113947/#review43971
-----------------------------------------------------------


On Nov. 19, 2013, 5: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, 5: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/