Hi Hervé,
I have a few comments and concerns about possibly missing tests below.
On Fri, Jan 15, 2010 at 06:30:28PM +0100, Hervé COMMOWICK wrote:
> + unsigned int first_packet_len;
> + first_packet_len = ((unsigned int) trash[0]) + (((unsigned int)
> trash[1]) << 8) + (((unsigned int) trash[2]) << 16);
> +
> + /* MySQL Error packet always begin with field_count = 0xff */
> if (trash[4] != '\xff') {
here we should first test that len > 4.
> - /* We set the MySQL Version in description for
> information purpose
> - * FIXME : it can be cool to use MySQL Version for
> other purpose,
> - * like mark as down old MySQL server.
> - */
> - if (len > 51) {
> + /* Check if we have only one MySQL packet in buffer */
> + if (len == first_packet_len + 4) {
> + /* We have only one MySQL paquet
> + * and it seems to be a Handshake
> Initialization packet
> + * It is not normal because we must normally
> have 2 packet
> + * but it can be good on *real* low latency
> network
> + */
> +
> + /* We set the MySQL Version in description for
> information purpose */
> desc = ltrim(&trash[5], ' ');
> set_server_check_status(s, HCHK_STATUS_L7OKD,
> desc);
> }
I think we should enter the test above if (len >= first_packet_len + 4 and <
(size of 2 packets)).
> + else if (len > first_packet_len + 4) {
here, we're not validating enough data, we should have at least
len > first_packet_len + 6. Imagine len == first_packet_len + 5,
see below :
> + unsigned int second_packet_len;
> + second_packet_len = ((unsigned int)
> trash[first_packet_len+4]) + (((unsigned int) trash[first_packet_len+5]) <<
> 8) + (((unsigned int) trash[first_packet_len+6]) << 16);
we use trash[first_packet_len+5] and trash[first_packet_len+6] which were not
fed
by this read but which were lying there unused in the buffer.
> + if (len == first_packet_len + 4 +
> second_packet_len + 4 ) {
> + /* We have 2 packet and that's good */
I don't know here if we can get more than two packets (eg: newer versions ?) but
just in case, maybe replace == with >= ?
> + /* Check if the second packet is not a
> MySQL Error packet */
> + if (trash[first_packet_len+8] !=
> '\xff') {
Trash[first_packet_len+8] may be out of the buffer if second_packet_len == 0.
So I think a more reliable way to do this consists in the following sequence :
if (len <= 4 || trash[4] == '\xff') fail;
first_packet_len = ...
if (len < first_packet_len + 4) fail; /* truncated packet */
/* now you have *at least* one packet */
second_packet_len = ...
if (len < first_packet_len + 7 || !second_packet_len ||
len <= first_packet_len + 4 + second_packet_len + 4 ) {
/* single packet only */
}
/* here you know you have at least two full packets */
> --- haproxy/doc/configuration.txt 2010-01-14 09:37:09.000000000 +0100
> +++ haproxy.dev/doc/configuration.txt 2010-01-15 16:09:02.858912373 +0100
> @@ -2780,11 +2780,20 @@
> yes | no | yes | yes
> Arguments : none
>
> - The check consists in parsing Mysql Handshake Initialisation packet or
> Error
> - packet, which is sent by MySQL server on connect. It is a basic but useful
> - test which does not produce any logging on the server. However, it does not
> - check database presence nor database consistency, nor user permission to
> - access. To do this, you can use an external check with xinetd for example.
> + The check consists of sending a Client Authentication packet, with
> "haproxy"
> + username. We parse the Mysql Handshake Initialisation packet and/or
> + Error packet. It is a basic but useful test which does not produce error
> + on the server.
> +
> + However, it requires adding an authorization in the MySQL table, like this
> :
> + USE mysql;
> + INSERT INTO user (Host,User) values ('<ip_of_haproxy>','haproxy');
> + FLUSH PRIVILEGES;
Since this test is much more intrusive than previous version, shouldn't it
be left as an option of the previous one ? That way users could decide whether
to use the less complete test with no change on the server or the more complete
one with changes. This especially makes sense where the LB is shared by several
customers and the LB's admin does not have to impact security parameters of
their customers.
BTW, do you know what is logged on the server when attempting to authenticate
with an empty username ("\0") ? If it's silent and we can recognize the return
code, it could also serve as a valid check ?
Just my 2 cents,
Willy
