Skip to content

Commit

Permalink
Http2: Delay sending client preface
Browse files Browse the repository at this point in the history 
We need to enable users to abort the connection to a website if, after
performing the TLS handshake, they decide not to continue. Without
sending any data.

The problem with QHttp2Connection was that it would always send
the preface as soon as the object was created, along with the
SETTINGS frame, which may in turn be seen as leaking info from
the user (disregarding that ALPN negotiations already succeeded).

This change requires us to create the first stream before the SETTINGS
gets sent, so this also rearranges all of the QHttp2Connection tests
to create a stream before waiting for the SETTINGS ACK.

Not cherry-picking because this was never considered a problem for
gRPC, but was something we worked to make happen for QNAM.

Change-Id: I84063adf3401a143046514499c2443506c60192c
Reviewed-by: Mate Barany <mate.barany@qt.io>
  • Loading branch information
@Morten242
Morten242 committed Nov 19, 2024
1 parent 4f83c8c commit 09b1ec9
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 26 deletions.
23 changes: 14 additions & 9 deletions src/network/access/qhttp2connection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,17 +817,15 @@ QHttp2Connection *QHttp2Connection::createUpgradedConnection(QIODevice *socket,
auto connection = std::unique_ptr<QHttp2Connection>(new QHttp2Connection(socket));
connection->setH2Configuration(config);
connection->m_connectionType = QHttp2Connection::Type::Client;
connection->m_upgradedConnection = true;
// HTTP2 connection is already established and request was sent, so stream 1
// is already 'active' and is closed for any further outgoing data.
QHttp2Stream *stream = connection->createLocalStreamInternal().unwrap();
Q_ASSERT(stream->streamID() == 1);
stream->setState(QHttp2Stream::State::HalfClosedLocal);
connection->m_upgradedConnection = true;

if (!connection->sendClientPreface()) {
qCWarning(qHttp2ConnectionLog, "[%p] Failed to send client preface", connection.get());
if (!connection->m_prefaceSent) // Preface is sent as part of initial stream-creation.
return nullptr;
}

return connection.release();
}
Expand All @@ -845,11 +843,6 @@ QHttp2Connection *QHttp2Connection::createDirectConnection(QIODevice *socket,
connection->setH2Configuration(config);
connection->m_connectionType = QHttp2Connection::Type::Client;

if (!connection->sendClientPreface()) {
qCWarning(qHttp2ConnectionLog, "[%p] Failed to send client preface", connection.get());
return nullptr;
}

return connection.release();
}

Expand Down Expand Up @@ -909,6 +902,11 @@ QHttp2Stream *QHttp2Connection::createStreamInternal_impl(quint32 streamID)
{
Q_ASSERT(streamID > m_lastIncomingStreamID || streamID >= m_nextStreamID);

if (m_connectionType == Type::Client && !m_prefaceSent && !sendClientPreface()) {
qCWarning(qHttp2ConnectionLog, "[%p] Failed to send client preface", this);
return nullptr;
}

qsizetype numStreams = m_streams.size();
QPointer<QHttp2Stream> &stream = m_streams[streamID];
if (numStreams == m_streams.size()) // stream already existed
Expand Down Expand Up @@ -1094,6 +1092,9 @@ void QHttp2Connection::handleReadyRead()
socket->bytesAvailable());

using namespace Http2;
if (!m_prefaceSent)
return;

while (!m_goingAway || std::any_of(m_streams.cbegin(), m_streams.cend(), streamIsActive)) {
const auto result = frameReader.read(*socket);
if (result != FrameStatus::goodFrame)
Expand Down Expand Up @@ -1287,6 +1288,9 @@ bool QHttp2Connection::sendClientPreface()
qCWarning(qHttp2ConnectionLog, "[%p] Failed to send SETTINGS", this);
return false;
}
m_prefaceSent = true;
if (socket->bytesAvailable()) // We ignore incoming data until preface is sent, so handle it now
QMetaObject::invokeMethod(this, &QHttp2Connection::handleReadyRead, Qt::QueuedConnection);
return true;
}

Expand All @@ -1298,6 +1302,7 @@ bool QHttp2Connection::sendServerPreface()
qCWarning(qHttp2ConnectionLog, "[%p] Failed to send SETTINGS", this);
return false;
}
m_prefaceSent = true;
return true;
}

