Welcome! Log In Create A New Profile

Advanced

[PATCH] BUG: NetScaler CIP handling is incorrect

Posted by Andreas Mahnke 
Andreas Mahnke
[PATCH] BUG: NetScaler CIP handling is incorrect
July 06, 2017 01:40PM
Hello,

this patch resolves a bug inside the "NetScaler CIP handling"
implementation.
The issue is explained deeper in the existing mailling list thread under
[1].

[1]: https://www.mail-archive.com/[email protected]/msg25100.html

Best regards,
Andreas

From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
From: mahnke <[email protected]>
Date: Thu, 6 Jul 2017 12:56:10 +0200
Subject: [PATCH] BUG: NetScaler CIP handling is incorrect

The "NetScaler CIP handling" implementation was not working as expected.

Based on tcp dump analysis of the CIP data and the specificaton from
citrix:

- https://support.citrix.com/article/CTX205670
-
https://www.citrix.com/blogs/2016/04/25/how-to-enable-client-ip-in-tcpip-option-of-netscaler/

some adjustments had to be done in the code in order to get the feature
working as expected.

The fix was tested with IPv4 and IPv6 backends using NetScaler VPX
version "NS11.1: Build 54.14.nc"
---
src/connection.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 3629094..492fbfe 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -722,7 +722,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
int flag)
if (trash.len < 28)
goto missing;

- line += 8;
+ line += 12;

/* Get IP version from the first four bits */
ip_v = (*line & 0xf0) >> 4;
@@ -741,7 +741,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
int flag)
/* The protocol does not include a TCP header */
conn->err_code = CO_ER_CIP_BAD_PROTO;
goto fail;
- } else if (trash.len < (28 + ntohs(hdr_ip4->ip_len))) {
+ } else if (trash.len < (12 + ntohs(hdr_ip4->ip_len))) {
/* Fail if buffer length is not large enough to contain
* CIP magic, CIP length, IPv4 header, TCP header */
goto missing;
@@ -799,7 +799,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
int flag)
goto fail;
}

- line += cip_len;
+ line += (cip_len - 12);
trash.len = line - trash.str;

