[Openvpn-devel,v5,01/14] Allow changing fallback cipher from ccd files/client-connect

  This allows to control the fallback cipher that is used when the
client/server do have any common cipher on a per client basis.

The patch is similar to Steffan's
[PATCH v4] Allow changing cipher from a ccd file.

Steffan's old patch also moves the cipher negotiation to
multi_established_connection() which I independently discovered and
implemented in

Extract process_incoming_push_reply from process_incoming_push_msg
(#FIXME add commitsh when commited to master)

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/options.c | 2 +-
 src/openvpn/options.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
  

Acked-by: Gert Doering <gert@greenie.muc.de>

The patch is trivial enough (it just allows "cipher" in ccd/ files, with
no logic changes) - it's built on the changes in the previous patches, which
makes it "just work".

Without the patch, trying to set & push a cipher from ccd/:

Jul 11 18:27:53 gentoo tap-udp-p2mp[12620]: Options error: option 'cipher' cannot be used in this context (ccd/freebsd-74-amd64)
Jul 11 18:27:55 gentoo tap-udp-p2mp[12620]: ... SENT CONTROL [freebsd-74-amd64]: 'PUSH_REPLY,...,cipher CAMELLIA-128-CBC,...,cipher AES-256-GCM' (status=1)

With the patch *and* forcing NCP on the server side by only allowing 
CAMELLIA-128-CBC:

  $ cat ccd/freebsd-74-amd64
  ncp-ciphers CAMELLIA-128-CBC
  cipher CAMELLIA-128-CBC

it will actually do that:

Jul 11 18:42:37 gentoo tap-udp-p2mp[13661]: Outgoing Data Channel: Cipher 'CAMELLIA-128-CBC' initialized with 128 bit key
Jul 11 18:42:37 gentoo tap-udp-p2mp[13661]: Incoming Data Channel: Cipher 'CAMELLIA-128-CBC' initialized with 128 bit key
Jul 11 18:42:38 gentoo tap-udp-p2mp[13661]: SENT CONTROL [freebsd-74-amd64]: 'PUSH_REPLY,...,peer-id 2,cipher CAMELLIA-128-CBC' (status=1)

(if I put "CAMELLIA and some of the AES-GCM variants" in there, I get the 
standard AES-256-GCM or AES-128-GCM variants - with no indication in the 
logs on why it doesn't want to take the cipher --> documenting this here,
so it can be found by googling: if you want "cipher" to work in CCD/ files,
you must also set "ncp-ciphers" accordingly).

Your patch has been applied to the master branch.

commit 6168f53d6b7274026d4f392a22e64524a9b264d6
Author: Arne Schwabe
Date:   Sat Jul 11 11:36:42 2020 +0200

     Allow changing fallback cipher from ccd files/client-connect

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200711093655.23686-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20281.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
  

Am 11.07.2020 um 18:48 schrieb Gert Doering:
> Acked-by: Gert Doering <gert@greenie.muc.de>
>
> The patch is trivial enough (it just allows "cipher" in ccd/ files, with
> no logic changes) - it's built on the changes in the previous patches, which
> makes it "just work".
>
> Without the patch, trying to set & push a cipher from ccd/:
>
> Jul 11 18:27:53 gentoo tap-udp-p2mp[12620]: Options error: option 'cipher' cannot be used in this context (ccd/freebsd-74-amd64)
> Jul 11 18:27:55 gentoo tap-udp-p2mp[12620]: ... SENT CONTROL [freebsd-74-amd64]: 'PUSH_REPLY,...,cipher CAMELLIA-128-CBC,...,cipher AES-256-GCM' (status=1)
>
> With the patch *and* forcing NCP on the server side by only allowing 
> CAMELLIA-128-CBC:
>
>   $ cat ccd/freebsd-74-amd64
>   ncp-ciphers CAMELLIA-128-CBC
>   cipher CAMELLIA-128-CBC
>
> it will actually do that:
>
> Jul 11 18:42:37 gentoo tap-udp-p2mp[13661]: Outgoing Data Channel: Cipher 'CAMELLIA-128-CBC' initialized with 128 bit key
> Jul 11 18:42:37 gentoo tap-udp-p2mp[13661]: Incoming Data Channel: Cipher 'CAMELLIA-128-CBC' initialized with 128 bit key
> Jul 11 18:42:38 gentoo tap-udp-p2mp[13661]: SENT CONTROL [freebsd-74-amd64]: 'PUSH_REPLY,...,peer-id 2,cipher CAMELLIA-128-CBC' (status=1)
>
> (if I put "CAMELLIA and some of the AES-GCM variants" in there, I get the 
> standard AES-256-GCM or AES-128-GCM variants - with no indication in the 
> logs on why it doesn't want to take the cipher --> documenting this here,
> so it can be found by googling: if you want "cipher" to work in CCD/ files,

cipher only sets the fallback cipher if we find no common cipher. All
ciphers in ncp-ciphers are still preferred to cipher. So to have the
server pick the --cipher from the either general config or ccd config,
none of the cipher in ncp-ciphers may be supported by the peer (so not
in ncp-ciphers/ncp-ciphers and not as --cipher)

Arne
  

Hi,

On Sun, Jul 12, 2020 at 01:28:56AM +0200, Arne Schwabe wrote:
> > With the patch *and* forcing NCP on the server side by only allowing 
> > CAMELLIA-128-CBC:
> >
> >   $ cat ccd/freebsd-74-amd64
> >   ncp-ciphers CAMELLIA-128-CBC
> >   cipher CAMELLIA-128-CBC
> >
> > it will actually do that:
[..]
> 
> cipher only sets the fallback cipher if we find no common cipher. All
> ciphers in ncp-ciphers are still preferred to cipher. So to have the
> server pick the --cipher from the either general config or ccd config,
> none of the cipher in ncp-ciphers may be supported by the peer (so not
> in ncp-ciphers/ncp-ciphers and not as --cipher)

More details on the scenario:

The client here is a stock 2.4 client, with "nothing" in the config - 
so it sends IV_NCP=1, but no cipher list, and OCC cipher is "bf-cbc".

In ccd/, if I have *just* "ncp-ciphers CAMELLIA-128-CBC", it will actually
fallback to "bf-cbc".  Which matches your description: no common ciphers
(IV_NCP=1 = AES-128-GCM:AES-256-GCM) -> fallback cipher (bf-cbc).


So, shorter: you're right :-) - and if we want to force a cipher for a
NCP-capable client, it needs "cipher" *and* "ncp-ciphers" in ccd/, because
otherwise NCP will just override our config.

gert
  

grammar:

On 11/07/2020 10:36, Arne Schwabe wrote:
> This allows to control the fallback cipher that is used when the
> client/server do have any common cipher on a per client basis.

client/server do not have any common cipher


> 
> The patch is similar to Steffan's
> [PATCH v4] Allow changing cipher from a ccd file.
> 
> Steffan's old patch also moves the cipher negotiation to
> multi_established_connection() which I independently discovered and
> implemented in

implemented in:

(otherwise it looks like you forgot "in what" - That is if my 
interpretation is correct)

> 
> Extract process_incoming_push_reply from process_incoming_push_msg
> (#FIXME add commitsh when commited to master)
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/options.c | 2 +-
>   src/openvpn/options.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b93fd4fe..bf2760e1 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -7892,7 +7892,7 @@ add_option(struct options *options,
>       }
>       else if (streq(p[0], "cipher") && p[1] && !p[2])
>       {
> -        VERIFY_PERMISSION(OPT_P_NCP);
> +        VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>           options->ciphername = p[1];
>       }
>       else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index c83a46aa..c37006d3 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -677,7 +677,7 @@ struct options
>   #define OPT_P_MTU             (1<<14) /* TODO */
>   #define OPT_P_NICE            (1<<15)
>   #define OPT_P_PUSH            (1<<16)
> -#define OPT_P_INSTANCE        (1<<17)
> +#define OPT_P_INSTANCE        (1<<17) /**< allowed in ccd, client-connect etc*/
>   #define OPT_P_CONFIG          (1<<18)
>   #define OPT_P_EXPLICIT_NOTIFY (1<<19)
>   #define OPT_P_ECHO            (1<<20)
>
  

Hi,

On Mon, Jul 13, 2020 at 01:30:11PM +0100, tincanteksup wrote:
> grammar:
> 
> On 11/07/2020 10:36, Arne Schwabe wrote:
> > This allows to control the fallback cipher that is used when the
> > client/server do have any common cipher on a per client basis.
> 
> client/server do not have any common cipher

Too late, that patch went in yesterday already.

> (otherwise it looks like you forgot "in what" - That is if my 
> interpretation is correct)

It's missing the commit ID here, which I added :-) - so the commit
message is a bit more complete.

gert