| Message ID | 20171112172237.8285-1-steffan@karger.me |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2] Use P_DATA_V2 for server->client packets too | expand |
Hi, On 13/11/17 01:22, Steffan Karger wrote: > P_DATA_V2 introduced the peer-id. This allows clients to float, but as a > side-effect 32-bit aligns the encrypted data. That alignment improves > performance particularly on cheaper/older CPUs. So although servers don't > actually have a peer-id, still use the V2 packet format (with a zero-id) > for server->client traffic too. > > Signed-off-by: Steffan Karger <steffan@karger.me> Code looks good and this time I trust the tests performed by Steffan :) Acked-by: Antonio Quartulli <a@unstable.cc>
Hi, as just discussed on IRC, I'm not fully happy with this, and want a v3... On Sun, Nov 12, 2017 at 06:22:37PM +0100, Steffan Karger wrote: [..] > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index 5947a31f..16a4101f 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -366,6 +366,7 @@ prepare_push_reply(struct context *c, struct gc_arena *gc, > push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", > tls_multi->peer_id); > } > + tls_multi->use_peer_id = true; > } While this *works* today, it relies on the fact that only clients that can do IV_PROTO=2 will ever send IV_PROTO=<anything>. But it still creates weird code (more context): /* Send peer-id if client supports it */ optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL; if (optstr) { int proto = 0; int r = sscanf(optstr, "IV_PROTO=%d", &proto); if ((r == 1) && (proto >= 2)) { push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", tls_multi->peer_id); } + tls_multi->use_peer_id = true; } where we set tls_multi->use_peer_id = true for any client that sends IV_PROTO=<anything>, but the rules for "push a peer ID to the client" is much stricter, requiring it to be IV_PROTO=<num> with num>=2 - so the "tls_multi->use_peer_id = true;" should go inside that block. gert
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 1b7455bb..a868a8ff 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -496,7 +496,7 @@ encrypt_sign(struct context *c, bool comp_frag) /* If using P_DATA_V2, prepend the 1-byte opcode and 3-byte peer-id to the * packet before openvpn_encrypt(), so we can authenticate the opcode too. */ - if (c->c2.buf.len > 0 && !c->c2.tls_multi->opt.server && c->c2.tls_multi->use_peer_id) + if (c->c2.buf.len > 0 && c->c2.tls_multi->use_peer_id) { tls_prepend_opcode_v2(c->c2.tls_multi, &b->encrypt_buf); } @@ -512,7 +512,7 @@ encrypt_sign(struct context *c, bool comp_frag) /* Do packet administration */ if (c->c2.tls_multi) { - if (c->c2.buf.len > 0 && (c->c2.tls_multi->opt.server || !c->c2.tls_multi->use_peer_id)) + if (c->c2.buf.len > 0 && !c->c2.tls_multi->use_peer_id) { tls_prepend_opcode_v1(c->c2.tls_multi, &c->c2.buf); } diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 5947a31f..16a4101f 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -366,6 +366,7 @@ prepare_push_reply(struct context *c, struct gc_arena *gc, push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", tls_multi->peer_id); } + tls_multi->use_peer_id = true; } /* Push cipher if client supports Negotiable Crypto Parameters */
P_DATA_V2 introduced the peer-id. This allows clients to float, but as a side-effect 32-bit aligns the encrypted data. That alignment improves performance particularly on cheaper/older CPUs. So although servers don't actually have a peer-id, still use the V2 packet format (with a zero-id) for server->client traffic too. Signed-off-by: Steffan Karger <steffan@karger.me> --- v2: actually enable P_DATA_V2... Now tested with: 2.4<>2.4 (V2), 2.4-srv<>2.3-clt (V2), 2.3-srv<>2.4-clt (V1), 2.4-srv<>2.2-clt (V1) src/openvpn/forward.c | 4 ++-- src/openvpn/push.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)