[Openvpn-devel,8/9] Rename ncp-ciphers to data-ciphers

Message ID 20200717134739.21168-8-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,1/9] Indicate that a client is in pull mode in IV_PROTO | expand

Commit Message

Arne Schwabe July 17, 2020, 3:47 a.m. UTC
The change in name signals that data-ciphers is the preferred way to
configure data channel (and not --cipher). The data prefix is chosen
to avoid ambiguity and make it distinct from tls-cipher for the TLS
ciphers.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst                            | 13 ++++++++++---
 doc/man-sections/protocol-options.rst  | 11 +++++++----
 doc/man-sections/server-options.rst    |  4 ++--
 sample/sample-config-files/client.conf |  2 +-
 src/openvpn/multi.c                    |  4 ++--
 src/openvpn/options.c                  |  5 +++--
 src/openvpn/ssl_ncp.c                  |  4 ++--
 7 files changed, 27 insertions(+), 16 deletions(-)

Comments

David Sommerseth July 22, 2020, 4:32 a.m. UTC | #1
On 17/07/2020 15:47, Arne Schwabe wrote:
> The change in name signals that data-ciphers is the preferred way to
> configure data channel (and not --cipher). The data prefix is chosen
> to avoid ambiguity and make it distinct from tls-cipher for the TLS
> ciphers.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  Changes.rst                            | 13 ++++++++++---
>  doc/man-sections/protocol-options.rst  | 11 +++++++----
>  doc/man-sections/server-options.rst    |  4 ++--
>  sample/sample-config-files/client.conf |  2 +-
>  src/openvpn/multi.c                    |  4 ++--
>  src/openvpn/options.c                  |  5 +++--
>  src/openvpn/ssl_ncp.c                  |  4 ++--
>  7 files changed, 27 insertions(+), 16 deletions(-)
> 
[...snip...]
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 31e33ae3..896abcde 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -536,7 +536,7 @@ static const char usage_message[] =
>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>      "                  (default=%s).\n"
>      "                  Set alg=none to disable encryption.\n"
> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
> +    "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>      "--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
>      "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
>      "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>          options->ciphername = p[1];
>      }
> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
> +            && p[1] && !p[2])

I do agree to using --data-ciphers instead of --ncp-ciphers, that is far more
user-friendly naming of this feature.  NCP is a more technical
"under-the-hood" terminology which users don't really need to relate to, where
--data-ciphers better explains what it is used for.

But I do reject NOT adding a deprecation path for --ncp-ciphers.  We should
support --ncp-ciphers for 1-2 major releases, but after that it should be
removed.  We have too many options and we certainly should avoid duplicating
options with the exact same functionality.
Arne Schwabe July 23, 2020, 1:36 a.m. UTC | #2
>> +++ b/src/openvpn/options.c
>> @@ -536,7 +536,7 @@ static const char usage_message[] =
>>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>>      "                  (default=%s).\n"
>>      "                  Set alg=none to disable encryption.\n"
>> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>> +    "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>>      "--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
>>      "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
>>      "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
>> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>          options->ciphername = p[1];
>>      }
>> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
>> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>> +            && p[1] && !p[2])
> 
> I do agree to using --data-ciphers instead of --ncp-ciphers, that is far more
> user-friendly naming of this feature.  NCP is a more technical
> "under-the-hood" terminology which users don't really need to relate to, where
> --data-ciphers better explains what it is used for.
> 
> But I do reject NOT adding a deprecation path for --ncp-ciphers.  We should
> support --ncp-ciphers for 1-2 major releases, but after that it should be
> removed.  We have too many options and we certainly should avoid duplicating
> options with the exact same functionality.

This was a deliberate decision. We really want to people to move towards
ncp and putting another hurdle with having an option that works better
on but gives a warning and a option that does not work on 2.4 does not
help here. If we decide that really aliases are a no-go in OpenVPN then
I would rather drop data-ciphers and stay with ncp-ciphers forever for
this reason.

Arne
David Sommerseth July 23, 2020, 6:09 a.m. UTC | #3
On 23/07/2020 13:36, Arne Schwabe wrote:
> 
>>> +++ b/src/openvpn/options.c
>>> @@ -536,7 +536,7 @@ static const char usage_message[] =
>>>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>>>      "                  (default=%s).\n"
>>>      "                  Set alg=none to disable encryption.\n"
>>> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>>> +    "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>>>      "--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
>>>      "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
>>>      "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
>>> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>>>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>>          options->ciphername = p[1];
>>>      }
>>> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
>>> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>>> +            && p[1] && !p[2])
>>
>> I do agree to using --data-ciphers instead of --ncp-ciphers, that is far more
>> user-friendly naming of this feature.  NCP is a more technical
>> "under-the-hood" terminology which users don't really need to relate to, where
>> --data-ciphers better explains what it is used for.
>>
>> But I do reject NOT adding a deprecation path for --ncp-ciphers.  We should
>> support --ncp-ciphers for 1-2 major releases, but after that it should be
>> removed.  We have too many options and we certainly should avoid duplicating
>> options with the exact same functionality.

Just let me clarify .... the "1-2 major releases" was too hasty and not well
thought numbers.  Please read my reasoning below for what I really intended to
say.

> This was a deliberate decision. We really want to people to move towards
> ncp and putting another hurdle with having an option that works better
> on but gives a warning and a option that does not work on 2.4 does not
> help here. If we decide that really aliases are a no-go in OpenVPN then
> I would rather drop data-ciphers and stay with ncp-ciphers forever for
> this reason.

Lets take a few steps back try to see a broader picture.

* --ncp-ciphers was introduced in OpenVPN 2.4 as a brand new option.

* Steffan has suggested to add --data-ciphers alias into the next v2.4
  release; to which I agree(!).

* I propose we add a warning whenever --ncp-ciphers used, recommending the
  user to switch to --data-ciphers as soon as possible.

* We keep both --ncp-ciphers *and* --data-ciphers in v2.5 and v2.6.

* When v2.5 is released v2.4 goes into "old stable support" - where both
  alternatives will work.  v2.4 is guaranteed to be supported by the community
  for at least 6 months with full support [0] after v2.5 is released.

