diff mbox series

[Openvpn-devel,1/7] simplify condition detecting pure P2P mode

Message ID 20210904095629.6273-2-a@unstable.cc
State Accepted
Headers
Series change defaults and introduce compat-mode |

Commit Message

Antonio Quartulli Sept. 3, 2021, 11:56 p.m. UTC 
  The new condition is equivalent to the old one, but easier to grasp.

Also add message to inform uset that cipher negotiation, in this case,
it indeed disabled.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/options.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Arne Schwabe Sept. 6, 2021, 3:19 a.m. UTC | #1 
Am 04.09.21 um 11:56 schrieb Antonio Quartulli:
> The new condition is equivalent to the old one, but easier to grasp.
> 
> Also add message to inform uset that cipher negotiation, in this case,
> it indeed disabled.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>  src/openvpn/options.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 00ba6044..0d6b85cf 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3076,8 +3076,12 @@ options_postprocess_verify(const struct options *o)
>  static void
>  options_postprocess_cipher(struct options *o)
>  {
> -    if (!o->pull && !(o->mode == MODE_SERVER))
> +    if (!o->tls_server && !o->tls_client)
>      {
> +        /* we are in the classic P2P mode */
> +        msg(M_WARN, "Cipher negotiation is disabled since TLS "
> +                    "mode is not enabled");
> +
>          /* If the cipher is not set, use the old default of BF-CBC. We will
>           * warn that this is deprecated on cipher initialisation, no need
>           * to warn here as well */
> 


Yes. Makes sense. The change makes it is a lot more clear. I think it is
actually not equivalent but the new one is definitively the correct one.

Acked-By: Arne Schwabe <arne@rfc2549.org>

Arne
haixiao.yan.cn--- via Openvpn-devel Sept. 6, 2021, 3:30 a.m. UTC | #2 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



Sent with ProtonMail Secure Email.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On Monday, September 6th, 2021 at 14:19, Arne Schwabe <arne@rfc2549.org> wrote:

> Am 04.09.21 um 11:56 schrieb Antonio Quartulli:
>
> > The new condition is equivalent to the old one, but easier to grasp.
> >
> > Also add message to inform uset that cipher negotiation, in this case,
> >

uset -> user


> > it indeed disabled.
> >

it -> is

> > Signed-off-by: Arne Schwabe arne@rfc2549.org
> >
> > Signed-off-by: Antonio Quartulli a@unstable.cc
> > --------------------------------------------------------------------------------------------
> >
> > src/openvpn/options.c | 6 +++++-
> >
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> >
> > index 00ba6044..0d6b85cf 100644
> >
> > --- a/src/openvpn/options.c
> >
> > +++ b/src/openvpn/options.c
> >
> > @@ -3076,8 +3076,12 @@ options_postprocess_verify(const struct options *o)
> >
> > static void
> >
> > options_postprocess_cipher(struct options *o)
> >
> > {
> >
> > -   if (!o->pull && !(o->mode == MODE_SERVER))
> >
> > -   if (!o->tls_server && !o->tls_client)
> >
> >     {
> > -          /* we are in the classic P2P mode */
> >
> >
> > -          msg(M_WARN, "Cipher negotiation is disabled since TLS "
> >
> >
> > -                      "mode is not enabled");
> >
> >
> > -          /* If the cipher is not set, use the old default of BF-CBC. We will
> >             * warn that this is deprecated on cipher initialisation, no need
> >             * to warn here as well */
> >
> >
>
> Yes. Makes sense. The change makes it is a lot more clear. I think it is
>
> actually not equivalent but the new one is definitively the correct one.
>
> Acked-By: Arne Schwabe arne@rfc2549.org
>
> Arne
>
> Openvpn-devel mailing list
>
> Openvpn-devel@lists.sourceforge.net
>
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wsBzBAEBCAAGBQJhNhflACEJEE+XnPZrkLidFiEECbw9RGejjXJ5xVVVT5ec
9muQuJ2H0wgAup4i4ArQzzTGnSsFtHYAjzeKiwIPemMiOw76gv64TqI7dRST
6SsSkpS4Xs10HQysdYzyoCW4Pju2u1zEn654UeonDa43HPYtrL0V3NDmtRYT
3+aJOtqkE74MAZXpMxh1zo1KVkP+UMtpJmFVngK9IvMnSMjBnEj/1np2aZeE
KJSRKpskDgBXC5ISzo6JB3T48zbbl1+4zq8TOKrr9uvyXluw5Cme+YGL1yC5
l0ApubT29ANQMOIwBMwGaHm49sRZtkFod/hO630NNcjD/veksS2eyT0fo7oY
P38ZVT0rbQWO06D1/0D4PGlXeukYHa0aVRqCR+nWm01dqa22gedDzg==
=S+qj
-----END PGP SIGNATURE-----
Arne Schwabe Sept. 7, 2021, 1:10 a.m. UTC | #3 
> 
> Yes. Makes sense. The change makes it is a lot more clear. I think it is
> actually not equivalent but the new one is definitively the correct one.
>

I looked at the code again and I have to actually retract my ACK.

The previous code means P2P mode with static key or P2P mode without
--pull while the new condition means only P2P mode with static key.


The code that follows for that section should be better commented:

        if (!o->ciphername)
        {
            o->ciphername = "BF-CBC";
        }
        else
        {
            o->enable_ncp_fallback = true;
        }
        return;

It basically condeses to having always a valid string in o->ciphername,
which is then used in static key code (which ignores data-ciphers) or
allowing falling back to the cipher explicitly set via --cipher if we
are in TLS mode (with NCP) but not to the implicit BF-CBC.

Arne
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 00ba6044..0d6b85cf 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3076,8 +3076,12 @@  options_postprocess_verify(const struct options *o)
 static void
 options_postprocess_cipher(struct options *o)
 {
-    if (!o->pull && !(o->mode == MODE_SERVER))
+    if (!o->tls_server && !o->tls_client)
     {
+        /* we are in the classic P2P mode */
+        msg(M_WARN, "Cipher negotiation is disabled since TLS "
+                    "mode is not enabled");
+
         /* If the cipher is not set, use the old default of BF-CBC. We will
          * warn that this is deprecated on cipher initialisation, no need
          * to warn here as well */