Hi Dirkjan, CCing Emeric who will know better than me how to respond, but I have some questions at the end.
On Mon, Aug 29, 2016 at 02:22:23PM +0200, Dirkjan Bussink wrote: > Hi all, > > I've attached a patch for support for OpenSSL 1.1.0. That version changes > quite a few things, mostly it makes a lot of the structures now opaque and > private and provides functions to interact with them. Most of this change > consists of using these new functions on OpenSSL 1.1.0 and newer. > > There are a few things worth calling out specifically in this patch: > > - I was not 100% clear on the handshake logic. It looked like it tried to > detect if a handshake was ever attempted and distinguish that from a failure > case. I've used the new state mechanism available through SSL_get_state to > hopefully mimic similar behavior. I might have gotten this totally wrong > though. > - The Makefile change was needed so that linking the OpenSSL bits also pulls > in dl if needed (OpenSSL uses this itself). Also OpenSSL will now use pthread > by default, so maybe that also should be added? Although I've used > USE_PTHREAD_PSHARED for now in testing to link that. > - The code guarded with #ifdef SSL_CTX_get_tlsext_status_arg ideally would > also use that macro, but there seems to be a closing brace missing at > https://github.com/openssl/openssl/blob/fddfc0afc84728f8a5140685163e66ce6471742d/include/openssl/tls1.h#L300-L301 > so it throws an error. That's why I've used the implementation of that macro > in the code instead. > > What this does not address at the moment is fixing the use of deprecated > functions. These are the warnings still present with these changes: > > [jessie-amd64] src/ssl_sock.c: In function 'ssl_tlsext_ticket_key_cb': > [jessie-amd64] src/ssl_sock.c:492:3: warning: 'RAND_pseudo_bytes' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/rand.h:47) > [-Wdeprecated-declarations] > [jessie-amd64] if(!RAND_pseudo_bytes(iv, EVP_MAX_IV_LENGTH)) > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c: In function 'ssl_sock_prepare_ctx': > [jessie-amd64] src/ssl_sock.c:2731:3: warning: 'TLSv1_server_method' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/ssl.h:1597) > [-Wdeprecated-declarations] > [jessie-amd64] SSL_CTX_set_ssl_version(ctx, TLSv1_server_method()); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c:2734:3: warning: 'TLSv1_1_server_method' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/ssl.h:1603) > [-Wdeprecated-declarations] > [jessie-amd64] SSL_CTX_set_ssl_version(ctx, TLSv1_1_server_method()); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c:2738:3: warning: 'TLSv1_2_server_method' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/ssl.h:1609) > [-Wdeprecated-declarations] > [jessie-amd64] SSL_CTX_set_ssl_version(ctx, TLSv1_2_server_method()); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c:2824:13: warning: assignment discards 'const' > qualifier from pointer target type > [jessie-amd64] cipher = sk_SSL_CIPHER_value(ciphers, idx); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c: In function 'ssl_sock_prepare_srv_ctx': > [jessie-amd64] src/ssl_sock.c:3111:3: warning: 'TLSv1_client_method' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/ssl.h:1598) > [-Wdeprecated-declarations] > [jessie-amd64] SSL_CTX_set_ssl_version(srv->ssl_ctx.ctx, > TLSv1_client_method()); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c:3114:3: warning: 'TLSv1_1_client_method' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/ssl.h:1604) > [-Wdeprecated-declarations] > [jessie-amd64] SSL_CTX_set_ssl_version(srv->ssl_ctx.ctx, > TLSv1_1_client_method()); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c:3118:3: warning: 'TLSv1_2_client_method' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/ssl.h:1610) > [-Wdeprecated-declarations] > [jessie-amd64] SSL_CTX_set_ssl_version(srv->ssl_ctx.ctx, > TLSv1_2_client_method()); > [jessie-amd64] ^ > [jessie-amd64] src/ssl_sock.c: In function '__ssl_sock_deinit': > [jessie-amd64] src/ssl_sock.c:6254:9: warning: 'ERR_remove_state' is > deprecated (declared at > /data/build/jessie-amd64/packages/haproxy/tmp-install/openssl-static/include/openssl/err.h:247) > [-Wdeprecated-declarations] > [jessie-amd64] ERR_remove_state(0); > [jessie-amd64] > > > Let me know what you all think. Since the API experienced a major change and causes a lot of #ifdefs, what do you think about proceeding differently and definining these API functions/macros for older versions so that only the new code remains instead ? It seems to me this should be quite possible. Example below : +#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL) + const unsigned char *sid_ctx_data; + + sid_data = SSL_SESSION_get_id(sess, &sid_length); + sid_ctx_data = SSL_SESSION_get0_id_context(sess, &sid_ctx_length); + SSL_SESSION_set1_id(sess, sid_data, 0); + SSL_SESSION_set1_id_context(sess, sid_ctx_data, 0); +#else sid_length = sess->session_id_length; - sess->session_id_length = 0; sid_ctx_length = sess->sid_ctx_length; + sess->session_id_length = 0; sess->sid_ctx_length = 0; + sid_data = sess->session_id; +#endif We'd have to declare SSL_SESSION_get_id(), SSL_SESSION_get0_id_context() and SSL_SESSION_set1_id() if OPENSSL_VERSION_NUMBER < 0x1010000fL and that would be done, we'd only keep the new version. It's interesting to work this way because it avoids the issues that happen when fixing bugs only in one half of the ifdef without realizing that the other half needs a different fix. The part below is an even more obvious candidate : +#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL) + SSL_SESSION_set1_id(sess, sid_data, sid_length); + SSL_SESSION_set1_id_context(sess, sid_ctx_data, sid_ctx_length); +#else sess->session_id_length = sid_length; sess->sid_ctx_length = sid_ctx_length; +#endif And here we see an example where you had to do it but it would rather be centralized at the top of ssl_sock.h for example : - if (!ctx->tlsext_status_cb) { +#ifndef SSL_CTX_get_tlsext_status_cb +# define SSL_CTX_get_tlsext_status_cb(ctx, cb) \ + *cb = (void (*) (void))ctx->tlsext_status_cb; +#endif + SSL_CTX_get_tlsext_status_cb(ctx, &callback); Thanks! Willy
