diff --git a/fizz/client/Actions.h b/fizz/client/Actions.h index 7b7ee491ad..55e273edc4 100644 --- a/fizz/client/Actions.h +++ b/fizz/client/Actions.h @@ -68,6 +68,30 @@ struct NewCachedPsk { CachedPsk psk; }; +/** + * New ech retry config available. This action is emitted whenever an ECH retry + * config is received from the server's encrypted extensions. + */ +struct ECHRetryAvailable { + /** + * The original SNI that was used in the `Connect` event, prior to any + * modifications due to ECH. + * + * It is present here in order to associate this `ECHRetryAvailable` + * action with a connection attempt. + * + * An empty string indicates that no SNI was sent, but the peer responded + * with a set of ECHConfigs regardless + */ + std::string sni; + /** + * A list of ECHConfigs sent by the peer. It is intended to indicate + * the set of acceptable ECHConfigs to use the next time the local + * sender intends to open a TLS connection to `sni`. + */ + std::vector configs; +}; + #define FIZZ_CLIENT_ACTIONS(F, ...) \ F(DeliverAppData, __VA_ARGS__) \ F(WriteToSocket, __VA_ARGS__) \ @@ -79,7 +103,8 @@ struct NewCachedPsk { F(MutateState, __VA_ARGS__) \ F(WaitForData, __VA_ARGS__) \ F(NewCachedPsk, __VA_ARGS__) \ - F(SecretAvailable, __VA_ARGS__) + F(SecretAvailable, __VA_ARGS__) \ + F(ECHRetryAvailable, __VA_ARGS__) FIZZ_DECLARE_VARIANT_TYPE(Action, FIZZ_CLIENT_ACTIONS) diff --git a/fizz/client/AsyncFizzClient-inl.h b/fizz/client/AsyncFizzClient-inl.h index d127b43c78..ed4ee42be7 100644 --- a/fizz/client/AsyncFizzClient-inl.h +++ b/fizz/client/AsyncFizzClient-inl.h @@ -652,6 +652,12 @@ void AsyncFizzClientT::ActionMoveVisitor::operator()( } } +template +void AsyncFizzClientT::ActionMoveVisitor::operator()( + ECHRetryAvailable& echRetry) { + client_.echRetryAvailable(echRetry); +} + template void AsyncFizzClientT::ActionMoveVisitor::operator()( SecretAvailable& secret) { diff --git a/fizz/client/AsyncFizzClient.h b/fizz/client/AsyncFizzClient.h index 246c1de4ca..4916915324 100644 --- a/fizz/client/AsyncFizzClient.h +++ b/fizz/client/AsyncFizzClient.h @@ -21,6 +21,23 @@ namespace fizz { namespace client { +/** + * ECHRetryCallback is used to convey ECHRetryConfigs that are + * received by the client. + */ +class ECHRetryCallback { + public: + virtual ~ECHRetryCallback() = default; + + /** + * retryAvailable may be invoked whenever the client receives a list of + * ECHRetryConfigs from the server. + * + * There is no guarantee that this callback will be invoked on a connection. + */ + virtual void retryAvailable(ECHRetryAvailable retry) = 0; +}; + template class AsyncFizzClientT : public AsyncFizzBase, private folly::AsyncSocket::ConnectCallback { @@ -172,6 +189,16 @@ class AsyncFizzClientT : public AsyncFizzBase, */ folly::Optional> getEchRetryConfigs() const; + void echRetryAvailable(const ECHRetryAvailable& retry) noexcept { + if (echRetryCallback_) { + echRetryCallback_->retryAvailable(retry); + } + } + + void setECHRetryCallback(ECHRetryCallback* cb) { + echRetryCallback_ = cb; + } + protected: ~AsyncFizzClientT() override = default; void writeAppData( @@ -215,6 +242,7 @@ class AsyncFizzClientT : public AsyncFizzBase, void operator()(WaitForData&); void operator()(MutateState&); void operator()(NewCachedPsk&); + void operator()(ECHRetryAvailable&); void operator()(SecretAvailable&); void operator()(EndOfData&); @@ -299,6 +327,8 @@ class AsyncFizzClientT : public AsyncFizzBase, folly::Optional callback_; + ECHRetryCallback* echRetryCallback_{nullptr}; + std::shared_ptr fizzContext_; std::shared_ptr extensions_; diff --git a/fizz/client/ClientProtocol.cpp b/fizz/client/ClientProtocol.cpp index 9253f93707..7b94c66e15 100644 --- a/fizz/client/ClientProtocol.cpp +++ b/fizz/client/ClientProtocol.cpp @@ -1779,11 +1779,15 @@ Actions EventHandler< } folly::Optional> retryConfigs; + folly::Optional echRetryAvailable; if (state.echState().has_value()) { // Check if we were sent retry configs auto serverECH = getExtension(ee.extensions); if (serverECH.has_value()) { retryConfigs = std::move(serverECH->retry_configs); + echRetryAvailable = ECHRetryAvailable{}; + echRetryAvailable->sni = state.echState()->sni.value_or(""); + echRetryAvailable->configs = retryConfigs.value(); } } @@ -1791,7 +1795,8 @@ Actions EventHandler< state.extensions()->onEncryptedExtensions(ee.extensions); } - MutateState mutateState( + Actions ret; + ret.emplace_back(MutateState( [appProto = std::move(appProto), earlyDataType, retryConfigs = std::move(retryConfigs)](State& newState) mutable { @@ -1801,17 +1806,18 @@ Actions EventHandler< if (retryConfigs.has_value()) { newState.echState()->retryConfigs = std::move(retryConfigs); } - }); + })); - if (isPskAccepted(*state.pskType())) { - return actions( - std::move(mutateState), - MutateState(&Transition)); - } else { - return actions( - std::move(mutateState), - MutateState(&Transition)); + if (echRetryAvailable) { + ret.emplace_back(std::move(*echRetryAvailable)); } + + auto nextState = (isPskAccepted(*state.pskType())) + ? &Transition + : &Transition; + ret.emplace_back(MutateState(nextState)); + + return ret; } static folly::Optional< diff --git a/fizz/client/FizzClient-inl.h b/fizz/client/FizzClient-inl.h index fd43ccdf84..e4d534191e 100644 --- a/fizz/client/FizzClient-inl.h +++ b/fizz/client/FizzClient-inl.h @@ -100,6 +100,9 @@ void FizzClient::visitActions( case Action::Type::SecretAvailable_E: this->visitor_(*action.asSecretAvailable()); break; + case Action::Type::ECHRetryAvailable_E: + this->visitor_(*action.asECHRetryAvailable()); + break; } } } diff --git a/fizz/client/test/AsyncFizzClientTest.cpp b/fizz/client/test/AsyncFizzClientTest.cpp index ae10228e4e..9758387cd1 100644 --- a/fizz/client/test/AsyncFizzClientTest.cpp +++ b/fizz/client/test/AsyncFizzClientTest.cpp @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -121,7 +122,8 @@ class AsyncFizzClientTest : public Test { std::shared_ptr clientCert = nullptr, std::shared_ptr serverCert = nullptr, bool pskResumed = false, - ECHMode echMode = ECHMode::NotRequested) { + ECHMode echMode = ECHMode::NotRequested, + folly::Optional expectedECHRetry = {}) { EXPECT_CALL(*machine_, _processSocketData(_, _, _)) .WillOnce(InvokeWithoutArgs([=]() { MutateState addToState([=](State& newState) { @@ -161,8 +163,18 @@ class AsyncFizzClientTest : public Test { }); ReportHandshakeSuccess reportSuccess; reportSuccess.earlyDataAccepted = acceptEarlyData; - return detail::actions( - std::move(addToState), std::move(reportSuccess), WaitForData()); + ECHRetryAvailable echRetryAvailable; + if (expectedECHRetry.has_value()) { + echRetryAvailable = expectedECHRetry.value(); + return detail::actions( + std::move(addToState), + std::move(echRetryAvailable), + std::move(reportSuccess), + WaitForData()); + } else { + return detail::actions( + std::move(addToState), std::move(reportSuccess), WaitForData()); + } })); socketReadCallback_->readBufferAvailable(IOBuf::copyBuffer("ServerData")); } @@ -1217,6 +1229,34 @@ TEST_F(AsyncFizzClientTest, TestTransportEof) { EXPECT_FALSE(client_->good()); } +TEST_F(AsyncFizzClientTest, TestECHRetryAvailableAction) { + connect(); + EXPECT_CALL(handshakeCallback_, _fizzHandshakeSuccess()); + auto callback = MockECHRetryCallback(); + EXPECT_CALL(callback, retryAvailable(_)) + .WillOnce(Invoke([](ECHRetryAvailable gotRetryConfig) { + EXPECT_EQ( + gotRetryConfig.configs.at(0).version, ech::ECHVersion::Draft15); + EXPECT_EQ( + gotRetryConfig.configs.at(0).ech_config_content->moveToFbString(), + "retryConfig"); + })); + client_->setECHRetryCallback(&callback); + ech::ECHConfig retryConfig; + retryConfig.version = ech::ECHVersion::Draft15; + retryConfig.ech_config_content = folly::IOBuf::copyBuffer("retryConfig"); + ECHRetryAvailable expectedECHRetries; + expectedECHRetries.configs.push_back(retryConfig); + fullHandshakeSuccess( + false, + "h2", + nullptr, + nullptr, + false, + ECHMode::Rejected, + expectedECHRetries); +} + TEST_F(AsyncFizzClientTest, TestNewCachedPskActions) { completeHandshake(); client_->setReadCB(&readCallback_); diff --git a/fizz/client/test/BUCK b/fizz/client/test/BUCK index fedd24c76c..7ece8af730 100644 --- a/fizz/client/test/BUCK +++ b/fizz/client/test/BUCK @@ -88,6 +88,7 @@ cpp_unittest( deps = [ ":mocks", "//fizz/client:async_fizz_client", + "//fizz/protocol/ech/test:test_util", "//fizz/protocol/test:mocks", "//folly/io:socket_option_map", "//folly/io/async/test:async_socket_test_lib", diff --git a/fizz/client/test/ClientProtocolTest.cpp b/fizz/client/test/ClientProtocolTest.cpp index ee261093e8..2fd2a044c5 100644 --- a/fizz/client/test/ClientProtocolTest.cpp +++ b/fizz/client/test/ClientProtocolTest.cpp @@ -3712,8 +3712,10 @@ TEST_F(ClientProtocolTest, TestEncryptedExtensionsECHRetryConfigs) { setupExpectingEncryptedExtensions(); state_.requestedExtensions()->push_back( ExtensionType::encrypted_client_hello); + state_.sni() = "www.facebook.com"; state_.echState().emplace(); state_.echState()->status = ECHStatus::Rejected; + state_.echState()->sni = "private.facebook.com"; EXPECT_CALL( *mockHandshakeContext_, appendToTranscript(BufMatches("eeencoding"))); @@ -3726,13 +3728,20 @@ TEST_F(ClientProtocolTest, TestEncryptedExtensionsECHRetryConfigs) { ee.extensions.push_back(encodeExtension(std::move(serverECH))); auto actions = detail::processEvent(state_, std::move(ee)); - expectActions(actions); + expectActions(actions); processStateMutations(actions); EXPECT_EQ(*state_.alpn(), "h2"); EXPECT_TRUE(folly::IOBufEqualTo()( state_.echState()->retryConfigs.value()[0].ech_config_content, folly::IOBuf::copyBuffer("retryconfig"))); EXPECT_EQ(state_.state(), StateEnum::ExpectingCertificate); + auto retry = expectAction(actions); + EXPECT_EQ(retry.sni, "private.facebook.com"); + ASSERT_EQ(retry.configs.size(), 1); + EXPECT_EQ(retry.configs.at(0).version, ech::ECHVersion::Draft15); + EXPECT_TRUE(folly::IOBufEqualTo()( + retry.configs.at(0).ech_config_content, + folly::IOBuf::copyBuffer("retryconfig"))); } TEST_F(ClientProtocolTest, TestCertificateFlow) { diff --git a/fizz/client/test/Mocks.h b/fizz/client/test/Mocks.h index 8451c85969..8148e5ee2e 100644 --- a/fizz/client/test/Mocks.h +++ b/fizz/client/test/Mocks.h @@ -133,6 +133,11 @@ class MockPskCache : public PskCache { MOCK_METHOD(void, removePsk, (const std::string& identity)); }; +class MockECHRetryCallback : public ECHRetryCallback { + public: + MOCK_METHOD(void, retryAvailable, (ECHRetryAvailable retry)); +}; + class MockECHPolicy : public fizz::client::ECHPolicy { public: MOCK_METHOD(