[squid-dev] [PATCH] TLS: Add support for EECDH

[Amos Jeffries]

On 5/06/2015 6:51 a.m., Paulo Matias wrote:
> Hi all,
> 
> This patch adds support for Ephemeral Elliptic Curve Diffie-Hellman (EECDH)
> key exchange, which allows for forward secrecy with better performance than
> traditional ephemeral DH.
> 
> This was previously posted to squid-users, but modified since then to implement
> Amos's suggestions:
> 
>> * the DH parameters I think would be better added as a new option
>> "tls-dh=curve:/path/to/params" where the 'curve' part is optional and
>> implies EC when present - non-EC when absent.
> 
> OK, I have left the "dhparams=" option working as before, but emitting a
> deprecation warning message. There is a new "tls-dh=" option which should
> replace "dhparams=", and supports the suggested syntax. The following
> are also accepted:
> 
>  * "tls-dh=:/path/to/params" -> equivalent to "tls-dh=/path/to/params"
>  * "tls-dh=curve:" -> allows for enabling only EECDH (no EDH)
> 
>> * SINGLE_ECDH_USE needs to be documented in release-4.sgml
>>  "New <em>options=SINGLE_ECDH_USE</em> parameter to ..."
> 
> OK.
> 
>> * The ECDH changes affect both https_port and http_port. They need
>> separate listings for each under changed directives, duplicate text on
>> the line items is fine.
> 
> OK.
> 
>> * please implement (duplicate) all this UI parse change using the
>> Security::PeerOptions object (src/security/PeerOptions.*)
>>  - the src/ssl/* code for UI parsing and config storage is 'legacy' only
>> use by http(s)_port directives.
>>  - this may require some small changes suitable for use on client contexts
>>  - UI options added to Security::PeerOptions get documented in
>> release-4.sgml as changes for both cache_peer and tls_outgoing_options.
>>  - also in cf.data.pre for those directives
> 
> If I understood correctly, Security::PeerOptions is currently only used
> for client TLS contexts. Currently, it does not implement e.g. the "dhparams="
> option, which is only supposed to be present when configuring a server
> TLS context. According to
> http://openssl.6102.n7.nabble.com/question-about-ecdh-functions-tp38239p38242.html,
> the curve should also be configured only in the server context. Also,
> https://github.com/openssl/openssl/blob/e481f9b90b164fd1053015d1c4e0a0d92076d7a8/ssl/ssl_conf.c#L450
> shows that the "named_curve" parameter is only allowed in the s_server
> OpenSSL tool, therefore not allowed in s_client.
> 
> Therefore this is not included in the patch below. Please correct me if I
> misunderstood anything.

Okay, thats reasonable. I was hoping to be able to avoid porting it
later, but that not a problem.



> 
>> * configureSslEECDH() return true in the event that the chosen
>> configuration options are not even available.
>>  - please make an #else condition that displays an ERROR message at
>> level DBG_CRITICAL about the option(s) not being available, then return
>> false.
>>  - variable 'ok' can then become const (define on assignment) and move
>> fully inside the #if case.
> 
> OK.
> 
> We welcome any additional suggestions or comments.


Just a few minor nit-pick bits:

 * the uses of "eecdhCurve[0]" look like thay can all become
"*eecdhCurve" for slightly better compiler optimization.
 (yeah its weird that the compilers are not smart enough to do it
themselves, but we tested and its faster with *X=0 than X[0] = '\0').


* this sequence:

> +        safe_free(eecdhCurve);
> +        eecdhCurve = xstrdup(tls_dh);

can become xfree() instead of safe_free(). They do the same pre-checks,
but xfree() works slightly faster if its guaranteed to be assigned
before next use.


* The debugs output "EECDH is not available in this build" would be a
little more useful if it could suggest a minimum OpenSSL library version
to build against. So its clear the problem comes from OpenSSL imposed
restrictions, not just Squid.


Otherwise +1.

Amos