[Openvpn-devel,1/2] make tls-auth a per-connection-block option

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
Related show

Commit Message

Antonio Quartulli June 2, 2018, 3:42 a.m.
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(-)

Comments

Steffan Karger June 3, 2018, 8:27 a.m. | #1
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
Antonio Quartulli June 3, 2018, 8:30 a.m. | #2
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,
Antonio Quartulli June 4, 2018, 2:15 a.m. | #3
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,
Jan Just Keijser June 4, 2018, 7:10 a.m. | #4
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
Gert Doering June 4, 2018, 7:15 a.m. | #5
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
Antonio Quartulli June 4, 2018, 7:20 a.m. | #6
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,
Jan Just Keijser June 4, 2018, 8 a.m. | #7
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

Patch

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