>> The default for \-\-tls\-ciphersuites is to use the crypto library's >> default. >> .\"********************************************************* >> .TP >> +.B \-\-tls\-groups l >> +A list >> +.B l >> +of allowable groups/curves in order of preference. > ^^^ shouldn't this be "allowed" ?
It is a very tiny semantic difference and both forms are correct. We use
the same wording for tls-cipher and considering that we ignore unknown
curves allowable is IMO the better fit. I prefer allowable but will not
fight for it.
>> options->show_tls_ciphers = true;
>> }
>> - else if (streq(p[0], "show-curves") && !p[1])
>> + else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) &&
>> !p[1])
> shouldn't we deprecate --show-curves at this point?
> what's the point of keeping both options?
It is a nice thing to and does not extra complexity. So still accepting
is a thing gesture for users.
>> +void
>> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
>> +{
>> + ASSERT(ctx);
>> + struct gc_arena gc = gc_new();
>> +
>> + /* Get number of groups and allocate an array in ctx */
>> + int groups_count = get_num_elements(groups, ':');
>> + ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
>> +
>> + /* Parse allowed ciphers, getting IDs */
>> + int i = 0;
>> + char *tmp_groups = string_alloc(groups, &gc);
>> +
>> + const char *token = strsep(&tmp_groups, ":");
>> + while (token)
>
> Couldn't we avoid having the assignment here and at the end of the loop
> by doing:
>
> const char *token;
> while ((token = strsep(&tmp_groups, ":"))
>
Yes, this is a left over from an earlier version where the first call
had to be made differently iirc.
>
>> + {
>> + const mbedtls_ecp_curve_info *ci =
>> + mbedtls_ecp_curve_info_from_name(token);
>> + if (!ci)
>> + {
>> + msg(M_WARN, "Warning unknown curve/group specified: %s", token);
>> + }
>> + else
>> + {
>> + ctx->groups[i] = ci->grp_id;
>> + i++;
>> + }
>> + token = strsep(&tmp_groups, ":");
>> + }
>> + ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
>> +
>> + gc_free(&gc);
>> +}
>> +
>> +
>> void
>> tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>> {
>> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>> mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config,
>> ssl_ctx->allowed_ciphers);
>> }
>>
>> + if (ssl_ctx->groups)
>> + {
>> + mbedtls_ssl_conf_curves(&ks_ssl->ssl_config, ssl_ctx->groups);
>
> the '&' should not be there I guess.
>
> /usr/include/mbedtls/ssl.h:2925:51: note: expected ‘mbedtls_ssl_config
> *’ but argument is of type ‘mbedtls_ssl_config **’
The commit mbedTLS: Make sure TLS session survives was commited in the
meantime that change the argument from * to **
https://github.com/openvpn/openvpn/commit/a59e0754afd37a606d96cf24cea771ace3467289
>> #ifndef OPENSSL_NO_EC
>> EC_builtin_curve *curves = NULL;
>> size_t crv_len = 0;
>> @@ -2188,7 +2242,7 @@ show_available_curves(void)
>> ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
>> if (EC_get_builtin_curves(curves, crv_len))
>> {
>> - printf("Available Elliptic curves:\n");
>> + printf("\nAvailable Elliptic curves/groups:\n");
>> for (n = 0; n < crv_len; n++)
>> {
>> const char *sname;
>>
>
>
> The rest looks good.
>
> I applied this patch on top of master with git am -3 because there are
> some trivial conflicts to fix.
Will send a V3 with the changes.
Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
