Hi Willy,
Hi Manu, On Mon, Feb 05, 2018 at 05:10:05PM +0100, Emmanuel Hocdet wrote: Hi,
Series of patches to support CRC32c checksum to proxy protocol v2 header
(as describe in "doc/proxy-protocol.txt »)
. add hash_crc32c function
. add « crc32c » option to proxy-v2-options
. check crc32c checksum when CRC32C tlv is received.
Thanks. I'm having a few comments here : From a62ca59e175631a0adbb9b8adf1d58492f6bce5a Mon Sep 17 00:00:00 2001
From: Emmanuel Hocdet <[email protected]>
Date: Mon, 5 Feb 2018 15:26:43 +0100
Subject: [PATCH 2/3] MINOR: proxy-v2-options: add crc32c
This patch add option crc32c (PP2_TYPE_CRC32C) to proxy protocol v2.
It compute the checksum of proxy protocol v2 header as describe in
"doc/proxy-protocol.txt".
(...)
diff --git a/include/types/server.h b/include/types/server.h
index 6d0566be2..e68b2036b 100644
--- a/include/types/server.h
+++ b/include/types/server.h
@@ -195,7 +196,7 @@ struct server {
enum obj_type obj_type; /* object type == OBJ_TYPE_SERVER */
enum srv_state next_state, cur_state; /* server state among SRV_ST_* */
enum srv_admin next_admin, cur_admin; /* server maintenance status : SRV_ADMF_* */
- unsigned char pp_opts; /* proxy protocol options (SRV_PP_*) */
+ unsigned int pp_opts; /* proxy protocol options (SRV_PP_*) */
struct server *next;
This is going to punch a 8-bit hole before the struct member. While not dramatic for such a struct, it's a good practice to place a comment before indicating that 8 bits are available there.
I see, enum are packed. I don’t like hole, optinal patch 4 added to reorg a little :). diff --git a/src/connection.c b/src/connection.c
index 412fe3f41..833763e4c 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -990,6 +991,7 @@ static int make_tlv(char *dest, int dest_len, char type, uint16_t length, const
int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connection *remote)
{
const char pp2_signature[] = PP2_SIGNATURE;
+ uint32_t *tlv_crc32c_p = NULL;
int ret = 0;
Spaces instead of tab above, lazy copy-paste ? :-)
Or bad auto-indent mode :-) @@ -1037,6 +1039,14 @@ int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connec
ret = PP2_HDR_LEN_UNSPEC;
}
+ if (srv->pp_opts & SRV_PP_V2_CRC32C) {
+ uint32_t zero_crc32c = 0;
+ if ((buf_len - ret) < sizeof(struct tlv))
+ return 0;
+ tlv_crc32c_p = (uint32_t *)((struct tlv *)&buf[ret])->value;
+ ret += make_tlv(&buf[ret], (buf_len - ret), PP2_TYPE_CRC32C, sizeof(zero_crc32c), (const char *)&zero_crc32c);
+ }
+
if (conn_get_alpn(remote, &value, &value_len)) {
if ((buf_len - ret) < sizeof(struct tlv))
return 0;
@@ -1115,6 +1125,10 @@ int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connec
hdr->len = htons((uint16_t)(ret - PP2_HEADER_LEN));
+ if (tlv_crc32c_p) {
+ *tlv_crc32c_p = htonl(hash_crc32c(buf, ret));
How can you be sure the pointer is 32-bit aligned there ? I don't think we have anything guaranteeing it. Please take a look at write_u32() in net_helper.h, it's provided exactly for this, and does exactly the same thing on architectures supporting unaligned accesses (x86, armv7/8). And in order to avoid any accidental misuse you can then declare tlv_crc32c_p as a void*.
Indeed, I asked myself the question and I forgot to return to it. I like net_helper.h :)
From 4a22d7ca1d4f711b0d777095e5ce793e04a93c78 Mon Sep 17 00:00:00 2001
From: Emmanuel Hocdet <[email protected]>
Date: Mon, 5 Feb 2018 16:23:23 +0100
Subject: [PATCH 3/3] MINOR: accept-proxy: support proxy protocol v2 CRC32c
checksum
When proxy protocol v2 CRC32c tlv is received, check it before accept
connection (as describe in "doc/proxy-protocol.txt").
---
src/connection.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/connection.c b/src/connection.c
index 833763e4c..8fd3209d5 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -612,6 +612,13 @@ int conn_recv_proxy(struct connection *conn, int flag)
tlv_offset += tlv_len + TLV_HEADER_SIZE;
switch (tlv_packet->type) {
+ case PP2_TYPE_CRC32C: {
+ uint32_t n_crc32c = ntohl(*(uint32_t *)tlv_packet->value);
+ *(uint32_t *)tlv_packet->value = 0;
Same here, please see read_u32() and write_u32(). Otherwise looks good. Thanks! Willy
Thank you for taking the time to review.
About PROXYv2, it’s now possible to extract all known TLV from varnish via
++ Manu
|
0001-MINOR-hash-add-new-function-hash_crc32c.patch
