Minor cleanup of the QUIC-LB library. Take review comments from late in the process and apply them to earlier classes. PiperOrigin-RevId: 440172029
diff --git a/quiche/quic/load_balancer/load_balancer_config.cc b/quiche/quic/load_balancer/load_balancer_config.cc index 2061abb..2ff5b20 100644 --- a/quiche/quic/load_balancer/load_balancer_config.cc +++ b/quiche/quic/load_balancer/load_balancer_config.cc
@@ -17,7 +17,7 @@ // Validates all non-key parts of the input. bool CommonValidation(const uint8_t config_id, const uint8_t server_id_len, const uint8_t nonce_len) { - if (config_id > 2 || server_id_len == 0 || + if (config_id >= kNumLoadBalancerConfigs || server_id_len == 0 || nonce_len < kLoadBalancerMinNonceLen || nonce_len > kLoadBalancerMaxNonceLen || server_id_len > @@ -54,25 +54,25 @@ // TakePlaintextFrom{Left,Right}() reads the left or right half of 'from' and // expands it into a full encryption block ('to') in accordance with the // internet-draft. -void TakePlaintextFromLeft(uint8_t *to, uint8_t *from, uint8_t total_len, +void TakePlaintextFromLeft(uint8_t *to, uint8_t *from, uint8_t plaintext_len, uint8_t index) { - uint8_t half = total_len / 2; + uint8_t half = plaintext_len / 2; memset(to, 0, kLoadBalancerBlockSize - 1); memcpy(to, from, half); - if (total_len % 2) { + if (plaintext_len % 2) { to[half] = from[half] & 0xf0; } to[kLoadBalancerBlockSize - 1] = index; } -void TakePlaintextFromRight(uint8_t *to, uint8_t *from, uint8_t total_len, +void TakePlaintextFromRight(uint8_t *to, uint8_t *from, uint8_t plaintext_len, uint8_t index) { - const uint8_t half = total_len / 2; + const uint8_t half = plaintext_len / 2; const uint8_t write_point = kLoadBalancerBlockSize - half; - const uint8_t read_point = total_len - half; + const uint8_t read_point = plaintext_len - half; memset((to + 1), 0, kLoadBalancerBlockSize - 1); memcpy(to + write_point, from + read_point, half); - if (total_len % 2) { + if (plaintext_len % 2) { to[write_point - 1] = from[read_point - 1] & 0x0f; } to[0] = index; @@ -81,21 +81,21 @@ // CiphertextXorWith{Left,Right}() takes the relevant end of the ciphertext in // 'from' and XORs it with half of the ConnectionId stored at 'to', in // accordance with the internet-draft. -void CiphertextXorWithLeft(uint8_t *to, uint8_t *from, uint8_t total_len) { - uint8_t half = total_len / 2; +void CiphertextXorWithLeft(uint8_t *to, uint8_t *from, uint8_t plaintext_len) { + uint8_t half = plaintext_len / 2; for (int i = 0; i < half; i++) { *(to + i) ^= *(from + i); } - if (total_len % 2) { + if (plaintext_len % 2) { *(to + half) ^= (*(from + half) & 0xf0); } } -void CiphertextXorWithRight(uint8_t *to, uint8_t *from, uint8_t total_len) { - const uint8_t half = total_len / 2; - const uint8_t write_point = total_len - half; +void CiphertextXorWithRight(uint8_t *to, uint8_t *from, uint8_t plaintext_len) { + const uint8_t half = plaintext_len / 2; + const uint8_t write_point = plaintext_len - half; const uint8_t read_point = kLoadBalancerBlockSize - half; - if (total_len % 2) { + if (plaintext_len % 2) { *(to + write_point - 1) ^= (*(from + read_point - 1) & 0x0f); } for (int i = 0; i < half; i++) { @@ -144,18 +144,18 @@ return false; } if (index % 2) { // Odd indices go from left to right - TakePlaintextFromLeft(plaintext, target, total_len(), index); + TakePlaintextFromLeft(plaintext, target, plaintext_len(), index); } else { - TakePlaintextFromRight(plaintext, target, total_len(), index); + TakePlaintextFromRight(plaintext, target, plaintext_len(), index); } if (!BlockEncrypt(plaintext, ciphertext)) { return false; } // XOR bits over the correct half. if (index % 2) { - CiphertextXorWithRight(target, ciphertext, total_len()); + CiphertextXorWithRight(target, ciphertext, plaintext_len()); } else { - CiphertextXorWithLeft(target, ciphertext, total_len()); + CiphertextXorWithLeft(target, ciphertext, plaintext_len()); } return true; }
diff --git a/quiche/quic/load_balancer/load_balancer_config.h b/quiche/quic/load_balancer/load_balancer_config.h index 8b21a1f..e7d7bcc 100644 --- a/quiche/quic/load_balancer/load_balancer_config.h +++ b/quiche/quic/load_balancer/load_balancer_config.h
@@ -11,12 +11,14 @@ namespace quic { +inline constexpr uint8_t kNumLoadBalancerConfigs = 3; inline constexpr uint8_t kLoadBalancerKeyLen = 16; // Regardless of key length, the AES block size is always 16 Bytes. inline constexpr uint8_t kLoadBalancerBlockSize = 16; // The spec says nonces can be 18 bytes, but 16 lets it be a uint128. inline constexpr uint8_t kLoadBalancerMaxNonceLen = 16; inline constexpr uint8_t kLoadBalancerMinNonceLen = 4; +inline constexpr uint8_t kNumLoadBalancerCryptoPasses = 4; // This the base class for QUIC-LB configuration. It contains configuration // elements usable by both encoders (servers) and decoders (load balancers). @@ -61,7 +63,10 @@ uint8_t config_id() const { return config_id_; } uint8_t server_id_len() const { return server_id_len_; } uint8_t nonce_len() const { return nonce_len_; } - uint8_t total_len() const { return server_id_len_ + nonce_len_; } + // Returns length of all but the first octet. + uint8_t plaintext_len() const { return server_id_len_ + nonce_len_; } + // Returns length of the entire connection ID. + uint8_t total_len() const { return server_id_len_ + nonce_len_ + 1; } bool IsEncrypted() const { return key_.has_value(); } private:
diff --git a/quiche/quic/load_balancer/load_balancer_config_test.cc b/quiche/quic/load_balancer/load_balancer_config_test.cc index 1dad91c..8c58c5d 100644 --- a/quiche/quic/load_balancer/load_balancer_config_test.cc +++ b/quiche/quic/load_balancer/load_balancer_config_test.cc
@@ -67,7 +67,8 @@ EXPECT_EQ(config->config_id(), 0); EXPECT_EQ(config->server_id_len(), 3); EXPECT_EQ(config->nonce_len(), 4); - EXPECT_EQ(config->total_len(), 7); + EXPECT_EQ(config->plaintext_len(), 7); + EXPECT_EQ(config->total_len(), 8); EXPECT_FALSE(config->IsEncrypted()); auto config2 = LoadBalancerConfig::Create(2, 6, 7, absl::string_view(raw_key, 16)); @@ -75,7 +76,8 @@ EXPECT_EQ(config2->config_id(), 2); EXPECT_EQ(config2->server_id_len(), 6); EXPECT_EQ(config2->nonce_len(), 7); - EXPECT_EQ(config2->total_len(), 13); + EXPECT_EQ(config2->plaintext_len(), 13); + EXPECT_EQ(config2->total_len(), 14); EXPECT_TRUE(config2->IsEncrypted()); }
diff --git a/quiche/quic/load_balancer/load_balancer_decoder.cc b/quiche/quic/load_balancer/load_balancer_decoder.cc index ead70d8..4f328e8 100644 --- a/quiche/quic/load_balancer/load_balancer_decoder.cc +++ b/quiche/quic/load_balancer/load_balancer_decoder.cc
@@ -17,7 +17,7 @@ } void LoadBalancerDecoder::DeleteConfig(uint8_t config_id) { - if (config_id > 2) { + if (config_id >= kNumLoadBalancerConfigs) { QUIC_BUG(quic_bug_438896865_01) << "Decoder deleting config with invalid config_id " << static_cast<int>(config_id); @@ -38,7 +38,7 @@ if (!config.has_value()) { return absl::optional<LoadBalancerServerId>(); } - if (connection_id.length() < (1 + config->total_len())) { + if (connection_id.length() < config->total_len()) { // Connection ID wasn't long enough return absl::optional<LoadBalancerServerId>(); } @@ -50,7 +50,7 @@ absl::Span<const uint8_t>(data, config->server_id_len())); } uint8_t result[kQuicMaxConnectionIdWithLengthPrefixLength]; - if (config->total_len() == kLoadBalancerKeyLen) { // single pass + if (config->plaintext_len() == kLoadBalancerKeyLen) { // single pass if (!config->BlockDecrypt(data, result)) { return absl::optional<LoadBalancerServerId>(); } @@ -58,9 +58,9 @@ // Do 3 or 4 passes. Only 3 are necessary if the server_id is short enough // to fit in the first half of the connection ID (the decoder doesn't need // to extract the nonce). - memcpy(result, data, config->total_len()); + memcpy(result, data, config->plaintext_len()); uint8_t end = (config->server_id_len() > config->nonce_len()) ? 1 : 2; - for (uint8_t i = 4; i >= end; i--) { + for (uint8_t i = kNumLoadBalancerCryptoPasses; i >= end; i--) { if (!config->EncryptionPass(result, i)) { return absl::optional<LoadBalancerServerId>(); } @@ -77,7 +77,7 @@ } const uint8_t first_byte = connection_id.data()[0]; uint8_t codepoint = (first_byte >> 6); - if (codepoint <= 2) { + if (codepoint < kNumLoadBalancerConfigs) { return codepoint; } return absl::optional<uint8_t>();
diff --git a/quiche/quic/load_balancer/load_balancer_decoder.h b/quiche/quic/load_balancer/load_balancer_decoder.h index 34c298c..2c326cb 100644 --- a/quiche/quic/load_balancer/load_balancer_decoder.h +++ b/quiche/quic/load_balancer/load_balancer_decoder.h
@@ -17,16 +17,15 @@ // Returns false if the config_id codepoint is already occupied. bool AddConfig(const LoadBalancerConfig& config); - // Remove support for a config + // Remove support for a config. Does nothing if there is no config for + // |config_id|. Does nothing and creates a bug if |config_id| is greater than + // 2. void DeleteConfig(const uint8_t config_id); - // For these "Get" functions, the calling code might not know the length of - // the connection ID. That's OK; if not, just send at least - // kQuicMaxConnectionIdWithLengthPrefixLength bytes in a QuicConnectionId. - - // Extract a server ID from a connection ID. If there is no config for the - // codepoint, the connection ID is too short, or there's a decrypt error, - // returns empty. + // Extract a server ID from |connection_id|. If there is no config for the + // codepoint, |connection_id| is too short, or there's a decrypt error, + // returns empty. Will accept |connection_id| that is longer than necessary + // without error. absl::optional<LoadBalancerServerId> GetServerId( const QuicConnectionId& connection_id) const; @@ -37,7 +36,7 @@ private: // Decoders can support up to 3 configs at once. - absl::optional<LoadBalancerConfig> config_[3]; + absl::optional<LoadBalancerConfig> config_[kNumLoadBalancerConfigs]; }; } // namespace quic
diff --git a/quiche/quic/load_balancer/load_balancer_decoder_test.cc b/quiche/quic/load_balancer/load_balancer_decoder_test.cc index 379bacd..e38fc0c 100644 --- a/quiche/quic/load_balancer/load_balancer_decoder_test.cc +++ b/quiche/quic/load_balancer/load_balancer_decoder_test.cc
@@ -4,7 +4,6 @@ #include "quiche/quic/load_balancer/load_balancer_decoder.h" -#include "absl/base/macros.h" #include "quiche/quic/load_balancer/load_balancer_server_id.h" #include "quiche/quic/platform/api/quic_expect_bug.h" #include "quiche/quic/platform/api/quic_test.h" @@ -51,11 +50,10 @@ 0x5f, 0xee, 0x15, 0xda, 0x27, 0xc4}), MakeServerId(kServerId, 8), }}; - for (uint8_t i = 0; i < ABSL_ARRAYSIZE(test_vectors); i++) { + for (const auto& test : test_vectors) { LoadBalancerDecoder decoder; - EXPECT_TRUE(decoder.AddConfig(test_vectors[i].config)); - EXPECT_EQ(decoder.GetServerId(test_vectors[i].connection_id), - test_vectors[i].server_id); + EXPECT_TRUE(decoder.AddConfig(test.config)); + EXPECT_EQ(decoder.GetServerId(test.connection_id), test.server_id); } } @@ -92,12 +90,10 @@ MakeServerId(kServerId, 9), }, }; - - for (uint8_t i = 0; i < ABSL_ARRAYSIZE(test_vectors); i++) { + for (const auto& test : test_vectors) { LoadBalancerDecoder decoder; - EXPECT_TRUE(decoder.AddConfig(test_vectors[i].config)); - EXPECT_EQ(decoder.GetServerId(test_vectors[i].connection_id), - test_vectors[i].server_id); + EXPECT_TRUE(decoder.AddConfig(test.config)); + EXPECT_EQ(decoder.GetServerId(test.connection_id), test.server_id); } }
diff --git a/quiche/quic/load_balancer/load_balancer_encoder.cc b/quiche/quic/load_balancer/load_balancer_encoder.cc index 852e904..6a4655c 100644 --- a/quiche/quic/load_balancer/load_balancer_encoder.cc +++ b/quiche/quic/load_balancer/load_balancer_encoder.cc
@@ -102,7 +102,7 @@ } QuicConnectionId LoadBalancerEncoder::GenerateConnectionId() { - uint8_t length = (config_.has_value()) ? (config_->total_len() + 1) + uint8_t length = (config_.has_value()) ? config_->total_len() : unroutable_connection_id_len_; uint8_t config_id = config_.has_value() ? (config_->config_id() << 6) : kLoadBalancerUnroutableConfigId; @@ -146,14 +146,14 @@ if (!WriteUint128(nonce_hash, config_->nonce_len(), rewriter)) { return QuicConnectionId(); } - } else if (config_->total_len() == kLoadBalancerBlockSize) { + } else if (config_->plaintext_len() == kLoadBalancerBlockSize) { // Use one encryption pass. if (!config_->BlockEncrypt(block_start, block_start)) { QUIC_LOG(ERROR) << "Block encryption failed"; return QuicConnectionId(); } } else { - for (uint8_t i = 1; i <= 4; i++) { + for (uint8_t i = 1; i <= kNumLoadBalancerCryptoPasses; i++) { if (!config_->EncryptionPass(block_start, i)) { QUIC_LOG(ERROR) << "Block encryption failed"; return QuicConnectionId();
diff --git a/quiche/quic/load_balancer/load_balancer_encoder.h b/quiche/quic/load_balancer/load_balancer_encoder.h index 98e1cdb..2dc33c6 100644 --- a/quiche/quic/load_balancer/load_balancer_encoder.h +++ b/quiche/quic/load_balancer/load_balancer_encoder.h
@@ -54,8 +54,8 @@ public: // Returns a newly created encoder with no active config, if // |unroutable_connection_id_length| is valid. |visitor| specifies an optional - // interface to receive callbacks when connection IDs need to be retired. - // If |encode_length| is true, then the first byte of any generated + // interface to receive callbacks when config status changes. + // If |len_self_encoded| is true, then the first byte of any generated // connection ids will encode the length. Otherwise, those bits will be // random. |unroutable_connection_id_length| specifies the length of // connection IDs to be generated when there is no active config. It must not @@ -78,7 +78,7 @@ // on. void DeleteConfig(); - // Returns the number of additional connections IDs that can be generated with + // Returns the number of additional connection IDs that can be generated with // the current config, or 0 if there is no current config. absl::uint128 num_nonces_left() const { return num_nonces_left_; }
diff --git a/quiche/quic/load_balancer/load_balancer_encoder_test.cc b/quiche/quic/load_balancer/load_balancer_encoder_test.cc index ed6db41..c267a2d 100644 --- a/quiche/quic/load_balancer/load_balancer_encoder_test.cc +++ b/quiche/quic/load_balancer/load_balancer_encoder_test.cc
@@ -155,28 +155,32 @@ EXPECT_EQ(visitor.num_adds(), 1u); } +struct LoadBalancerEncoderTestCase { + LoadBalancerConfig config; + LoadBalancerServerId server_id; + QuicConnectionId connection_id; +}; + TEST_F(LoadBalancerEncoderTest, UnencryptedConnectionIdTestVectors) { - const uint8_t server_id_lens[] = {3, 8}; - const uint8_t nonce_lens[] = {4, 5}; - static_assert(sizeof(server_id_lens) == sizeof(nonce_lens)); - const std::vector<QuicConnectionId> expected_connection_ids{ - QuicConnectionId({0x07, 0xed, 0x79, 0x3a, 0x80, 0x49, 0x71, 0x8a}), - QuicConnectionId({0x4d, 0xed, 0x79, 0x3a, 0x51, 0xd4, 0x9b, 0x8f, 0x5f, - 0xee, 0x15, 0xda, 0x27, 0xc4}), + const struct LoadBalancerEncoderTestCase test_vectors[2] = { + { + *LoadBalancerConfig::CreateUnencrypted(0, 3, 4), + MakeServerId(kServerId, 3), + QuicConnectionId({0x07, 0xed, 0x79, 0x3a, 0x80, 0x49, 0x71, 0x8a}), + }, + { + *LoadBalancerConfig::CreateUnencrypted(1, 8, 5), + MakeServerId(kServerId, 8), + QuicConnectionId({0x4d, 0xed, 0x79, 0x3a, 0x51, 0xd4, 0x9b, 0x8f, + 0x5f, 0xee, 0x15, 0xda, 0x27, 0xc4}), + }, }; - EXPECT_EQ(sizeof(server_id_lens), expected_connection_ids.size()); - for (uint8_t i = 0; i < sizeof(server_id_lens); i++) { - uint8_t config_id = i % 3; + for (const auto &test : test_vectors) { random_.AddNextValues(kNonceHigh, kNonceLow); auto encoder = LoadBalancerEncoder::Create(random_, nullptr, true, 8); - EXPECT_TRUE(encoder.has_value()); - auto config = LoadBalancerConfig::CreateUnencrypted( - config_id, server_id_lens[i], nonce_lens[i]); - EXPECT_TRUE(config.has_value()); - EXPECT_TRUE(encoder->UpdateConfig( - *config, MakeServerId(kServerId, server_id_lens[i]))); + EXPECT_TRUE(encoder->UpdateConfig(test.config, test.server_id)); absl::uint128 nonces_left = encoder->num_nonces_left(); - EXPECT_EQ(encoder->GenerateConnectionId(), expected_connection_ids[i]); + EXPECT_EQ(encoder->GenerateConnectionId(), test.connection_id); EXPECT_EQ(encoder->num_nonces_left(), nonces_left - 1); } } @@ -215,41 +219,48 @@ // (2) server_id_len > nonce_len, so there is a fourth decryption pass // (3) the single-pass encryption case // (4) An even total length. - uint8_t server_id_lens[] = {3, 10, 8, 9}; - uint8_t nonce_lens[] = {4, 5, 8, 9}; - static_assert(sizeof(server_id_lens) == sizeof(nonce_lens)); - const std::vector<QuicConnectionId> expected_connection_ids{ - QuicConnectionId({0x07, 0xfb, 0xfe, 0x05, 0xf7, 0x31, 0xb4, 0x25}), - QuicConnectionId({0x4f, 0x01, 0x09, 0x56, 0xfb, 0x5c, 0x1d, 0x4d, 0x86, - 0xe0, 0x10, 0x18, 0x3e, 0x0b, 0x7d, 0x1e}), - QuicConnectionId({0x90, 0x4d, 0xd2, 0xd0, 0x5a, 0x7b, 0x0d, 0xe9, 0xb2, - 0xb9, 0x90, 0x7a, 0xfb, 0x5e, 0xcf, 0x8c, 0xc3}), - QuicConnectionId({0x12, 0x7a, 0x28, 0x5a, 0x09, 0xf8, 0x52, 0x80, 0xf4, - 0xfd, 0x6a, 0xbb, 0x43, 0x4a, 0x71, 0x59, 0xe4, 0xd3, - 0xeb}), + const LoadBalancerEncoderTestCase test_vectors[4] = { + { + *LoadBalancerConfig::Create(0, 3, 4, kKey), + MakeServerId(kServerId, 3), + QuicConnectionId({0x07, 0xfb, 0xfe, 0x05, 0xf7, 0x31, 0xb4, 0x25}), + }, + { + *LoadBalancerConfig::Create(1, 10, 5, kKey), + MakeServerId(kServerId, 10), + QuicConnectionId({0x4f, 0x01, 0x09, 0x56, 0xfb, 0x5c, 0x1d, 0x4d, + 0x86, 0xe0, 0x10, 0x18, 0x3e, 0x0b, 0x7d, 0x1e}), + }, + { + *LoadBalancerConfig::Create(2, 8, 8, kKey), + MakeServerId(kServerId, 8), + QuicConnectionId({0x90, 0x4d, 0xd2, 0xd0, 0x5a, 0x7b, 0x0d, 0xe9, + 0xb2, 0xb9, 0x90, 0x7a, 0xfb, 0x5e, 0xcf, 0x8c, + 0xc3}), + }, + { + *LoadBalancerConfig::Create(0, 9, 9, kKey), + MakeServerId(kServerId, 9), + QuicConnectionId({0x12, 0x7a, 0x28, 0x5a, 0x09, 0xf8, 0x52, 0x80, + 0xf4, 0xfd, 0x6a, 0xbb, 0x43, 0x4a, 0x71, 0x59, + 0xe4, 0xd3, 0xeb}), + }, }; - EXPECT_EQ(sizeof(server_id_lens), expected_connection_ids.size()); - for (uint8_t i = 0; i < sizeof(server_id_lens); i++) { - uint8_t config_id = i % 3; + for (const auto &test : test_vectors) { auto encoder = LoadBalancerEncoder::Create(random_, nullptr, true, 8); EXPECT_TRUE(encoder.has_value()); - auto config = LoadBalancerConfig::Create(config_id, server_id_lens[i], - nonce_lens[i], kKey); - EXPECT_TRUE(config.has_value()); random_.AddNextValues(kNonceHigh, kNonceLow); - EXPECT_TRUE(encoder->UpdateConfig( - *config, MakeServerId(kServerId, server_id_lens[i]))); - EXPECT_EQ(encoder->GenerateConnectionId(), expected_connection_ids[i]); + EXPECT_TRUE(encoder->UpdateConfig(test.config, test.server_id)); + EXPECT_EQ(encoder->GenerateConnectionId(), test.connection_id); } } TEST_F(LoadBalancerEncoderTest, RunOutOfNonces) { - const uint8_t config_id = 0, server_id_len = 3, nonce_len = 4; + const uint8_t server_id_len = 3; TestLoadBalancerEncoderVisitor visitor; auto encoder = LoadBalancerEncoder::Create(random_, &visitor, true, 8); EXPECT_TRUE(encoder.has_value()); - auto config = - LoadBalancerConfig::Create(config_id, server_id_len, nonce_len, kKey); + auto config = LoadBalancerConfig::Create(0, server_id_len, 4, kKey); EXPECT_TRUE(config.has_value()); EXPECT_TRUE( encoder->UpdateConfig(*config, MakeServerId(kServerId, server_id_len)));
diff --git a/quiche/quic/load_balancer/load_balancer_server_id_map.h b/quiche/quic/load_balancer/load_balancer_server_id_map.h index 02671bb..2e79e66 100644 --- a/quiche/quic/load_balancer/load_balancer_server_id_map.h +++ b/quiche/quic/load_balancer/load_balancer_server_id_map.h
@@ -13,7 +13,8 @@ // This class wraps an absl::flat_hash_map which associates server IDs to an // arbitrary type T. It validates that all server ids are of the same fixed -// length. +// length. This might be used by a load balancer to connect a server ID with a +// pool member data structure. template <typename T> class QUIC_EXPORT_PRIVATE LoadBalancerServerIdMap { public: