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 <a...@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 <stef...@karger.me>

NAK.


-- 
kind regards,

David Sommerseth
OpenVPN Inc


Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to