Peer-id patch v7
PATCH 1/1 Peer-id patch v7
Added new packet format P_DATA_V2, which includes peer-id. If server supports, client sends all data packets in the new format. When data packet arrives, server identifies peer by peer-id. If peer's ip/port has changed, server assumes that client has floated, verifies HMAC and updates ip/port in internal structs.
Changes in v7:
A few nitpicks.
Changes in v6:
Fixed: Make sure float won't happen if hmac check failed (regression).
Fixed: Access outside of bounds of array, which has caused memory corruption and crash. Various review fixes.
Changes in v5:
Protection agains replay attack by commiting float changes only after existing packet processing flow has completed.
If peer floats to an address which is already taken by another active session, drop float packet, otherwise disconnect existing session.
Changes in v4:
Handles correctly float to an address which is used by another peer. This also has fixed crash on assert in multi_client_disconnect.
Changes in v3:
Bugfix: If float happens after TLS renegotiation and there are no data packets between reneg and float, server will not recognize floated client.
--- src/openvpn/forward.c | 50 ++++++++++++++++--------- src/openvpn/forward.h | 30 ++++++++++++--- src/openvpn/init.c | 12 +++++- src/openvpn/mudp.c | 57 +++++++++++++++++++++------- src/openvpn/mudp.h | 2 +- src/openvpn/multi.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++-- src/openvpn/multi.h | 19 ++++++++++ src/openvpn/options.c | 6 +++ src/openvpn/options.h | 4 ++ src/openvpn/push.c | 13 +++++++ src/openvpn/ssl.c | 74 ++++++++++++++++++++++++++++++++---- src/openvpn/ssl.h | 15 +++++++- src/openvpn/ssl_common.h | 4 ++ 13 files changed, 332 insertions(+), 51 deletions(-)
src/openvpn/forward.c +33 -17
@@ -722,20 +722,11 @@ read_incoming_link (struct context *c)perf_pop (); } -/* -* Input: c->c2.buf -* Output: c->c2.to_tun -*/ - -void -process_incoming_link (struct context *c) +bool +process_incoming_link_part1 (struct context *c,struct link_socket_info *lsi,bool floated) {struct gc_arena gc =gc_new (); -bool decrypt_status; -struct link_socket_info *lsi =get_link_socket_info (c); -const uint8_t *orig_buf = c->c2.buf.data; - -perf_push (PERF_PROC_IN_LINK ); +bool decrypt_status = false;if (c->c2.buf.len >0 ) {
@@ -805,7 +796,7 @@ process_incoming_link (struct context *c)* will load crypto_options with the correct encryption key * and return false. */ -if (tls_pre_decrypt (c->c2.tls_multi, &c->c2.from, &c->c2.buf, &c->c2.crypto_options)) +if (tls_pre_decrypt (c->c2.tls_multi, &c->c2.from, &c->c2.buf, &c->c2.crypto_options, floated)) {interval_action (&c->c2.tmp_int);
@@ -832,11 +823,25 @@ process_incoming_link (struct context *c)/* decryption errors are fatal in TCP mode */ register_signal (c,SIGUSR1 ,"decryption-error" );/* SOFT-SIGUSR1 -- decryption error in TCP mode */ msg (D_STREAM_ERRORS, "Fatal decryption error (process_incoming_link), restarting"); - goto done; } - +#else /* ENABLE_CRYPTO */ + decrypt_status = true;#endif /* ENABLE_CRYPTO */ + } +else + { +buf_reset (&c->c2.to_tun); + } +gc_free (&gc); +return decrypt_status; +} + +void +process_incoming_link_part2 (struct context *c,struct link_socket_info *lsi,const uint8_t *orig_buf) +{ +if (c->c2.buf.len >0 ) + {#ifdef ENABLE_FRAGMENT if (c->c2.fragment)fragment_incoming (c->c2.fragment, &c->c2.buf, &c->c2.frame_fragment);
@@ -903,9 +908,20 @@ process_incoming_link (struct context *c) {buf_reset (&c->c2.to_tun); } - done: +} + +void +process_incoming_link (struct context *c) +{ +perf_push (PERF_PROC_IN_LINK ); + +struct link_socket_info *lsi =get_link_socket_info (c); +const uint8_t *orig_buf = c->c2.buf.data; + +process_incoming_link_part1 (c, lsi, false); +process_incoming_link_part2 (c, lsi, orig_buf); +perf_pop (); -gc_free (&gc); }/*
src/openvpn/forward.h +25 -5
@@ -127,12 +127,11 @@ void encrypt_sign (struct context *c, bool comp_frag);*/ void read_incoming_link (struct context *c); -/** -* Process a packet read from the external network interface. +* Starts processing a packet read from the external network interface. * @ingroup external_multiplexer * -* This function controls the processing of a data channel packet which +* This function starts the processing of a data channel packet which * has come out of a VPN tunnel. It's high-level structure is as follows: * - Verify that a nonzero length packet has been received from a valid * source address for the given context \a c.
@@ -146,6 +145,25 @@ void read_incoming_link (struct context *c);* - Call \c openvpn_decrypt() of the \link data_crypto Data Channel * Crypto module\endlink to authenticate and decrypt the packet using * the security parameters loaded by \c tls_pre_decrypt() above. +* +* @param c - The context structure of the VPN tunnel associated with the +* packet. +* @param lsi - link_socket_info obtained from context before processing. +* @param floated - Flag indicates that peer has floated. +* +* @return true if packet is authenticated, false otherwise. +*/ +bool process_incoming_link_part1 (struct context *c,struct link_socket_info *lsi,bool floated); + +/** +* Continues processing a packet read from the external network interface. +* @ingroup external_multiplexer +* +* This function continues the processing of a data channel packet which +* has come out of a VPN tunnel. It must be called after +* \c process_incoming_link_part1() function. +* +* It's high-level structure is as follows: * - Call \c fragment_incoming() of the \link fragmentation Data Channel * Fragmentation module\endlink to reassemble the packet if it's * fragmented.
@@ -158,9 +176,11 @@ void read_incoming_link (struct context *c);* * @param c - The context structure of the VPN tunnel associated with the * packet. +* @param lsi - link_socket_info obtained from context before processing. +* @param orig_buf - Pointer to a buffer data. +* */ -void process_incoming_link (struct context *c); - +void process_incoming_link_part2 (struct context *c,struct link_socket_info *lsi,const uint8_t *orig_buf);/** * Write a packet to the external network interface.
src/openvpn/init.c +11 -1
@@ -1718,7 +1718,8 @@ pull_permission_mask (const struct context *c) | OPT_P_MESSAGES |OPT_P_EXPLICIT_NOTIFY |OPT_P_ECHO - |OPT_P_PULL_MODE ; + |OPT_P_PULL_MODE + |OPT_P_PEER_ID ;if (!c->options.route_nopull) flags |= (OPT_P_ROUTE |OPT_P_IPWIN32 );
@@ -1795,6 +1796,15 @@ do_deferred_options (struct context *c, const unsigned int found)msg (D_PUSH ,"OPTIONS IMPORT: --ip-win32 and/or --dhcp-option options modified" );if (found &OPT_P_SETENV )msg (D_PUSH ,"OPTIONS IMPORT: environment modified" ); + +#ifdef ENABLE_SSL +if (found &OPT_P_PEER_ID ) + { +msg (D_PUSH ,"OPTIONS IMPORT: peer-id set" ); + c->c2.tls_multi->use_peer_id = true; + c->c2.tls_multi->peer_id = c->options.peer_id; + } +#endif }/*
src/openvpn/mudp.c +43 -14
92 lines not on this page · raw message
src/openvpn/mudp.h +1 -1
@@ -65,7 +65,7 @@ void tunnel_server_udp (struct context *top);* packet's source address or if one was a newly created successfully. * NULL if one did not yet exist and a new one was not created. */ -struct multi_instance *multi_get_create_instance_udp (struct multi_context *m); +struct multi_instance *multi_get_create_instance_udp (struct multi_context *m,bool *floated);#endif #endif
src/openvpn/multi.c +93 -4
150 lines not on this page · raw message
src/openvpn/multi.h +19 -0
@@ -125,6 +125,9 @@ struct multi_context {# define MC_WORK_THREAD (MC_MULTI_THREADED_WORKER|MC_MULTI_THREADED_SCHEDULER)int thread_mode; +struct multi_instance ** instances;/**< Array of multi_instances. An instance can be +* accessed using peer-id as an index. */ +struct hash *hash;/**< VPN tunnel instances indexed by real * address of the remote peer. */ struct hash *vhash;/**< VPN tunnel instances indexed by
@@ -218,6 +221,16 @@ void multi_close_instance (struct multi_context *m, struct multi_instance *mi, bbool multi_process_timeout (struct multi_context *m,const unsigned int mpp_flags); +/** +* Handles peer floating. +* +* If peer is floated to a taken address, either drops packet +* (if peer that owns address has different CN) or disconnects +* existing peer. Updates multi_instance with new address, +* updates hashtables in multi_context. +*/ +void multi_process_float (struct multi_context * m,struct multi_instance * mi); +#define MPP_PRE_SELECT (1<<0)#define MPP_CONDITIONAL_PRE_SELECT (1<<1)#define MPP_CLOSE_ON_SIGNAL (1<<2)
@@ -419,6 +432,12 @@ multi_route_defined (const struct multi_context *m, }/* +* Takes prefix away from multi_instance. +*/ +void +ungenerate_prefix (struct multi_instance *mi); + +/* * Set a msg() function prefix with our current client instance ID. */
src/openvpn/options.c +6 -0
@@ -6985,6 +6985,12 @@ add_option (struct options *options, options->persist_mode =1 ; }#endif + else if (streq (p[0], "peer-id")) + { +VERIFY_PERMISSION (OPT_P_PEER_ID ); + options->use_peer_id = true; + options->peer_id =atoi (p[1 ]); + } else {int i;
src/openvpn/options.h +4 -0
@@ -598,6 +598,9 @@ struct optionsbool show_net_up;int route_method;#endif + +bool use_peer_id; +uint32_t peer_id; };#define streq (x, y) (!strcmp((x), (y)))
@@ -633,6 +636,7 @@ struct options#define OPT_P_SOCKBUF (1<<25)#define OPT_P_SOCKFLAGS (1<<26)#define OPT_P_CONNECTION (1<<27) +#define OPT_P_PEER_ID (1<<28)#define OPT_P_DEFAULT (~(OPT_P_INSTANCE|OPT_P_PULL_MODE))
src/openvpn/push.c +13 -0
@@ -305,6 +305,19 @@ send_push_reply (struct context *c)if (multi_push)buf_printf (&buf,",push-continuation 1" ); +/* Send peer-id if client supports it */ +if (c->c2.tls_multi->peer_info) + { +const char * proto_str =strstr (c->c2.tls_multi->peer_info,"IV_PROTO=" ); +if (proto_str) + { +int proto =0 ; +int r =sscanf (proto_str,"IV_PROTO=%d" , &proto); +if ((r ==1 ) && (proto >=2 )) +buf_printf (&buf,",peer-id %d" , c->c2.tls_multi->peer_id); + } + } +if (BLEN (&buf) >sizeof (cmd)-1 ) {const bool status =send_control_channel_string (c,BSTR (&buf),D_PUSH );
src/openvpn/ssl.c +67 -7
141 lines not on this page · raw message
src/openvpn/ssl.h +13 -2
@@ -60,6 +60,7 @@#define P_CONTROL_V1 4/* control channel packet (usually TLS ciphertext) */ #define P_ACK_V1 5/* acknowledgement for packets received */ #define P_DATA_V1 6/* data channel packet */ +#define P_DATA_V2 9/* data channel packet with peer-id */ /* indicates key_method >= 2 */ #define P_CONTROL_HARD_RESET_CLIENT_V2 7/* initial key from client, forget previous state */
@@ -67,7 +68,7 @@/* define the range of legal opcodes */ #define P_FIRST_OPCODE 1 -#define P_LAST_OPCODE 8 +#define P_LAST_OPCODE 9/* Should we aggregate TLS * acknowledgements, and tack them onto
@@ -305,7 +306,8 @@ int tls_multi_process (struct tls_multi *multi,bool tls_pre_decrypt (struct tls_multi *multi,const struct link_socket_actual *from,struct buffer *buf, -struct crypto_options *opt); +struct crypto_options *opt, +bool floated);/**************************************************************************/
@@ -430,6 +432,15 @@ bool tls_send_payload (struct tls_multi *multi,bool tls_rec_payload (struct tls_multi *multi,struct buffer *buf); +/** +* Updates remote address in TLS sessions. +* +* @param multi - Tunnel to update +* @param addr - new address +*/ +void tls_update_remote_addr (struct tls_multi *multi, +const struct link_socket_actual *addr); +#ifdef MANAGEMENT_DEF_AUTH static inline char *tls_get_peer_info (const struct tls_multi *multi)
src/openvpn/ssl_common.h +4 -0
@@ -495,6 +495,10 @@ struct tls_multichar *peer_info;#endif +/* For P_DATA_V2 */ +uint32_t peer_id; +bool use_peer_id; +/* * Our session objects. */
Signature · 1 line
1.9.1
Quoted 5 lines · Lev Stipakov
Added new packet format
P_DATA_V2, which includes peer-id. If server supports, client sends all data packets in the new format. When data packet arrives, server identifies peer by peer-id. If peer's ip/port has changed, server assumes that client has floated, verifies HMAC and updates ip/port in internal structs.
ACK
During the hackathon, the group decided that this indeed is a nice way to support floating clients. I did a code review, but no real testing (just 'make check', including t_client). Lev, Gert and Arne have however performed real-life tests with multiple iterations of this patch.
A remaining issue is that the memory usage of a client increases for each 'float', because generate_prefix(mi) allocates memory on each call, which will only be free'd when the client disconnects. However, we agreed to fix that in a separate patch.
-Steffan
PATCH v2 1/1 Add client-only support for peer-id.
In reply to Lev StipakovThis is a reduced version of the peer-id patch from Lev Stipakov implementing only the client side bits - send IV_PROTO=2, accept "peer-id <n>" as pushed option, support P_DATA_V2 packets.
v2: remove addition of "struct tls_multi;" to options.h, not needed
--- src/openvpn/init.c | 12 +++++++++++- src/openvpn/options.c | 6 ++++++ src/openvpn/options.h | 4 ++++ src/openvpn/ssl.c | 40 +++++++++++++++++++++++++++++++++++----- src/openvpn/ssl.h | 3 ++- src/openvpn/ssl_common.h | 4 ++++ 6 files changed, 62 insertions(+), 7 deletions(-)
src/openvpn/init.c +11 -1
@@ -1711,7 +1711,8 @@ pull_permission_mask (const struct context *c) | OPT_P_MESSAGES |OPT_P_EXPLICIT_NOTIFY |OPT_P_ECHO - |OPT_P_PULL_MODE ; + |OPT_P_PULL_MODE + |OPT_P_PEER_ID ;if (!c->options.route_nopull) flags |= (OPT_P_ROUTE |OPT_P_IPWIN32 );
@@ -1790,6 +1791,15 @@ do_deferred_options (struct context *c, const unsigned int found)msg (D_PUSH ,"OPTIONS IMPORT: --ip-win32 and/or --dhcp-option options modified" );if (found &OPT_P_SETENV )msg (D_PUSH ,"OPTIONS IMPORT: environment modified" ); + +#ifdef ENABLE_SSL +if (found &OPT_P_PEER_ID ) + { +msg (D_PUSH ,"OPTIONS IMPORT: peer-id set" ); + c->c2.tls_multi->use_peer_id = true; + c->c2.tls_multi->peer_id = c->options.peer_id; + } +#endif }/*
src/openvpn/options.c +6 -0
@@ -6996,6 +6996,12 @@ add_option (struct options *options, options->persist_mode =1 ; }#endif + else if (streq (p[0], "peer-id")) + { +VERIFY_PERMISSION (OPT_P_PEER_ID ); + options->use_peer_id = true; + options->peer_id =atoi (p[1 ]); + } else {int i;
src/openvpn/options.h +4 -0
@@ -594,6 +594,9 @@ struct optionsbool show_net_up;int route_method;#endif + +bool use_peer_id; +uint32_t peer_id; };#define streq (x, y) (!strcmp((x), (y)))
@@ -629,6 +632,7 @@ struct options#define OPT_P_SOCKBUF (1<<25)#define OPT_P_SOCKFLAGS (1<<26)#define OPT_P_CONNECTION (1<<27) +#define OPT_P_PEER_ID (1<<28)#define OPT_P_DEFAULT (~(OPT_P_INSTANCE|OPT_P_PULL_MODE))
src/openvpn/ssl.c +35 -5
@@ -627,6 +627,8 @@ packet_opcode_name (int op)return "P_ACK_V1" ;case P_DATA_V1 :return "P_DATA_V1" ; +case P_DATA_V2 : +return "P_DATA_V2" ;default :return "P_???" ; }
@@ -1053,6 +1055,9 @@ tls_multi_init (struct tls_options *tls_options) ret->key_scan[1 ] = &ret->session[TM_ACTIVE ].key[KS_LAME_DUCK ]; ret->key_scan[2 ] = &ret->session[TM_LAME_DUCK ].key[KS_LAME_DUCK ]; +/* By default not use P_DATA_V2 */ + ret->use_peer_id = false; +return ret; }
@@ -1828,6 +1833,8 @@ push_peer_info(struct buffer *buf, struct tls_session *session)#ifdef ENABLE_LZO_STUB buf_printf (&out,"IV_LZO_STUB=1\n" );#endif +/* support for P_DATA_V2 */ +buf_printf (&out,"IV_PROTO=2\n" );if (session->opt->push_peer_info_detail >=2 ) {
@@ -2777,8 +2784,9 @@ tls_pre_decrypt (struct tls_multi *multi, key_id = c &P_KEY_ID_MASK ; } -if (op ==P_DATA_V1 ) - {/* data channel packet */ +if ((op ==P_DATA_V1 ) || (op ==P_DATA_V2 )) + { +/* data channel packet */ for (i =0 ; i <KEY_SCAN_SIZE ; ++i) {struct key_state *ks = multi->key_scan[i];
@@ -2810,7 +2818,19 @@ tls_pre_decrypt (struct tls_multi *multi, opt->pid_persist =NULL ; opt->flags &= multi->opt.crypto_flags_and; opt->flags |= multi->opt.crypto_flags_or; +ASSERT (buf_advance (buf,1 )); +if (op ==P_DATA_V2 ) + { +if (buf->len <4 ) + { +msg (D_TLS_ERRORS ,"Protocol error: received P_DATA_V2 from %s but length is < 4" , +print_link_socket_actual (from, &gc)); + goto error; + } +ASSERT (buf_advance (buf,3 )); + } + ++ks->n_packets; ks->n_bytes += buf->len; dmsg (D_TLS_KEYSELECT,
@@ -3375,14 +3395,24 @@ tls_post_encrypt (struct tls_multi *multi, struct buffer *buf) {struct key_state *ks;uint8_t *op; +uint32_t peer; ks = multi->save_ks; multi->save_ks =NULL ;if (buf->len >0 ) {ASSERT (ks); -ASSERT (op =buf_prepend (buf,1 )); - *op = (P_DATA_V1 <<P_OPCODE_SHIFT ) | ks->key_id; + +if (!multi->opt.server && multi->use_peer_id) + { + peer =htonl (((P_DATA_V2 <<P_OPCODE_SHIFT ) | ks->key_id) <<24 | (multi->peer_id &0xFFFFFF )); +ASSERT (buf_write_prepend (buf, &peer,4 )); + } +else + { +ASSERT (op =buf_prepend (buf,1 )); + *op = (P_DATA_V1 <<P_OPCODE_SHIFT ) | ks->key_id; + } ++ks->n_packets; ks->n_bytes += buf->len; }
@@ -3489,7 +3519,7 @@ protocol_dump (struct buffer *buffer, unsigned int flags, struct gc_arena *gc) key_id = c &P_KEY_ID_MASK ;buf_printf (&out,"%s kid=%d" ,packet_opcode_name (op), key_id); -if (op ==P_DATA_V1 ) +if ((op ==P_DATA_V1 ) || (op ==P_DATA_V2 )) goto print_data;/*
src/openvpn/ssl.h +2 -1
@@ -61,6 +61,7 @@#define P_CONTROL_V1 4/* control channel packet (usually TLS ciphertext) */ #define P_ACK_V1 5/* acknowledgement for packets received */ #define P_DATA_V1 6/* data channel packet */ +#define P_DATA_V2 9/* data channel packet with peer-id */ /* indicates key_method >= 2 */ #define P_CONTROL_HARD_RESET_CLIENT_V2 7/* initial key from client, forget previous state */
@@ -68,7 +69,7 @@/* define the range of legal opcodes */ #define P_FIRST_OPCODE 1 -#define P_LAST_OPCODE 8 +#define P_LAST_OPCODE 9/* Should we aggregate TLS * acknowledgements, and tack them onto
src/openvpn/ssl_common.h +4 -0
@@ -490,6 +490,10 @@ struct tls_multitime_t tas_last;#endif +/* For P_DATA_V2 */ +uint32_t peer_id; +bool use_peer_id; +/* * Our session objects. */
Signature · 1 line
2.1.2
Re: [Openvpn-devel] [PATCH v2] Add client-only support for peer-id.
ACK. Code is similar to the full-blown patch for master, and float works nicely.
-Steffan
Quoted 222 lines · Gert Doering
This is a reduced version of the peer-id patch from Lev Stipakov implementing only the client side bits - send IV_PROTO=2, accept "peer-id <n>" as pushed option, support
P_DATA_V2packets.v2: remove addition of "struct
tls_multi;" tooptions.h, not neededSigned-off-by Gert Doering <[email protected]...>--- src/openvpn/init.c | 12 +++++++++++- src/openvpn/options.c | 6 ++++++ src/openvpn/options.h | 4 ++++ src/openvpn/ssl.c | 40 +++++++++++++++++++++++++++++++++++----- src/openvpn/ssl.h | 3 ++- src/openvpn/ssl_common.h | 4 ++++ 6 files changed, 62 insertions(+), 7 deletions(-)src/openvpn/init.c +11
-1@@ -1711,7 +1711,8 @@ pull_permission_mask (const struct context *c) | OPT_P_MESSAGES |OPT_P_EXPLICIT_NOTIFY |OPT_P_ECHO - |OPT_P_PULL_MODE ; + |OPT_P_PULL_MODE + |OPT_P_PEER_ID ;if (!c->options.route_nopull) flags |= (OPT_P_ROUTE |OPT_P_IPWIN32 );@@ -1790,6 +1791,15 @@ do_deferred_options (struct context *c, const unsigned int found)msg (D_PUSH ,"OPTIONS IMPORT: --ip-win32 and/or --dhcp-option options modified" );if (found &OPT_P_SETENV )msg (D_PUSH ,"OPTIONS IMPORT: environment modified" ); + +#ifdef ENABLE_SSL +if (found &OPT_P_PEER_ID ) + { +msg (D_PUSH ,"OPTIONS IMPORT: peer-id set" ); + c->c2.tls_multi->use_peer_id = true; + c->c2.tls_multi->peer_id = c->options.peer_id; + } +#endif }/* src/openvpn/options.c +6
-0@@ -6996,6 +6996,12 @@ add_option (struct options *options, options->persist_mode =1 ; }#endif + else if (streq (p[0], "peer-id")) + { +VERIFY_PERMISSION (OPT_P_PEER_ID ); + options->use_peer_id = true; + options->peer_id =atoi (p[1 ]); + } else {int i;src/openvpn/options.h +4
-0@@ -594,6 +594,9 @@ struct optionsbool show_net_up;int route_method;#endif + +bool use_peer_id; +uint32_t peer_id; };#define streq (x, y) (!strcmp((x), (y)))@@ -629,6 +632,7 @@ struct options#define OPT_P_SOCKBUF (1<<25)#define OPT_P_SOCKFLAGS (1<<26)#define OPT_P_CONNECTION (1<<27) +#define OPT_P_PEER_ID (1<<28)#define OPT_P_DEFAULT (~(OPT_P_INSTANCE|OPT_P_PULL_MODE))src/openvpn/ssl.c +35
-5@@ -627,6 +627,8 @@ packet_opcode_name (int op)return "P_ACK_V1" ;case P_DATA_V1 :return "P_DATA_V1" ; +case P_DATA_V2 : +return "P_DATA_V2" ;default :return "P_???" ; }@@ -1053,6 +1055,9 @@ tls_multi_init (struct tls_options *tls_options) ret->key_scan[1 ] = &ret->session[TM_ACTIVE ].key[KS_LAME_DUCK ]; ret->key_scan[2 ] = &ret->session[TM_LAME_DUCK ].key[KS_LAME_DUCK ]; +/* By default not use P_DATA_V2 */ + ret->use_peer_id = false; +return ret; }@@ -1828,6 +1833,8 @@ push_peer_info(struct buffer *buf, struct tls_session *session)#ifdef ENABLE_LZO_STUB buf_printf (&out,"IV_LZO_STUB=1\n" );#endif +/* support for P_DATA_V2 */ +buf_printf (&out,"IV_PROTO=2\n" );if (session->opt->push_peer_info_detail >=2 ) {@@ -2777,8 +2784,9 @@ tls_pre_decrypt (struct tls_multi *multi, key_id = c &P_KEY_ID_MASK ; } -if (op ==P_DATA_V1 ) - {/* data channel packet */ +if ((op ==P_DATA_V1 ) || (op ==P_DATA_V2 )) + { +/* data channel packet */ for (i =0 ; i <KEY_SCAN_SIZE ; ++i) {struct key_state *ks = multi->key_scan[i];@@ -2810,7 +2818,19 @@ tls_pre_decrypt (struct tls_multi *multi, opt->pid_persist =NULL ; opt->flags &= multi->opt.crypto_flags_and; opt->flags |= multi->opt.crypto_flags_or; +ASSERT (buf_advance (buf,1 )); +if (op ==P_DATA_V2 ) + { +if (buf->len <4 ) + { +msg (D_TLS_ERRORS ,"Protocol error: received P_DATA_V2 from %s but length is < 4" , +print_link_socket_actual (from, &gc)); + goto error; + } +ASSERT (buf_advance (buf,3 )); + } + ++ks->n_packets; ks->n_bytes += buf->len; dmsg (D_TLS_KEYSELECT,@@ -3375,14 +3395,24 @@ tls_post_encrypt (struct tls_multi *multi, struct buffer *buf) {struct key_state *ks;uint8_t *op; +uint32_t peer; ks = multi->save_ks; multi->save_ks =NULL ;if (buf->len >0 ) {ASSERT (ks); -ASSERT (op =buf_prepend (buf,1 )); - *op = (P_DATA_V1 <<P_OPCODE_SHIFT ) | ks->key_id; + +if (!multi->opt.server && multi->use_peer_id) + { + peer =htonl (((P_DATA_V2 <<P_OPCODE_SHIFT ) | ks->key_id) <<24 | (multi->peer_id &0xFFFFFF )); +ASSERT (buf_write_prepend (buf, &peer,4 )); + } +else + { +ASSERT (op =buf_prepend (buf,1 )); + *op = (P_DATA_V1 <<P_OPCODE_SHIFT ) | ks->key_id; + } ++ks->n_packets; ks->n_bytes += buf->len; }@@ -3489,7 +3519,7 @@ protocol_dump (struct buffer *buffer, unsigned int flags, struct gc_arena *gc) key_id = c &P_KEY_ID_MASK ;buf_printf (&out,"%s kid=%d" ,packet_opcode_name (op), key_id); -if (op ==P_DATA_V1 ) +if ((op ==P_DATA_V1 ) || (op ==P_DATA_V2 )) goto print_data;/* src/openvpn/ssl.h +2
-1@@ -61,6 +61,7 @@#define P_CONTROL_V1 4/* control channel packet (usually TLS ciphertext) */ #define P_ACK_V1 5/* acknowledgement for packets received */ #define P_DATA_V1 6/* data channel packet */ +#define P_DATA_V2 9/* data channel packet with peer-id */ /* indicates key_method >= 2 */ #define P_CONTROL_HARD_RESET_CLIENT_V2 7/* initial key from client, forget previous state */ @@ -68,7 +69,7 @@/* define the range of legal opcodes */ #define P_FIRST_OPCODE 1 -#define P_LAST_OPCODE 8 +#define P_LAST_OPCODE 9/* Should we aggregate TLS * acknowledgements, and tack them onto src/openvpn/ssl_common.h +4
-0@@ -490,6 +490,10 @@ struct tls_multitime_t tas_last;#endif +/* For P_DATA_V2 */ +uint32_t peer_id; +bool use_peer_id; +/* * Our session objects. */
[Openvpn-devel] [PATCH applied] Re: Add client-only support for peer-id.
In reply to Gert DoeringYour patch has been applied to the release/2.3 branch.
commit 0e1fd33247460bdfa65d306e8bcdd3cbafed8b73
Author: Gert Doering Date: Sun Nov 23 20:17:30 2014 +0100
Add client-only support for peer-id.
Signed-off-by: Gert Doering <[email protected]...>
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9274--
kind regards,
Gert Doering
ACK from me as well.
Your patch has been applied to the master branch (and a client-only version of this has been applied to release/2.3 - separate mail coming).
commit 65eedc353349d2967fc03c54da807727e416e1b0 (master)
Author: Lev Stipakov Date: Sun Nov 23 17:17:11 2014 +0200
Peer-id patch v7
Acked-by: Steffan Karger <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9270
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
Hi,
Quoted 2 lines · Lev Stipakov
Changes in v7:
A few nitpicks.
Went in, and has just been pushed. Time for your dance now :-)
Question to you:
Quoted 4 lines
src/openvpn/mudp.c +0
-0@@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m) {hash_add_fast (hash, bucket, &mi->real, hv, mi); mi->did_real_hash = true;
src/openvpn/mudp.c multi_get_create_instance_udp (struct multi_context *m) · line 113
Quoted 10 lines
src/openvpn/mudp.c +10
-0+ +for (i =0 ; i < m->max_clients; ++i) + { +if (!m->instances[i]) + { + mi->context.c2.tls_multi->peer_id = i; + m->instances[i] = mi; +break ; + } + }
Is this code "safe"? That is, if max_clients is set too low, and you have more than this number of clients trying to connect, what will happen?
Will the server check this case before this loop, or will it just "not find a free instance" and leave peer_id as "0"?
Maybe we should add an ASSERT() here, to be sure that whatever other pieces of the code do, this will always end up in a defined state (as a separate patch).
Second, question to the group. Right now, this code is active unconditionally on the server side - what if someone does not want this behaviour, for whatever reason? Shall we make it depend on --float (which right now doesn't do anything for a --server OpenVPN server)?
This should be a fairly small change, I think, just never send the client a "peer-id <n>" push-reply (so it won't use T_PACKET_V2), and check "if (options.float)" before checking whether it's a floated client... so the question is more along the line "is this a useful thing to do, nor not?".
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Hello,
Currently it should be safe, since multi_create_instance returns NULL if amount of clients >= max_clients. In this case we won't reach that "for" loop thanks to "if (mi)" check.
But it probably won't harm to assert if we've reached "for" loop and could not find available "instance" item. I can implement that.
As for the second question - hard to say. If we make it opt-in, we probably will need to announce this feature loudly to make users aware of that. From the other side, it is not inconceivable to assume that someone might not want this. Maybe we could make it opt-out and have "-no-peer-id" config option?
-Lev
2014-11-27 16:22 GMT+02:00 Gert Doering <[email protected]...>:
Quoted 54 lines
Hi,
Quoted 2 lines · Lev Stipakov
Changes in v7:
A few nitpicks.Went in, and has just been pushed. Time for your dance now :-)
Question to you:
Quoted 4 lines
src/openvpn/mudp.c +0
-0@@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m) {hash_add_fast (hash, bucket, &mi->real, hv, mi); mi->did_real_hash = true;src/openvpn/mudp.c multi_get_create_instance_udp (struct multi_context *m) · line 113
Quoted 10 lines
src/openvpn/mudp.c +10
-0+ +for (i =0 ; i < m->max_clients; ++i) + { +if (!m->instances[i]) + { + mi->context.c2.tls_multi->peer_id = i; + m->instances[i] = mi; +break ; + } + }Is this code "safe"? That is, if
max_clientsis set too low, and you have more than this number of clients trying to connect, what will happen?Will the server check this case before this loop, or will it just "not find a free instance" and leave
peer_idas "0"?Maybe we should add an
ASSERT()here, to be sure that whatever other pieces of the code do, this will always end up in a defined state (as a separate patch).Second, question to the group. Right now, this code is active unconditionally on the server side - what if someone does not want this behaviour, for whatever reason? Shall we make it depend on --float (which right now doesn't do anything for a --server OpenVPN server)?
This should be a fairly small change, I think, just never send the client a "peer-id <n>" push-reply (so it won't use
T_PACKET_V2), and check "if (options.float)" before checking whether it's a floated client... so the question is more along the line "is this a useful thing to do, nor not?".gert
--
USENET is *not* the non-clickable part of WWW!//www.muc.de/~gert/Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...
Signature · 1 line
-Lev
Hi,
Quoted 5 lines · Lev Stipakov
As for the second question - hard to say. If we make it opt-in, we probably will need to announce this feature loudly to make users aware of that. From the other side, it is not inconceivable to assume that someone might not want this. Maybe we could make it opt-out and have "-no-peer-id" config option?
Definitely not "a new option", as we already have --float :-)
But I can see the argument about opt-in/opt-out - I found it a useful thing to explicitely enable the feature (so, no surprises here) but if there is not enough advertising, people won't be able to benefit from it...
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...