Hi Emeric,
> Le 27 mars 2017 à 14:06, Emeric Brun <[email protected]> a écrit :
>>
>
> Hi Manu,
>
> those patches are very annoying to review because incremental. Perhaps it
> would be better to split them differently (by feature).
>
I don't see how because of rework and factorization. Perhaps split the 0002
but i will merge part of 0002 factorization to 0001 (not to spend too much time)
0001 rework and factorization
and have 3 patches for . min/max . tlsv1.3 . compat(0004) (order is not
important)
> Lack of comment on this loop, i'm still trying to understand what is done:
>
> + for (i = 1; i < sizeof(methodVersions)/sizeof(*methodVersions); i++)
> + if (methodVersions[i].flag && !(conf_ssl_methods &
> methodVersions[i].conf)) {
> + if (min) {
> + if (hole) {
> + Warning("config : %s '%s': SSL/TLS
> versions range not contiguous for server '%s'. "
> + "Hole find for %s. Use only
> 'min-tlsvX' and 'max-tlsvY' to fix.\n",
> + proxy_type_str(curproxy),
> curproxy->id, srv->id,
> + methodVersions[hole].name);
> + hole = 0;
> + }
> + max = i;
> + }
> + else {
> + min = max = i;
> + }
> + }
> + else {
> + if (min)
> + hole = i;
> + options |= methodVersions[i].flag;
> + }
> + if (!min)
> + Warning("config : %s '%s': all SSL/TLS versions are disabled
> for server '%s'.\n",
> + proxy_type_str(curproxy), curproxy->id, srv->id);
> +
> +
> +#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) && !defined(OPENSSL_IS_BORINGSSL)
> + /* Keep force-xxx implementation as it is in older haproxy. It's a
> + precautionary measure to avoid any suprise with older openssl
> version. */
> + if (min == max)
> + methodVersions[min].set_version(ctx, 0 /* client */);
> +#else /* openssl >= 1.1.0 */
> + /* set the max_version is required to activate TLSv1.3 */
> + methodVersions[min].set_version(ctx, 0);
> + methodVersions[max].set_version(ctx, 1);
> +#endif
> +
>
> What i think i understand is that you try to re-implement min/max features
> using 'options' for older openssl versions which haven't new
> min/max-set-version methods.
>
yes but it’s already the case in 0002 with min-max parameter. The set
‘options’, in the loop, is as in 0001 (without the loop).
The loop determine min/max and warn on ‘hole’.
> But i don't understand what happen to options if min and max are set on
> openssl v1.1 > 0. Do we authorize holes?
>
yes ‘holes’ is authorize with warning, because openssl 1.1.0 and boring deal
with it (keep the upper or lower range depending the case and version).
In openssl min/max is used to start/stop the scan of ‘options’ to determine
what to do.
> Willy and I discussed, and we thought that using a single keyword in conf for
> min and one for max with a limited set of value (enum) is a better approach
> for API users:
>
> i.e.
> bind 1.2.3.4:443 ssl crt my.pem ssl-min TLSv1.0 ssl-max TLSv1.3
What kind of api and dependency? To generate haproxy configuration?
Generate min-tlsv10 or ssl-min tlsv10 will not change anything.
++
Manu
