Re: [Openvpn-devel] [PATCH v2 07/11] Refactor extract_var_peer_info into standalone function and add ssl_util.c
Hi,
On Mon, Jan 25, 2021 at 01:56:24PM +0100, Arne Schwabe wrote:
> Our "natural" place for this function would be ssl.c but ssl.c has a lot of
> dependencies on all kinds of other compilation units so including ssl.c into
> unit tests is near impossible currently. Instead create a new file ssl_util.c
> that holds small utility functions like this one.
>
> Patch v2: add newline add the end of sll_util.h and ssl_util.c
Even if it already got an ACK, I find the function could benefit from a
v3... "if we refactor, go all the way"
- change to early-return
if (!peer_info || ((var_start = strstr(peer_info, var)) == NULL))
{
return NULL;
}
- possibly split the assignment-and-compare if() into easier to read
const char *var_start = strstr(peer_info, var);
if (!var_start)
{
return NULL;
}
- half the function has been converted to "var" and "var_start", and
the rest still talks "char *ncp_ciphers_peer"... wat? - maybe that
should be "char *value" (and "var" should be "key"?) or something.
- the v2 hunk has a newline-at-end-of-file mishap in ssl.c
> index 14c8116f..f59b409f 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -4201,4 +4201,4 @@ void
> ssl_clean_user_pass(void)
> {
> purge_user_pass(&auth_user_pass, false);
> -}
> +}
> \ No newline at end of file
(this is a new "no newline"), while v2 fixes the other one).
On the plus side, I tested "make distcheck" on linux, and all the Makefile
bits and pieces are proper (we tend to break that for new C files...)
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Add S_EXITCODE flag for openvpn_run_script to report exit code
Your patch has been applied to the master branch.
Again, out of sequence, as this does not depend on 03/11 or 05/11.
Lightly tested and stared at the code a bit. Added a line break before
a "{"...
commit 04876274b5059f4c27b1f481fd92ff5e8ab15f1c
Author: Arne Schwabe
Date: Mon Jan 25 13:56:23 2021 +0100
Add S_EXITCODE flag for openvpn_run_script to report exit code
Signed-off-by: Arne Schwabe
Acked-by: Lev Stipakov
Message-Id: <20210125125628.30364-7-a...@rfc2549.org>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21487.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Introduce management client state for AUTH_PENDING notifications
Your patch has been applied to the master branch.
As this is not depending on 03/11, I've applied it out of sequence.
One typo fixed ("techhnically"). Test run on the client, unsurprisingly
no breakage - I have nothing that excercises the new code yet, but
it still looks very reasonable :-)
commit b29f7dffc073ebd2a3b04eac5f7aee2edcf5da49
Author: Arne Schwabe
Date: Mon Jan 25 13:56:21 2021 +0100
Introduce management client state for AUTH_PENDING notifications
Signed-off-by: Arne Schwabe
Acked-by: Lev Stipakov
Message-Id: <20210125125628.30364-5-a...@rfc2549.org>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21498.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Implement client side handling of AUTH_PENDING message
Your patch has been applied to the master branch. I'm not sure I understand the code, though. It receives the new timeout from the server (that is easy), but then caps it by "hand_window", which is never increased - so the maximum timeout stays at "60", unless increased manually on the client. Where am I misreading this? Or is this a prerequisite for this functionality? c->c2.push_request_timeout = ks->established + min_uint(max_timeout, server_timeout); (or am I totally misunderstanding push_request_timeout, and this has nothing to do with hand-window, except "it's using this as a default value", but only for triggering re-sends, not for "final give up"?) However, for my standard test cases, this does not break anything, and the code looks reasonable. So in it goes :) commit 3f8fb2b2c1d664f421d36181846da89c4330c6cc Author: Arne Schwabe Date: Mon Jan 25 13:56:19 2021 +0100 Implement client side handling of AUTH_PENDING message Signed-off-by: Arne Schwabe Acked-by: Lev Stipakov Message-Id: <20210125125628.30364-3-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21491.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Allow running a default configuration with TLS libraries without BF-CBC
Hi,
On Mon, Jan 25, 2021 at 01:43:30PM +0100, Arne Schwabe wrote:
> Modern TLS libraries might drop Blowfish by default or distributions
> might disable Blowfish in OpenSSL/mbed TLS. We still signal OCC
> options with BF-CBC compatible strings. To avoid requiring BF-CBC
> for this, special this one usage of BF-CBC enough to avoid a hard
> requirement on Blowfish in the default configuration.
I was about to merge this, based on Antonio's ACK, but this part of
the code confuses me:
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b81137cf..d52057cc 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3836,18 +3856,32 @@ options_string(const struct options *o,
> + (TLS_SERVER == true)
> <= 1);
>
> -init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
> - false);
> +/* Skip resolving BF-CBC to allow SSL libraries without BF-CBC
> + * to work here in the default configuration */
> +const char *ciphername = o->ciphername;
> +int keysize;
> +
> +if (strcmp(o->ciphername, "BF-CBC") == 0) {
> +init_key_type(&kt, "none", o->authname, o->keysize, true,
> + false);
> +ciphername = cipher_kt_name(kt.cipher);
> +keysize = 128;
> +}
> +else
> +{
> +init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
> + false);
> +keysize = kt.cipher_length * 8;
> +}
> /* Only announce the cipher to our peer if we are willing to
> * support it */
> -const char *ciphername = cipher_kt_name(kt.cipher);
So the old code always sends "cipher_kt_name(kt.cipher)".
The new code adds a special case handling for "BF-CBC", calling
init_key_type(none), but then does the "cipher_kt_name()" only for
the "BF-CBC/none" case, no more for the "all other ciphers".
This looks like the wrong way around - shouldn't it do the
> +ciphername = cipher_kt_name(kt.cipher);
for the "not BF-CBC" case, and "ciphername = o->cipher" for "only BF-CBC"?
Call me confused...
As a side note, it seems to fail two of my t_client test cases,
namely "talking to a 2.3 server with --cipher BF-CBC" and "talking to
a 2.4 server with --ncp-disable", so maybe that code needs some more
discussion. I have not investigated more into these failures, first
want to understand what the code tries to do.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Check return values in md_ctx_init and hmac_ctx_init
Your patch has been applied to the master branch. Whitespace has been adjusted in a totally space-neutral way. As instructed by the master of whitespace distribution. (On the patch itself: only compile-tested, but it seems to be "obviously correct", according to the man pages) commit 0714ed804e40f80b48a7571193d7e4d81d2bcd4b Author: Arne Schwabe Date: Mon Feb 1 18:43:08 2021 +0100 Check return values in md_ctx_init and hmac_ctx_init Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli Message-Id: <20210201174310.22153-2-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21546.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
