| Message ID | 20190122150333.1061-2-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2,1/6] Rename tls_crypt_v2_read_keyfile into generic pem_read_key_file | expand |
On 22/01/2019 16:03, Arne Schwabe wrote: > From: Arne Schwabe <arne@openvpn.net> > > This is useful for features that can use either a persistent > or an ephemeral key. Agreed, this is valuable. But I generally don't like "hiding" this feature behind an argument to {read,write}_pem_key_file(). I prefer having functions doing explicit things, not "I can do this, or this, or this" depending on the arguments given. Rather add a generate_random_pem_key() which does just that. > --- > src/openvpn/crypto.c | 23 ++++++++++++++++++++--- > src/openvpn/crypto.h | 4 +++- > src/openvpn/tls_crypt.c | 5 +++-- > 3 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c > index ff9dbfdc..68a28dee 100644 > --- a/src/openvpn/crypto.c > +++ b/src/openvpn/crypto.c > @@ -1885,13 +1885,30 @@ cleanup: > > bool > read_pem_key_file(struct buffer *key, const char *pem_name, > - const char *key_file, const char *key_inline) > + const char *key_file, const char *key_inline, > + bool allow_random) > { > bool ret = false; > struct buffer key_pem = { 0 }; > struct gc_arena gc = gc_new(); > > - if (strcmp(key_file, INLINE_FILE_TAG)) > + > + if (allow_random && key_file == NULL) > + { > + msg(M_INFO, "Using random %s.", pem_name); > + uint8_t rand[BCAP(key)]; > + if (!rand_bytes(rand, BCAP(key))) > + { > + msg(M_WARN, "ERROR: could not generate random key"); > + } > + else > + { > + buf_write(key, rand, BCAP(key)); > + ret = true; > + } The if (rand_bytes(rand, BCAP(key))) logic could be improved when splitting it into a separate function. if (!rand_bytes(rand, BCAP(key))) { msg(...); return false; } buf_write(...); return true; [...] > - if (strcmp(key_file, INLINE_FILE_TAG)) > + if (key_file && strcmp(key_file, INLINE_FILE_TAG)) Is this fixing a bug? I'd recommend putting such fixes in a separate commit.
On 08/02/2019 15:50, Arne Schwabe wrote: > Am 08.02.19 um 13:30 schrieb David Sommerseth: >> On 22/01/2019 16:03, Arne Schwabe wrote: >>> From: Arne Schwabe <arne@openvpn.net> >>> >>> This is useful for features that can use either a persistent >>> or an ephemeral key. >> Agreed, this is valuable. But I generally don't like "hiding" this feature >> behind an argument to {read,write}_pem_key_file(). I prefer having functions >> doing explicit things, not "I can do this, or this, or this" depending on the >> arguments given. >> >> Rather add a generate_random_pem_key() which does just that. >> > That name would completely confusing that is not what it does. It is > more really load a key from a file or generate one on the fly. > > Should add a small wrapper function that is something > load_key_or_random? I also don't really want to rename the function > read_pem_key_file as that is the main function of it. Return the > ephermal key is just for function that can work with that. What I find confusing is: if (!read_pem_key_file(&client_key, tls_crypt_v2_cli_pem_name, key_file, key_inline, false)) Flip that 'false' to 'true' and you get a different behaviour if key_file == NULL. In this case I would prefer to have the "which key will I use" logic in auth_token_init_secret(). In the following patch 3/6, you have this: if (!read_pem_key_file(&server_secret_key, auth_token_pem_name, key_file, key_inline, true)) I would prefer: if (NULL == key_file) { generate_key_file(...); .... } else if (!read_pem_key_file(&server_secret_key, auth_token_pem_name, key_file, key_inline)) { .... } This is much more explicit in the behaviour when reading the code, you don't need to parse through the arguments of read_pem_key_file() to understand how it behaves. This will also not requiring touching tls_crypt_v2_init_client_key() or read_pem_key_file() at all. So this change would even be less intrusive. >>> - if (strcmp(key_file, INLINE_FILE_TAG)) >>> + if (key_file && strcmp(key_file, INLINE_FILE_TAG)) >> Is this fixing a bug? I'd recommend putting such fixes in a separate commit. > > No, key_file can now be null since in this case we generate the random > key material. Fine, then it makes sense where it is :)
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c index ff9dbfdc..68a28dee 100644 --- a/src/openvpn/crypto.c +++ b/src/openvpn/crypto.c @@ -1885,13 +1885,30 @@ cleanup: bool read_pem_key_file(struct buffer *key, const char *pem_name, - const char *key_file, const char *key_inline) + const char *key_file, const char *key_inline, + bool allow_random) { bool ret = false; struct buffer key_pem = { 0 }; struct gc_arena gc = gc_new(); - if (strcmp(key_file, INLINE_FILE_TAG)) + + if (allow_random && key_file == NULL) + { + msg(M_INFO, "Using random %s.", pem_name); + uint8_t rand[BCAP(key)]; + if (!rand_bytes(rand, BCAP(key))) + { + msg(M_WARN, "ERROR: could not generate random key"); + } + else + { + buf_write(key, rand, BCAP(key)); + ret = true; + } + goto cleanup; + } + else if (strcmp(key_file, INLINE_FILE_TAG)) { key_pem = buffer_read_from_file(key_file, &gc); if (!buf_valid(&key_pem)) @@ -1914,7 +1931,7 @@ read_pem_key_file(struct buffer *key, const char *pem_name, ret = true; cleanup: - if (strcmp(key_file, INLINE_FILE_TAG)) + if (key_file && strcmp(key_file, INLINE_FILE_TAG)) { buf_clear(&key_pem); } diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h index 09f7bb25..dfbfb38b 100644 --- a/src/openvpn/crypto.h +++ b/src/openvpn/crypto.h @@ -436,11 +436,13 @@ write_pem_key_file(const char *filename, const char *pem_name); * @param pem_name the name used in the pem encoding start/end lines * @param key_file name of the file to read * @param key_inline a string holding the data in case of an inline key + * @param allow_random allow generating a random key if no file is provided * @return true if reading into key was successful */ bool read_pem_key_file(struct buffer *key, const char *pem_name, - const char *key_file, const char *key_inline); + const char *key_file, const char *key_inline, + bool allow_random); /* Minimum length of the nonce used by the PRNG */ #define NONCE_SECRET_LEN_MIN 16 diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index d6a82252..baa193c3 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -301,7 +301,7 @@ tls_crypt_v2_init_client_key(struct key_ctx_bi *key, struct buffer *wkc_buf, + TLS_CRYPT_V2_MAX_WKC_LEN); if (!read_pem_key_file(&client_key, tls_crypt_v2_cli_pem_name, - key_file, key_inline)) + key_file, key_inline, false)) { msg(M_FATAL, "ERROR: invalid tls-crypt-v2 client key format"); } @@ -326,8 +326,9 @@ tls_crypt_v2_init_server_key(struct key_ctx *key_ctx, bool encrypt, struct buffer srv_key_buf; buf_set_write(&srv_key_buf, (void *)&srv_key, sizeof(srv_key)); + if (!read_pem_key_file(&srv_key_buf, tls_crypt_v2_srv_pem_name, - key_file, key_inline)) + key_file, key_inline, false)) { msg(M_FATAL, "ERROR: invalid tls-crypt-v2 server key format"); }
From: Arne Schwabe <arne@openvpn.net> This is useful for features that can use either a persistent or an ephemeral key. --- src/openvpn/crypto.c | 23 ++++++++++++++++++++--- src/openvpn/crypto.h | 4 +++- src/openvpn/tls_crypt.c | 5 +++-- 3 files changed, 26 insertions(+), 6 deletions(-)