Re: [Openvpn-devel] [PATCH v3] Extend verify-hash to allow multiple hashes
Hi,
This patch looks good to me.
There is just one minor note below:
On 21/03/2021 15:25, Arne Schwabe wrote:
> This patch introduces support for verify-hash inlining.
> When inlined, this options now allows to specify multiple fingerprints,
> one per line.
>
> Since this is a new syntax, there is no backwards compatibility to take
> care of, therefore we can drop support for SHA1. Inlined fingerprints
> are assumed be to SHA-256 only.
>
> Also print a warning about SHA1 hash being deprecated to verify
> certificates as it is not "industry standard" anymore.
>
> Patch v2: fix/clarify various comments, fix a few minor problems, allow
> the option to be specified multiple times and have that
> added to the list.
>
> Patch v3: Remove leftover variable, always call
> parse_hash_fingerprint_multiline, add comments clarifying list
> appending
>
> Signed-off-by: Arne Schwabe
> ---
> doc/man-sections/inline-files.rst | 4 +-
> doc/man-sections/tls-options.rst | 10 +++
> src/openvpn/options.c | 102 ++
> src/openvpn/options.h | 10 ++-
> src/openvpn/ssl_common.h | 2 +-
> src/openvpn/ssl_verify.c | 16 -
> 6 files changed, 127 insertions(+), 17 deletions(-)
>
> diff --git a/doc/man-sections/inline-files.rst
> b/doc/man-sections/inline-files.rst
> index 819bd3c8..303bb3c8 100644
> --- a/doc/man-sections/inline-files.rst
> +++ b/doc/man-sections/inline-files.rst
> @@ -4,8 +4,8 @@ INLINE FILE SUPPORT
> OpenVPN allows including files in the main configuration for the ``--ca``,
> ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``,
> ``--secret``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``,
> -``--auth-gen-token-secret``, ``--tls-crypt`` and ``--tls-crypt-v2``
> -options.
> +``--auth-gen-token-secret``, ``--tls-crypt``, ``--tls-crypt-v2`` and
> +``--verify-hash`` options.
>
> Each inline file started by the line and ended by the line
>
> diff --git a/doc/man-sections/tls-options.rst
> b/doc/man-sections/tls-options.rst
> index e13fb3c8..d8f9800e 100644
> --- a/doc/man-sections/tls-options.rst
> +++ b/doc/man-sections/tls-options.rst
> @@ -582,6 +582,16 @@ certificates and keys:
> https://github.com/OpenVPN/easy-rsa
>The ``algo`` flag can be either :code:`SHA1` or :code:`SHA256`. If not
>provided, it defaults to :code:`SHA1`.
>
> + This option can also be inlined
> + ::
> +
> +
> +
> 00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff
> +
> 11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00
> +
> +
> +If the option is inlined, ``algo`` is always :code:`SHA256`.
> +
> --verify-x509-name args
>Accept connections only if a host's X.509 name is equal to **name.** The
>remote host must also pass all other tests of verification.
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 86e78b05..3b1c69ba 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1078,12 +1078,24 @@ string_substitute(const char *src, int from, int to,
> struct gc_arena *gc)
> return ret;
> }
>
> -static uint8_t *
> +/**
> + * Parses a hexstring and checks if the string has the correct length. Return
> + * a verify_hash_list containing the parsed hash string.
> + *
> + * @param str String to check/parse
> + * @param nbytes Number of bytes expected in the hexstr (e.g. 20 for
> SHA1)
> + * @param msglevelmessage level to use when printing warnings/errors
> + * @param gc The returned object will be allocated in this gc
> + */
> +static struct verify_hash_list *
> parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct
> gc_arena *gc)
> {
> int i;
> const char *cp = str;
> -uint8_t *ret = (uint8_t *) gc_malloc(nbytes, true, gc);
> +
> +struct verify_hash_list *ret;
> +ALLOC_OBJ_CLEAR_GC(ret, struct verify_hash_list, gc);
> +
> char term = 1;
> int byte;
> char bs[3];
> @@ -1102,7 +1114,7 @@ parse_hash_fingerprint(const char *str, int nbytes, int
> msglevel, struct gc_aren
> {
> msg(msglevel, "format error in hash fingerprint hex byte: %s",
> str);
> }
> -ret[i] = (uint8_t)byte;
> +ret->hash[i] = (uint8_t)byte;
> term = *cp++;
> if (term != ':' && term != 0)
> {
> @@ -1116,10 +1128,55 @@ parse_hash_fingerprint(const char *str, int nbytes,
> int msglevel, struct gc_aren
> if (term != 0 || i != nbytes-1)
> {
> msg(msglevel, "hash fingerprint is different length than expected
> (%d bytes): %s", nbytes, str);
> +return NULL;
The rest of this function assumes that msglevel is set to a FATAL level.
As you can also see in the hunk above, if blocks handling other errors
have no return statement, because they assume the progra
[Openvpn-devel] [PATCH v3] Extend verify-hash to allow multiple hashes
This patch introduces support for verify-hash inlining.
When inlined, this options now allows to specify multiple fingerprints,
one per line.
Since this is a new syntax, there is no backwards compatibility to take
care of, therefore we can drop support for SHA1. Inlined fingerprints
are assumed be to SHA-256 only.
Also print a warning about SHA1 hash being deprecated to verify
certificates as it is not "industry standard" anymore.
Patch v2: fix/clarify various comments, fix a few minor problems, allow
the option to be specified multiple times and have that
added to the list.
Patch v3: Remove leftover variable, always call
parse_hash_fingerprint_multiline, add comments clarifying list
appending
Signed-off-by: Arne Schwabe
---
doc/man-sections/inline-files.rst | 4 +-
doc/man-sections/tls-options.rst | 10 +++
src/openvpn/options.c | 102 ++
src/openvpn/options.h | 10 ++-
src/openvpn/ssl_common.h | 2 +-
src/openvpn/ssl_verify.c | 16 -
6 files changed, 127 insertions(+), 17 deletions(-)
diff --git a/doc/man-sections/inline-files.rst
b/doc/man-sections/inline-files.rst
index 819bd3c8..303bb3c8 100644
--- a/doc/man-sections/inline-files.rst
+++ b/doc/man-sections/inline-files.rst
@@ -4,8 +4,8 @@ INLINE FILE SUPPORT
OpenVPN allows including files in the main configuration for the ``--ca``,
``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``,
``--secret``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``,
-``--auth-gen-token-secret``, ``--tls-crypt`` and ``--tls-crypt-v2``
-options.
+``--auth-gen-token-secret``, ``--tls-crypt``, ``--tls-crypt-v2`` and
+``--verify-hash`` options.
Each inline file started by the line and ended by the line
diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst
index e13fb3c8..d8f9800e 100644
--- a/doc/man-sections/tls-options.rst
+++ b/doc/man-sections/tls-options.rst
@@ -582,6 +582,16 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
The ``algo`` flag can be either :code:`SHA1` or :code:`SHA256`. If not
provided, it defaults to :code:`SHA1`.
+ This option can also be inlined
+ ::
+
+
+
00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff
+
11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00
+
+
+If the option is inlined, ``algo`` is always :code:`SHA256`.
+
--verify-x509-name args
Accept connections only if a host's X.509 name is equal to **name.** The
remote host must also pass all other tests of verification.
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 86e78b05..3b1c69ba 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1078,12 +1078,24 @@ string_substitute(const char *src, int from, int to,
struct gc_arena *gc)
return ret;
}
-static uint8_t *
+/**
+ * Parses a hexstring and checks if the string has the correct length. Return
+ * a verify_hash_list containing the parsed hash string.
+ *
+ * @param str String to check/parse
+ * @param nbytes Number of bytes expected in the hexstr (e.g. 20 for
SHA1)
+ * @param msglevelmessage level to use when printing warnings/errors
+ * @param gc The returned object will be allocated in this gc
+ */
+static struct verify_hash_list *
parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct
gc_arena *gc)
{
int i;
const char *cp = str;
-uint8_t *ret = (uint8_t *) gc_malloc(nbytes, true, gc);
+
+struct verify_hash_list *ret;
+ALLOC_OBJ_CLEAR_GC(ret, struct verify_hash_list, gc);
+
char term = 1;
int byte;
char bs[3];
@@ -1102,7 +1114,7 @@ parse_hash_fingerprint(const char *str, int nbytes, int
msglevel, struct gc_aren
{
msg(msglevel, "format error in hash fingerprint hex byte: %s",
str);
}
-ret[i] = (uint8_t)byte;
+ret->hash[i] = (uint8_t)byte;
term = *cp++;
if (term != ':' && term != 0)
{
@@ -1116,10 +1128,55 @@ parse_hash_fingerprint(const char *str, int nbytes, int
msglevel, struct gc_aren
if (term != 0 || i != nbytes-1)
{
msg(msglevel, "hash fingerprint is different length than expected (%d
bytes): %s", nbytes, str);
+return NULL;
}
return ret;
}
+/**
+ * Parses a string consisting of multiple lines of hexstrings and checks if
each
+ * string has the correct length. Empty lines are ignored. Returns
+ * a linked list of (possibly) multiple verify_hash_list objects.
+ *
+ * @param str String to check/parse
+ * @param nbytes Number of bytes expected in the hexstring (e.g. 20 for
SHA1)
+ * @param msglevelmessage level to use when printing warnings/errors
+ * @param gc The returned list items will be allocated in this gc
+ */
+static str
