Hi Christopher, Emeric,
Op 4-3-2016 om 10:44 schreef Christopher Faulet:
Hi guys,
Sorry for the delay, I was pretty busy. I've checked your patch. It is
quite interesting. First of all, I think that "tcp fallback" option and
"conditional SSL offloading" are redundant. Your way to do is more
flexible and generic, but the spirit is the same. So I'm tempted to
trash my patch.
Then, about your changes, there are 2/3 things that can be discussed.
You've added integers in the connection structure. Willy already
expressed the need to keep it as small as possible.
I should probably have taken more effort to use bit flags for the
tracking the 'state', the loaded handshake bytes where needed with my
implementation using the peeking method though. Anyway all of it is gone
now :).
So it might be fine
to find another way to do. Using BIO is probably a good alternative (but
no the easier one...).
I couldn't find how to get the first handshake set through the BIO and
then directly switch to FD, looks clean the way you did it, it would
have taken me a long time to find. This avoids the whole need to peek
the data, and i expect it will work better overall.
And, your way to peeking data seems to "penalize"
all streams. This is not huge of course, but this could be improved. I'm
also curious to see if it works with a complex "tcp-request content"
ruleset. Finally, defining the "offloadssl" action on "tcp-response
content" is useless and unused.
In my test configuration it was used to make the difference between
backend-ssl and backend-offload which seemed to work. Also it was more
of a 'proof of concept' then a final patch to be actually applied to
haproxy..
But I really like your idea. So I've reworked it. Let me known if it's
ok for you.
I have tested your new patch with "OpenSSL 1.0.1r-freebsd 28 Jan 2016"
and it seems to work correctly for all circumstances i tried it with.
It uses BIO to let connection structure untouched and to have no
overhead when the feature is not used. I've done some test but not much.
And I don't know if overhead imposed by BIO is huge or not. I'm not an
OpenSSL expert, so maybe there is a better way to do. I've renamed
"offloadssl" action to use "ssl-upgrade" and "no-ssl-upgrade" instead.
My patch is done on the HEAD of the master branch, ignoring my previous
patch. Note that you need to postpone the SSL upgrade to use this
feature. To do so, you must add "defer-ssl-upgrade" on the bind line.
Willy, if you are agree, this new patch can replace my previous one. Of
course, all remarks are welcome. I'll try to do more tests. I quickly
checked it on OpenSSL 0.9.8zg and 1.0.2f.
Thanks for your time to re-implement the patch as this is more flexible
than your original, and cleaner than my addition.
I'm adding Emeric in the mail as he is the SSL maintainer for haproxy,
maybe he can tell if the patch is good to merge or if using the BIO like
this could become a problem somewhere.?
Regards,
PiBa-NL
