more tests and some fixes

Signed-off-by: Greg Greenway <[email protected]>

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}];
 }

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<quic::QuicConnectionId>
 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<const uint8_t>(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<KeyAndVersion> 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<QuicLbConnectionIdGenerator::ThreadLocalData>::makeUnique(
+      context.serverFactoryContext().threadLocal());
+
+  // using InitializeCb = std::function<std::shared_ptr<T>(Event::Dispatcher & dispatcher)>;
+  ret->tls_slot_->set(
+      [=](Event::Dispatcher&) -> std::shared_ptr<QuicLbConnectionIdGenerator::ThreadLocalData> {
+        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<QuicLbConnectionIdGenerator::ThreadLocalData> 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<QuicLbConnectionIdGenerator::ThreadLocalData>::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<std::shared_ptr<T>(Event::Dispatcher & dispatcher)>;
-  ret->tls_slot_->set(
-      [=](Event::Dispatcher&) -> std::shared_ptr<QuicLbConnectionIdGenerator::ThreadLocalData> {
-        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<QuicLbConnectionIdGenerator::ThreadLocalData> 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) {

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<quic::QuicConnectionId>
   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_;

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",
     ],
 )

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"));

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<QuicLbConnectionIdGenerator> createTypedIdGenerator(Factory& factory,
+                                                                    uint32_t worker_index = 0) {
+  QuicConnectionIdGeneratorPtr generator = factory.createQuicConnectionIdGenerator(worker_index);
+  return std::unique_ptr<QuicLbConnectionIdGenerator>(
+      static_cast<QuicLbConnectionIdGenerator*>(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<Server::Configuration::MockFactoryContext> factory_context;
+
+  // Missing static secret.
+  cfg.mutable_encryption_parameters()->set_name("test");
+  absl::StatusOr<std::unique_ptr<Factory>> 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<Server::Configuration::MockFactoryContext> 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<std::unique_ptr<Factory>> 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<const uint8_t*>(new_cid->data()), sizeof(expected)),
+            absl::Span(expected, sizeof(expected)));
 }
 
 } // namespace QuicLb

test/mocks/server/transport_socket_factory_context.cc

@@ -11,8 +11,8 @@ namespace Configuration {
 
 using ::testing::ReturnRef;
 
-MockTransportSocketFactoryContext::MockTransportSocketFactoryContext()
-    : secret_manager_(std::make_unique<Secret::SecretManagerImpl>(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<Secret::SecretManagerImpl>(config_tracker_);
+}
+
 } // namespace Configuration
 } // namespace Server
 } // namespace Envoy

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<StatelessMockServerFactoryContext> server_context_;
   testing::NiceMock<Upstream::MockClusterManager> cluster_manager_;
   testing::NiceMock<Api::MockApi> api_;