* When 2.6 is released, v2.4 goes into "git tree only" support but will have a

  12 month "old stable support" guarantee [0].

* 12 months *after* the v2.6 release is out, we can remove --ncp-ciphers.  But
  since we will not do option changes mid-release easily, it might be natural
  to remove this in OpenVPN 2.7 at earliest.

This means --ncp-ciphers will be supported in 2.4 for the life time of that
release.  And v2.4 is supported for *at least* 18 months after v2.5 is
released.  It also guarantees --ncp-ciphers will first be removed when we
release OpenVPN 2.7.

What would be a problem with such a schedule?  Because I don't understand the
real objection you have to remove options which ends up duplicating other options.


At the same time ... it is also important for me that we try to see this at
from an even bigger perspective than just --ncp-ciphers/--data-ciphers or even
--udp-mtu alone.  Now I'm shifting the discussion to a more generic product
life cycle perspective.

If we skimp through this and just allow whatever option duplications we head
into, just because we're too concerned about various breakages with old
configurations or setups - it will not be the last time we might end up in
such discussions UNLESS we can find a proper and reasonable process for
deprecation (which we in fact already defined for feature deprecation 10 years
ago! [1]).

If we cannot remove options during the whole life time of OpenVPN, we cannot
touch compression options or possibly even deprecate any compression at all -
because we need to support both compression and decompression for the whole
lifetime of OpenVPN.  This also extends into how to ensure proper OpenVPN 3
compatibility as well.

And if we cannot remove any options during the eternal life time of OpenVPN, I
will see no other alternative to being even more critical and rejective to add
new options.  We already have pretty close to 300 options in OpenVPN.  That is
an insane amount - where a typical common OpenVPN setup might need up to 20 of
these options, the rest are for all kinds of special cases.  And we have
several competing solutions which are far simpler in many aspects which new
users might just as well prefer.

Even though I highlight the number of options we have, I do NOT say that we
should swipe them all out and reduce the amount to 50 or so; for some users
they have a _real_ value and provides really useful features.  But I want us
to save the options which do have a REAL value, not because unsupported
OpenVPN versions might break or "10 bytes extra source code" is too heavy to
carry around for an alias option.  I'm arguing for keeping options covering
_REAL_ USE CASE for users.  And no, breaking unsupported OpenVPN releases is
NOT a real use case from my point of view.

But back to the deprecated options  ... If we cannot remove options, we also
need to consider bringing back the --tls-remote option, and --compat-names -
both with the capability to break client configs (in fact, it already did for
several Fedora EPEL users [2]).  The proposal to remove --remote-nopull needs
to be reconsidered, as well as --secret, --max-routes, and possibly even
--ncp-disable.  Maybe even more of these deprecated options needs to be
reconsidered [3].


We really need a proper and sane processes to allow the development of OpenVPN
to have a chance to move on and leave things behind when appropriate, to be
able to evolve and grow with the future - without being strangled by what
existed in the far past (meaning: no longer community supported releases).
Otherwise I do fear for the future of OpenVPN 2.x.

By having a clear strategy and adhering to a process of feature/option
management in OpenVPN, we give clearly defined time-window for stability and
functionality for our users.  This predictability is, in my experience, much
more important to users than if a specifically named option is supported or not.



[0] <https://community.openvpn.net/openvpn/wiki/SupportedVersions>
[1] <https://community.openvpn.net/openvpn/wiki/FRP>
[2]
<https://src.fedoraproject.org/rpms/openvpn/c/c738486b3576df4829c9cef5ccd12c10c4fb7b84?branch=epel7>
[3] <https://community.openvpn.net/openvpn/wiki/DeprecatedOptions>
Steffan Karger July 23, 2020, 10:06 p.m. UTC | #4
Hi,

On 23-07-2020 18:09, David Sommerseth wrote:
>> This was a deliberate decision. We really want to people to move towards
>> ncp and putting another hurdle with having an option that works better
>> on but gives a warning and a option that does not work on 2.4 does not
>> help here. If we decide that really aliases are a no-go in OpenVPN then
>> I would rather drop data-ciphers and stay with ncp-ciphers forever for
>> this reason.
> 
> Lets take a few steps back try to see a broader picture.
> 
> [..snip..]
> 
> We really need a proper and sane processes to allow the development of OpenVPN
> to have a chance to move on and leave things behind when appropriate, to be
> able to evolve and grow with the future - without being strangled by what
> existed in the far past (meaning: no longer community supported releases).
> Otherwise I do fear for the future of OpenVPN 2.x.
> 
> By having a clear strategy and adhering to a process of feature/option
> management in OpenVPN, we give clearly defined time-window for stability and
> functionality for our users.  This predictability is, in my experience, much
> more important to users than if a specifically named option is supported or not.

Yes, you've made these points clear earlier on IRC. I (and with me Arne
and Gert) just don't agree with you on some of the details, resulting in
a different verdict on this patch.

None of us has trouble with deprecating options. We appreciate the work
you've put into the DeprecatedOptions page, and all of us have sent
and/or acked patched to remove dangerous or obsolete options.

This difference is in how we weigh the pros and cons per option. So I'll
leave the broader picture for now, and summarize why I'm going to ACK
Arne's patch exactly because is *doesn't* print a warning when the old
name is used.

Option name aliases add negligible code complexity and are trivial to
maintain. (Just look at --udp-mtu.) Keeping them in allows users to
write configs that work well and do not produce any warnings on both
older and newer versions. (Printing warnings for harmless things reduces
the value of the other warnings we print.)

Let's focus our time and effort on reducing actual complexity.

-Steffan
Steffan Karger July 23, 2020, 10:14 p.m. UTC | #5
Hi,

