httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kaspar Brand <httpd-dev.2...@velox.ch>
Subject Re: [PATCH] mod_ssl: improving session caching for SNI configurations
Date Sat, 07 Nov 2009 14:56:00 GMT
Dr Stephen Henson wrote:
> A few comments about that:

Thanks for the review!

> These are cryptographic keys (or at least the HMAC and AES keys are) so you
> should use RAND_bytes(), not RAND_pseudo_bytes().

Ok - when looking at ssl_lib.c:SSL_CTX_new(), I didn't realize that
RAND_pseudo_bytes() is only used for tlsext_tick_key_name. Changed
accordingly.

> Don't dereference the structures directly as at some point the sizes might
> change, the structure made opaque or a different mechanism used for storing keys
> (e.g. HSM support).

I was looking at a way to determine the size at compile time, but if you
think that's an unsafe method (note: it's only expected to work for
0.9.8f through 0.9.8l), then let's change it.

> The approved way is to call:
> 
> SSL_CTX_set_tlsext_ticket_keys(sc->server->ssl_ctx, NULL, -1)
> 
> which will return the combined length of all keys.

Did that - does v3 of the patch (attached) look better? Is it ok to use
apr_palloc here?

> Finally:
> 
> +            sid_ctx = ap_md5_binary(c->pool, (unsigned char*)sc->vhost_id,
> +                                    sc->vhost_id_len);
> 
> should we be using MD5 now if it can be avoided?

That's what is currently used in mod_ssl.c:ssl_init_ssl_connection() -
these two should be kept in sync. I'm not opposed to a change (at both
places), but the question is whether now is the right time for doing
that. For distcache setups e.g. I would assume that it has some unwanted
consequences if two 2.2.x releases calculate the session id context in a
different way... but I'm interested to hear others' opinions on this.

Kaspar

Mime
View raw message