Hi Rémi,
On Tue, May 20, 2014 at 10:55:55AM +0200, Remi Gacogne wrote:
> >> Is it possible to only generate the dh-param and warnings if a cipher that
> >> needs it is enabled?
> >
> > I thought it was the case where the code was placed, but maybe I was
> > wrong. Rémi, what do you think ?
>
> Well, the warning won't show up if DHE support has been disabled at
> compile time (using OPENSSL_NO_DH for example), but Bryan is right, it
> will be displayed even if no DHE cipher suite has been enabled.
>
> Right now, the warning is displayed in ssl_sock_load_dh_params(), which
> is called whenever a certificate file is parsed. At that time, I don't
> think we can determine whether some cipher suites using DHE are enabled
> or not, because the "ciphers" keyword might not have been parsed yet.
>
> We could probably move the warning to a later time, for example in
> ssl_sock_prepare_ctx(), iterate on each enabled ciphers and only display
> the warning if we find at least one DHE cipher enabled.
You said that users can put the DH params in the cert file and that they'll
be loaded along with the cert. From what I understand, the params you
generate are produced as alternatives for the cases where they're not
provided with the cert (please correct me if I'm wrong, this is still
obscure to me).
Do you think that openssl only tries to load these DH params from a cert
when DHE is enabled ? If so, maybe we can register a callback there.
Otherwise I have a comment below :
> Something like this (not tested):
>
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -724,6 +996,15 @@ int ssl_sock_load_cert_list_file(char *file, struct
> bind_conf *bind_conf, struct
> #ifndef SSL_MODE_RELEASE_BUFFERS /* needs
> OpenSSL >= 1.0.0 */
> #define SSL_MODE_RELEASE_BUFFERS 0
> #endif
> +
> +#ifndef SSL_kEDH
> +#if OPENSSL_VERSION_NUMBER < 0x1000000fL
> +#define SSL_kEDH 0x00000010L
> +#else
> +#define SSL_kEDH 0x00000008L
> +#endif
> +#endif
> +
> int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, SSL_CTX *ctx,
> struct proxy *curproxy)
> {
> int cfgerr = 0;
> @@ -740,6 +1021,10 @@ int ssl_sock_prepare_ctx(struct bind_conf
> *bind_conf, SSL_CTX *ctx, struct proxy
> SSL_MODE_ENABLE_PARTIAL_WRITE |
> SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER |
> SSL_MODE_RELEASE_BUFFERS;
> + STACK_OF(SSL_CIPHER) * ciphers = NULL;
> + SSL_CIPHER * cipher = NULL;
> + int idx = 0;
> + int dhe_found = 0;
>
> /* Make sure openssl opens /dev/urandom before the chroot */
> if (!ssl_initialize_random()) {
> @@ -828,6 +1113,26 @@ int ssl_sock_prepare_ctx(struct bind_conf
> *bind_conf, SSL_CTX *ctx, struct proxy
> cfgerr++;
> }
>
> + if (global.tune.ssl_default_dh_param < 1024) {
> + ciphers = ctx->cipher_list;
> +
> + if (ciphers) {
> + for (idx = 0; idx < sk_SSL_CIPHER_num(ciphers);
> idx++) {
> + cipher = sk_SSL_CIPHER_value(ciphers, idx);
> + if (cipher->algorithm_mkey & SSL_kEDH) {
> + dhe_found = 1;
> + break;
> + }
> + }
> + }
> +
> + if (dhe_found) {
> + Warning("Setting tune.ssl.default-dh-param to
> 1024 by default, if your workload permits it you should set it to at
> least 2048. Please set a value >= 1024 to make this warning disappear.\n");
> + }
> +
> + global.tune.ssl_default_dh_param = 1024;
> + }
> +
>
> But I think it's kind of ugly, especially because I can't find anything
> in the OpenSSL API to check the key exchange algorithm used by a given
> cipher suite and therefore have to directly access the SSL_CIPHER struct.
Shouldn't we simply look for "DHE" in the cipher string if that's the way
people use to declare the algorithms (as a full match, not substring) ?
Just my few cents,
Willy
