Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SSL] Investigate if we really use Session Tickets #3159

Closed
guruz opened this issue Apr 24, 2015 · 4 comments
Closed

[SSL] Investigate if we really use Session Tickets #3159

guruz opened this issue Apr 24, 2015 · 4 comments
Assignees
Labels
Needs info Performance ReadyToTest QA, please validate the fix/enhancement

Comments

@guruz
Copy link
Contributor

guruz commented Apr 24, 2015

Started here but got sidetracked, will continue next time.
We need this because we want to save roundtrips when establishing connections (this is temporarily until we can have SPDY/HTTP2 (#3146)

diff --cc src/libsync/accessmanager.cpp
index 331f9d0,331f9d0..e6b34fb
--- a/src/libsync/accessmanager.cpp
+++ b/src/libsync/accessmanager.cpp
@@@ -63,6 -63,6 +63,8 @@@ QNetworkReply* AccessManager::createReq
  {
      QNetworkRequest newRequest(request);

++    newRequest.setRawHeader("Connection", "Close");
++
      if (newRequest.hasRawHeader("cookie")) {
          // This will set the cookie into the QNetworkCookieJar which will then override the cookie header
          setRawCookie(request.rawHeader("cookie"), request.url());
diff --cc src/libsync/account.cpp
index 9b8941f,9b8941f..5de75de
--- a/src/libsync/account.cpp
+++ b/src/libsync/account.cpp
@@@ -383,6 -383,6 +383,9 @@@ QSslConfiguration Account::createSslCon
          qDebug() << "Added SSL client certificate to the query";
      }

++    qDebug() << "ALLOWING REUSE HERE";
++    sslConfig.setSslOption(QSsl::SslOptionDisableSessionTickets, false);
++
      return sslConfig;
  }

diff --cc src/libsync/networkjobs.cpp
index 7e92d96,7e92d96..94a264f
--- a/src/libsync/networkjobs.cpp
+++ b/src/libsync/networkjobs.cpp
@@@ -26,6 -26,6 +26,7 @@@
  #include <QMutex>
  #include <QDebug>
  #include <QCoreApplication>
++#include <QSslConfiguration>

  #include "json.h"

@@@ -507,6 -507,6 +508,10 @@@ void LsColJob::start(
  // not in all in one big blobb at the end.
  bool LsColJob::finished()
  {
++    QSslConfiguration conf = reply()->sslConfiguration();
++    qDebug() << "SSL TICKET:" << reply()->sslConfiguration().sessionTicket();
++
++
      QString contentType = reply()->header(QNetworkRequest::ContentTypeHeader).toString();
      int httpCode = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
      if (httpCode == 207 && contentType.contains("application/xml; charset=utf-8")) {
@guruz guruz self-assigned this Apr 24, 2015
@guruz
Copy link
Contributor Author

guruz commented Apr 28, 2015

There is two parts to this
One is we need

sslConfig.setSslOption(QSsl::SslOptionDisableSessionTickets, false);
sslConfig.setSslOption(QSsl::SslOptionDisableSessionSharing, false);
sslConfig.setSslOption(QSsl::SslOptionDisableSessionPersistence, false);

The other one is that it looks like the QSslConfig creation in #3027 overrides something all the time so the session ticket is not reused.

Let's see..

FYI @peter-ha @danimo

@guruz guruz added this to the 2.0 - Multi-account milestone Apr 28, 2015
@peter-ha
Copy link

IIRC you only need

sslConfig.setSslOption(QSsl::SslOptionDisableSessionPersistence, false);

At least session sharing should be enabled by default...

@guruz guruz modified the milestones: 1.9, 2.0 - Multi-account May 8, 2015
@guruz guruz added the ReadyToTest QA, please validate the fix/enhancement label May 8, 2015
guruz added a commit that referenced this issue May 8, 2015
This also improves the SSL configuration creation and fixes #3027
@guruz
Copy link
Contributor Author

guruz commented May 8, 2015

Done, it should nag now with a warning if the SSL session is not reused.

FYI @ckamm @dragotin @danimo

guruz added a commit to owncloud-archive/documentation that referenced this issue May 8, 2015
Mentioned SSL session ticket configuration 
Originally via owncloud/client#3159
@dragotin dragotin modified the milestones: 1.8.2 - Bugfix 2, 1.9 May 8, 2015
@Dianafg76
Copy link

@guruz I need information on what I need to test
Thanks

@guruz guruz closed this as completed Jun 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs info Performance ReadyToTest QA, please validate the fix/enhancement
Projects
None yet
Development

No branches or pull requests

4 participants