Am 28.07.20 um 14:27 schrieb Steffan Karger:
>> * - peer id
>> */
>> -static void
>> +static bool
>> multi_client_set_protocol_options(struct context *c)
>> {
>>
>> @@ -1807,8 +1807,11 @@ multi_client_set_protocol_options(struct context *c)
>> }
>>
>> /* Select cipher if client supports Negotiable Crypto Parameters */
>> - if (o->ncp_enabled)
>> + if (!o->ncp_enabled)
>> {
>> + return true;
>> + }
>> +
>
> Hm, in this case I don't think this improves things. This turns
>
Yes. But this code will also be removed as soon as we hit 2.6 master and
remove ncp-disable and then the flow should be good.
>
> Can we please avoid using strcpy and strcat? Using openvpn_snprintf()
> should result in simpler code, and makes it easier to ensure that we
> won't overflow.
>
> Actually, looks like this already overflows, since there is no space for
> the null-byte in ncp_ciphers.
Yes, I still hate string handling in C. I hope my next attempt is better...
>>
>> buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
>> - buf_printf(&out, ",link-mtu %u", (unsigned int)
>> calc_options_string_link_mtu(o, frame));
>> + if (o->ciphername)
>> + {
>> + /* the link-mtu that we send has only a meaning if have a fixed
>> + * cipher (p2p) or have a fallback cipher for older non ncp
>> + * clients. If we do have a fallback cipher, do not send it */
>
> This confuses me. The code reads like it's dependent on --cipher, rather
> than --fallbck-cipher. Also shouldn't this be "If we *don't* have a
> fallback cipher"?
Good catch. First I wanted to set ciphername == NULL in case we don't
have a ciphername but to be to difficult for the time being. This an
oversight from that try.
>> {
>> const char *ios = ifconfig_options_string(tt, remote,
>> o->ifconfig_nowarn, gc);
>> if (ios && strlen(ios))
>> @@ -3751,8 +3812,15 @@ options_string(const struct options *o,
>>
>> init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
>> false);
>> -
>> - buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
>> + /* Only announce the cipher to our peer if we are willing to
>> + * support it */
>> + const char *ciphername = cipher_kt_name(kt.cipher);
>> + if (p2p_nopull || !o->ncp_enabled
>> + || (o->ncp_enabled
>> + && tls_item_in_cipher_list(ciphername, o->ncp_ciphers)))
>
> This second check for o->ncp_enabled is not needed. You already ensured
> it's true before. (Saves you a line wrap.)
I added to make the condition a bit clearer but I will remove it.
> I wonder though, isn't it too soon to stop sending cipher? Looks like
> both 2.3 and 2.4 clients will currently print options warnings if cipher
> is missing from OCC. The earlier NCP versions carefully tries to send
> what the peer expected here to prevent bogus warnings.
The main reason that I added it that sending it would break
compatibility with 2.5 master servers. Since --cipher BF-CBC, a server
would assume that it was supported. But I think we can keep sending
and instead assume that a client that sends IV_CIPHERS will only support
those and not necessarily the one in the OCC cipher. Basically disabling
Poor man's NCP when we see a 2.5 client. I would need to fix OpenVPN3 to
also include its --cipher in the IV_CIPHERS string but that is doable.
That would bascially break support of David's currently released
openpvn3-linux clients since they use openvpn3 master that has currently
has the hardcoded list
IV_CIPHERS=CHACHA20-POLY1305:AES-256-GCM:AES-128-GCM
and we would ignore the OCC cipher. But I think that is acceptable
because you need to actively set a ncp-ciphers option on the server to
something that removes the GCM ciphers.
>> }
>> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
>> index d00c222d..98f80286 100644
>> --- a/src/openvpn/ssl_ncp.h
>> +++ b/src/openvpn/ssl_ncp.h
>> @@ -44,10 +44,13 @@ tls_peer_supports_ncp(const char *peer_info);
>> * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
>> * Allows non-NCP peers to upgrade their cipher individually.
>> *
>> + * Returns true if we switched to the peer's cipher
>> + *
>> * Make sure to call tls_session_update_crypto_params() after calling this
>> * function.
>> */
>> -void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>> +bool
>> +tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>
> I think we almost always put the return type on the same line in header
> files. (Don't know why, but it seems quite consistent.)
> -Steffan
Depends on the header, but you are right.
Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
