| Message ID | 20180602034206.9459-1-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel,1/2] make tls-auth a per-connection-block option | expand |
Hi, On 02-06-18 05:42, Antonio Quartulli wrote: > Different VPN servers may use different tls-auth keys. For this > reason it is convenient to make tls-auth a per-connection-block > option so that the user is allowed to specify one key per remote. Want! This also helps with tls-auth key rollover. Feature-ACK. > If no tls-auth option is specified in a given connection block, > the global one, if any, is used. > > Trac: #720 > Cc: Steffan Karger <steffan@karger.me> > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > doc/openvpn.8 | 1 + > src/openvpn/init.c | 10 +++--- > src/openvpn/options.c | 82 ++++++++++++++++++++++++++++++++++--------- > src/openvpn/options.h | 5 +++ > 4 files changed, 77 insertions(+), 21 deletions(-) > > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index 4114f408..e7bc3f4f 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -372,6 +372,7 @@ block: > .B remote, > .B rport, > .B socks\-proxy, > +.B tls\-auth, > .B tun\-mtu and > .B tun\-mtu\-extra. Shouldn't this also include key-direction? (Didn't really review or test yet, but otherwise looks good at first glance.) -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 03/06/18 16:27, Steffan Karger wrote: > Hi, > > On 02-06-18 05:42, Antonio Quartulli wrote: >> Different VPN servers may use different tls-auth keys. For this >> reason it is convenient to make tls-auth a per-connection-block >> option so that the user is allowed to specify one key per remote. > > Want! This also helps with tls-auth key rollover. Feature-ACK. > >> If no tls-auth option is specified in a given connection block, >> the global one, if any, is used. >> >> Trac: #720 >> Cc: Steffan Karger <steffan@karger.me> >> Signed-off-by: Antonio Quartulli <a@unstable.cc> >> --- >> doc/openvpn.8 | 1 + >> src/openvpn/init.c | 10 +++--- >> src/openvpn/options.c | 82 ++++++++++++++++++++++++++++++++++--------- >> src/openvpn/options.h | 5 +++ >> 4 files changed, 77 insertions(+), 21 deletions(-) >> >> diff --git a/doc/openvpn.8 b/doc/openvpn.8 >> index 4114f408..e7bc3f4f 100644 >> --- a/doc/openvpn.8 >> +++ b/doc/openvpn.8 >> @@ -372,6 +372,7 @@ block: >> .B remote, >> .B rport, >> .B socks\-proxy, >> +.B tls\-auth, >> .B tun\-mtu and >> .B tun\-mtu\-extra. > > Shouldn't this also include key-direction? > good catch! I added the manpage change at the end and I forgot about key-direction. Will wait a bit more before sending v2. > (Didn't really review or test yet, but otherwise looks good at first > glance.) > Thanks so far Cheers,
Hi all, On 02/06/18 11:42, Antonio Quartulli wrote: > Different VPN servers may use different tls-auth keys. For this > reason it is convenient to make tls-auth a per-connection-block > option so that the user is allowed to specify one key per remote. > > If no tls-auth option is specified in a given connection block, > the global one, if any, is used. > > Trac: #720 > Cc: Steffan Karger <steffan@karger.me> > Signed-off-by: Antonio Quartulli <a@unstable.cc> as reported by Steffan on IRC, this feature breaks when using "--persist-key". It happens because, when moving to the next connection block, OpenVPN won't load the new tls-auth key and therefore will trigger an assertion. After further discussing this issue, it was agreed that we have two main options to tackle this issue: 1) pre-load all the tls-auth keyfiles (like if they were embedded in the config file) 2) make per-connection-block tls-auth keys mutually exclusive with --persist-key while point 2) would be the easiest option and would require the least amount of code, we believe that 1) is still the best from the user perspective and from the option semantics point of view (as it would not lead to any behaviour change). Therefore a v2 patch will be sent implementing approach 1). Cheers,
Hi Antonio, On 04/06/18 04:15, Antonio Quartulli wrote: > Hi all, > > On 02/06/18 11:42, Antonio Quartulli wrote: >> Different VPN servers may use different tls-auth keys. For this >> reason it is convenient to make tls-auth a per-connection-block >> option so that the user is allowed to specify one key per remote. >> >> If no tls-auth option is specified in a given connection block, >> the global one, if any, is used. >> >> Trac: #720 >> Cc: Steffan Karger <steffan@karger.me> >> Signed-off-by: Antonio Quartulli <a@unstable.cc> > as reported by Steffan on IRC, this feature breaks when using > "--persist-key". > It happens because, when moving to the next connection block, OpenVPN > won't load the new tls-auth key and therefore will trigger an assertion. > > After further discussing this issue, it was agreed that we have two main > options to tackle this issue: > > 1) pre-load all the tls-auth keyfiles (like if they were embedded in the > config file) > 2) make per-connection-block tls-auth keys mutually exclusive with > --persist-key > > > while point 2) would be the easiest option and would require the least > amount of code, we believe that 1) is still the best from the user > perspective and from the option semantics point of view (as it would not > lead to any behaviour change). > > Therefore a v2 patch will be sent implementing approach 1). > > this is a very interesting patch ! And I agree, approach 1) is the way to go, as we've been advising people to use "persist-key" for a looong time now. What's the particular use case for putting tls-auth files in connection blocks? Does it apply only to tls-auth/tls-crypt files or also the certificate/private keys? I could see a use case for that as well... JM2CW, JJK ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Mon, Jun 04, 2018 at 09:10:23AM +0200, Jan Just Keijser wrote: > What's the particular use case for putting tls-auth files in connection > blocks? "I have one existing server that is not using tls-auth yet, and a new one that has tls-auth, and I want both in the same config file" Plus, what Steffan mentioned: tls-auth rollover > Does it apply only to tls-auth/tls-crypt files or also the > certificate/private keys? I could see a use case for that as well... Right now, only tls-auth/tls-crypt, but this is the same question I asked on IRC yesterday :-) The "traditional" use case ("my key/cert is my identity") would see "a single identity for all remotes", but indeed, upgrading to a new server with a new CA (--ca -> <connection>) and newly distributed identities might also be an interesting use case. For @work, I've decided to tackle this part with "just distribute two .ovpn files", but it's worth thinking through the idea. gert
Hi, Gert has been faster to reply :-) On 04/06/18 15:15, Gert Doering wrote: > Hi, > > On Mon, Jun 04, 2018 at 09:10:23AM +0200, Jan Just Keijser wrote: >> What's the particular use case for putting tls-auth files in connection >> blocks? > > "I have one existing server that is not using tls-auth yet, and a new one > that has tls-auth, and I want both in the same config file" > Exactly. Or even extend the same reasoning to tls-crypt: "one server was migrated to tls-crypt and one is still on tls-auth, and both have to be in the same config file". > Plus, what Steffan mentioned: tls-auth rollover > >> Does it apply only to tls-auth/tls-crypt files or also the >> certificate/private keys? I could see a use case for that as well... > > Right now, only tls-auth/tls-crypt, but this is the same question I asked > on IRC yesterday :-) > > The "traditional" use case ("my key/cert is my identity") would see > "a single identity for all remotes", but indeed, upgrading to a new server > with a new CA (--ca -> <connection>) and newly distributed identities > might also be an interesting use case. For @work, I've decided to tackle > this part with "just distribute two .ovpn files", but it's worth thinking > through the idea. > Once the issue with "--persist-key" has been fixed (will be in v2 of this patchset) I think it should be easy[tm] to also implement key/cert/ca per connection block. Cheers,
Hi, On 04/06/18 09:15, Gert Doering wrote: > On Mon, Jun 04, 2018 at 09:10:23AM +0200, Jan Just Keijser wrote: >> What's the particular use case for putting tls-auth files in connection >> blocks? > "I have one existing server that is not using tls-auth yet, and a new one > that has tls-auth, and I want both in the same config file" > > Plus, what Steffan mentioned: tls-auth rollover > hmmm, of course, some people even asked me what the best way to do tls-auth rollover is... I could also see a use case where you have a single config with "proto udp" and "proto tcp" , where the "proto udp" block has a tls-auth key but the "proto tcp" does not - the use case for "tls-auth" is mostly UDP anyways, as TCP connections are not so easy to protect against DoS attacks. I assume that will also be possible? JJK ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 4114f408..e7bc3f4f 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -372,6 +372,7 @@ block: .B remote, .B rport, .B socks\-proxy, +.B tls\-auth, .B tun\-mtu and .B tun\-mtu\-extra. diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 36c1a4c4..1c43c495 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2546,7 +2546,7 @@ do_init_crypto_tls_c1(struct context *c) prng_init(options->prng_hash, options->prng_nonce_secret_len); /* TLS handshake authentication (--tls-auth) */ - if (options->tls_auth_file) + if (options->ce.tls_auth_file) { /* Initialize key_type for tls-auth with auth only */ CLEAR(c->c1.ks.tls_auth_key_type); @@ -2563,8 +2563,10 @@ do_init_crypto_tls_c1(struct context *c) } crypto_read_openvpn_key(&c->c1.ks.tls_auth_key_type, - &c->c1.ks.tls_wrap_key, options->tls_auth_file, - options->tls_auth_file_inline, options->key_direction, + &c->c1.ks.tls_wrap_key, + options->ce.tls_auth_file, + options->ce.tls_auth_file_inline, + options->ce.key_direction, "Control Channel Authentication", "tls-auth"); } @@ -2783,7 +2785,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags) #endif /* TLS handshake authentication (--tls-auth) */ - if (options->tls_auth_file) + if (options->ce.tls_auth_file) { to.tls_wrap.mode = TLS_WRAP_AUTH; to.tls_wrap.opt.key_ctx_bi = c->c1.ks.tls_wrap_key; diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 426057ab..a9dbcb83 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1506,6 +1506,10 @@ show_connection_entry(const struct connection_entry *o) #ifdef ENABLE_OCC SHOW_INT(explicit_exit_notification); #endif + + SHOW_STR(tls_auth_file); + SHOW_PARM(key_direction, keydirection2ascii(o->key_direction, false, true), + "%s"); } @@ -1786,7 +1790,6 @@ show_settings(const struct options *o) SHOW_BOOL(push_peer_info); SHOW_BOOL(tls_exit); - SHOW_STR(tls_auth_file); SHOW_STR(tls_crypt_file); #ifdef ENABLE_PKCS11 @@ -2869,6 +2872,15 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce) } } + /* + * Set per-connection block tls-auth fields if no other method was defined + */ + if (!ce->tls_auth_file) + { + ce->tls_auth_file = o->tls_auth_file; + ce->tls_auth_file_inline = o->tls_auth_file_inline; + ce->key_direction = o->key_direction; + } } #ifdef _WIN32 @@ -3285,12 +3297,20 @@ options_postprocess_filechecks(struct options *options) options->crl_file, R_OK, "--crl-verify"); } - errs |= check_file_access(CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE, - options->tls_auth_file, R_OK, "--tls-auth"); + ASSERT(options->connection_list); + for (int i = 0; i < options->connection_list->len; ++i) + { + struct connection_entry *ce = options->connection_list->array[i]; + + errs |= check_file_access(CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE, + ce->tls_auth_file, R_OK, "--tls-auth"); + } + errs |= check_file_access(CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE, options->tls_crypt_file, R_OK, "--tls-crypt"); errs |= check_file_access(CHKACC_FILE|CHKACC_INLINE|CHKACC_PRIVATE, options->shared_secret_file, R_OK, "--secret"); + errs |= check_file_access(CHKACC_DIRPATH|CHKACC_FILEXSTWR, options->packet_id_file, R_OK|W_OK, "--replay-persist"); @@ -3647,7 +3667,7 @@ options_string(const struct options *o, { if (TLS_CLIENT || TLS_SERVER) { - if (o->tls_auth_file) + if (o->ce.tls_auth_file) { buf_printf(&out, ",tls-auth"); } @@ -7420,10 +7440,19 @@ add_option(struct options *options, { int key_direction; + VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_CONNECTION); + key_direction = ascii2keydirection(msglevel, p[1]); if (key_direction >= 0) { - options->key_direction = key_direction; + if (permission_mask & OPT_P_GENERAL) + { + options->key_direction = key_direction; + } + else if (permission_mask & OPT_P_CONNECTION) + { + options->ce.key_direction = key_direction; + } } else { @@ -7992,26 +8021,45 @@ add_option(struct options *options, } else if (streq(p[0], "tls-auth") && p[1] && !p[3]) { - VERIFY_PERMISSION(OPT_P_GENERAL); - if (streq(p[1], INLINE_FILE_TAG) && p[2]) + int key_direction = -1; + + VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_CONNECTION); + + if (permission_mask & OPT_P_GENERAL) { - options->tls_auth_file_inline = p[2]; + if (streq(p[1], INLINE_FILE_TAG) && p[2]) + { + options->tls_auth_file_inline = p[2]; + } + else if (p[2]) + { + key_direction = ascii2keydirection(msglevel, p[2]); + if (key_direction < 0) + { + goto err; + } + options->key_direction = key_direction; + } + options->tls_auth_file = p[1]; } - else if (p[2]) + else if (permission_mask & OPT_P_CONNECTION) { - int key_direction; - - key_direction = ascii2keydirection(msglevel, p[2]); - if (key_direction >= 0) + options->ce.key_direction = KEY_DIRECTION_BIDIRECTIONAL; + if (streq(p[1], INLINE_FILE_TAG) && p[2]) { - options->key_direction = key_direction; + options->ce.tls_auth_file_inline = p[2]; } - else + else if (p[2]) { - goto err; + key_direction = ascii2keydirection(msglevel, p[2]); + if (key_direction < 0) + { + goto err; + } + options->ce.key_direction = key_direction; } + options->ce.tls_auth_file = p[1]; } - options->tls_auth_file = p[1]; } else if (streq(p[0], "tls-crypt") && p[1] && !p[3]) { diff --git a/src/openvpn/options.h b/src/openvpn/options.h index f7d0145a..77c963d2 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -130,6 +130,11 @@ struct connection_entry #define CE_MAN_QUERY_REMOTE_MASK (0x07) #define CE_MAN_QUERY_REMOTE_SHIFT (2) unsigned int flags; + + /* Shared secret used for TLS control channel authentication */ + const char *tls_auth_file; + const char *tls_auth_file_inline; + int key_direction; }; struct remote_entry
Different VPN servers may use different tls-auth keys. For this reason it is convenient to make tls-auth a per-connection-block option so that the user is allowed to specify one key per remote. If no tls-auth option is specified in a given connection block, the global one, if any, is used. Trac: #720 Cc: Steffan Karger <steffan@karger.me> Signed-off-by: Antonio Quartulli <a@unstable.cc> --- doc/openvpn.8 | 1 + src/openvpn/init.c | 10 +++--- src/openvpn/options.c | 82 ++++++++++++++++++++++++++++++++++--------- src/openvpn/options.h | 5 +++ 4 files changed, 77 insertions(+), 21 deletions(-)