/* remove the NetScaler Client IP header from the request. For this
--
2.7.4
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
July 07, 2017 02:30PM
Hi Andreas,

[ Ccing Bertrand ]

On Thu, Jul 06, 2017 at 01:31:27PM +0200, Andreas Mahnke wrote:
> Hello,
>
> this patch resolves a bug inside the "NetScaler CIP handling"
> implementation.
> The issue is explained deeper in the existing mailling list thread under
> [1].
>
> [1]: https://www.mail-archive.com/[email protected]/msg25100.html

We didn't get feedback from Bertrand who was trying to contact Citrix
regarding this. However, I agree that your changes match the examples
shown on the support links you provided. Maybe Bertrand is/was using
a different version ?

Thanks,
Willy

----
> From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
> From: mahnke <[email protected]>
> Date: Thu, 6 Jul 2017 12:56:10 +0200
> Subject: [PATCH] BUG: NetScaler CIP handling is incorrect
>
> The "NetScaler CIP handling" implementation was not working as expected.
>
> Based on tcp dump analysis of the CIP data and the specificaton from
> citrix:
>
> - https://support.citrix.com/article/CTX205670
> -
> https://www.citrix.com/blogs/2016/04/25/how-to-enable-client-ip-in-tcpip-option-of-netscaler/
>
> some adjustments had to be done in the code in order to get the feature
> working as expected.
>
> The fix was tested with IPv4 and IPv6 backends using NetScaler VPX
> version "NS11.1: Build 54.14.nc"
> ---
> src/connection.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index 3629094..492fbfe 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -722,7 +722,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> int flag)
> if (trash.len < 28)
> goto missing;
>
> - line += 8;
> + line += 12;
>
> /* Get IP version from the first four bits */
> ip_v = (*line & 0xf0) >> 4;
> @@ -741,7 +741,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> int flag)
> /* The protocol does not include a TCP header */
> conn->err_code = CO_ER_CIP_BAD_PROTO;
> goto fail;
> - } else if (trash.len < (28 + ntohs(hdr_ip4->ip_len))) {
> + } else if (trash.len < (12 + ntohs(hdr_ip4->ip_len))) {
> /* Fail if buffer length is not large enough to contain
> * CIP magic, CIP length, IPv4 header, TCP header */
> goto missing;
> @@ -799,7 +799,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> int flag)
> goto fail;
> }
>
> - line += cip_len;
> + line += (cip_len - 12);
> trash.len = line - trash.str;
>
> /* remove the NetScaler Client IP header from the request. For this
> --
> 2.7.4
Andreas Mahnke
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
July 07, 2017 02:40PM
Hi Willy,

I had some direct mail conversations with him and he wanted to create a
patch based in the findings.
In the meantime we got it working using the patch I provided, therefore I
sent it yesterday so that it gets integrated and we do not need to patch
haproxy on our end everytime a new version comes out.

I wrote him yesterday that the patch was sent by me, but he seems to be out
of office until monday - so maybe he will get back to us then.

Regards,
Andreas

On Fri, Jul 7, 2017 at 2:25 PM, Willy Tarreau <[email protected]> wrote:

> Hi Andreas,
>
> [ Ccing Bertrand ]
>
> On Thu, Jul 06, 2017 at 01:31:27PM +0200, Andreas Mahnke wrote:
> > Hello,
> >
> > this patch resolves a bug inside the "NetScaler CIP handling"
> > implementation.
> > The issue is explained deeper in the existing mailling list thread under
> > [1].
> >
> > [1]: https://www.mail-archive.com/[email protected]/msg25100.html
>
> We didn't get feedback from Bertrand who was trying to contact Citrix
> regarding this. However, I agree that your changes match the examples
> shown on the support links you provided. Maybe Bertrand is/was using
> a different version ?
>
> Thanks,
> Willy
>
> ----
> > From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
> > From: mahnke <[email protected]>
> > Date: Thu, 6 Jul 2017 12:56:10 +0200
> > Subject: [PATCH] BUG: NetScaler CIP handling is incorrect
> >
> > The "NetScaler CIP handling" implementation was not working as expected.
> >
> > Based on tcp dump analysis of the CIP data and the specificaton from
> > citrix:
> >
> > - https://support.citrix.com/article/CTX205670
> > -
> > https://www.citrix.com/blogs/2016/04/25/how-to-enable-
> client-ip-in-tcpip-option-of-netscaler/
> >
> > some adjustments had to be done in the code in order to get the feature
> > working as expected.
> >
> > The fix was tested with IPv4 and IPv6 backends using NetScaler VPX
> > version "NS11.1: Build 54.14.nc"
> > ---
> > src/connection.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/connection.c b/src/connection.c
> > index 3629094..492fbfe 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -722,7 +722,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> > int flag)
> > if (trash.len < 28)
> > goto missing;
> >
> > - line += 8;
> > + line += 12;
> >
> > /* Get IP version from the first four bits */
> > ip_v = (*line & 0xf0) >> 4;
> > @@ -741,7 +741,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> > int flag)
> > /* The protocol does not include a TCP header */
> > conn->err_code = CO_ER_CIP_BAD_PROTO;
> > goto fail;
> > - } else if (trash.len < (28 + ntohs(hdr_ip4->ip_len))) {
> > + } else if (trash.len < (12 + ntohs(hdr_ip4->ip_len))) {
> > /* Fail if buffer length is not large enough to contain
> > * CIP magic, CIP length, IPv4 header, TCP header */
> > goto missing;
> > @@ -799,7 +799,7 @@ int conn_recv_netscaler_cip(struct connection *conn,
> > int flag)
> > goto fail;
> > }
> >
> > - line += cip_len;
> > + line += (cip_len - 12);
> > trash.len = line - trash.str;
> >
> > /* remove the NetScaler Client IP header from the request. For this
> > --
> > 2.7.4
>
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
July 07, 2017 02:50PM
On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
> Hi Willy,
>
> I had some direct mail conversations with him and he wanted to create a
> patch based in the findings.
> In the meantime we got it working using the patch I provided, therefore I
> sent it yesterday so that it gets integrated and we do not need to patch
> haproxy on our end everytime a new version comes out.
>
> I wrote him yesterday that the patch was sent by me, but he seems to be out
> of office until monday - so maybe he will get back to us then.

Yep I noticed the out-of-office notification as well. Let's wait for him to
get back. I'm pretty fine with merging your patch, I just want to be certain
that everything is OK on his side as well, especially before backporting it
(since it's a bug fix).

Thanks!
Willy
Andreas Mahnke
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 07, 2017 02:50PM
Hello everybody,

Is there an update regarding the merging of the patch? We are thinking to
switch to version 1.8 in the near future and it would be nice to have the
patch merged, so that we do not need to merge each update on our own.

Best regards,
Andreas

On Fri, Jul 7, 2017 at 2:43 PM, Willy Tarreau <[email protected]> wrote:

> On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
> > Hi Willy,
> >
> > I had some direct mail conversations with him and he wanted to create a
> > patch based in the findings.
> > In the meantime we got it working using the patch I provided, therefore I
> > sent it yesterday so that it gets integrated and we do not need to patch
> > haproxy on our end everytime a new version comes out.
> >
> > I wrote him yesterday that the patch was sent by me, but he seems to be
> out
> > of office until monday - so maybe he will get back to us then.
>
> Yep I noticed the out-of-office notification as well. Let's wait for him to
> get back. I'm pretty fine with merging your patch, I just want to be
> certain
> that everything is OK on his side as well, especially before backporting it
> (since it's a bug fix).
>
> Thanks!
> Willy
>
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 07, 2017 07:50PM
Hi Andreas,

On Thu, Dec 07, 2017 at 02:41:29PM +0100, Andreas Mahnke wrote:
> Hello everybody,
>
> Is there an update regarding the merging of the patch? We are thinking to
> switch to version 1.8 in the near future and it would be nice to have the
> patch merged, so that we do not need to merge each update on our own.

It seems nobody involved had any time to get back on this unfortunately.

Bertrand, just as a quick refresh, Andreas proposed a patch in this
thread changing the way you parse the NS CIP. If you're using it and
it works for you, then I suspect there might be several incompatible
versions in the wild and the fix could break other use cases. Otherwise
maybe it only affects a part you don't rely on. So I just need your
ACK/NACK on his patch (the first one in this thread started in July).

Thanks!
Willy
Bertrand Jacquin
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 11, 2017 04:10PM
Hi Andreas,

I got this really side tracked, my apology. Let me take a look at that
this evening again. Some corps need to be unburied.

I'm afraid the patch, as is, will break compatibility with other version
of the CIP protocol, I'd like haproxy to support both of them.

Cheers,
Bertrand

On 07/12/17 13:41, Andreas Mahnke wrote:
> Hello everybody,
>
> Is there an update regarding the merging of the patch? We are thinking
> to switch to version 1.8 in the near future and it would be nice to have
> the patch merged, so that we do not need to merge each update on our own.
>
> Best regards,
> Andreas
>
> On Fri, Jul 7, 2017 at 2:43 PM, Willy Tarreau <[email protected]
> <mailto:[email protected]>> wrote:
>
> On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
> > Hi Willy,
> >
> > I had some direct mail conversations with him and he wanted to create a
> > patch based in the findings.
> > In the meantime we got it working using the patch I provided, therefore I
> > sent it yesterday so that it gets integrated and we do not need to patch
> > haproxy on our end everytime a new version comes out.
> >
> > I wrote him yesterday that the patch was sent by me, but he seems to be out
> > of office until monday - so maybe he will get back to us then.
>
> Yep I noticed the out-of-office notification as well. Let's wait for
> him to
> get back. I'm pretty fine with merging your patch, I just want to be
> certain
> that everything is OK on his side as well, especially before
> backporting it
> (since it's a bug fix).
>
> Thanks!
> Willy
>
>

--
Bertrand
Payments Infrastructure Engineering, Amazon

Amazon Data Services Ireland Limited registered office: One Burlington Plaza, Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 390566.
Bertrand Jacquin
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 20, 2017 12:20AM
Hi Andreas and Willy,

Please find attached a patch serie adding support for both legacy and
standard CIP protocol while keeping compatibility with current
configuration format.

This also fixes numerous bugs spotted during this dev cycle and present
since the first version of the patch.

This serie applies cleanly on v1.9-dev0-76-g789691778fde but also on
v1.8.1-20-gdd8ea125889d while I only tested it on v1.9.

Cheers,
Bertrand

On 11/12/17 15:04, Bertrand Jacquin wrote:
> Hi Andreas,
>
> I got this really side tracked, my apology. Let me take a look at that
> this evening again. Some corps need to be unburied.
>
> I'm afraid the patch, as is, will break compatibility with other version
> of the CIP protocol, I'd like haproxy to support both of them.
>
> Cheers,
> Bertrand
>
> On 07/12/17 13:41, Andreas Mahnke wrote:
>> Hello everybody,
>>
>> Is there an update regarding the merging of the patch? We are thinking
>> to switch to version 1.8 in the near future and it would be nice to have
>> the patch merged, so that we do not need to merge each update on our own.
>>
>> Best regards,
>> Andreas
>>
>> On Fri, Jul 7, 2017 at 2:43 PM, Willy Tarreau <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>> On Fri, Jul 07, 2017 at 02:36:07PM +0200, Andreas Mahnke wrote:
>> > Hi Willy,
>> >
>> > I had some direct mail conversations with him and he wanted to create a
>> > patch based in the findings.
>> > In the meantime we got it working using the patch I provided, therefore I
>> > sent it yesterday so that it gets integrated and we do not need to patch
>> > haproxy on our end everytime a new version comes out.
>> >
>> > I wrote him yesterday that the patch was sent by me, but he seems to be out
>> > of office until monday - so maybe he will get back to us then.
>>
>> Yep I noticed the out-of-office notification as well. Let's wait for
>> him to
>> get back. I'm pretty fine with merging your patch, I just want to be
>> certain
>> that everything is OK on his side as well, especially before
>> backporting it
>> (since it's a bug fix).
>>
>> Thanks!
>> Willy
>>
>>
>

--
Bertrand
Payments Infrastructure Engineering, Amazon
Amazon Data Services Ireland Limited registered office: One Burlington Plaza, Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 390566.
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 20, 2017 07:10AM
On Tue, Dec 19, 2017 at 11:10:58PM +0000, Bertrand Jacquin wrote:
> Hi Andreas and Willy,
>
> Please find attached a patch serie adding support for both legacy and
> standard CIP protocol while keeping compatibility with current
> configuration format.

Excellent, now applied to 1.9, will backport it to 1.8 later.

Thanks a lot guys, I've seen how many round trips it required to
validate these changes on your respective infrastructures, it was
a very productive cooperation!

Cheers,
Willy
Andreas Mahnke
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 20, 2017 09:30AM
Great,

thank you guys!

Best regards,
Andreas

On Wed, Dec 20, 2017 at 7:06 AM, Willy Tarreau <[email protected]> wrote:

> On Tue, Dec 19, 2017 at 11:10:58PM +0000, Bertrand Jacquin wrote:
> > Hi Andreas and Willy,
> >
> > Please find attached a patch serie adding support for both legacy and
> > standard CIP protocol while keeping compatibility with current
> > configuration format.
>
> Excellent, now applied to 1.9, will backport it to 1.8 later.
>
> Thanks a lot guys, I've seen how many round trips it required to
> validate these changes on your respective infrastructures, it was
> a very productive cooperation!
>
> Cheers,
> Willy
>
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 21, 2017 11:10AM
Bertrand, Andreas,

The NS CIP fixes were backported to 1.8. Initially I was not considering
backporting your latest changes to support the new version of the protocol
since it's a feature addition. But now that I'm thinking about it, the
change is minimal and very well isolated and you very likely are the two
only users of this feature, so even if it would break anything it would
be your problem :-) Thus my question is simple : do you *need* support
of the standard protocol support in 1.8 or not, and is any of you against
the backport if the other one needs it ?

Please just le me know.

Thanks,
Willy
Andreas Mahnke
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 21, 2017 11:10AM
Hi Willy,

The support of the standard protocol in 1.8 would be nice, because we are
planning to migrate to haproxy 1.8 from our self - patched 1.7 instances.

Regards,
Andreas



On Thu, Dec 21, 2017 at 10:57 AM, Willy Tarreau <[email protected]> wrote:

> Bertrand, Andreas,
>
> The NS CIP fixes were backported to 1.8. Initially I was not considering
> backporting your latest changes to support the new version of the protocol
> since it's a feature addition. But now that I'm thinking about it, the
> change is minimal and very well isolated and you very likely are the two
> only users of this feature, so even if it would break anything it would
> be your problem :-) Thus my question is simple : do you *need* support
> of the standard protocol support in 1.8 or not, and is any of you against
> the backport if the other one needs it ?
>
> Please just le me know.
>
> Thanks,
> Willy
>
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 21, 2017 11:20AM
On Thu, Dec 21, 2017 at 11:05:30AM +0100, Andreas Mahnke wrote:
> Hi Willy,
>
> The support of the standard protocol in 1.8 would be nice, because we are
> planning to migrate to haproxy 1.8 from our self - patched 1.7 instances.

OK. Bertrand, if you don't scream quickly, I'll pick your patches in 1.8 :-)

Willy
Bertrand Jacquin
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 21, 2017 12:00PM
Hi Willy,

On 21/12/17 10:08, Willy Tarreau wrote:
> On Thu, Dec 21, 2017 at 11:05:30AM +0100, Andreas Mahnke wrote:
>> Hi Willy,
>>
>> The support of the standard protocol in 1.8 would be nice, because we are
>> planning to migrate to haproxy 1.8 from our self - patched 1.7 instances.
>
> OK. Bertrand, if you don't scream quickly, I'll pick your patches in 1.8 :-)

I'm all good with backporting this in 1.8. Feel free to.

Cheers,
Bertrand

--
Bertrand

Amazon Data Services Ireland Limited registered office: One Burlington Plaza, Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 390566.
Willy Tarreau
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 21, 2017 02:40PM
On Thu, Dec 21, 2017 at 10:46:17AM +0000, Bertrand Jacquin wrote:
> I'm all good with backporting this in 1.8. Feel free to.

Great, now merged. Do not hesitate to report back any issues you
would notice on your infrastructure.

cheers,
Willy
Bertrand Jacquin
Re: [PATCH] BUG: NetScaler CIP handling is incorrect
December 21, 2017 03:20PM
On 21/12/17 13:28, Willy Tarreau wrote:
> On Thu, Dec 21, 2017 at 10:46:17AM +0000, Bertrand Jacquin wrote:
>> I'm all good with backporting this in 1.8. Feel free to.
>
> Great, now merged. Do not hesitate to report back any issues you
> would notice on your infrastructure.

Thanks Willy, sure I will!

--
Bertrand


Amazon Data Services Ireland Limited registered office: One Burlington Plaza, Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 390566.
Sorry, only registered users may post in this forum.

Click here to login