fix compilation for --disable-multi

PATCH In review Antonio Quartulli 6 participants 12 messages Jul 13, 2017 – Aug 16, 2017

Share & tools

Antonio Quartulli

author

2017-07-13 03:38 UTC #0 lore raw

PATCH 1/1 fix compilation for --disable-multi

Due to the introduction of the options.pull attribute and the ncp logic, various parts of the code try to access members that are not available when --disable-multi is selected.

Avoid such issue by placing several "#if P2MP" around the faulty code.

Introduced-by 598e03f0e7bc ("Always push basic set of peer info values to server.")

Reported-by ValdikSS <[email protected]...>

Signed-off-by Antonio Quartulli <[email protected]...>

---
 src/openvpn/init.c    | 23 ++++++++++++++++++++++-
 src/openvpn/options.c |  4 +++-
 2 files changed, 25 insertions(+), 2 deletions(-)

src/openvpn/init.c +22 -1

index a54307ad..1105f081 100644
@@ -2150,6 +2150,7 @@ do_deferred_options(struct context *c, const unsigned int found)
     }
 
     /* process (potentially pushed) crypto options */
+#if P2MP
     if (c->options.pull)
     {
         struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];

@@ -2169,6 +2170,7 @@ do_deferred_options(struct context *c, const unsigned int found)
             return false;
         }
     }
+#endif
 #endif /* ifdef ENABLE_CRYPTO */
     return true;
 }

@@ -2498,9 +2500,11 @@ do_init_crypto_tls_c1(struct context *c)
                                options->tls_crypt_inline, options->tls_server);
         }
 
+#if P2MP
         c->c1.ciphername = options->ciphername;
         c->c1.authname = options->authname;
         c->c1.keysize = options->keysize;
+#endif
 
 #if 0 /* was: #if ENABLE_INLINE_FILES --  Note that enabling this code will break restarts */
         if (options->priv_key_file_inline)

@@ -2514,10 +2518,12 @@ do_init_crypto_tls_c1(struct context *c)
     {
         msg(D_INIT_MEDIUM, "Re-using SSL/TLS context");
 
+#if P2MP
         /* Restore pre-NCP cipher options */
         c->options.ciphername = c->c1.ciphername;
         c->options.authname = c->c1.authname;
         c->options.keysize = c->c1.keysize;
+#endif
     }
 }

@@ -2549,6 +2555,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
     /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
     packet_id_long_form = cipher_kt_mode_ofb_cfb(c->c1.ks.key_type.cipher);
 
+#if P2MP
     /* Compute MTU parameters (postpone if we push/pull options) */
     if (c->options.pull || c->options.mode == MODE_SERVER)
     {

@@ -2556,6 +2563,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
         frame_add_to_extra_frame(&c->c2.frame, crypto_max_overhead());
     }
     else
+#endif
     {
         crypto_adjust_frame_parameters(&c->c2.frame, &c->c1.ks.key_type,
                                        options->replay, packet_id_long_form);

@@ -2584,8 +2592,10 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
     to.replay_window = options->replay_window;
     to.replay_time = options->replay_time;
     to.tcp_mode = link_socket_proto_connection_oriented(options->ce.proto);
+#if P2MP
     to.config_ciphername = c->c1.ciphername;
     to.config_authname = c->c1.authname;
+#endif
     to.ncp_enabled = options->ncp_enabled;
     to.transition_window = options->transition_window;
     to.handshake_window = options->handshake_window;

@@ -2595,6 +2605,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
     to.renegotiate_seconds = options->renegotiate_seconds;
     to.single_session = options->single_session;
     to.mode = options->mode;
+#if P2MP
     to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
     if (options->push_peer_info)        /* all there is */

@@ -2610,6 +2621,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
         to.push_peer_info_detail = 0;
     }
 #endif
+#endif
 
     /* should we not xmit any packets until we get an initial
      * response from client? */

@@ -3698,7 +3710,14 @@ management_callback_network_change(void *arg, bool samenetwork)
     }
 
     socketfd = c->c2.link_socket->sd;
