| Message ID | 20200711093655.23686-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v5,01/14] Allow changing fallback cipher from ccd files/client-connect | expand |
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
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)
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(-)