On 17-07-2020 15:47, Arne Schwabe wrote:
> The change in name signals that data-ciphers is the preferred way to
> configure data channel (and not --cipher). The data prefix is chosen
> to avoid ambiguity and make it distinct from tls-cipher for the TLS
> ciphers.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  Changes.rst                            | 13 ++++++++++---
>  doc/man-sections/protocol-options.rst  | 11 +++++++----
>  doc/man-sections/server-options.rst    |  4 ++--
>  sample/sample-config-files/client.conf |  2 +-
>  src/openvpn/multi.c                    |  4 ++--
>  src/openvpn/options.c                  |  5 +++--
>  src/openvpn/ssl_ncp.c                  |  4 ++--
>  7 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 6e283270..2158c8e7 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -14,12 +14,19 @@ ChaCha20-Poly1305 cipher support
>      channel.
>  
>  Improved Data channel cipher negotiation
> +    The option ``ncp-ciphers`` has been renamed to ``data-ciphers``.
> +    The old name is still accepted. The change in name signals that
> +    ``data-ciphers`` is the preferred way to configure data channel
> +    ciphers and the data prefix is chosen to avoid the ambiguity that
> +    exists with ``--cipher`` for the data cipher and ``tls-cipher``
> +    for the TLS ciphers.
> +
>      OpenVPN clients will now signal all supported ciphers from the
> -    ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
> -    servers will select the first common cipher from the ``ncp-ciphers``
> +    ``data-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
> +    servers will select the first common cipher from the ``data-ciphers``
>      list instead of blindly pushing the first cipher of the list. This
>      allows to use a configuration like
> -    ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
> +    ``data-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
>      prefers ChaCha20-Poly1305 but uses it only if the client supports it.
>  
>  Asynchronous (deferred) authentication support for auth-pam plugin.
> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
> index 923d2da0..051f1d32 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -62,7 +62,7 @@ configured in a compatible way between both the local and remote side.
>    The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
>    Block Chaining mode. When cipher negotiation (NCP) is allowed,
>    OpenVPN 2.4 and newer on both client and server side will automatically
> -  upgrade to :code:`AES-256-GCM`.  See ``--ncp-ciphers`` and
> +  upgrade to :code:`AES-256-GCM`.  See ``--data-ciphers`` and
>    ``--ncp-disable`` for more details on NCP.
>  
>    Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
> @@ -169,7 +169,7 @@ configured in a compatible way between both the local and remote side.
>    non-standard key lengths, and a larger key may offer no real guarantee
>    of greater security, or may even reduce security.
>  
> ---ncp-ciphers cipher-list
> +--data-ciphers cipher-list
>    Restrict the allowed ciphers to be negotiated to the ciphers in
>    ``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
>    and defaults to :code:`AES-256-GCM:AES-128-GCM`.
> @@ -189,9 +189,9 @@ configured in a compatible way between both the local and remote side.
>    Additionally, to allow for more smooth transition, if NCP is enabled,
>    OpenVPN will inherit the cipher of the peer if that cipher is different
>    from the local ``--cipher`` setting, but the peer cipher is one of the
> -  ciphers specified in ``--ncp-ciphers``. E.g. a non-NCP client (<=v2.3,
> +  ciphers specified in ``--data-ciphers``. E.g. a non-NCP client (<=v2.3,
>    or with --ncp-disabled set) connecting to a NCP server (v2.4+) with
> -  ``--cipher BF-CBC`` and ``--ncp-ciphers AES-256-GCM:AES-256-CBC`` set can
> +  ``--cipher BF-CBC`` and ``--data-ciphers AES-256-GCM:AES-256-CBC`` set can
>    either specify ``--cipher BF-CBC`` or ``--cipher AES-256-CBC`` and both
>    will work.
>  
> @@ -201,6 +201,9 @@ configured in a compatible way between both the local and remote side.
>    This list is restricted to be 127 chars long after conversion to OpenVPN
>    ciphers.
>  
> +  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> +  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> +
>  --ncp-disable
>    Disable "Negotiable Crypto Parameters". This completely disables cipher
>    negotiation.
> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
> index c24aec0b..74ad5e18 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -473,8 +473,8 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>          *AES-GCM-128* and *AES-GCM-256*.
>  
>    :code:`IV_CIPHERS=<ncp-ciphers>`
> -        The client pushes the list of configured ciphers with the
> -        ``--ciphers`` option to the server.
> +        The client announces the list of supported ciphers configured with the
> +        ``--data-ciphers`` option to the server.
>  
>    :code:`IV_GUI_VER=<gui_id> <version>`
>          The UI version of a UI if one is running, for example
> diff --git a/sample/sample-config-files/client.conf b/sample/sample-config-files/client.conf
> index 7f2f30a3..47ca4099 100644
> --- a/sample/sample-config-files/client.conf
> +++ b/sample/sample-config-files/client.conf
> @@ -112,7 +112,7 @@ tls-auth ta.key 1
>  # then you must also specify it here.
>  # Note that v2.4 client/server will automatically
>  # negotiate AES-256-GCM in TLS mode.
> -# See also the ncp-cipher option in the manpage
> +# See also the data-ciphers option in the manpage
>  cipher AES-256-CBC
>  
>  # Enable compression on the VPN link.
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 88ba9db2..d2549bca 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1827,7 +1827,7 @@ multi_client_set_protocol_options(struct context *c)
>          else
>          {
>              /*
> -             * Push the first cipher from --ncp-ciphers to the client that
> +             * Push the first cipher from --data-ciphers to the client that
>               * the client announces to be supporting.
>               */
>              char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
> @@ -1847,7 +1847,7 @@ multi_client_set_protocol_options(struct context *c)
>                  {
>                      msg(M_INFO, "PUSH: No common cipher between server and "
>                          "client. Expect this connection not to work. Server "
> -                        "ncp-ciphers: '%s', client supported ciphers '%s'",
> +                        "data-ciphers: '%s', client supported ciphers '%s'",
>                          o->ncp_ciphers, peer_ciphers);
>                  }
>                  else
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 31e33ae3..896abcde 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -536,7 +536,7 @@ static const char usage_message[] =
>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>      "                  (default=%s).\n"
>      "                  Set alg=none to disable encryption.\n"
> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
> +    "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>      "--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
>      "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
>      "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>          options->ciphername = p[1];
>      }
> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
> +            && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>          options->ncp_ciphers = p[1];
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index e057a40b..6760884e 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -111,7 +111,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>          const cipher_kt_t *ktc = cipher_kt_get(token);
>          if (!ktc)
>          {
> -            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
> +            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
>              error_found = true;
>          }
>          else
> @@ -130,7 +130,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>              if (!(buf_forward_capacity(&new_list) >
>                    strlen(ovpn_cipher_name) + 2))
>              {
> -                msg(M_WARN, "Length of --ncp-ciphers is over the "
> +                msg(M_WARN, "Length of --data-ciphers is over the "
>                      "limit of 127 chars");
>                  error_found = true;
>              }
> 

Thanks. Patch looks good, and passes manual tests.

Acked-by: Steffan Karger <steffan@karger.me>

-Steffan
David Sommerseth July 23, 2020, 10:56 p.m. UTC | #6
On 24/07/2020 10:14, Steffan Karger wrote:
> Hi,
> 
> On 17-07-2020 15:47, Arne Schwabe wrote:
>> The change in name signals that data-ciphers is the preferred way to
>> configure data channel (and not --cipher). The data prefix is chosen
>> to avoid ambiguity and make it distinct from tls-cipher for the TLS
>> ciphers.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>  Changes.rst                            | 13 ++++++++++---
>>  doc/man-sections/protocol-options.rst  | 11 +++++++----
>>  doc/man-sections/server-options.rst    |  4 ++--
>>  sample/sample-config-files/client.conf |  2 +-
>>  src/openvpn/multi.c                    |  4 ++--
>>  src/openvpn/options.c                  |  5 +++--
>>  src/openvpn/ssl_ncp.c                  |  4 ++--
>>  7 files changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/Changes.rst b/Changes.rst
>> index 6e283270..2158c8e7 100644
>> --- a/Changes.rst
>> +++ b/Changes.rst
>> @@ -14,12 +14,19 @@ ChaCha20-Poly1305 cipher support
>>      channel.
>>  
>>  Improved Data channel cipher negotiation
>> +    The option ``ncp-ciphers`` has been renamed to ``data-ciphers``.
>> +    The old name is still accepted. The change in name signals that
>> +    ``data-ciphers`` is the preferred way to configure data channel
>> +    ciphers and the data prefix is chosen to avoid the ambiguity that
>> +    exists with ``--cipher`` for the data cipher and ``tls-cipher``
>> +    for the TLS ciphers.
>> +
>>      OpenVPN clients will now signal all supported ciphers from the
>> -    ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
>> -    servers will select the first common cipher from the ``ncp-ciphers``
>> +    ``data-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
>> +    servers will select the first common cipher from the ``data-ciphers``
>>      list instead of blindly pushing the first cipher of the list. This
>>      allows to use a configuration like
>> -    ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
>> +    ``data-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
>>      prefers ChaCha20-Poly1305 but uses it only if the client supports it.
>>  
>>  Asynchronous (deferred) authentication support for auth-pam plugin.
>> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
>> index 923d2da0..051f1d32 100644
>> --- a/doc/man-sections/protocol-options.rst
>> +++ b/doc/man-sections/protocol-options.rst
>> @@ -62,7 +62,7 @@ configured in a compatible way between both the local and remote side.
>>    The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
>>    Block Chaining mode. When cipher negotiation (NCP) is allowed,
>>    OpenVPN 2.4 and newer on both client and server side will automatically
>> -  upgrade to :code:`AES-256-GCM`.  See ``--ncp-ciphers`` and
>> +  upgrade to :code:`AES-256-GCM`.  See ``--data-ciphers`` and
>>    ``--ncp-disable`` for more details on NCP.
>>  
>>    Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
>> @@ -169,7 +169,7 @@ configured in a compatible way between both the local and remote side.
>>    non-standard key lengths, and a larger key may offer no real guarantee
>>    of greater security, or may even reduce security.
>>  
>> ---ncp-ciphers cipher-list
>> +--data-ciphers cipher-list
>>    Restrict the allowed ciphers to be negotiated to the ciphers in
>>    ``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
>>    and defaults to :code:`AES-256-GCM:AES-128-GCM`.
>> @@ -189,9 +189,9 @@ configured in a compatible way between both the local and remote side.
>>    Additionally, to allow for more smooth transition, if NCP is enabled,
>>    OpenVPN will inherit the cipher of the peer if that cipher is different
>>    from the local ``--cipher`` setting, but the peer cipher is one of the
>> -  ciphers specified in ``--ncp-ciphers``. E.g. a non-NCP client (<=v2.3,
>> +  ciphers specified in ``--data-ciphers``. E.g. a non-NCP client (<=v2.3,
>>    or with --ncp-disabled set) connecting to a NCP server (v2.4+) with
>> -  ``--cipher BF-CBC`` and ``--ncp-ciphers AES-256-GCM:AES-256-CBC`` set can
>> +  ``--cipher BF-CBC`` and ``--data-ciphers AES-256-GCM:AES-256-CBC`` set can
>>    either specify ``--cipher BF-CBC`` or ``--cipher AES-256-CBC`` and both
>>    will work.
>>  
>> @@ -201,6 +201,9 @@ configured in a compatible way between both the local and remote side.
>>    This list is restricted to be 127 chars long after conversion to OpenVPN
>>    ciphers.
>>  
>> +  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
>> +  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
>> +
>>  --ncp-disable
>>    Disable "Negotiable Crypto Parameters". This completely disables cipher
>>    negotiation.
>> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
>> index c24aec0b..74ad5e18 100644
>> --- a/doc/man-sections/server-options.rst
>> +++ b/doc/man-sections/server-options.rst
>> @@ -473,8 +473,8 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>>          *AES-GCM-128* and *AES-GCM-256*.
>>  
>>    :code:`IV_CIPHERS=<ncp-ciphers>`
>> -        The client pushes the list of configured ciphers with the
>> -        ``--ciphers`` option to the server.
>> +        The client announces the list of supported ciphers configured with the
>> +        ``--data-ciphers`` option to the server.
>>  
>>    :code:`IV_GUI_VER=<gui_id> <version>`
>>          The UI version of a UI if one is running, for example
>> diff --git a/sample/sample-config-files/client.conf b/sample/sample-config-files/client.conf
>> index 7f2f30a3..47ca4099 100644
>> --- a/sample/sample-config-files/client.conf
>> +++ b/sample/sample-config-files/client.conf
>> @@ -112,7 +112,7 @@ tls-auth ta.key 1
>>  # then you must also specify it here.
>>  # Note that v2.4 client/server will automatically
>>  # negotiate AES-256-GCM in TLS mode.
>> -# See also the ncp-cipher option in the manpage
>> +# See also the data-ciphers option in the manpage
>>  cipher AES-256-CBC
>>  
>>  # Enable compression on the VPN link.
>> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
>> index 88ba9db2..d2549bca 100644
>> --- a/src/openvpn/multi.c
>> +++ b/src/openvpn/multi.c
>> @@ -1827,7 +1827,7 @@ multi_client_set_protocol_options(struct context *c)
>>          else
>>          {
>>              /*
>> -             * Push the first cipher from --ncp-ciphers to the client that
>> +             * Push the first cipher from --data-ciphers to the client that
>>               * the client announces to be supporting.
>>               */
>>              char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
>> @@ -1847,7 +1847,7 @@ multi_client_set_protocol_options(struct context *c)
>>                  {
>>                      msg(M_INFO, "PUSH: No common cipher between server and "
>>                          "client. Expect this connection not to work. Server "
>> -                        "ncp-ciphers: '%s', client supported ciphers '%s'",
>> +                        "data-ciphers: '%s', client supported ciphers '%s'",
>>                          o->ncp_ciphers, peer_ciphers);
>>                  }
>>                  else
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index 31e33ae3..896abcde 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -536,7 +536,7 @@ static const char usage_message[] =
>>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>>      "                  (default=%s).\n"
>>      "                  Set alg=none to disable encryption.\n"
>> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>> +    "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
>>      "--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
>>      "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
>>      "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
>> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>          options->ciphername = p[1];
>>      }
>> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
>> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>> +            && p[1] && !p[2])
>>      {
>>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>>          options->ncp_ciphers = p[1];
>> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
>> index e057a40b..6760884e 100644
>> --- a/src/openvpn/ssl_ncp.c
>> +++ b/src/openvpn/ssl_ncp.c
>> @@ -111,7 +111,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>>          const cipher_kt_t *ktc = cipher_kt_get(token);
>>          if (!ktc)
>>          {
>> -            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
>> +            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
>>              error_found = true;
>>          }
>>          else
>> @@ -130,7 +130,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>>              if (!(buf_forward_capacity(&new_list) >
>>                    strlen(ovpn_cipher_name) + 2))
>>              {
>> -                msg(M_WARN, "Length of --ncp-ciphers is over the "
>> +                msg(M_WARN, "Length of --data-ciphers is over the "
>>                      "limit of 127 chars");
>>                  error_found = true;
>>              }
>>
> 
> Thanks. Patch looks good, and passes manual tests.
> 
> Acked-by: Steffan Karger <steffan@karger.me>

NAK.
Arne Schwabe July 24, 2020, 12:45 a.m. UTC | #7
First of all I did not want to reply to this since we had a lengthy
discussion on IRC.

> Lets take a few steps back try to see a broader picture.
> 
> * --ncp-ciphers was introduced in OpenVPN 2.4 as a brand new option.
> 
> * Steffan has suggested to add --data-ciphers alias into the next v2.4
>   release; to which I agree(!).

Yes, this sounds like a simple idea but also has the danger of creating
more confusion.

I feel it better to have ncp-cipher and data-ciphers coexist. Then you
also have the understand that ncp-cipher is an older/less capable
version of data-ciphers.

OpenVPN 2.4 has a number of oddities in NCP support:

- the client always announces AES-GCM support even if disabled in
ncp-ciphers
- the server will always push the first cipher of its ncp-ciphers list
to the client.


From a user perspective data-ciphers is a true superset of ncp-ciphers.
All existing ncp-ciphers setup will continue to work on 2.5 but the
data-ciphers option from 2.5 is likely to break on 2.5 if you don't take
into account the 2.4 oddities.

> * I propose we add a warning whenever --ncp-ciphers used, recommending the
>   user to switch to --data-ciphers as soon as possible.

As compromise maybe something like

NOTE: Rewriting old-option x y z to new new-option x y z


> 
> * We keep both --ncp-ciphers *and* --data-ciphers in v2.5 and v2.6.
> 
> * When v2.5 is released v2.4 goes into "old stable support" - where both
>   alternatives will work.  v2.4 is guaranteed to be supported by the community
>   for at least 6 months with full support [0] after v2.5 is released.
> 
> * When 2.6 is released, v2.4 goes into "git tree only" support but will have a
> 
>   12 month "old stable support" guarantee [0].
> 
> * 12 months *after* the v2.6 release is out, we can remove --ncp-ciphers.  But
>   since we will not do option changes mid-release easily, it might be natural
>   to remove this in OpenVPN 2.7 at earliest.
> 
> This means --ncp-ciphers will be supported in 2.4 for the life time of that
> release.  And v2.4 is supported for *at least* 18 months after v2.5 is
> released.  It also guarantees --ncp-ciphers will first be removed when we
> release OpenVPN 2.7.
> 
> What would be a problem with such a schedule?  Because I don't understand the
> real objection you have to remove options which ends up duplicating other options.


My general problem is that I don't see a real advantage in removing
ncp-cipher but see a lot of downsides removing it.

> 
> At the same time ... it is also important for me that we try to see this at
> from an even bigger perspective than just --ncp-ciphers/--data-ciphers or even
> --udp-mtu alone.  Now I'm shifting the discussion to a more generic product
> life cycle perspective.
> 
> If we skimp through this and just allow whatever option duplications we head
> into, just because we're too concerned about various breakages with old
> configurations or setups - it will not be the last time we might end up in
> such discussions UNLESS we can find a proper and reasonable process for
> deprecation (which we in fact already defined for feature deprecation 10 years
> ago! [1]).
> 
> If we cannot remove options during the whole life time of OpenVPN, we cannot
> touch compression options or possibly even deprecate any compression at all -
> because we need to support both compression and decompression for the whole
> lifetime of OpenVPN.  This also extends into how to ensure proper OpenVPN 3
> compatibility as well.

The amount of time we spend on testing/developing etc on compression and
the complexity it introduces is vastly different from ncp-cipher

> And if we cannot remove any options during the eternal life time of OpenVPN, I
> will see no other alternative to being even more critical and rejective to add
> new options.  We already have pretty close to 300 options in OpenVPN.  That is
> an insane amount - where a typical common OpenVPN setup might need up to 20 of
> these options, the rest are for all kinds of special cases.  And we have
> several competing solutions which are far simpler in many aspects which new
> users might just as well prefer.
> 
> Even though I highlight the number of options we have, I do NOT say that we
> should swipe them all out and reduce the amount to 50 or so; for some users
> they have a _real_ value and provides really useful features.  But I want us
> to save the options which do have a REAL value, not because unsupported
> OpenVPN versions might break or "10 bytes extra source code" is too heavy to
> carry around for an alias option.  I'm arguing for keeping options covering
> _REAL_ USE CASE for users.  And no, breaking unsupported OpenVPN releases is
> NOT a real use case from my point of view.

This is the point where we disagree. I still think that we should weigh
the pros and cons if we still want have compatibility. And also to
consider if there are alternatives/workaround for the setups/users
affected.

> But back to the deprecated options  ... If we cannot remove options, we also
> need to consider bringing back the --tls-remote option, and --compat-names -
> both with the capability to break client configs (in fact, it already did for
> several Fedora EPEL users [2]).  

Well if weighing the pros of bringing it back outwights the cons, then
the decision to compeletly remove it might have been wrong.


> The proposal to remove --remote-nopull needs
> to be reconsidered, as well as --secret, --max-routes, and possibly even
> --ncp-disable.  Maybe even more of these deprecated options needs to be
> reconsidered [3].

For ncp-disable I did the weighing of pros and cons and answer that
question. ncp-disable add complexity to the code paths and is in
conflict which the direction we want to go. And for users that currently
use the option the workable workaround is to just remove that options as
it should not be necessary in 2.5 anymore.

> We really need a proper and sane processes to allow the development of OpenVPN
> to have a chance to move on and leave things behind when appropriate, to be
> able to evolve and grow with the future - without being strangled by what
> existed in the far past (meaning: no longer community supported releases).
> Otherwise I do fear for the future of OpenVPN 2.x.

There is more a to software than the development. There is also how it
is used/etc. And we should not make it harder than it needs to be. And
even for our own company internal use case of Access server the reality
is that we even though we do not like supporting the old clients (back
to 2.2 in some cases), we still have to do it because there is demand
for it.


> By having a clear strategy and adhering to a process of feature/option
> management in OpenVPN, we give clearly defined time-window for stability and
> functionality for our users.  This predictability is, in my experience, much
> more important to users than if a specifically named option is supported or not.

Yes. If we decide to remove an option we should have this predictable
removal process. But if we remove an option/drop support for something
something that should still be a weighing of pros and cons.

For this specific option of ncp-ciphers/data-ciphers. This not just a
fringe option. This is an option that affects one of the core things of
OpenVPN, the chosen chipher. I want to make the transition to NCP as
painless as possible and  keeping ncp-cipher as alias to data-cipher is
just the better choice in my opinion.

Arne
David Sommerseth July 24, 2020, 1:24 a.m. UTC | #8
On 24/07/2020 12:45, Arne Schwabe wrote:
> First of all I did not want to reply to this since we had a lengthy
> discussion on IRC.
> 
>> Lets take a few steps back try to see a broader picture.
>>
>> * --ncp-ciphers was introduced in OpenVPN 2.4 as a brand new option.
>>
>> * Steffan has suggested to add --data-ciphers alias into the next v2.4
>>   release; to which I agree(!).
> 
> Yes, this sounds like a simple idea but also has the danger of creating
> more confusion.
> 
> I feel it better to have ncp-cipher and data-ciphers coexist. Then you
> also have the understand that ncp-cipher is an older/less capable
> version of data-ciphers.
> 
> OpenVPN 2.4 has a number of oddities in NCP support:
> 
> - the client always announces AES-GCM support even if disabled in
> ncp-ciphers
> - the server will always push the first cipher of its ncp-ciphers list
> to the client.

These oddities, they sounds like bugs, or?  If I try to put on my
Never-used-NCP-before-hat, this would not be what I would expect.  Why haven't
we resolved this within the current 2.4 scope without changing options?



> From a user perspective data-ciphers is a true superset of ncp-ciphers.
> All existing ncp-ciphers setup will continue to work on 2.5 but the
> data-ciphers option from 2.5 is likely to break on 2.5 if you don't take
> into account the 2.4 oddities.
> 
>> * I propose we add a warning whenever --ncp-ciphers used, recommending the
>>   user to switch to --data-ciphers as soon as possible.
> 
> As compromise maybe something like
> 
> NOTE: Rewriting old-option x y z to new new-option x y z

As mentioned on IRC, yes.  I am willing to accept

msg(M_INFO, "Rewriting deprecated option X to the replacement Y" ...



[...]
>> * 12 months *after* the v2.6 release is out, we can remove --ncp-ciphers.  But
>>   since we will not do option changes mid-release easily, it might be natural
>>   to remove this in OpenVPN 2.7 at earliest.
>>
>> This means --ncp-ciphers will be supported in 2.4 for the life time of that
>> release.  And v2.4 is supported for *at least* 18 months after v2.5 is
>> released.  It also guarantees --ncp-ciphers will first be removed when we
>> release OpenVPN 2.7.
>>
>> What would be a problem with such a schedule?  Because I don't understand the
>> real objection you have to remove options which ends up duplicating other options.
> 
> 
> My general problem is that I don't see a real advantage in removing
> ncp-cipher but see a lot of downsides removing it.

I hear about you seeing downsides ... but I have not seen any argument here
convincing me otherwise.  This overall process should take care of supporting
all reasonable OpenVPN 2.x versions.



>> At the same time ... it is also important for me that we try to see this at
>> from an even bigger perspective than just --ncp-ciphers/--data-ciphers or even
>> --udp-mtu alone.  Now I'm shifting the discussion to a more generic product
>> life cycle perspective.
>>
>> If we skimp through this and just allow whatever option duplications we head
>> into, just because we're too concerned about various breakages with old
>> configurations or setups - it will not be the last time we might end up in
>> such discussions UNLESS we can find a proper and reasonable process for
>> deprecation (which we in fact already defined for feature deprecation 10 years
>> ago! [1]).
>>
>> If we cannot remove options during the whole life time of OpenVPN, we cannot
>> touch compression options or possibly even deprecate any compression at all -
>> because we need to support both compression and decompression for the whole
>> lifetime of OpenVPN.  This also extends into how to ensure proper OpenVPN 3
>> compatibility as well.
> 
> The amount of time we spend on testing/developing etc on compression and
> the complexity it introduces is vastly different from ncp-cipher

That is true.  But at the same time, if we do not have a proper deprecation
process where we draw the line in which OpenVPN versions we are willing to
support (from a functional perspective), we cannot touch compression.

Which is why I'm trying to raise the points of a bigger picture.  We need to
find a reasonable solution to which OpenVPN versions we are willing to
support, and when those versions enters the unsupported side we should be free
to cleanup code and options related to the behavior and functionality only
these now not supported releases provided.



>> And if we cannot remove any options during the eternal life time of OpenVPN, I
>> will see no other alternative to being even more critical and rejective to add
>> new options.  We already have pretty close to 300 options in OpenVPN.  That is
>> an insane amount - where a typical common OpenVPN setup might need up to 20 of
>> these options, the rest are for all kinds of special cases.  And we have
>> several competing solutions which are far simpler in many aspects which new
>> users might just as well prefer.
>>
>> Even though I highlight the number of options we have, I do NOT say that we
>> should swipe them all out and reduce the amount to 50 or so; for some users
>> they have a _real_ value and provides really useful features.  But I want us
>> to save the options which do have a REAL value, not because unsupported
>> OpenVPN versions might break or "10 bytes extra source code" is too heavy to
>> carry around for an alias option.  I'm arguing for keeping options covering
>> _REAL_ USE CASE for users.  And no, breaking unsupported OpenVPN releases is
>> NOT a real use case from my point of view.
> 
> This is the point where we disagree. I still think that we should weigh
> the pros and cons if we still want have compatibility. And also to
> consider if there are alternatives/workaround for the setups/users
> affected.

This is again the same issue, to be able to draw the line somewhere to what we
are willing to support.  What kind of life cycles do we want for OpenVPN?



>> But back to the deprecated options  ... If we cannot remove options, we also
>> need to consider bringing back the --tls-remote option, and --compat-names -
>> both with the capability to break client configs (in fact, it already did for
>> several Fedora EPEL users [2]).  
> 
> Well if weighing the pros of bringing it back outwights the cons, then
> the decision to compeletly remove it might have been wrong.

So the question is ... do we care about broken OpenVPN 2.3 (or older)
configurations today?  Which is technically supported by us another 12 months
after the v2.5 release.



>> The proposal to remove --remote-nopull needs
>> to be reconsidered, as well as --secret, --max-routes, and possibly even
>> --ncp-disable.  Maybe even more of these deprecated options needs to be
>> reconsidered [3].
> 
> For ncp-disable I did the weighing of pros and cons and answer that
> question. ncp-disable add complexity to the code paths and is in
> conflict which the direction we want to go. And for users that currently
> use the option the workable workaround is to just remove that options as
> it should not be necessary in 2.5 anymore.

But it still will break users configs without any prior warning in 2.4.  It
was added to our git master only in 15 days ago.  Including potential client
configurations.

Again, this exemplifies that we are lacking a proper product life cycle
process and even adhering to a feature deprecation process.



>> We really need a proper and sane processes to allow the development of OpenVPN
>> to have a chance to move on and leave things behind when appropriate, to be
>> able to evolve and grow with the future - without being strangled by what
>> existed in the far past (meaning: no longer community supported releases).
>> Otherwise I do fear for the future of OpenVPN 2.x.
> 
> There is more a to software than the development. There is also how it
> is used/etc. And we should not make it harder than it needs to be. And
> even for our own company internal use case of Access server the reality
> is that we even though we do not like supporting the old clients (back
> to 2.2 in some cases), we still have to do it because there is demand
> for it.

Why Access Server supports such old OpenVPN releases is a completely different
discussion not belong here in the community.  There might be valid use cases
for these customers, but in such situations additional (and often fairly
expensive) support contracts are required by the vast majority of vendor
agreements I've seen.  And these corporate special needs should not end up in
the community.

The community needs to be decoupled to be fully functional.  I say this as a
OpenVPN Inc hire - and has also said this internally to the management over
the years I've been there.  From what I experience, there is a general
understanding for the need of this separation.



>> By having a clear strategy and adhering to a process of feature/option
>> management in OpenVPN, we give clearly defined time-window for stability and
>> functionality for our users.  This predictability is, in my experience, much
>> more important to users than if a specifically named option is supported or not.
> 
> Yes. If we decide to remove an option we should have this predictable
> removal process. But if we remove an option/drop support for something
> something that should still be a weighing of pros and cons.
> 
> For this specific option of ncp-ciphers/data-ciphers. This not just a
> fringe option. This is an option that affects one of the core things of
> OpenVPN, the chosen chipher. I want to make the transition to NCP as
> painless as possible and  keeping ncp-cipher as alias to data-cipher is
> just the better choice in my opinion.

I agree that --data-cipher is a better name for it; I have not rejected that.

But that doesn't mean we should have two thoughts active at the same time: a)
NCP improvements, and b) product life cycle, what we support and how we
support OpenVPN in the long run ... in this case they do impact each other.

But when we only focus on a) without properly considering b), that is the
point where I object.
Gert Doering July 26, 2020, 10:08 p.m. UTC | #9
Your patch has been applied to the master branch.

I have not done more than basic compile-time testing and a quick
glance at "git show" here.

Patch 10/9 with the message will follow right away.

commit 30d19c6ebee05de4cc35155a6c7742ccbf021a82
Author: Arne Schwabe
Date:   Fri Jul 17 15:47:38 2020 +0200

     Rename ncp-ciphers to data-ciphers

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Steffan Karger <steffan.karger@foxcrypto.com>
     Message-Id: <20200717134739.21168-8-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20444.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/Changes.rst b/Changes.rst
index 6e283270..2158c8e7 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -14,12 +14,19 @@  ChaCha20-Poly1305 cipher support
     channel.
 
 Improved Data channel cipher negotiation
+    The option ``ncp-ciphers`` has been renamed to ``data-ciphers``.
+    The old name is still accepted. The change in name signals that
+    ``data-ciphers`` is the preferred way to configure data channel
+    ciphers and the data prefix is chosen to avoid the ambiguity that
+    exists with ``--cipher`` for the data cipher and ``tls-cipher``
+    for the TLS ciphers.
+
     OpenVPN clients will now signal all supported ciphers from the
-    ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
-    servers will select the first common cipher from the ``ncp-ciphers``
+    ``data-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
+    servers will select the first common cipher from the ``data-ciphers``
     list instead of blindly pushing the first cipher of the list. This
     allows to use a configuration like
-    ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
+    ``data-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
     prefers ChaCha20-Poly1305 but uses it only if the client supports it.
 
 Asynchronous (deferred) authentication support for auth-pam plugin.
diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 923d2da0..051f1d32 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -62,7 +62,7 @@  configured in a compatible way between both the local and remote side.
   The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
   Block Chaining mode. When cipher negotiation (NCP) is allowed,
   OpenVPN 2.4 and newer on both client and server side will automatically
-  upgrade to :code:`AES-256-GCM`.  See ``--ncp-ciphers`` and
+  upgrade to :code:`AES-256-GCM`.  See ``--data-ciphers`` and
   ``--ncp-disable`` for more details on NCP.
 
   Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
@@ -169,7 +169,7 @@  configured in a compatible way between both the local and remote side.
   non-standard key lengths, and a larger key may offer no real guarantee
   of greater security, or may even reduce security.
 
---ncp-ciphers cipher-list
+--data-ciphers cipher-list
   Restrict the allowed ciphers to be negotiated to the ciphers in
   ``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
   and defaults to :code:`AES-256-GCM:AES-128-GCM`.
@@ -189,9 +189,9 @@  configured in a compatible way between both the local and remote side.
   Additionally, to allow for more smooth transition, if NCP is enabled,
   OpenVPN will inherit the cipher of the peer if that cipher is different
   from the local ``--cipher`` setting, but the peer cipher is one of the
-  ciphers specified in ``--ncp-ciphers``. E.g. a non-NCP client (<=v2.3,
+  ciphers specified in ``--data-ciphers``. E.g. a non-NCP client (<=v2.3,
   or with --ncp-disabled set) connecting to a NCP server (v2.4+) with
-  ``--cipher BF-CBC`` and ``--ncp-ciphers AES-256-GCM:AES-256-CBC`` set can
+  ``--cipher BF-CBC`` and ``--data-ciphers AES-256-GCM:AES-256-CBC`` set can
   either specify ``--cipher BF-CBC`` or ``--cipher AES-256-CBC`` and both
   will work.
 
@@ -201,6 +201,9 @@  configured in a compatible way between both the local and remote side.
   This list is restricted to be 127 chars long after conversion to OpenVPN
   ciphers.
 
+  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
+  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
+
 --ncp-disable
   Disable "Negotiable Crypto Parameters". This completely disables cipher
   negotiation.
diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index c24aec0b..74ad5e18 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -473,8 +473,8 @@  fast hardware. SSL/TLS authentication must be used in this mode.
         *AES-GCM-128* and *AES-GCM-256*.
 
   :code:`IV_CIPHERS=<ncp-ciphers>`
-        The client pushes the list of configured ciphers with the
-        ``--ciphers`` option to the server.
+        The client announces the list of supported ciphers configured with the
+        ``--data-ciphers`` option to the server.
 
   :code:`IV_GUI_VER=<gui_id> <version>`
         The UI version of a UI if one is running, for example
diff --git a/sample/sample-config-files/client.conf b/sample/sample-config-files/client.conf
index 7f2f30a3..47ca4099 100644
--- a/sample/sample-config-files/client.conf
+++ b/sample/sample-config-files/client.conf
@@ -112,7 +112,7 @@  tls-auth ta.key 1
 # then you must also specify it here.
 # Note that v2.4 client/server will automatically
 # negotiate AES-256-GCM in TLS mode.
-# See also the ncp-cipher option in the manpage
+# See also the data-ciphers option in the manpage
 cipher AES-256-CBC
 
 # Enable compression on the VPN link.
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 88ba9db2..d2549bca 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1827,7 +1827,7 @@  multi_client_set_protocol_options(struct context *c)
         else
         {
             /*
-             * Push the first cipher from --ncp-ciphers to the client that
+             * Push the first cipher from --data-ciphers to the client that
              * the client announces to be supporting.
              */
             char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
@@ -1847,7 +1847,7 @@  multi_client_set_protocol_options(struct context *c)
                 {
                     msg(M_INFO, "PUSH: No common cipher between server and "
                         "client. Expect this connection not to work. Server "
-                        "ncp-ciphers: '%s', client supported ciphers '%s'",
+                        "data-ciphers: '%s', client supported ciphers '%s'",
                         o->ncp_ciphers, peer_ciphers);
                 }
                 else
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 31e33ae3..896abcde 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -536,7 +536,7 @@  static const char usage_message[] =
     "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
     "                  (default=%s).\n"
     "                  Set alg=none to disable encryption.\n"
-    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
+    "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
     "--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
     "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
     "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
@@ -7866,7 +7866,8 @@  add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
         options->ciphername = p[1];
     }
-    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
+    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
+            && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
         options->ncp_ciphers = p[1];
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index e057a40b..6760884e 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -111,7 +111,7 @@  mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
         const cipher_kt_t *ktc = cipher_kt_get(token);
         if (!ktc)
         {
-            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
             error_found = true;
         }
         else
@@ -130,7 +130,7 @@  mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
             if (!(buf_forward_capacity(&new_list) >
                   strlen(ovpn_cipher_name) + 2))
             {
-                msg(M_WARN, "Length of --ncp-ciphers is over the "
+                msg(M_WARN, "Length of --data-ciphers is over the "
                     "limit of 127 chars");
                 error_found = true;
             }