-    if (!c->options.pull || c->c2.tls_multi->use_peer_id || samenetwork)
+
+    bool pull = false;
+
+#if P2MP
+    pull = c->options.pull;
+#endif
+
+    if (!pull || c->c2.tls_multi->use_peer_id || samenetwork)
     {
         return socketfd;
     }

@@ -4260,11 +4279,13 @@ inherit_context_child(struct context *dest,
     dest->c1.ks.ssl_ctx = src->c1.ks.ssl_ctx;
     dest->c1.ks.tls_wrap_key = src->c1.ks.tls_wrap_key;
     dest->c1.ks.tls_auth_key_type = src->c1.ks.tls_auth_key_type;
+#if P2MP
     /* inherit pre-NCP ciphers */
     dest->c1.ciphername = src->c1.ciphername;
     dest->c1.authname = src->c1.authname;
     dest->c1.keysize = src->c1.keysize;
 #endif
+#endif
 
     /* options */
     dest->options = src->options;

src/openvpn/options.c +3 -1

index 505c5b2e..9f1eb3ff 100644
@@ -3006,6 +3006,7 @@ options_postprocess_mutate(struct options *o)
         }
     }
 
+#if P2MP
     /* cipher negotiation (NCP) currently assumes --pull or --mode server */
     if (o->ncp_enabled
         && !(o->pull || o->mode == MODE_SERVER) )

@@ -3015,6 +3016,7 @@ options_postprocess_mutate(struct options *o)
         o->ncp_enabled = false;
     }
 #endif
+#endif
 
 #if ENABLE_MANAGEMENT
     if (o->http_proxy_override)

