Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Bertrand Jacquin]

On 21/12/17 13:28, Willy Tarreau wrote:
> On Thu, Dec 21, 2017 at 10:46:17AM +, 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




signature.asc
Description: OpenPGP digital signature
Amazon Data Services Ireland Limited registered office: One Burlington Plaza, 
Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 
390566.

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

On Thu, Dec 21, 2017 at 10:46:17AM +, 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

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Bertrand Jacquin]

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



signature.asc
Description: OpenPGP digital signature
Amazon Data Services Ireland Limited registered office: One Burlington Plaza, 
Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 
390566.

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

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

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Andreas Mahnke]

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  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
>

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

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

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Andreas Mahnke]

Great,

thank you guys!

Best regards,
Andreas

On Wed, Dec 20, 2017 at 7:06 AM, Willy Tarreau  wrote:

> On Tue, Dec 19, 2017 at 11:10:58PM +, 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
>

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

On Tue, Dec 19, 2017 at 11:10:58PM +, 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

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Bertrand Jacquin]

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 > > 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
From 3245e08747ed3c72fa429365010450c2ca04caac Mon Sep 17 00:00:00 2001
From: Bertrand Jacquin 
Date: Tue, 12 Dec 2017 01:17:23 +
Subject: [PATCH 8/8] MEDIUM: netscaler: add support for standard NetScaler CIP
 protocol

It looks like two version of the protocol exist as reported by
Andreas Mahnke. This patch add support for both legacy and standard CIP
protocol according to NetScaler specifications.
---
 doc/netscaler-client-ip-insertion-protocol.txt | 28 ++-
 src/connection.c   | 38 --
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/doc/netscaler-client-ip-insertion-protocol.txt b/doc/netscaler-client-ip-insertion-protocol.txt
index 6f77f6522c7f..559d98a82a92 100644
--- a/doc/netscaler-client-ip-insertion-protocol.txt
+++ b/doc/netscaler-client-ip-insertion-protocol.txt
@@ -10,7 +10,9 @@ Specifications and documentations from NetScaler:
   https://www.citrix.com/blogs/2016/04/25/how-to-enable-client-ip-in-tcpip-option-of-netscaler/
 
 When CIP is enabled on the NetScaler, then a TCP packet is inserted just after
-the TCP handshake. This is composed as:
+the TCP handshake. Two versions of the CIP extension exist.
+
+Legacy (NetScaler < 10.5)
 
   - CIP magic number : 4 bytes
 Both sender and receiver have to agree on a magic number so that
@@ -27,3 +29,27 @@ the TCP handshake. This is composed as:
   - TCP header : >= 20 bytes
 Contains the header of the last TCP packet sent by the client during TCP
 handshake.
+
+Standard (NetScaler >= 10.5)
+
+  - CIP magic number : 4 bytes
+Both sender and receiver have to agree on a magic number so that
+they both handle the incoming data as a NetScaler Client IP insertion
+packet.
+
+  - CIP length : 4 bytes
+Defines the total length on the CIP header.
+
+  - CIP type: 2 bytes
+Always set to 1.
+
+  - Header length : 2 bytes
+Defines the length on the remaining data.
+
+  - IP header : >= 20 bytes if IPv4, 40 bytes if IPv6
+Contains the header of the last IP packet sent by the client during TCP
+handshake.
+
+  - TCP header : >= 20 bytes
+Contains the header of the last TCP packet sent by the client during TCP
+handshake.
diff --git a/src/connection.c b/src/connection.c
index 58bf4a5f85f5..0f8acb02dbdb 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -678,14 +678,8 @@ int conn_recv_proxy(struct connection *conn, int flag)
 }
 
 /* This handshake handler waits a NetScaler Client IP insertion header
- * at the beginning of the raw data stream. The header looks like this:
- *
- *   4 bytes:   CIP magic number
- *   4 bytes:   Header length
- *   20+ bytes: Header of the last IP packet sent by the client 

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Bertrand Jacquin]

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  > 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



signature.asc
Description: OpenPGP digital signature
Amazon Data Services Ireland Limited registered office: One Burlington Plaza, 
Burlington Road, Dublin 4, Ireland. Registered in Ireland. Registration number 
390566.

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

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

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

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

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Andreas Mahnke]

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 <w...@1wt.eu> 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/haproxy@formilux.org/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 <andreas.mah...@hansemerkur.de>
> > 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
>

Re: [PATCH] BUG: NetScaler CIP handling is incorrect

[Willy Tarreau]

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/haproxy@formilux.org/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 <andreas.mah...@hansemerkur.de>
> 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

[PATCH] BUG: NetScaler CIP handling is incorrect

[Andreas Mahnke]

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/haproxy@formilux.org/msg25100.html

Best regards,
Andreas

>From 2482fc346033cbc8a74cd36d9ef27db4bb0daf6c Mon Sep 17 00:00:00 2001
From: mahnke <andreas.mah...@hansemerkur.de>
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