Hi Christopher,
On Fri, Oct 16, 2015 at 10:03:06AM +0200, Christopher Faulet wrote:
> First the LRU tree is only initialized when the SSL certs generation is
> configured on a bind line. So, in the most of cases, it is NULL (it is
> not the same thing than empty).
> When the SSL certs generation is used, if the cache is not NULL, a such
> certificate is pushed in the cache and there is no need to release it
> when the connection is closed.
> But it can be disabled in the configuration. So in that case, we must
> free the generated certificate when the connection is closed.
>
> Then here, we have really a bug. Here is the buggy part:
>
> 3125) if (conn->xprt_ctx) {
> 3126) #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
> 3127) if (!ssl_ctx_lru_tree && objt_listener(conn->target)) {
> 3128) SSL_CTX *ctx = SSL_get_SSL_CTX(conn->xprt_ctx);
> 3129) if (ctx != 3130)
> SSL_CTX_free(ctx);
> 3131) }
> #endif
> 3133) SSL_free(conn->xprt_ctx);
> 3134) conn->xprt_ctx = NULL;
> 3135) sslconns--;
> 3136) }
>
> The check on the line 3127 is not enough to determine if this is a
> generated certificate or not. Because ssl_ctx_lru_tree is NULL,
> generated certificates, if any, must be freed. But here ctx should also
> be compared to all SNI certificates and not only to default_ctx. Because
> of this bug, when a SNI certificate is used for a connection, it is
> erroneously freed when this connection is closed.
Yep, thanks for the reminder. You already explained this to me once but
it appears that it didn't remain obvious in my mind.
Thus this sparks a new question : when the cache is disabled, are we sure
to always free the ssl_ctx on all error paths after it's generated ? Or
are we certain that we always pass through ssl_sock_close() ?
The other problem I'm having is related to how we manage the LRU cache.
Is there a risk that we kill some objects in this cache while they're
still in use ?
> In my patch, I chosen to use a specific flag on the connection instead
> of doing certificates comparisons. This seems to me easier to understand
> and more efficient. But it could be discussed, there are many other
> solutions I guess.
I'm not disagreeing with your proposal, it may even end up being the only
solution. I'm just having a problem with keeping this information outside
of the ssl_ctx itself while I think we have to deal with a ref count there
due to it being present both in the tree and in sessions which make use of
it. Very likely we'll have the flag in the connection to indicate that the
cert was generated and must be freed one way or another, but I'm still
bothered by checking the lru_tree when doing this because I fear that it
means that we don't properly track the ssl_ctx's usage.
> finally, we can of course discuss the design of this feature. There is
> no problem. I will be happy to find a more elegant way to handle it, if
> it is possible.
Ideally we'd have the info in the ssl_ctx itself, but I remember that Emeric
told me a while ago that we couldn't store anything in an ssl_ctx. Thus I
can understand that we can't easily "tag" the ssl_ctx as being statically
or dynamically allocated, which is why I understand the need for the flag
on the connection as an alternative.
Willy