Expand Down
2 changes: 2 additions & 0 deletions src/network/access/qhttp2connection_p.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,8 @@ public Q_SLOTS:
bool pushPromiseEnabled = false;
quint32 m_lastIncomingStreamID = Http2::connectionStreamID;

bool m_prefaceSent = false;

// Server-side only:
bool m_waitingForClientPreface = false;

Expand Down
38 changes: 21 additions & 17 deletions tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ void tst_QHttp2Connection::testSETTINGSFrame()
config.setSessionReceiveWindowSize(SessionReceiveWindowSize);
auto connection = QHttp2Connection::createDirectConnection(&buffer, config);
Q_UNUSED(connection);
QHttp2Stream *stream = connection->createStream().unwrap();
QVERIFY(stream);
QCOMPARE_GE(buffer.size(), PrefaceLength);

// Preface
Expand Down Expand Up @@ -272,6 +274,8 @@ void tst_QHttp2Connection::testPING()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy serverPingSpy{ serverConnection, &QHttp2Connection::pingFrameRecived };
Expand Down Expand Up @@ -302,13 +306,13 @@ void tst_QHttp2Connection::testRSTServerSide()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
clientStream->sendHEADERS(headers, false);

Expand All @@ -335,13 +339,13 @@ void tst_QHttp2Connection::testRSTClientSide()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
clientStream->sendHEADERS(headers, false);

Expand All @@ -368,13 +372,13 @@ void tst_QHttp2Connection::testRSTReplyOnDATAEND()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
clientStream->sendHEADERS(headers, false);

Expand Down Expand Up @@ -440,13 +444,13 @@ void tst_QHttp2Connection::testBadFrameSize()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
clientStream->sendHEADERS(headers, false);

Expand Down Expand Up @@ -501,13 +505,13 @@ void tst_QHttp2Connection::testDataFrameAfterRSTIncoming()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
clientStream->sendHEADERS(headers, false);

Expand Down Expand Up @@ -543,11 +547,11 @@ void tst_QHttp2Connection::testDataFrameAfterRSTOutgoing()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
Expand Down Expand Up @@ -585,14 +589,14 @@ void tst_QHttp2Connection::connectToServer()
auto connection = makeHttp2Connection(client.get(), {}, Client);
auto serverConnection = makeHttp2Connection(server.get(), {}, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };
QSignalSpy clientIncomingStreamSpy{ connection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QVERIFY(clientStream);
HPack::HttpHeader headers = getRequiredHeaders();
clientStream->sendHEADERS(headers, false);

Expand All @@ -619,14 +623,14 @@ void tst_QHttp2Connection::WINDOW_UPDATE()
config.setStreamReceiveWindowSize(1024); // Small window on server to provoke WINDOW_UPDATE
auto serverConnection = makeHttp2Connection(server.get(), config, Server);

QHttp2Stream *clientStream = connection->createStream().unwrap();
QVERIFY(clientStream);
QVERIFY(waitForSettingsExchange(connection, serverConnection));

QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream };

QHttp2Stream *clientStream = connection->createStream().unwrap();
QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived };
QSignalSpy clientDataReceivedSpy{ clientStream, &QHttp2Stream::dataReceived };
QVERIFY(clientStream);
HPack::HttpHeader expectedRequestHeaders = HPack::HttpHeader{
{ ":authority", "example.com" },
{ ":method", "POST" },
Expand Down Expand Up @@ -747,6 +751,8 @@ void tst_QHttp2Connection::testCONTINUATIONFrame()
auto [client, server] = makeFakeConnectedSockets(); \
auto clientConnection = makeHttp2Connection(client.get(), {}, Client); \
auto serverConnection = makeHttp2Connection(server.get(), {}, Server); \
QHttp2Stream *clientStream = clientConnection->createStream().unwrap(); \
QVERIFY(clientStream); \
QVERIFY(waitForSettingsExchange(clientConnection, serverConnection)); \
\
HPack::Encoder encoder = HPack::Encoder(HPack::FieldLookupTable::DefaultSize, true); \
Expand All @@ -755,8 +761,6 @@ void tst_QHttp2Connection::testCONTINUATIONFrame()
QSignalSpy serverIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; \
QSignalSpy receivedGOAWAYSpy{ clientConnection, &QHttp2Connection::receivedGOAWAY }; \
\
QHttp2Stream *clientStream = clientConnection->createStream().unwrap(); \
QVERIFY(clientStream);

// Send multiple CONTINUATION frames
{
Expand Down

0 comments on commit 09b1ec9

Please sign in to comment.