Re: [Openvpn-devel] [PATCH v2 3/6] Rewrite auth-token-gen to be based on HMAC based tokens
Am 22.02.19 um 03:42 schrieb Eric Thorpe:
> Thanks for doing this one Arne, this has been on my bucket list for a
> while. I've given this a reasonable test now and it's working as I'd
> expect. A few comments from my testing:
>
> On 23/01/2019 2:03 am, Arne Schwabe wrote:
>
>> +/* Accept session tokens that not expired are in the acceptable range
>> + * for renogiations */
>> +bool in_renog_time = now >= timestamp
>> + && now < timestamp + 2 *
>> session->opt->renegotiate_seconds;
> I'd like to see the valid time of an auth-token have it's own value,
> however I understand why you've done this. I can't find a nice way to
> get through the last active time of a session through to auth without a
> reasonable refactor. I'd like to see the auth token have the option
> "--auth-gen-token [inactive timeout] [total timeout]" or something along
> those lines. So while this isn't an ideal solution, it's good enough.
I am not really sure what talking about. There are two lifetimes for
auth token.
- the total max lifetime of an auth-token session (also specified in the
config)
- the max lifetime of an individual auth-token.
The second one is dervived from renegotiate_seconds as setting this
lower than this time will break renogiation.
The reason that I did not do any refactoring is a client with auth-token
can switch to another server and that server needs to verify that
auth-token with the information from the client and its config alone.
>> +/* We could still have a client that does not update
>> + * its auth-token, so also allow the initial auth-token */
>> +bool initialtoken = multi->auth_token_initial
>> +&& memcmp_constant_time(up->password,
>> multi->auth_token_initial,
>> +
>> strlen(multi->auth_token_initial)) == 0;
> I don't agree with this being in place, only the most recently generated
> token should be valid imo.
The reality is that we don't want to exclude all the older OpenVPN3
clients that do not update their token. Without this special, after
2*renogiation time, the clinets will fail.
> When an auth-token is authenticated, the server log will print out:
>
>> TLS: Username/auth-token authentication succeeded for username is still
>> displayed.
>> TLS: Username/Password authentication succeeded for username is still
>> displayed.
> The second line seems redundant and might cause some confusion. "if
> (skip_auth)" above this log line is probably enough I think?
>
> Finally, the patch won't build under MSVC without the following change:
>
>> +struct push_list push_list = {};
> to
>
>> +struct push_list push_list = {0};
>
> auth_token.c and auth_token.h will need to be added to the VS solution
> as well however I'm happy to submit that one myself once this gets acked
> to save you the trouble.
I will look into it.
Arne
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/6] Rewrite auth-token-gen to be based on HMAC based tokens
Thanks for doing this one Arne, this has been on my bucket list for a
while. I've given this a reasonable test now and it's working as I'd
expect. A few comments from my testing:
On 23/01/2019 2:03 am, Arne Schwabe wrote:
+/* Accept session tokens that not expired are in the acceptable range
+ * for renogiations */
+bool in_renog_time = now >= timestamp
+ && now < timestamp + 2 *
session->opt->renegotiate_seconds;
I'd like to see the valid time of an auth-token have it's own value,
however I understand why you've done this. I can't find a nice way to
get through the last active time of a session through to auth without a
reasonable refactor. I'd like to see the auth token have the option
"--auth-gen-token [inactive timeout] [total timeout]" or something along
those lines. So while this isn't an ideal solution, it's good enough.
+/* We could still have a client that does not update
+ * its auth-token, so also allow the initial auth-token */
+bool initialtoken = multi->auth_token_initial
+&& memcmp_constant_time(up->password,
multi->auth_token_initial,
+
strlen(multi->auth_token_initial)) == 0;
I don't agree with this being in place, only the most recently generated
token should be valid imo.
When an auth-token is authenticated, the server log will print out:
TLS: Username/auth-token authentication succeededfor username is still
displayed.
TLS: Username/Password authentication succeededfor username is still displayed.
The second line seems redundant and might cause some confusion. "if
(skip_auth)" above this log line is probably enough I think?
Finally, the patch won't build under MSVC without the following change:
+struct push_list push_list = {};
to
+struct push_list push_list = {0};
auth_token.c and auth_token.h will need to be added to the VS solution
as well however I'm happy to submit that one myself once this gets acked
to save you the trouble.
Cheers,
Eric
--
Eric Thorpe
SparkLabs Developer
https://www.sparklabs.com
https://twitter.com/sparklabs
supp...@sparklabs.com
On 23/01/2019 2:03 am, Arne Schwabe wrote:
The previous auth-token implementation had a serious problem, especially when
paired with an unpatched OpenVPN client that keeps trying the auth-token
(commit e61b401a).
The auth-token-gen implementation forgot the auth-token on reconnect, this
lead to reconnect with auth-token never working.
This new implementation implements the auth-token in a stateles variant. By
using HMAC to sign the auth-token the server can verify if a token has been
authenticated and by checking the embedded timestamp in the token it can
also verify that the auth-token is still valid.
Patch V2: cleaned up code, use refactored read_pem_key_file function
---
doc/openvpn.8| 27
src/openvpn/Makefile.am | 1 +
src/openvpn/auth_token.c | 260 +++
src/openvpn/auth_token.h | 116 +
src/openvpn/init.c | 34 -
src/openvpn/openvpn.h| 1 +
src/openvpn/options.c| 24 +++-
src/openvpn/options.h| 4 +
src/openvpn/push.c | 70 +--
src/openvpn/push.h | 8 ++
src/openvpn/ssl.c| 7 +-
src/openvpn/ssl_common.h | 36 +++---
src/openvpn/ssl_verify.c | 182 ---
13 files changed, 635 insertions(+), 135 deletions(-)
create mode 100644 src/openvpn/auth_token.c
create mode 100644 src/openvpn/auth_token.h
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 7abcaf1e..b1924898 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3720,6 +3720,9 @@ the token authentication internally and it will NOT do any
additional authentications against configured external
user/password authentication mechanisms.
+The tokens implemented by this mechanism include a initial timestamp
+and a renew timestamp and are secured by HMAC.
+
The
.B lifetime
argument defines how long the generated token is valid. The
@@ -3732,6 +3735,29 @@ authentications and that authentication mechanism does
not
implement any auth\-token support.
.\"*
.TP
+.B \-\-auth\-gen\-token\-secret [file]
+Specifies a file that hold a secret for the HMAC used in
+.B \-\-auth\-gen\-token
+If not present OpenVPN will generate a random secret on startup. This file
+should be used if auth-token should valid after restarting a server or if
+client should be able to roam between multiple OpenVPN server with their
+auth\-token.
+
+.\"*
+.TP
+.B \-\-auth\-gen\-token\-secret\-genkey
+When used together with the
+.B \-\-auth\-gen\-token\-secret
+option, this option will generate a new secret that can be used
+with
+.B \-\-auth\-gen\-token\-secret
+
+.B Note:
+this file should be kept secret to the server as anyone
+that access to this
Re: [Openvpn-devel] [PATCH v2 3/6] Rewrite auth-token-gen to be based on HMAC based tokens
On 22/01/2019 16:03, Arne Schwabe wrote:
> The previous auth-token implementation had a serious problem, especially when
> paired with an unpatched OpenVPN client that keeps trying the auth-token
> (commit e61b401a).
>
> The auth-token-gen implementation forgot the auth-token on reconnect, this
> lead to reconnect with auth-token never working.
>
> This new implementation implements the auth-token in a stateles variant. By
> using HMAC to sign the auth-token the server can verify if a token has been
> authenticated and by checking the embedded timestamp in the token it can
> also verify that the auth-token is still valid.
>
> Patch V2: cleaned up code, use refactored read_pem_key_file function
> ---
> doc/openvpn.8| 27
> src/openvpn/Makefile.am | 1 +
> src/openvpn/auth_token.c | 260 +++
> src/openvpn/auth_token.h | 116 +
> src/openvpn/init.c | 34 -
> src/openvpn/openvpn.h| 1 +
> src/openvpn/options.c| 24 +++-
> src/openvpn/options.h| 4 +
> src/openvpn/push.c | 70 +--
> src/openvpn/push.h | 8 ++
> src/openvpn/ssl.c| 7 +-
> src/openvpn/ssl_common.h | 36 +++---
> src/openvpn/ssl_verify.c | 182 ---
> 13 files changed, 635 insertions(+), 135 deletions(-)
> create mode 100644 src/openvpn/auth_token.c
> create mode 100644 src/openvpn/auth_token.h
>
[...snip...]
This review cannot be complete, due to my remarks to the previous commit
suggesting not touching read_pem_key_file() at all.
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 0cf8db76..87632551 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -6769,6 +6774,23 @@ add_option(struct options *options,
> options->auth_token_generate = true;
> options->auth_token_lifetime = p[1] ? positive_atoi(p[1]) : 0;
> }
> +else if (streq(p[0], "auth-gen-token-secret") && p[1] && (!p[2]
> + || (p[2] &&
> streq(p[1], INLINE_FILE_TAG
> +{
> +VERIFY_PERMISSION(OPT_P_GENERAL);
> +options->auth_token_secret_file = p[1];
> +
> +if (streq(p[1], INLINE_FILE_TAG) && p[2])
> +{
> +options->auth_token_secret_file_inline = p[2];
> +}
> +}
> +else if (streq(p[0], "auth-gen-token-secret-genkey") && !p[1])
> +{
> +VERIFY_PERMISSION(OPT_P_GENERAL);
> +options->auth_token_gen_secret_file = true;
> +}
> +
I see you add --auth-gen-token-secret and --auth-gen-token-secret-genkey ... I
find this a bit confusing ... gen-token-secret-genkey ...
Why not extend --auth-gen-token to take an additional file argument. Today it
is defined as:
--auth-gen-token [lifetime]
I suggest:
--auth-gen-token [lifetime] [secret-key]
If you want non-expiring tokens using a secret key, you can set the lifetime
value to 0.
Then instead of the --auth-gen-token-secret-genkey I suggest renaming
that to: --genkey-auth-token-secret
I see that your initial suggetion maps nicely to the same structure we have in
--tls-crypt-v2-genkey. But I find auth-gen-token-secret-genkey too long and
repetetive without being that explicit on what it does. I think
--genkey-auth-token-secret is a bit clearer in what that option does.
Another segment (I'm shuffling things around a bit, sorry about that)
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 560d87db..983b49e4 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1098,6 +1099,17 @@ do_genkey(const struct options *options)
>
> msg(M_USAGE, "--tls-crypt-v2-genkey type should be \"client\" or
> \"server\"");
> }
> +
> +if (options->auth_token_gen_secret_file)
> +{
> +if (!options->auth_token_secret_file)
> +{
> +msg(M_USAGE, "--auth-gen-token-secret-genkey requires a server
> key "
> +"to be set via --auth-gen-token-secret to create a shared
> secret");
> +}
> +auth_token_write_server_key_file(options->auth_token_secret_file);
Any reason we can't just skip this wrapper function and call the line below
directly?
write_pem_key_file(filename, auth_token_pem_name);
Otherwise, the rest looks reasonable. But I've only glared at the code for
now. I will dig deeper when starting to test the code.
--
kind regards,
David Sommerseth
OpenVPN Inc
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2 3/6] Rewrite auth-token-gen to be based on HMAC based tokens
The previous auth-token implementation had a serious problem, especially when
paired with an unpatched OpenVPN client that keeps trying the auth-token
(commit e61b401a).
The auth-token-gen implementation forgot the auth-token on reconnect, this
lead to reconnect with auth-token never working.
This new implementation implements the auth-token in a stateles variant. By
using HMAC to sign the auth-token the server can verify if a token has been
authenticated and by checking the embedded timestamp in the token it can
also verify that the auth-token is still valid.
Patch V2: cleaned up code, use refactored read_pem_key_file function
---
doc/openvpn.8| 27
src/openvpn/Makefile.am | 1 +
src/openvpn/auth_token.c | 260 +++
src/openvpn/auth_token.h | 116 +
src/openvpn/init.c | 34 -
src/openvpn/openvpn.h| 1 +
src/openvpn/options.c| 24 +++-
src/openvpn/options.h| 4 +
src/openvpn/push.c | 70 +--
src/openvpn/push.h | 8 ++
src/openvpn/ssl.c| 7 +-
src/openvpn/ssl_common.h | 36 +++---
src/openvpn/ssl_verify.c | 182 ---
13 files changed, 635 insertions(+), 135 deletions(-)
create mode 100644 src/openvpn/auth_token.c
create mode 100644 src/openvpn/auth_token.h
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 7abcaf1e..b1924898 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3720,6 +3720,9 @@ the token authentication internally and it will NOT do any
additional authentications against configured external
user/password authentication mechanisms.
+The tokens implemented by this mechanism include a initial timestamp
+and a renew timestamp and are secured by HMAC.
+
The
.B lifetime
argument defines how long the generated token is valid. The
@@ -3732,6 +3735,29 @@ authentications and that authentication mechanism does
not
implement any auth\-token support.
.\"*
.TP
+.B \-\-auth\-gen\-token\-secret [file]
+Specifies a file that hold a secret for the HMAC used in
+.B \-\-auth\-gen\-token
+If not present OpenVPN will generate a random secret on startup. This file
+should be used if auth-token should valid after restarting a server or if
+client should be able to roam between multiple OpenVPN server with their
+auth\-token.
+
+.\"*
+.TP
+.B \-\-auth\-gen\-token\-secret\-genkey
+When used together with the
+.B \-\-auth\-gen\-token\-secret
+option, this option will generate a new secret that can be used
+with
+.B \-\-auth\-gen\-token\-secret
+
+.B Note:
+this file should be kept secret to the server as anyone
+that access to this file will be to generate auth tokens
+that the OpenVPN server will accept as valid.
+.\"*
+.TP
.B \-\-opt\-verify
Clients that connect with options that are incompatible
with those of the server will be disconnected.
@@ -6973,6 +6999,7 @@ X509_1_C=KG
OpenVPN allows including files in the main configuration for the
.B \-\-ca, \-\-cert, \-\-dh, \-\-extra\-certs, \-\-key, \-\-pkcs12, \-\-secret,
.B \-\-crl\-verify, \-\-http\-proxy\-user\-pass, \-\-tls\-auth,
+.B \-\-auth\-gen\-token\-secret
.B \-\-tls\-crypt,
and
.B \-\-tls\-crypt-v2
diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index 197e62ba..78f94762 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -39,6 +39,7 @@ sbin_PROGRAMS = openvpn
openvpn_SOURCES = \
argv.c argv.h \
+ auth_token.c auth_token.h \
base64.c base64.h \
basic.h \
buffer.c buffer.h \
diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
new file mode 100644
index ..dc80456c
--- /dev/null
+++ b/src/openvpn/auth_token.c
@@ -0,0 +1,260 @@
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include "base64.h"
+#include "buffer.h"
+#include "crypto.h"
+#include "openvpn.h"
+#include "ssl_common.h"
+#include "auth_token.h"
+#include "push.h"
+#include "integer.h"
+#include "ssl.h"
+
+const char *auth_token_pem_name = "OpenVPN auth-token server key";
+
+
+/* Size of the data of the token (not b64 encoded and without prefix) */
+#define TOKEN_DATA_LEN (2 * sizeof(int64_t) + 32)
+
+static struct key_type
+auth_token_kt(void)
+{
+struct key_type kt;
+/* We do not encrypt our session tokens */
+kt.cipher = NULL;
+kt.digest = md_kt_get("SHA256");
+
+if (!kt.digest)
+{
+msg(M_WARN, "ERROR: --tls-crypt requires HMAC-SHA-256 support.");
+return (struct key_type) { 0 };
+}
+
+kt.hmac_length = md_kt_size(kt.digest);
+
+return kt;
+}
+
+
+void
+auth_token_write_server_key_file(const char *filename)
+{
+write_pem_key_file(filename, auth_token_pem_name);
+}
+
+void
+auth_token_init_secret(struct key_ctx *key_ctx, const