@@ -3442,7 +3444,7 @@ static size_t
 calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
 {
     size_t link_mtu = EXPANDED_SIZE(frame);
-#ifdef ENABLE_CRYPTO
+#if defined(ENABLE_CRYPTO) && P2MP
     if (o->pull || o->mode == MODE_SERVER)
     {
         struct frame fake_frame = *frame;

Signature · 1 line

2.13.2

------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Antonio Quartulli

author

2017-07-13 06:05 UTC #2 lore raw

Quoted 1 line · Илья Шипицин

Should we add --disable-multi to some travis-ci build ?

Might be a good idea. It was actually broken since 2013, therefore it seems like not many people are using it....either we drop it or yes, we should have some automated test.

Cheers,

Signature · 1 line

Antonio Quartulli

Samuli Seppänen

2017-07-13 06:16 UTC #3 lore raw

Quoted 6 lines · Antonio Quartulli

Quoted 1 line · Илья Шипицин
Should we add --disable-multi to some travis-ci build ?
Might be a good idea. It was actually broken since 2013, therefore it seems like not many people are using it....either we drop it or yes, we should have some automated test.

There were no bugs reports about --disable-multi in Trac. I say scrap that option altogether and make the code slightly cleaner and less entangled.

Signature · 5 lines

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Илья Шипицин

2017-07-13 06:34 UTC #4 lore raw

2017-07-13 11:16 GMT+05:00 Samuli Seppänen <[email protected]...>:

Quoted 12 lines · Antonio Quartulli

Quoted 6 lines · Илья Шипицин
Quoted 1 line
Should we add --disable-multi to some travis-ci build ?
Might be a good idea. It was actually broken since 2013, therefore it seems like not many people are using it....either we drop it or yes, we should have some automated test.
There were no bugs reports about --disable-multi in Trac. I say scrap that option altogether and make the code slightly cleaner and less entangled.

was it broken in 2.3 as well ?
I'm afraid, not many people are using 2.4 since 2013

Quoted 8 lines

--
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc
irc freenode net: mattock

Antonio Quartulli

author

2017-07-13 06:45 UTC #5 lore raw

Quoted 10 lines · Илья Шипицин

2017-07-13 11:16 GMT+05:00 Samuli Seppänen <[email protected]...>:
Quoted 8 lines · Antonio Quartulli
Quoted 6 lines · Илья Шипицин
Quoted 1 line
Should we add --disable-multi to some travis-ci build ?
Might be a good idea. It was actually broken since 2013, therefore it seems like not many people are using it....either we drop it or yes, we should have some automated test.

Quoted 8 lines

Quoted 4 lines
There were no bugs reports about --disable-multi in Trac. I say scrap that option altogether and make the code slightly cleaner and less entangled.
was it broken in 2.3 as well ?
I'm afraid, not many people are using 2.4 since 2013

As reported in my patch, this is the commit that introduced the first problem:

commit 598e03f0e7bce434e501a9895819f2af0714d5f6

Author: James Yonan <[email protected]...>
Date:   Mon May 20 12:13:21 2013 +0200

Always push basic set of peer info values to server.

Checking what tag contains this id, we get:

$ git tag --contains 598e03f0e7bce434e501a9895819f2af0714d5f6
892e64b6b9
v2.4.0
v2.4.1
v2.4.2
v2.4.3
v2.4_alpha1
v2.4_alpha2
v2.4_beta1
v2.4_beta2
v2.4_rc1
v2.4_rc2

However, if you check the release/2.3 branch, you will see that this commit was cherry-picked manually and therefore it has a new id in the 2.3 history: eed9b8eec911a26a952f07ad18d4397c334ac089

Checking this commit ID shows that:

$ git tag --contains eed9b8eec911a26a952f07ad18d4397c334ac089
v2.3.10
v2.3.11
v2.3.12
v2.3.13
v2.3.14
v2.3.15
v2.3.16
v2.3.17
v2.3.2
v2.3.3
v2.3.4
v2.3.5
v2.3.6
v2.3.7
v2.3.8
v2.3.9

Hence, 2.3 is broken starting at v2.3.9. This is confirmed by the fact that release/2.3 does not compile with --disable-multi.

Cheers,

p.s. this is an example of how cherry-picking instead of forward merging can be troublesome for the git history.

Cheers²,

Signature · 1 line

Antonio Quartulli

Antonio Quartulli

author

2017-07-13 06:46 UTC #6 lore raw

Quoted 19 lines · Antonio Quartulli

$ git tag --contains eed9b8eec911a26a952f07ad18d4397c334ac089
v2.3.10
v2.3.11
v2.3.12
v2.3.13
v2.3.14
v2.3.15
v2.3.16
v2.3.17
v2.3.2
v2.3.3
v2.3.4
v2.3.5
v2.3.6
v2.3.7
v2.3.8
v2.3.9

Quoted 3 lines

Hence, 2.3 is broken starting at v2.3.9. This is confirmed by the fact that release/2.3 does not compile with --disable-multi.

typ0: I meant starting at 2.3.2.

Cheers,

Signature · 1 line

Antonio Quartulli

Илья Шипицин

2017-07-13 06:49 UTC #7 lore raw

2017-07-13 11:46 GMT+05:00 Antonio Quartulli <[email protected]...>:

Quoted 22 lines · Antonio Quartulli

Quoted 19 lines

$ git tag --contains eed9b8eec911a26a952f07ad18d4397c334ac089
v2.3.10
v2.3.11
v2.3.12
v2.3.13
v2.3.14
v2.3.15
v2.3.16
v2.3.17
v2.3.2
v2.3.3
v2.3.4
v2.3.5
v2.3.6
v2.3.7
v2.3.8
v2.3.9

Quoted 6 lines

Quoted 3 lines
Hence, 2.3 is broken starting at v2.3.9. This is confirmed by the fact that release/2.3 does not compile with --disable-multi.
typ0: I meant starting at 2.3.2.

so, if nobody care about something broken since 2.3.2, lets drop it

Quoted 7 lines

Cheers,
--
Antonio Quartulli

Steffan Karger

2017-07-13 09:53 UTC #8 lore raw

Quoted 28 lines · Илья Шипицин

2017-07-13 11:46 GMT+05:00 Antonio Quartulli <[email protected]... <mailto:[email protected]...>>:

    On 13/07/17 14:45, Antonio Quartulli wrote:
    > $ git tag --contains eed9b8eec911a26a952f07ad18d4397c334ac089
    > v2.3.10
    > v2.3.11
    > v2.3.12
    > v2.3.13
    > v2.3.14
    > v2.3.15
    > v2.3.16
    > v2.3.17
    > v2.3.2
    > v2.3.3
    > v2.3.4
    > v2.3.5
    > v2.3.6
    > v2.3.7
    > v2.3.8
    > v2.3.9
    >
    >
    > Hence, 2.3 is broken starting at v2.3.9. This is confirmed by the fact
    > that release/2.3 does not compile with --disable-multi.
    >

    typ0: I meant starting at 2.3.2.

Quoted 1 line

so, if nobody care about something broken since 2.3.2, lets drop it

Yes, this. Let's drop the configure flag and unnecessary ifdefs.

-Steffan

Gert Doering

2017-07-13 06:59 UTC #9 lore raw

In reply to Samuli Seppänen

Hi,

Quoted 3 lines · Samuli Seppänen

There were no bugs reports about --disable-multi in Trac. I say scrap that option altogether and make the code slightly cleaner and less entangled.

I tend to agree with you here. There are way too many #ifdef in our code, and while Antonio's patch is technically fine, it adds more #ifdef for a variant that nobody seems to be using - so that's not the way to go.

P2MP_SERVER seems to have some uses, to be able to compile a client-only binary for really small systems. So I'd keep that one, plus the associated configure options.

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]...

Antonio Quartulli

author

2017-08-16 13:05 UTC #10 lore raw

Hi,

Quoted 11 lines · Gert Doering

Hi,
Quoted 3 lines · Samuli Seppänen
There were no bugs reports about --disable-multi in Trac. I say scrap that option altogether and make the code slightly cleaner and less entangled.
I tend to agree with you here. There are way too many #ifdef in our code, and while Antonio's patch is technically fine, it adds more #ifdef for a variant that nobody seems to be using - so that's not the way to go.

I just went over this again and unfortunately I realized that, even though we can get rid of the --disable-multi switch (for which I will send a patch in a minute), we can't get rid of the various "#if P2MP" because its value depends on ENABLE_CRYPTO, which can still be enabled or disabled.

Therefore, we have to keep the P2MP define as long we keep the ENABLE_CRYPTO logic.

Cheers,

Quoted 20 lines

P2MP_SERVER seems to have some uses, to be able to compile a client-only binary for really small systems. So I'd keep that one, plus the associated configure options.
gert
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Signature · 1 line

Antonio Quartulli

David Sommerseth

2017-08-16 13:58 UTC #11 lore raw

Quoted 15 lines · Antonio Quartulli

Hi,
Quoted 11 lines · Gert Doering
Hi,
Quoted 3 lines · Samuli Seppänen
There were no bugs reports about --disable-multi in Trac. I say scrap that option altogether and make the code slightly cleaner and less entangled.
I tend to agree with you here. There are way too many #ifdef in our code, and while Antonio's patch is technically fine, it adds more #ifdef for a variant that nobody seems to be using - so that's not the way to go.

Quoted 8 lines

I just went over this again and unfortunately I realized that, even though we can get rid of the --disable-multi switch (for which I will send a patch in a minute), we can't get rid of the various "#if P2MP" because its value depends on ENABLE_CRYPTO, which can still be enabled or disabled.
Therefore, we have to keep the P2MP define as long we keep the ENABLE_CRYPTO logic.

I'm leaning towards removing both ENABLE_CRYPTO and P2MP. However, we need to update the buildslave configs too before doing that ... otherwise quite a few of us will drown in failure reports for a while.

I have also pondered if there is any use cases for keeping them around. The only one I can imagine now is debugging, to see if something broke due to crypto or not. I honestly don't see the benefit of building OpenVPN in 2017 without crypto support for production environments (but I may just lack imagination; our users never cease to amaze me).

But in the end, we have way too many #ifdefs around and many of them are entangled into each other, so for me the vote for code clarity trumps fine grained debugging in this case. And this #ifdef entanglement does not come without a maintenance cost as well.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Technologies, Inc