New issue
Advanced search Search tips

Issue 737143 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Make SpdySessions always "secure".

Project Member Reported by b...@chromium.org, Jun 27 2017

Issue description

HttpStreamFactoryImpl::Job calls SpdySessionPool::CreateAvailableSessionFromSocket() with |is_secure| Boolean argument based on the URL of the origin.  This value then gets stored in SpdySession::is_secure_ forever, and influences SpdySession's security behavior, but not whether other requests can pool to this SpdySession instance.  This does not make sense: if there is an http and an https request that get served on the same SpdySession, then SpdySession::is_secure_ is determined by the order of the two requests.

(Note that http resources can indeed be served over HTTP/2 if a secure proxy is used.)

It seems like the best solution would be to remove SpdySession::is_secure_ and make SpdySession behave as if is_secure_ was always true.
 

Comment 1 by b...@chromium.org, Jun 27 2017

Cc: rch@chromium.org b...@chromium.org
 Issue 146066  has been merged into this issue.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/306378f56542fe3c18d29c984ce30760d06721d5

commit 306378f56542fe3c18d29c984ce30760d06721d5
Author: Bence Béky <bnc@chromium.org>
Date: Fri Jun 30 12:09:56 2017

Always call CreateAvailableSessionFromSocket with is_secure = true.

Always call CreateAvailableSessionFromSocket with is_secure = true
except for tests.

As described in the bug, it is fairly arbitrary that
SpdySession::is_secure_ depends on the scheme of the first request
URL, but other requests can freely pool to it regardless of their
scheme.  Ultimately every SpdySession should be secure.  This CL is
the first step, and CLs changing tests will follow.

Bug:  737143 
Change-Id: Ifeda3e7ee6c7335804a524384bf283af408ebc3b
Reviewed-on: https://chromium-review.googlesource.com/551055
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483680}
[modify] https://crrev.com/306378f56542fe3c18d29c984ce30760d06721d5/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/306378f56542fe3c18d29c984ce30760d06721d5/net/spdy/chromium/spdy_network_transaction_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2

commit 0ef1556e374c51f1ffc344e88cb1d9e6659b03a2
Author: Bence Béky <bnc@chromium.org>
Date: Fri Jun 30 19:52:52 2017

Remove is_secure from SpdySession.

Remove is_secure_ private member from SpdySession, and is_secure
argument from SpdySession::InitializeWithSocket(), since Chromium does
not support unencrypted HTTP/2 connections.

Remove unused SpdySession::GetServer().

Subclass FuzzedSocketFactory so that SpdySessionFuzzer works with secure
SpdySessions.  AddSSLSocketDataProvider() and CreateSSLClientSocket()
method definitions are mostly copied over from MockClientSocketFactory.

Remove obsolete
AltSvcFrameTest.DoNotProcessAltSvcFrameOnInsecureSession.

Remove now unused CreateInsecureSpdySession() test-only function.

Rename CreateSecureSpdySession() test-only function
to CreateSpdySession().

BUG= 737143 

Change-Id: I785c7e25170423294e4ed71fe7cdfba7d7e00808
Reviewed-on: https://chromium-review.googlesource.com/558146
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483794}
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/http/http_proxy_client_socket_wrapper.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/socket/ssl_client_socket_pool_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/bidirectional_stream_spdy_impl_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_http_stream_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_proxy_client_socket_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session_fuzzer.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session_pool.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session_pool.h
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session_pool_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_stream_unittest.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_test_util_common.cc
[modify] https://crrev.com/0ef1556e374c51f1ffc344e88cb1d9e6659b03a2/net/spdy/chromium/spdy_test_util_common.h

Comment 4 by b...@chromium.org, Jul 5 2017

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c7f648777c791fa4797096e9b4db4451872ca3bc

commit c7f648777c791fa4797096e9b4db4451872ca3bc
Author: Bence Béky <bnc@chromium.org>
Date: Tue Jul 18 18:54:40 2017

Rename CreateSecureSpdySessionWithIpBasedPoolingDisabled.

Rename CreateSecureSpdySessionWithIpBasedPoolingDisabled to
CreateSpdySessionWithIpBasedPoolingDisabled (since every SpdySession is
now secure).

BUG= 737143 

Change-Id: I0882d68eeb6f348da88d1904f04ab2b2748fe007
Reviewed-on: https://chromium-review.googlesource.com/570624
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487540}
[modify] https://crrev.com/c7f648777c791fa4797096e9b4db4451872ca3bc/net/spdy/chromium/spdy_session_pool_unittest.cc
[modify] https://crrev.com/c7f648777c791fa4797096e9b4db4451872ca3bc/net/spdy/chromium/spdy_test_util_common.cc
[modify] https://crrev.com/c7f648777c791fa4797096e9b4db4451872ca3bc/net/spdy/chromium/spdy_test_util_common.h

Sign in to add a comment