diff --git a/api/envoy/extensions/quic/connection_id_generator/quic_lb/v3/quic_lb.proto b/api/envoy/extensions/quic/connection_id_generator/quic_lb/v3/quic_lb.proto index 98259a8ad9d8..c2a637e276b3 100644 --- a/api/envoy/extensions/quic/connection_id_generator/quic_lb/v3/quic_lb.proto +++ b/api/envoy/extensions/quic/connection_id_generator/quic_lb/v3/quic_lb.proto @@ -56,7 +56,7 @@ message Config { // // "encryption_key" must contain the 16 byte encryption key. // - // "configuration_version" must contain a 1 byte unsigned integer of value less than 8. + // "configuration_version" must contain a 1 byte unsigned integer of value less than 7. // See https://datatracker.ietf.org/doc/html/draft-ietf-quic-load-balancers#name-config-rotation. transport_sockets.tls.v3.SdsSecretConfig encryption_parameters = 5 [(validate.rules).message = {required: true}]; } diff --git a/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.cc b/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.cc index 70eb14ccfdc8..407ccfdd50f6 100644 --- a/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.cc +++ b/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.cc @@ -61,7 +61,7 @@ uint8_t QuicLbConnectionIdGenerator::ConnectionIdLength(uint8_t first_byte) cons absl::optional QuicLbConnectionIdGenerator::appendRoutingId(quic::QuicConnectionId& new_connection_id, - const quic::QuicConnectionId& old_connection_id) { + const quic::QuicConnectionId&) { uint8_t buffer[quic::kQuicMaxConnectionIdWithLengthPrefixLength]; const uint16_t new_length = new_connection_id.length() + sizeof(WorkerRoutingIdValue); @@ -70,11 +70,6 @@ QuicLbConnectionIdGenerator::appendRoutingId(quic::QuicConnectionId& new_connect return {}; } - if (old_connection_id.length() < sizeof(WorkerRoutingIdValue)) { - IS_ENVOY_BUG("Unexpected very short connection id"); - return {}; - } - memcpy(buffer, new_connection_id.data(), new_connection_id.length()); WorkerRoutingIdValue* routing_info_destination = @@ -95,6 +90,8 @@ QuicLbConnectionIdGenerator::appendRoutingId(quic::QuicConnectionId& new_connect return quic::QuicConnectionId(absl::Span(buffer, new_length)); } +bool QuicLbConnectionIdGenerator::ready() const { return tls_slot_->encoder_.IsEncoding(); } + QuicLbConnectionIdGenerator::ThreadLocalData::ThreadLocalData( const envoy::extensions::quic::connection_id_generator::quic_lb::v3::Config& config, absl::string_view server_id) @@ -190,9 +187,9 @@ absl::StatusOr getAndValidateKeyAndVersion( version_or_result.value().size())); } const uint8_t version = version_or_result.value().data()[0]; - if (version > quic::kNumLoadBalancerConfigs) { + if (version >= quic::kNumLoadBalancerConfigs) { return absl::InvalidArgumentError( - fmt::format("'configuration_version' was {}, but must be less than or equal to {}", version, + fmt::format("'configuration_version' was {}, but must be less than {}", version, quic::kNumLoadBalancerConfigs)); } @@ -241,6 +238,17 @@ Factory::create(const envoy::extensions::quic::connection_id_generator::quic_lb: QuicLbConnectionIdGenerator::connectionIdLengthAddition(); } + ret->tls_slot_ = ThreadLocal::TypedSlot::makeUnique( + context.serverFactoryContext().threadLocal()); + + // using InitializeCb = std::function(Event::Dispatcher & dispatcher)>; + ret->tls_slot_->set( + [=](Event::Dispatcher&) -> std::shared_ptr { + auto result = QuicLbConnectionIdGenerator::ThreadLocalData::create(config, server_id); + ASSERT(result.status().ok()); // Configuration was validated above. + return result.value(); + }); + ret->secrets_provider_ = secretsProvider(config.encryption_parameters(), context.getTransportSocketFactoryContext(), context.initManager()); @@ -255,50 +263,41 @@ Factory::create(const envoy::extensions::quic::connection_id_generator::quic_lb: ret->secrets_provider_update_callback_handle_ = ret->secrets_provider_->addUpdateCallback( [&factory = *ret, &api = context.serverFactoryContext().api()]() -> absl::Status { - const envoy::extensions::transport_sockets::tls::v3::GenericSecret* secret = - factory.secrets_provider_->secret(); - if (secret == nullptr) { - return absl::NotFoundError("secret update callback called with empty secret"); - } - - auto data_or_result = getAndValidateKeyAndVersion(*secret, api); - - // `getAndValidateKeyAndVersion` was called via `addValidationCallback` already, so this - // should not be able to fail. - ENVOY_BUG(data_or_result.ok(), "quic_lb unexpected error in updating configuration; old " - "configuration will still be used"); - RETURN_IF_NOT_OK_REF(data_or_result.status()); - - factory.tls_slot_->runOnAllThreads( - [data = - data_or_result.value()](OptRef obj) { - ASSERT(obj.has_value(), - "Guaranteed if `set()` was previously called on the tls slot"); - - auto result = - obj->updateKeyAndVersion(data.encryption_key_, data.configuration_version_); - - // Because all parameters were validated earlier, it should not be possible for this - // to fail. - ENVOY_BUG(result.ok(), "quic_lb unexpected error in updating configuration; old " - "configuration will still be used"); - }); - - return absl::OkStatus(); + return factory.updateSecret(api); }); - ret->tls_slot_ = ThreadLocal::TypedSlot::makeUnique( - context.serverFactoryContext().threadLocal()); + if (ret->secrets_provider_->secret()) { + auto status = ret->updateSecret(context.serverFactoryContext().api()); + RETURN_IF_NOT_OK_REF(status); + } - // using InitializeCb = std::function(Event::Dispatcher & dispatcher)>; - ret->tls_slot_->set( - [=](Event::Dispatcher&) -> std::shared_ptr { - auto result = QuicLbConnectionIdGenerator::ThreadLocalData::create(config, server_id); - ASSERT(result.status().ok()); // Configuration was validated above. - return result.value(); + return ret; +} + +absl::Status Factory::updateSecret(Api::Api& api) { + const envoy::extensions::transport_sockets::tls::v3::GenericSecret* secret = + secrets_provider_->secret(); + if (secret == nullptr) { + return absl::NotFoundError("secret update callback called with empty secret"); + } + + auto data_or_result = getAndValidateKeyAndVersion(*secret, api); + + RETURN_IF_NOT_OK_REF(data_or_result.status()); + + tls_slot_->runOnAllThreads( + [data = data_or_result.value()](OptRef obj) { + ASSERT(obj.has_value(), "Guaranteed if `set()` was previously called on the tls slot"); + + auto result = obj->updateKeyAndVersion(data.encryption_key_, data.configuration_version_); + + // Because all parameters were validated earlier, it should not be possible for this + // to fail. + ENVOY_BUG(result.ok(), "quic_lb unexpected error in updating configuration; old " + "configuration will still be used"); }); - return ret; + return absl::OkStatus(); } QuicConnectionIdGeneratorPtr Factory::createQuicConnectionIdGenerator(uint32_t worker_id) { diff --git a/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.h b/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.h index 77187ff8b94e..7c09d6c1c396 100644 --- a/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.h +++ b/source/extensions/quic/connection_id_generator/quic_lb/quic_lb.h @@ -55,6 +55,9 @@ class QuicLbConnectionIdGenerator : public quic::ConnectionIdGeneratorInterface // creates. static uint8_t connectionIdLengthAddition() { return sizeof(WorkerRoutingIdValue); } + // Returns true if all configuration and secrets are valid and configured. + bool ready() const; + absl::optional appendRoutingId(quic::QuicConnectionId& new_connection_id, const quic::QuicConnectionId& old_connection_id); @@ -79,6 +82,7 @@ class Factory : public EnvoyQuicConnectionIdGeneratorFactory { private: Factory(const envoy::extensions::quic::connection_id_generator::quic_lb::v3::Config& config); + absl::Status updateSecret(Api::Api& api); const envoy::extensions::quic::connection_id_generator::quic_lb::v3::Config config_; Secret::GenericSecretConfigProviderSharedPtr secrets_provider_; diff --git a/test/extensions/quic/connection_id_generator/quic_lb/BUILD b/test/extensions/quic/connection_id_generator/quic_lb/BUILD index 49a6a32737cb..86ee15980a65 100644 --- a/test/extensions/quic/connection_id_generator/quic_lb/BUILD +++ b/test/extensions/quic/connection_id_generator/quic_lb/BUILD @@ -20,6 +20,7 @@ envoy_extension_cc_test( tags = ["nofips"], deps = [ "//source/extensions/quic/connection_id_generator/quic_lb:quic_lb_lib", + "//test/mocks/server:factory_context_mocks", "@com_github_google_quiche//:quic_test_tools_test_utils_lib", ], ) diff --git a/test/extensions/quic/connection_id_generator/quic_lb/integration_test.cc b/test/extensions/quic/connection_id_generator/quic_lb/integration_test.cc index 926cd69bbe04..beeb17b0eb55 100644 --- a/test/extensions/quic/connection_id_generator/quic_lb/integration_test.cc +++ b/test/extensions/quic/connection_id_generator/quic_lb/integration_test.cc @@ -40,7 +40,7 @@ class QuicLbIntegrationTest : public Envoy::Quic::QuicHttpMultiAddressesIntegrat envoy::extensions::quic::connection_id_generator::quic_lb::v3::Config config; *config.mutable_server_id()->mutable_inline_string() = "myid"; config.set_nonce_length_bytes(12); - auto* sds = config.mutable_encryption_parmeters(); + auto* sds = config.mutable_encryption_parameters(); sds->set_name("quic_lb"); sds->mutable_sds_config()->mutable_path_config_source()->set_path( TestEnvironment::substitute("{{ test_tmpdir }}/quic_lb.yaml")); diff --git a/test/extensions/quic/connection_id_generator/quic_lb/quic_lb_test.cc b/test/extensions/quic/connection_id_generator/quic_lb/quic_lb_test.cc index fb4e07c50838..ee3d5c788bbe 100644 --- a/test/extensions/quic/connection_id_generator/quic_lb/quic_lb_test.cc +++ b/test/extensions/quic/connection_id_generator/quic_lb/quic_lb_test.cc @@ -1,5 +1,7 @@ #include "source/extensions/quic/connection_id_generator/quic_lb/quic_lb.h" +#include "test/mocks/server/factory_context.h" + #include "gmock/gmock.h" #include "gtest/gtest.h" #include "quiche/quic/platform/api/quic_test.h" @@ -11,15 +13,155 @@ namespace Extensions { namespace ConnectionIdGenerator { namespace QuicLb { -TEST(QuicLbTest, testing) { +namespace { +std::unique_ptr createTypedIdGenerator(Factory& factory, + uint32_t worker_index = 0) { + QuicConnectionIdGeneratorPtr generator = factory.createQuicConnectionIdGenerator(worker_index); + return std::unique_ptr( + static_cast(generator.release())); +} +} // namespace + +TEST(QuicLbTest, InvalidConfig) { uint8_t id_data[] = {0xab, 0xcd, 0xef, 0x12, 0x34, 0x56}; envoy::extensions::quic::connection_id_generator::quic_lb::v3::Config cfg; cfg.mutable_server_id()->set_inline_bytes(id_data, sizeof(id_data)); - // auto generator = QuicLbConnectionIdGenerator::create(cfg).value(); - // quic::QuicConnectionId old_id("123456789", 9); - // auto id = generator->GenerateNextConnectionId(old_id); + cfg.set_nonce_length_bytes(10); + + testing::NiceMock factory_context; + + // Missing static secret. + cfg.mutable_encryption_parameters()->set_name("test"); + absl::StatusOr> factory_or_status = + Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), "invalid encryption_parameters config"); + + // Missing encryption key. + envoy::extensions::transport_sockets::tls::v3::Secret encryption_parameters; + encryption_parameters.set_name("test"); + encryption_parameters.mutable_generic_secret(); + auto status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), "Missing 'encryption_key'"); + + // Invalid key length. + auto& secrets = *encryption_parameters.mutable_generic_secret()->mutable_secrets(); + envoy::config::core::v3::DataSource key; + key.set_inline_string("bad"); + secrets["encryption_key"] = key; + factory_context.transport_socket_factory_context_.resetSecretManager(); + status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), + "'encryption_key' length was 3, but it must be length 16"); + + // Missing version. + key.set_inline_string("0123456789abcdef"); + secrets["encryption_key"] = key; + factory_context.transport_socket_factory_context_.resetSecretManager(); + status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), "Missing 'configuration_version'"); + + // Bad version length. + envoy::config::core::v3::DataSource version; + std::string version_bytes; + version_bytes.resize(2); + version.set_inline_bytes(version_bytes); + secrets["configuration_version"] = version; + factory_context.transport_socket_factory_context_.resetSecretManager(); + status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), + "'configuration_version' length was 2, but it must be length 1 byte"); + + // Bad version value. + version_bytes.resize(1); + version_bytes.data()[0] = 7; + version.set_inline_bytes(version_bytes); + secrets["configuration_version"] = version; + factory_context.transport_socket_factory_context_.resetSecretManager(); + status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), + "'configuration_version' was 7, but must be less than 7"); + + // Valid config. + version_bytes.data()[0] = 6; + version.set_inline_bytes(version_bytes); + secrets["configuration_version"] = version; + factory_context.transport_socket_factory_context_.resetSecretManager(); + status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_TRUE(factory_or_status.ok()); + + // Invalid concurrency. + EXPECT_CALL(factory_context.server_factory_context_.options_, concurrency()) + .WillOnce(testing::Return(257)); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_EQ(factory_or_status.status().message(), + "envoy.quic.connection_id_generator.quic_lb cannot be used " + "with a concurrency greater than 256"); +} + +// Validate that the server ID is present in plaintext when `unsafe_unencrypted_testing_mode` +// is enabled. +TEST(QuicLbTest, Unencrypted) { + uint8_t id_data[] = {0xab, 0xcd, 0xef, 0x12, 0x34, 0x56}; + envoy::extensions::quic::connection_id_generator::quic_lb::v3::Config cfg; + cfg.set_unsafe_unencrypted_testing_mode(true); + cfg.mutable_server_id()->set_inline_bytes(id_data, sizeof(id_data)); + cfg.set_nonce_length_bytes(10); + cfg.set_self_encode_length(true); // Results in deterministic output. + cfg.mutable_encryption_parameters()->set_name("test"); + + testing::NiceMock factory_context; + + envoy::extensions::transport_sockets::tls::v3::Secret encryption_parameters; + encryption_parameters.set_name("test"); + auto& secrets = *encryption_parameters.mutable_generic_secret()->mutable_secrets(); + envoy::config::core::v3::DataSource key; + key.set_inline_string("0123456789abcdef"); + secrets["encryption_key"] = key; + envoy::config::core::v3::DataSource version; + std::string version_bytes; + version_bytes.resize(1); + version_bytes.data()[0] = 0; + version.set_inline_bytes(version_bytes); + secrets["configuration_version"] = version; + factory_context.transport_socket_factory_context_.resetSecretManager(); + auto status = factory_context.transport_socket_factory_context_.secretManager().addStaticSecret( + encryption_parameters); + absl::StatusOr> factory_or_status = + Factory::create(cfg, factory_context); + EXPECT_TRUE(status.ok()); + factory_or_status = Factory::create(cfg, factory_context); + EXPECT_TRUE(factory_or_status.ok()); + auto generator = createTypedIdGenerator(*factory_or_status.value()); + EXPECT_TRUE(generator->ready()); + auto new_cid = generator->GenerateNextConnectionId(quic::QuicConnectionId{}); + EXPECT_TRUE(new_cid.has_value()); + uint8_t expected[1 + sizeof(id_data)]; + expected[0] = 16; // Configured length of encoded portion of CID. Zero version means the high bits + // are all unset. + memcpy(expected + 1, id_data, sizeof(id_data)); + ASSERT_GT(new_cid->length(), sizeof(expected)); - // EXPECT_EQ(id, old_id); + // First bytes should be the version followed by unencrypted server ID. + EXPECT_EQ(absl::Span(reinterpret_cast(new_cid->data()), sizeof(expected)), + absl::Span(expected, sizeof(expected))); } } // namespace QuicLb diff --git a/test/mocks/server/transport_socket_factory_context.cc b/test/mocks/server/transport_socket_factory_context.cc index 09859ef97131..dc4ab7be3239 100644 --- a/test/mocks/server/transport_socket_factory_context.cc +++ b/test/mocks/server/transport_socket_factory_context.cc @@ -11,8 +11,8 @@ namespace Configuration { using ::testing::ReturnRef; -MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() - : secret_manager_(std::make_unique(config_tracker_)) { +MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() { + resetSecretManager(); ON_CALL(*this, serverFactoryContext()).WillByDefault(ReturnRef(server_context_)); ON_CALL(*this, clusterManager()).WillByDefault(ReturnRef(cluster_manager_)); ON_CALL(*this, messageValidationVisitor()) @@ -30,6 +30,11 @@ MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() MockTransportSocketFactoryContext::~MockTransportSocketFactoryContext() = default; +void MockTransportSocketFactoryContext::resetSecretManager() { + config_tracker_.config_tracker_callbacks_.clear(); + secret_manager_ = std::make_unique(config_tracker_); +} + } // namespace Configuration } // namespace Server } // namespace Envoy diff --git a/test/mocks/server/transport_socket_factory_context.h b/test/mocks/server/transport_socket_factory_context.h index 828bf47b44d9..badf8ce33a82 100644 --- a/test/mocks/server/transport_socket_factory_context.h +++ b/test/mocks/server/transport_socket_factory_context.h @@ -32,6 +32,10 @@ class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { MOCK_METHOD(Stats::Scope&, statsScope, ()); MOCK_METHOD(Init::Manager&, initManager, ()); + // Useful for when a test needs to change a static secret, which normally cannot be changed or + // re-added after it is set. This creates a new secret manager and deletes the old one. + void resetSecretManager(); + testing::NiceMock server_context_; testing::NiceMock cluster_manager_; testing::NiceMock api_;