Split KeyExchange into synchronous and asynchronous variants
As part of the implementation of go/leto-II-design, an asynchronous interface was added to the KeyExchange class, to allow an implementation which would make an RPC to a service holding the private key. This fit awkwardly into the existing code, and we intended it as a short-term patch until we could come up with a better separation of concerns.
This CL improves matters by splitting the KeyExchange class into two. The AsyncKeyExchange interface has only an asynchronous interface. SyncKeyExchange has both synchronous and asynchronous interfaces, with the latter implemented in terms of the former. The existing "local" key-exchange classes inherit from SyncKeyExchange. Handshaking code which may or may not need to talk to Leto uses the AsyncKeyExchange uniformly, but depending on whether Leto is enabled or not, the concrete objects being used might be local or remote.
This CL also removes the "Factory" pattern previously used for creating KeyExchange objects. It required a bunch of boilerplate and provided little benefit.
gfe-relnote: no-op refactoring in the area of QUIC handshakes. No functional change intended, not flag-protected.
PiperOrigin-RevId: 238508479
Change-Id: Ib5ca6ae5afbdcb712c7d2f86a4d272ef168b90f3
diff --git a/quic/core/crypto/quic_crypto_server_config.cc b/quic/core/crypto/quic_crypto_server_config.cc
index 54af8cf..ff4cd13 100644
--- a/quic/core/crypto/quic_crypto_server_config.cc
+++ b/quic/core/crypto/quic_crypto_server_config.cc
@@ -74,40 +74,22 @@
DefaultKeyExchangeSource() = default;
~DefaultKeyExchangeSource() override = default;
- std::unique_ptr<KeyExchange> Create(std::string /*server_config_id*/,
- QuicTag type,
- QuicStringPiece private_key) override {
+ std::unique_ptr<AsynchronousKeyExchange> Create(
+ std::string server_config_id,
+ QuicTag type,
+ QuicStringPiece private_key) override {
if (private_key.empty()) {
QUIC_LOG(WARNING) << "Server config contains key exchange method without "
- "corresponding private key: "
- << type;
+ "corresponding private key of type "
+ << QuicTagToString(type);
return nullptr;
}
- std::unique_ptr<KeyExchange> ka;
- switch (type) {
- case kC255:
- ka = Curve25519KeyExchange::New(private_key);
- if (!ka) {
- QUIC_LOG(WARNING) << "Server config contained an invalid curve25519"
- " private key.";
- return nullptr;
- }
- break;
- case kP256:
- ka = P256KeyExchange::New(private_key);
- if (!ka) {
- QUIC_LOG(WARNING) << "Server config contained an invalid P-256"
- " private key.";
- return nullptr;
- }
- break;
- default:
- QUIC_LOG(WARNING)
- << "Server config message contains unknown key exchange "
- "method: "
- << type;
- return nullptr;
+ std::unique_ptr<SynchronousKeyExchange> ka =
+ CreateLocalSynchronousKeyExchange(type, private_key);
+ if (!ka) {
+ QUIC_LOG(WARNING) << "Failed to create key exchange method of type "
+ << QuicTagToString(type);
}
return ka;
}
@@ -250,8 +232,8 @@
const std::string curve25519_private_key =
Curve25519KeyExchange::NewPrivateKey(rand);
- std::unique_ptr<Curve25519KeyExchange> curve25519(
- Curve25519KeyExchange::New(curve25519_private_key));
+ std::unique_ptr<Curve25519KeyExchange> curve25519 =
+ Curve25519KeyExchange::New(curve25519_private_key);
QuicStringPiece curve25519_public_value = curve25519->public_value();
std::string encoded_public_values;
@@ -666,12 +648,12 @@
};
class QuicCryptoServerConfig::ProcessClientHelloAfterGetProofCallback
- : public KeyExchange::Callback {
+ : public AsynchronousKeyExchange::Callback {
public:
ProcessClientHelloAfterGetProofCallback(
const QuicCryptoServerConfig* config,
std::unique_ptr<ProofSource::Details> proof_source_details,
- const KeyExchange::Factory& key_exchange_factory,
+ QuicTag key_exchange_type,
std::unique_ptr<CryptoHandshakeMessage> out,
QuicStringPiece public_value,
QuicReferenceCountedPointer<ValidateClientHelloResultCallback::Result>
@@ -689,7 +671,7 @@
std::unique_ptr<ProcessClientHelloResultCallback> done_cb)
: config_(config),
proof_source_details_(std::move(proof_source_details)),
- key_exchange_factory_(key_exchange_factory),
+ key_exchange_type_(key_exchange_type),
out_(std::move(out)),
public_value_(public_value),
validate_chlo_result_(std::move(validate_chlo_result)),
@@ -707,7 +689,7 @@
void Run(bool ok) override {
config_->ProcessClientHelloAfterCalculateSharedKeys(
- !ok, std::move(proof_source_details_), key_exchange_factory_,
+ !ok, std::move(proof_source_details_), key_exchange_type_,
std::move(out_), public_value_, *validate_chlo_result_, connection_id_,
client_address_, version_, supported_versions_, clock_, rand_, params_,
signed_config_, requested_config_, primary_config_,
@@ -717,7 +699,7 @@
private:
const QuicCryptoServerConfig* config_;
std::unique_ptr<ProofSource::Details> proof_source_details_;
- const KeyExchange::Factory& key_exchange_factory_;
+ QuicTag key_exchange_type_;
std::unique_ptr<CryptoHandshakeMessage> out_;
std::string public_value_;
QuicReferenceCountedPointer<ValidateClientHelloResultCallback::Result>
@@ -955,24 +937,24 @@
return;
}
- const KeyExchange* key_exchange =
+ const AsynchronousKeyExchange* key_exchange =
requested_config->key_exchanges[key_exchange_index].get();
// TODO(rch): Would it be better to implement a move operator and just
// std::move(helper) instead of done_cb?
helper.DetachCallback();
auto cb = QuicMakeUnique<ProcessClientHelloAfterGetProofCallback>(
- this, std::move(proof_source_details), key_exchange->GetFactory(),
+ this, std::move(proof_source_details), key_exchange->type(),
std::move(out), public_value, validate_chlo_result, connection_id,
client_address, version, supported_versions, clock, rand, params,
signed_config, requested_config, primary_config, std::move(done_cb));
- key_exchange->CalculateSharedKey(
+ key_exchange->CalculateSharedKeyAsync(
public_value, ¶ms->initial_premaster_secret, std::move(cb));
}
void QuicCryptoServerConfig::ProcessClientHelloAfterCalculateSharedKeys(
bool found_error,
std::unique_ptr<ProofSource::Details> proof_source_details,
- const KeyExchange::Factory& key_exchange_factory,
+ QuicTag key_exchange_type,
std::unique_ptr<CryptoHandshakeMessage> out,
QuicStringPiece public_value,
const ValidateClientHelloResultCallback::Result& validate_chlo_result,
@@ -996,7 +978,8 @@
ProcessClientHelloHelper helper(&done_cb);
if (found_error) {
- helper.Fail(QUIC_INVALID_CRYPTO_MESSAGE_PARAMETER, "Invalid public value");
+ helper.Fail(QUIC_INVALID_CRYPTO_MESSAGE_PARAMETER,
+ "Failed to calculate shared key");
return;
}
@@ -1104,11 +1087,18 @@
}
std::string forward_secure_public_value;
- std::unique_ptr<KeyExchange> forward_secure_key_exchange =
- key_exchange_factory.Create(rand);
+ std::unique_ptr<SynchronousKeyExchange> forward_secure_key_exchange =
+ CreateLocalSynchronousKeyExchange(key_exchange_type, rand);
+ if (!forward_secure_key_exchange) {
+ QUIC_DLOG(WARNING) << "Failed to create keypair";
+ helper.Fail(QUIC_INVALID_CRYPTO_MESSAGE_PARAMETER,
+ "Failed to create keypair");
+ return;
+ }
+
forward_secure_public_value =
std::string(forward_secure_key_exchange->public_value());
- if (!forward_secure_key_exchange->CalculateSharedKey(
+ if (!forward_secure_key_exchange->CalculateSharedKeySync(
public_value, ¶ms->forward_secure_premaster_secret)) {
helper.Fail(QUIC_INVALID_CRYPTO_MESSAGE_PARAMETER, "Invalid public value");
return;
@@ -1716,8 +1706,8 @@
QuicReferenceCountedPointer<QuicCryptoServerConfig::Config>
QuicCryptoServerConfig::ParseConfigProtobuf(
const std::unique_ptr<QuicServerConfigProtobuf>& protobuf) {
- std::unique_ptr<CryptoHandshakeMessage> msg(
- CryptoFramer::ParseMessage(protobuf->config()));
+ std::unique_ptr<CryptoHandshakeMessage> msg =
+ CryptoFramer::ParseMessage(protobuf->config());
if (msg->tag() != kSCFG) {
QUIC_LOG(WARNING) << "Server config message has tag " << msg->tag()
@@ -1808,13 +1798,13 @@
}
}
- std::unique_ptr<KeyExchange> ka =
+ std::unique_ptr<AsynchronousKeyExchange> ka =
key_exchange_source_->Create(config->id, tag, private_key);
if (!ka) {
return nullptr;
}
for (const auto& key_exchange : config->key_exchanges) {
- if (key_exchange->GetFactory().tag() == tag) {
+ if (key_exchange->type() == tag) {
QUIC_LOG(WARNING) << "Duplicate key exchange in config: " << tag;
return nullptr;
}
