Re: Small patch for the appsession feature

2009-11-29 Thread Cyril Bonté
Hello,
considering what we said in this thread, here is the complete patch.

To sum up :
- len : it's now the max number of characters for the value, preventing 
garbaged results.
- a new option prefix is added, this allows to use dynamic cookie names (e.g. 
ASPSESSIONIDXXX).
Previously in the thread, I wanted to use the value found with capture cookie 
but when i started to update the documentation, I found this solution quite 
weird.
I've made a small rework to not depend on capture cookie.
-  There's the posssiblity to define the URL parser mode (path parameters or 
query string).

This patch is based on the haproxy-ss-20091129 snapshot.

Le mercredi 18 novembre 2009 06:40:52, Willy Tarreau a écrit :
 I think that if it causes some trouble, we can optionally limit the
 position of the argument in the query-string. But right now I don't
 think it is much of a problem, because :
1) we don't check the query string when we find the cookie in the
   headers (at least I believe :-))

Well, after a last minute check, the URL parameters were parsed before the 
client cookies.
I've interverted the 2 steps so that URL parameters are only parsed if no 
client cookie is found.

Aleksandar, Willy, is this ok for you ?

-- 
Cyril Bonté
diff -Naur haproxy-ss-20091129/doc/configuration.txt haproxy-ss-20091129-appsession-options/doc/configuration.txt
--- haproxy-ss-20091129/doc/configuration.txt	2009-11-28 08:21:29.0 +0100
+++ haproxy-ss-20091129-appsession-options/doc/configuration.txt	2009-11-29 19:57:34.0 +0100
@@ -861,7 +861,8 @@
   See section 7 about ACL usage.
 
 
-appsession cookie len length timeout holdtime [request-learn]
+appsession cookie len length timeout holdtime
+   [request-learn] [prefix] [mode path-parameters|query-string]
   Define session stickiness on an existing application cookie.
   May be used in sections :   defaults | frontend | listen | backend
  no|no|   yes  |   yes
@@ -869,7 +870,7 @@
 cookie   this is the name of the cookie used by the application and which
HAProxy will have to learn for each new session.
 
-length   this is the number of characters that will be memorized and
+length   this is the max number of characters that will be memorized and
checked in each cookie value.
 
 holdtime this is the time after which the cookie will be removed from
@@ -884,13 +885,34 @@
the application's session and the correct server is selected.
It is recommended to specify this option to improve reliability.
 
+prefix When this option is specified, haproxy will match on the cookie
+   prefix (or URL parameter prefix). The appsession value is the
+   data following this prefix.
+
+   Example :
+   appsession ASPSESSIONID len 64 timeout 3h prefix
+
+   This will match the cookie ASPSESSIONID=X,
+   the appsession value will be =X.
+
+mode   This option allows to change the URL parser mode.
+   2 modes are currently supported :
+   - path-parameters :
+ The parser looks for the appsession in the path parameters
+ part (each parameter is separated by a semi-colon), which is
+ convenient for JSESSIONID for example.
+ This is the default mode if the option is not set.
+   - query-string :
+ In this mode, the parser will look for the appsession in the
+ query string.
+ 
   When an application cookie is defined in a backend, HAProxy will check when
   the server sets such a cookie, and will store its value in a table, and
   associate it with the server's identifier. Up to length characters from
   the value will be retained. On each connection, haproxy will look for this
-  cookie both in the Cookie: headers, and as a URL parameter in the query
-  string. If a known value is found, the client will be directed to the server
-  associated with this value. Otherwise, the load balancing algorithm is
+  cookie both in the Cookie: headers, and as a URL parameter (depending on
+  the mode used). If a known value is found, the client will be directed to the
+  server associated with this value. Otherwise, the load balancing algorithm is
   applied. Cookies are automatically removed from memory when they have been
   unused for a duration longer than holdtime.
 
diff -Naur haproxy-ss-20091129/include/proto/proto_http.h haproxy-ss-20091129-appsession-options/include/proto/proto_http.h
--- haproxy-ss-20091129/include/proto/proto_http.h	2009-11-28 08:21:29.0 +0100
+++ haproxy-ss-20091129-appsession-options/include/proto/proto_http.h	2009-11-29 18:38:54.0 +0100
@@ -75,7 +75,7 @@
 int apply_filter_to_req_line(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_request(struct session *t, 

Re: Small patch for the appsession feature

2009-11-29 Thread Cyril Bonté
Le dimanche 29 novembre 2009 23:43:15, Willy Tarreau a écrit :
 The suggested way to to that is to enclose the values between the comment
 and the mask like here (though any other proposal might fit) :
 
 /* appsession */
 #define PR_O2_AS_REQL 0x4000  /* learn the session id from the 
 request */
 #define PR_O2_AS_PFX  0x8000  /* match on the cookie prefix */
 
 /* Encoding of appsession cookie matching modes : 2 possible values = 1 bit 
 */
 #define PR_O2_AS_M_PP 0x  /* path-parameters mode (the default 
 mode) */
 #define PR_O2_AS_M_QS 0x0001  /* query-string mode */
 #define PR_O2_AS_M_ANY0x0001  /* mask covering all 
 PR_O2_AS_M_* values */

I'll remember that if I have more patches to send ;)

   void get_srv_from_appsession(struct session *t, const char *begin, int len)
   {
  -   char *request_line;
  +   char *end_params, *first_param, *cur_param, *next_param;
  +   char separator;
  +   int value_len;
  +
  +   int mode = t-be-options2  PR_O2_AS_M_ANY;
   
  if (t-be-appsession_name == NULL ||
  -   (t-txn.meth != HTTP_METH_GET  t-txn.meth != HTTP_METH_POST) ||
  -   (request_line = memchr(begin, ';', len)) == NULL ||
  -   ((1 + t-be-appsession_name_len + 1 + t-be-appsession_len)  
  (begin + len - request_line)))
  +   (t-txn.meth != HTTP_METH_GET  t-txn.meth != HTTP_METH_POST)) {
  return;
  +   }
 
 I may be wrong but I think we lost a length check above and that
 we'll always onsider we have len bytes available in the buffer
 so that memchr(begin, ';', len) has no chance of going beyond the
 buffer's limit. Oh well, maybe it was already like that :-/
 
 Could you please check on your side and confirm/infirm my doubts ?
 Basically I want to ensure we never dereference the buffer past its
 end, so begin+len bust always be below the buffer size. If you think
 a control is missing, we can merge it as a separate patch because
 it's already missing in current code then.

Previously, the function was called with :
get_srv_from_appsession(s, req-data[msg-som], msg-sl.rq.l);

I assume that msg-sl.rq variables are already correctly calculated and that we 
can replace this call with :
get_srv_from_appsession(s, req-data[msg-som + msg-sl.rq.u], 
msg-sl.rq.u_l);

This allows to parse only the URL, skipping the http method and the protocol.
I believe this should always be contained in the buffer size.
The parser will then restrict to this area when it will extract the session 
value (It was missing in the previous code).

Did I understand what you wanted to know ?

 
 I'm just waiting for your response on this possible issue and I'm
 OK to merge it. Please tell me if you'd prefer to resend a different
 patch with the cosmetic changes or if I can do them myself. While
 you're at it, I noticed a mis-indented if statement alone in the
 remaining part. Also something I can fix if needed.

It would be great if you can do this changes (to prevent several patch versions 
in case I still leave some mis-indented code or misplaced comments in the 
options constants).
Thanks a lot.

-- 
Cyril Bonté



Re: Small patch for the appsession feature

2009-11-29 Thread Willy Tarreau
Hi Cyril,

On Mon, Nov 30, 2009 at 12:53:34AM +0100, Cyril Bonté wrote:
  Could you please check on your side and confirm/infirm my doubts ?
  Basically I want to ensure we never dereference the buffer past its
  end, so begin+len bust always be below the buffer size. If you think
  a control is missing, we can merge it as a separate patch because
  it's already missing in current code then.
 
 Previously, the function was called with :
   get_srv_from_appsession(s, req-data[msg-som], msg-sl.rq.l);
 
 I assume that msg-sl.rq variables are already correctly calculated and that 
 we can replace this call with :
   get_srv_from_appsession(s, req-data[msg-som + msg-sl.rq.u], 
 msg-sl.rq.u_l);
 
 This allows to parse only the URL, skipping the http method and the protocol.
 I believe this should always be contained in the buffer size.
 The parser will then restrict to this area when it will extract the session 
 value (It was missing in the previous code).
 
 Did I understand what you wanted to know ?

Yes, that's fine then, thanks for the check.

  I'm just waiting for your response on this possible issue and I'm
  OK to merge it. Please tell me if you'd prefer to resend a different
  patch with the cosmetic changes or if I can do them myself. While
  you're at it, I noticed a mis-indented if statement alone in the
  remaining part. Also something I can fix if needed.
 
 It would be great if you can do this changes (to prevent several patch 
 versions in case I still leave some mis-indented code or misplaced comments 
 in the options constants).

No problem, I will do that then. Anyway I would not ask you to
resubmit for such minor things, but some people prefer to have
the exact same version on their side as the one being committed,
reason why I asked.

Thanks!
Willy




Re: Small patch for the appsession feature

2009-11-17 Thread Aleksandar Lazic

On Mon 16.11.2009 22:52, Cyril Bonté wrote:

Le lundi 16 novembre 2009 13:41:14, Willy Tarreau a écrit :

On Mon, Nov 16, 2009 at 01:04:17PM +0100, Aleksandar Lazic wrote:


[snipp]


 For example:
 
 if in the url the delimiter changes from ';' to ',' then you should

 set the following
 
 appsession-option delimiterstart ,
 
 and the end point is not '=' it is ';' then you should set the

 following
 
 appsession-option delimiterstop ;

(...)


Yes, this was exactly why I first thought of adding options on several
lines. Having all the options on the same like can quickly make the
configuration difficult to read/debug.



Imagine a line like :



appsession PHPSESSID len 32 timeout 1h request-learn cookie-capture \
start-delimiters ? stop-delimiters 



I found it not really user friendly... (well, my idea to split the line
in several options looks not much better)


;-)


(...)
Also, if we have other special cases later, it might be easier to implement
some methods specific to some servers than to have the user explicitly
specify a number of complex arguments they won't always be able to figure
out.

For instance :   appsession-mode weblogic
(...)


I really like this idea. This would make the configuration simpler and
the algorithm to parse the URL can be easily optimized for each mode.


I agree.


Based on this idea, maybe we can stay with all the options on the same
line (looks like the server configuration part).



The most complex line would be :
appsession PHPSESSID len 32 timeout 1h mode query-string request-learn 
cookie-capture


Looks ok for me.


And for now, I see 2 modes :
- path-parameters (the default)
- query-string (the one that made me look at the code :-) )
(and if needed weblogic)


Looks ok for me.

What I'am not sure now is how we can prevent that notsuccessful searches
on long URLs have negative performance impact?

BR

Aleks



Re: Small patch for the appsession feature

2009-11-17 Thread Willy Tarreau
On Tue, Nov 17, 2009 at 11:25:56PM +0100, Aleksandar Lazic wrote:
 And for now, I see 2 modes :
 - path-parameters (the default)
 - query-string (the one that made me look at the code :-) )
 (and if needed weblogic)
 
 Looks ok for me.
 
 What I'am not sure now is how we can prevent that notsuccessful searches
 on long URLs have negative performance impact?

I think that if it causes some trouble, we can optionally limit the
position of the argument in the query-string. But right now I don't
think it is much of a problem, because :
   1) we don't check the query string when we find the cookie in the
  headers (at least I believe :-))
   2) when using the query-string mode, we're sure that almost all
  requests will have the string.
   3) we're just parsing a long string once per request, there are
  other processing occuring on headers, cookies, etc... which
  are at least as expensive and which do not noticeably hurt
  performance.

So all in all, it's not really a problem in my opinion.

Regards,
Willy




Re: Small patch for the appsession feature

2009-11-16 Thread Aleksandar Lazic

On Mon 16.11.2009 08:52, Willy Tarreau wrote:

Hi,

On Sun, Nov 15, 2009 at 10:28:21PM +0100, Aleksandar Lazic wrote:

Hi Cyril,

On Fre 13.11.2009 22:50, Cyril Bonté wrote:
Hello Willy,


[snipp]


First I added it as I did for request-learn but shouldn't it be
better to define these options more haproxy like (using option and
no option) ?

Which would give :
(no) option appsession-request-learn
(no) option appsession-capture-cookie

+1




[snipp]


What you can do however is to create a new prefix keyword like we have
for timeout or tcp-request and put the flags somewhere
else. appsession would have been fine but it's already used.  Maybe
you can use appcookie ? Something like this :

  appcookie request-learn
  appcookie capture


Both are ok for me.
My point of view is to be able to explain and handle it straight
forward.
For me both looks ok, mybe we use

appsession-option
or
app-op (= for abbreviation ;-) )

Due to the fact that not only the cookie is relevant for the application
session.

How about to add also a

app* delimiterstart (default ;)
app* delimiterstop  (defautl =)

in combination with the length you get the maximum flexibility for this
module ;-)

BR

Aleks



Re: Small patch for the appsession feature

2009-11-16 Thread Willy Tarreau
On Mon, Nov 16, 2009 at 10:08:56AM +0100, Aleksandar Lazic wrote:
 What you can do however is to create a new prefix keyword like we have
 for timeout or tcp-request and put the flags somewhere
 else. appsession would have been fine but it's already used.  Maybe
 you can use appcookie ? Something like this :
 
   appcookie request-learn
   appcookie capture
 
 Both are ok for me.
 My point of view is to be able to explain and handle it straight
 forward.
 For me both looks ok, mybe we use
 
 appsession-option
 or
 app-op (= for abbreviation ;-) )

 Due to the fact that not only the cookie is relevant for the application
 session.

OK but then I'd prefer the long form. We've had too many trouble with
abbreviations that nobody (including me) is able to spell correctly.

 How about to add also a
 
 app* delimiterstart (default ;)
 app* delimiterstop  (defautl =)

I'm sorry, I don't understand what you mean. Could you give an example
of configuration using this ?

Regards,
Willy




Re: Small patch for the appsession feature

2009-11-16 Thread Willy Tarreau
On Mon, Nov 16, 2009 at 01:04:17PM +0100, Aleksandar Lazic wrote:
(...)
 app* delimiterstart (default ;)
 app* delimiterstop  (defautl =)
 
 I'm sorry, I don't understand what you mean. Could you give an example
 of configuration using this ?
 
 For example:
 
 if in the url the delimiter changes from ';' to ',' then you should
 set the following
 
 appsession-option delimiterstart ,
 
 and the end point is not '=' it is ';' then you should set the
 following
 
 appsession-option delimiterstop ;

Ah OK I get it now, thanks.

 A better case is the BEA 'Cookie'. 
 
 JSESSIONID=GyvN6gTy7LKR2tyXDHvSzBhCfNH34YMG2f60xhVkXJNrrs5QvMjx!1267565914
 So the appsession-option delimiterstop is '!'.
 
 But maybe it can be handled with the length.

 After this talk I think the usecase is solved with the current available
 options.

Most likely.
Also, if we have other special cases later, it might be easier to implement
some methods specific to some servers than to have the user explicitly
specify a number of complex arguments they won't always be able to figure
out.

For instance :   appsession-mode weblogic

 Ok, lets keep it in mind maybe someone else find a better usecase so
 that we can decide it better if we should add this options or not.

agreed.

Regards,
Willy




Re: Small patch for the appsession feature

2009-11-16 Thread Cyril Bonté
Le lundi 16 novembre 2009 13:41:14, Willy Tarreau a écrit :
 On Mon, Nov 16, 2009 at 01:04:17PM +0100, Aleksandar Lazic wrote:
 (...)
  app* delimiterstart (default ;)
  app* delimiterstop  (defautl =)
  
  I'm sorry, I don't understand what you mean. Could you give an example
  of configuration using this ?
  
  For example:
  
  if in the url the delimiter changes from ';' to ',' then you should
  set the following
  
  appsession-option delimiterstart ,
  
  and the end point is not '=' it is ';' then you should set the
  following
  
  appsession-option delimiterstop ;
 (...)

Yes, this was exactly why I first thought of adding options on several lines. 
Having all the options on the same like can quickly make the configuration 
difficult to read/debug.
Imagine a line like :
appsession PHPSESSID len 32 timeout 1h request-learn cookie-capture 
start-delimiters ? stop-delimiters 
I found it not really user friendly... (well, my idea to split the line in 
several options looks not much better)

 (...)
 Also, if we have other special cases later, it might be easier to implement
 some methods specific to some servers than to have the user explicitly
 specify a number of complex arguments they won't always be able to figure
 out.
 
 For instance :   appsession-mode weblogic
 (...)

I really like this idea. This would make the configuration simpler and the 
algorithm to parse the URL can be easily optimized for each mode.

Based on this idea, maybe we can stay with all the options on the same line 
(looks like the server configuration part).
The most complex line would be :
appsession PHPSESSID len 32 timeout 1h mode query-string request-learn 
cookie-capture

And for now, I see 2 modes :
- path-parameters (the default)
- query-string (the one that made me look at the code :-) )
(and if needed weblogic)

-- 
Cyril Bonté



Re: Small patch for the appsession feature

2009-11-15 Thread Aleksandar Lazic

Hi Cyril,

On Fre 13.11.2009 22:50, Cyril Bonté wrote:

Hello Willy,
sorry, I didn't have time to work on the patch as I wanted.

Le jeudi 5 novembre 2009 06:19:41, Willy Tarreau a écrit :

 Sorry but I can't see in the haproxy sources how the cookie prefix can be 
used for appsession.
 capture cookie allows to find this cookie prefix but it seems there's no 
code to then use it in appsession. If it's the case, I can work on it these next days, if 
you want.

OK you're indeed right. So since this feature has never been
usable nor used, let's fix it as you initially proposed.


It works well but I have a question before considering the patch
finalized :)
As using the captured cookie may add an unwanted behaviour in some
existant configurations, I set it as an option.
First I added it as I did for request-learn but shouldn't it be
better to define these options more haproxy like (using option and
no option) ?

Which would give :
(no) option appsession-request-learn
(no) option appsession-capture-cookie


+1

Aleks



Re: Small patch for the appsession feature

2009-11-15 Thread Willy Tarreau
Hi,

On Sun, Nov 15, 2009 at 10:28:21PM +0100, Aleksandar Lazic wrote:
 Hi Cyril,
 
 On Fre 13.11.2009 22:50, Cyril Bonté wrote:
 Hello Willy,
 sorry, I didn't have time to work on the patch as I wanted.
 
 Le jeudi 5 novembre 2009 06:19:41, Willy Tarreau a écrit :
  Sorry but I can't see in the haproxy sources how the cookie prefix can 
 be used for appsession.
  capture cookie allows to find this cookie prefix but it seems there's 
 no code to then use it in appsession. If it's the case, I can work on it 
 these next days, if you want.
 
 OK you're indeed right. So since this feature has never been
 usable nor used, let's fix it as you initially proposed.
 
 It works well but I have a question before considering the patch
 finalized :)
 As using the captured cookie may add an unwanted behaviour in some
 existant configurations, I set it as an option.
 First I added it as I did for request-learn but shouldn't it be
 better to define these options more haproxy like (using option and
 no option) ?
 
 Which would give :
 (no) option appsession-request-learn
 (no) option appsession-capture-cookie
 
 +1

I'm not that much in favor of options for such uses, because
options are inherited from default sections and will cause
unexpected behaviours.

In my opinion, options should be used when they impact the
general behaviour and when it is desirable to inherit them
from default sections. Right now, appsessions are limited
to backends only and cannot be declared in defaults sections,
so I think it could become a bit awkward to have such a split
configuration.

What you can do however is to create a new prefix keyword like
we have for timeout or tcp-request and put the flags somewhere
else. appsession would have been fine but it's already used.
Maybe you can use appcookie ? Something like this :

   appcookie request-learn
   appcookie capture

Just an idea.

Regards,
Willy




Re: Small patch for the appsession feature

2009-11-14 Thread Cyril Bonté
Hello Willy,
sorry, I didn't have time to work on the patch as I wanted.

Le jeudi 5 novembre 2009 06:19:41, Willy Tarreau a écrit :
  Sorry but I can't see in the haproxy sources how the cookie prefix can be 
  used for appsession.
  capture cookie allows to find this cookie prefix but it seems there's no 
  code to then use it in appsession. If it's the case, I can work on it these 
  next days, if you want.
 
 OK you're indeed right. So since this feature has never been
 usable nor used, let's fix it as you initially proposed.

It works well but I have a question before considering the patch finalized :)
As using the captured cookie may add an unwanted behaviour in some existant 
configurations, I set it as an option.
First I added it as I did for request-learn but shouldn't it be better to 
define these options more haproxy like (using option and no option) ?

Which would give :
(no) option appsession-request-learn
(no) option appsession-capture-cookie

-- 
Cyril Bonté



Re: Small patch for the appsession feature

2009-11-02 Thread Willy Tarreau
Hi Cyril,

On Sun, Nov 01, 2009 at 12:19:05AM +0100, Cyril Bonté wrote:
 Hello Willy and Aleksandar,
 If you agree, I would like to apply this new patch to add some more integrity 
 checking on appsession.
 
 * the session value (provided by the URL or by the request/response cookie) 
 is now well delimited :
 currently, setting len 52 on a 32 chars value has a bad effect on load 
 balancing (for example, a part of the query string is considered as the 
 session id ; same thing happens with the cookie).
 = with the patch, len 52 is now a maximum length.

OK.

 * it adds a verification on the '=' char :
 currently (with appsession JSESSIONID for example), an URL like 
 http://haproxy/path;jsessionidfake=0123... matches the session id 
 ake=0123...
 = with the patch, jsessionidfake won't be recognized.

I think we must not do this, because one of the reasons for not
checking the equal sign was because of people who have to use IIS
with its ASPSESSION cookies. The end of the cookie name is part
of the value. So it's important that we can check cookie prefixes.

 Tell me if you're OK for the loop added in get_srv_from_appsession(). This 
 can be a little slower but I think this will help to eliminate some troubles 
 in production.

At first glance I'm not too much scared.
However, please avoid variable declaration in the middle of the
code such as below, this is not compatible with compilers before
gcc version 3.0.

 @@ -4080,7 +4080,11 @@
   /* first, let's see if the cookie is 
 our appcookie*/
  
   /* Cool... it's the right one */
 - manage_client_side_appsession(t, p3);
 + int value_len = p4 - p3;
 + if (value_len  t-be-appsession_len) {
 + value_len = 
 t-be-appsession_len;
 + }
 + manage_client_side_appsession(t, p3, 
 value_len);

Cheers,
Willy




Re: Small patch for the appsession feature

2009-11-02 Thread Cyril Bonté
Le lundi 2 novembre 2009 12:09:43, Willy Tarreau a écrit :
  * it adds a verification on the '=' char :
  currently (with appsession JSESSIONID for example), an URL like 
  http://haproxy/path;jsessionidfake=0123... matches the session id 
  ake=0123...
  = with the patch, jsessionidfake won't be recognized.
 
 I think we must not do this, because one of the reasons for not
 checking the equal sign was because of people who have to use IIS
 with its ASPSESSION cookies.

Interesting to know about this ASPSESSION cookies, I'm not an IIS user.

 The end of the cookie name is part of the value.

Ok, note that the only modification I made is for the url part, not for the 
cookie. So maybe this is not a problem.
After some searches, I've seen that IIS does'nt support natively cookieless 
persistance : it needs an ISAPI filter to do so (named Cookie munge).
I've read this filter sources and it looks like the URL part for 
ASPSESSIONID is -ASP= (without the left side value), which doesn't start 
with the semi-colon awaited by the appsession code.
Maybe I'm wrong, I'm really not an ASP developper and the information I found 
are maybe outdated (it concerned IIS 4.0/5.0).

 So it's important that we can check cookie prefixes.

Sorry but I can't see in the haproxy sources how the cookie prefix can be used 
for appsession.
capture cookie allows to find this cookie prefix but it seems there's no code 
to then use it in appsession. If it's the case, I can work on it these next 
days, if you want.

 However, please avoid variable declaration in the middle of the
 code such as below, this is not compatible with compilers before
 gcc version 3.0.
 
  @@ -4080,7 +4080,11 @@
  /* first, let's see if the cookie is 
  our appcookie*/
   
  /* Cool... it's the right one */
  -   manage_client_side_appsession(t, p3);
  +   int value_len = p4 - p3;
  +   if (value_len  t-be-appsession_len) {
  +   value_len = 
  t-be-appsession_len;
  +   }
  +   manage_client_side_appsession(t, p3, 
  value_len);

Oops, I missed it :)

-- 
Cyril Bonté



Re: Small patch for the appsession feature

2009-10-31 Thread Cyril Bonté
Hello Willy and Aleksandar,
If you agree, I would like to apply this new patch to add some more integrity 
checking on appsession.

* the session value (provided by the URL or by the request/response cookie) is 
now well delimited :
currently, setting len 52 on a 32 chars value has a bad effect on load 
balancing (for example, a part of the query string is considered as the session 
id ; same thing happens with the cookie).
= with the patch, len 52 is now a maximum length.

* it adds a verification on the '=' char :
currently (with appsession JSESSIONID for example), an URL like 
http://haproxy/path;jsessionidfake=0123... matches the session id 
ake=0123...
= with the patch, jsessionidfake won't be recognized.

Tell me if you're OK for the loop added in get_srv_from_appsession(). This can 
be a little slower but I think this will help to eliminate some troubles in 
production.

Note : the patch is based on haproxy 1.4 snapshot 20091030.

-- 
Cyril Bonté
diff -Naur a/include/proto/proto_http.h b/include/proto/proto_http.h
--- a/include/proto/proto_http.h	2009-10-29 12:00:11.0 +0100
+++ b/include/proto/proto_http.h	2009-10-31 21:53:06.0 +0100
@@ -75,7 +75,7 @@
 int apply_filter_to_req_line(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_request(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_response(struct session *t, struct buffer *rtr, struct hdr_exp *exp);
-void manage_client_side_appsession(struct session *t, const char *buf);
+void manage_client_side_appsession(struct session *t, const char *buf, int len);
 void manage_client_side_cookies(struct session *t, struct buffer *req);
 void manage_server_side_cookies(struct session *t, struct buffer *rtr);
 void check_response_for_cacheability(struct session *t, struct buffer *rtr);
diff -Naur a/src/proto_http.c b/src/proto_http.c
--- a/src/proto_http.c	2009-10-29 12:00:11.0 +0100
+++ b/src/proto_http.c	2009-10-31 23:40:39.0 +0100
@@ -2479,7 +2479,7 @@
 
 	/* It needs to look into the URI */
 	if (s-be-appsession_name) {
-		get_srv_from_appsession(s, req-data[msg-som], msg-sl.rq.l);
+		get_srv_from_appsession(s, req-data[msg-som + msg-sl.rq.u], msg-sl.rq.u_l);
 	}
 
 	/*
@@ -3778,7 +3778,7 @@
  * Try to retrieve the server associated to the appsession.
  * If the server is found, it's assigned to the session.
  */
-void manage_client_side_appsession(struct session *t, const char *buf) {
+void manage_client_side_appsession(struct session *t, const char *buf, int len) {
 	struct http_txn *txn = t-txn;
 	appsess *asession = NULL;
 	char *sessid_temp = NULL;
@@ -3796,8 +3796,8 @@
 			return;
 		}
 
-		memcpy(t-sessid, buf, t-be-appsession_len);
-		t-sessid[t-be-appsession_len] = 0;
+		memcpy(t-sessid, buf, len);
+		t-sessid[len] = 0;
 	}
 
 	if ((sessid_temp = pool_alloc2(apools.sessid)) == NULL) {
@@ -3806,8 +3806,8 @@
 		return;
 	}
 
-	memcpy(sessid_temp, buf, t-be-appsession_len);
-	sessid_temp[t-be-appsession_len] = 0;
+	memcpy(sessid_temp, buf, len);
+	sessid_temp[len] = 0;
 
 	asession = appsession_hash_lookup((t-be-htbl_proxy), sessid_temp);
 	/* free previously allocated memory */
@@ -4080,7 +4080,11 @@
 	/* first, let's see if the cookie is our appcookie*/
 
 	/* Cool... it's the right one */
-	manage_client_side_appsession(t, p3);
+	int value_len = p4 - p3;
+	if (value_len  t-be-appsession_len) {
+		value_len = t-be-appsession_len;
+	}
+	manage_client_side_appsession(t, p3, value_len);
 #if defined(DEBUG_HASH)
 	Alert(manage_client_side_cookies\n);
 	appsession_hash_dump((t-be-htbl_proxy));
@@ -4508,8 +4512,12 @@
 	send_log(t-be, LOG_ALERT, Not enough Memory process_srv():asession-sessid:malloc().\n);
 	return;
 }
-memcpy(t-sessid, p3, t-be-appsession_len);
-t-sessid[t-be-appsession_len] = 0;
+int value_len = p4 - p3;
+if (value_len  t-be-appsession_len) {
+	value_len = t-be-appsession_len;
+}
+memcpy(t-sessid, p3, value_len);
+t-sessid[value_len] = 0;
 			}/* end if ((t-proxy-appsession_name != NULL) ... */
 			break; /* we don't want to loop again since there cannot be another cookie on the same line */
 		} /* we're now at the end of the cookie value */
@@ -4653,25 +4661,35 @@
  */
 void get_srv_from_appsession(struct session *t, const char *begin, int len)
 {
-	char *request_line;
+	char *end_params, *first_param, *cur_param, *next_param;
 
 	if (t-be-appsession_name == NULL ||
 	(t-txn.meth != HTTP_METH_GET  t-txn.meth != HTTP_METH_POST) ||
-	(request_line = memchr(begin, ';', len)) == NULL ||
-	((1 + t-be-appsession_name_len + 1 + t-be-appsession_len)  (begin + len - request_line)))
+		((first_param = memchr(begin, ';', len)) == NULL))
 		return;
 
-	/* skip ';' */
-	request_line++;
-
-	/* look if we have a jsessionid */
-	if (strncasecmp(request_line, t-be-appsession_name, t-be-appsession_name_len) != 0)
-		return;
+	if ((end_params = 

Re: Small patch for the appsession feature

2009-10-31 Thread Cyril Bonté
Le dimanche 1 novembre 2009 00:19:05, Cyril Bonté a écrit :
 Hello Willy and Aleksandar,
 If you agree, I would like to apply this new patch to add some more integrity 
 checking on appsession.

Forget the previous patch file, this new one also prevents forcing the load 
balancing to one server when the url session value is empty
(http://haproxy/path;jsessionid=?querystring)

-- 
Cyril Bonté
diff -Naur a/include/proto/proto_http.h b/include/proto/proto_http.h
--- a/include/proto/proto_http.h	2009-10-29 12:00:11.0 +0100
+++ b/include/proto/proto_http.h	2009-10-31 21:53:06.0 +0100
@@ -75,7 +75,7 @@
 int apply_filter_to_req_line(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_request(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_response(struct session *t, struct buffer *rtr, struct hdr_exp *exp);
-void manage_client_side_appsession(struct session *t, const char *buf);
+void manage_client_side_appsession(struct session *t, const char *buf, int len);
 void manage_client_side_cookies(struct session *t, struct buffer *req);
 void manage_server_side_cookies(struct session *t, struct buffer *rtr);
 void check_response_for_cacheability(struct session *t, struct buffer *rtr);
diff -Naur a/src/proto_http.c b/src/proto_http.c
--- a/src/proto_http.c	2009-10-29 12:00:11.0 +0100
+++ b/src/proto_http.c	2009-11-01 01:28:03.0 +0100
@@ -2479,7 +2479,7 @@
 
 	/* It needs to look into the URI */
 	if (s-be-appsession_name) {
-		get_srv_from_appsession(s, req-data[msg-som], msg-sl.rq.l);
+		get_srv_from_appsession(s, req-data[msg-som + msg-sl.rq.u], msg-sl.rq.u_l);
 	}
 
 	/*
@@ -3778,7 +3778,7 @@
  * Try to retrieve the server associated to the appsession.
  * If the server is found, it's assigned to the session.
  */
-void manage_client_side_appsession(struct session *t, const char *buf) {
+void manage_client_side_appsession(struct session *t, const char *buf, int len) {
 	struct http_txn *txn = t-txn;
 	appsess *asession = NULL;
 	char *sessid_temp = NULL;
@@ -3796,8 +3796,8 @@
 			return;
 		}
 
-		memcpy(t-sessid, buf, t-be-appsession_len);
-		t-sessid[t-be-appsession_len] = 0;
+		memcpy(t-sessid, buf, len);
+		t-sessid[len] = 0;
 	}
 
 	if ((sessid_temp = pool_alloc2(apools.sessid)) == NULL) {
@@ -3806,8 +3806,8 @@
 		return;
 	}
 
-	memcpy(sessid_temp, buf, t-be-appsession_len);
-	sessid_temp[t-be-appsession_len] = 0;
+	memcpy(sessid_temp, buf, len);
+	sessid_temp[len] = 0;
 
 	asession = appsession_hash_lookup((t-be-htbl_proxy), sessid_temp);
 	/* free previously allocated memory */
@@ -4080,7 +4080,11 @@
 	/* first, let's see if the cookie is our appcookie*/
 
 	/* Cool... it's the right one */
-	manage_client_side_appsession(t, p3);
+	int value_len = p4 - p3;
+	if (value_len  t-be-appsession_len) {
+		value_len = t-be-appsession_len;
+	}
+	manage_client_side_appsession(t, p3, value_len);
 #if defined(DEBUG_HASH)
 	Alert(manage_client_side_cookies\n);
 	appsession_hash_dump((t-be-htbl_proxy));
@@ -4508,8 +4512,12 @@
 	send_log(t-be, LOG_ALERT, Not enough Memory process_srv():asession-sessid:malloc().\n);
 	return;
 }
-memcpy(t-sessid, p3, t-be-appsession_len);
-t-sessid[t-be-appsession_len] = 0;
+int value_len = p4 - p3;
+if (value_len  t-be-appsession_len) {
+	value_len = t-be-appsession_len;
+}
+memcpy(t-sessid, p3, value_len);
+t-sessid[value_len] = 0;
 			}/* end if ((t-proxy-appsession_name != NULL) ... */
 			break; /* we don't want to loop again since there cannot be another cookie on the same line */
 		} /* we're now at the end of the cookie value */
@@ -4653,25 +4661,37 @@
  */
 void get_srv_from_appsession(struct session *t, const char *begin, int len)
 {
-	char *request_line;
+	char *end_params, *first_param, *cur_param, *next_param;
 
 	if (t-be-appsession_name == NULL ||
 	(t-txn.meth != HTTP_METH_GET  t-txn.meth != HTTP_METH_POST) ||
-	(request_line = memchr(begin, ';', len)) == NULL ||
-	((1 + t-be-appsession_name_len + 1 + t-be-appsession_len)  (begin + len - request_line)))
+		((first_param = memchr(begin, ';', len)) == NULL))
 		return;
 
-	/* skip ';' */
-	request_line++;
-
-	/* look if we have a jsessionid */
-	if (strncasecmp(request_line, t-be-appsession_name, t-be-appsession_name_len) != 0)
-		return;
+	if ((end_params = memchr(first_param, '?', len - (begin - first_param))) == NULL) {
+		end_params = begin + len;
+	}
 
-	/* skip jsessionid= */
-	request_line += t-be-appsession_name_len + 1;
-	
-	manage_client_side_appsession(t, request_line);
+	cur_param = next_param = end_params;
+	while (cur_param  first_param) {
+		cur_param--;
+		if (cur_param[0] == ';') {
+			if ((cur_param + t-be-appsession_name_len + 1  next_param) 
+(cur_param[t-be-appsession_name_len + 1] == '=') 
+(strncasecmp(cur_param + 1, t-be-appsession_name, t-be-appsession_name_len) == 0)) {
+cur_param += 

Re: Small patch for the appsession feature

2009-10-18 Thread Willy Tarreau
On Thu, Oct 15, 2009 at 12:15:40AM +0200, Cyril Bonté wrote:
 OK, here comes the 2 patch files for haproxy-1.3.21 and haproxy-1.4-dev4.
 Please note 3 minor changes in those versions (proto_http.c / proto_http.h) :

Cyril, I have merged your two patches. I applied a very minor change,
I removed the appsession_option field and used the normal proxy options
instead ; it was not necessary to add 32 bits to the proxy while there
were plenty available for that purpose.

I have also added a few lines in the doc about the new keyword, and
committed with your first mail as a commit message.

Thanks !
Willy




Re: Small patch for the appsession feature

2009-10-18 Thread Cyril Bonté
Le dimanche 18 octobre 2009 12:05:55, Willy Tarreau a écrit :
 Cyril, I have merged your two patches.

Thanks ! I'm thinking of working on a second patch.
The documentation says that appsession looks for the session in the query 
string but this is not really the case. Currently, it parses the first path 
parameter (followed by ';').
I would like to add an option to decide if appsession parses path parameters 
(not only the first one) or the query string parameters so it can work for URLs 
like http://website/path?PHPSESSID=; for example.

 I applied a very minor change,
 I removed the appsession_option field and used the normal proxy options
 instead ; it was not necessary to add 32 bits to the proxy while there
 were plenty available for that purpose.

You're right, this was not useful, I didn't want to conflict with some 
potential other patches.

 I have also added a few lines in the doc about the new keyword

Yes, the most important part of the patch was missing :)

-- 
Cyril Bonté



Re: Small patch for the appsession feature

2009-10-14 Thread Willy Tarreau
On Tue, Oct 13, 2009 at 10:41:59PM +0200, Aleksandar Lazic wrote:
 On Die 13.10.2009 21:34, Cyril Bonté wrote:
 Le lundi 12 octobre 2009 23:17:43, Aleksandar Lazic a écrit :
 
 Yes, you're right, I missed it after several tests on different
 snapshots.
 Here comes a second patch to reintroduce these debug lines :
 
 Thanks.

OK Aleks, so is Cyril's patch OK for you and should I merge it now ?
Also, do you guys think it should it go to 1.3 too (balance affected
users vs risk of regression) ?

Thanks,
Willy




Re: Small patch for the appsession feature

2009-10-14 Thread Aleksandar Lazic

On Mit 14.10.2009 21:40, Willy Tarreau wrote:

On Tue, Oct 13, 2009 at 10:41:59PM +0200, Aleksandar Lazic wrote:

On Die 13.10.2009 21:34, Cyril Bonté wrote:
Le lundi 12 octobre 2009 23:17:43, Aleksandar Lazic a écrit :

Yes, you're right, I missed it after several tests on different
snapshots.
Here comes a second patch to reintroduce these debug lines :

Thanks.


OK Aleks, so is Cyril's patch OK for you and should I merge it now ?


Yes please.


Also, do you guys think it should it go to 1.3 too (balance affected
users vs risk of regression) ?


I think yes, if it is not too difficult.

BR

Aleks



Re: Small patch for the appsession feature

2009-10-14 Thread Willy Tarreau
On Wed, Oct 14, 2009 at 10:20:04PM +0200, Aleksandar Lazic wrote:
 On Mit 14.10.2009 21:40, Willy Tarreau wrote:
 On Tue, Oct 13, 2009 at 10:41:59PM +0200, Aleksandar Lazic wrote:
 On Die 13.10.2009 21:34, Cyril Bonté wrote:
 Le lundi 12 octobre 2009 23:17:43, Aleksandar Lazic a écrit :
 
 Yes, you're right, I missed it after several tests on different
 snapshots.
 Here comes a second patch to reintroduce these debug lines :
 
 Thanks.
 
 OK Aleks, so is Cyril's patch OK for you and should I merge it now ?
 
 Yes please.
 
 Also, do you guys think it should it go to 1.3 too (balance affected
 users vs risk of regression) ?
 
 I think yes, if it is not too difficult.

OK thanks for your review and comments Aleks!

Cyril, care to send an updated patch with the latest changes ?
Do it for either 1.3 or 1.4, I'll do the back/forward port. If you
prefer to proceed on both, of course feel free to do so :-)

Willy




Re: Small patch for the appsession feature

2009-10-14 Thread Cyril Bonté
Le mercredi 14 octobre 2009 22:23:54, Willy Tarreau a écrit :
 Cyril, care to send an updated patch with the latest changes ?

OK !

 Do it for either 1.3 or 1.4, I'll do the back/forward port. If you
 prefer to proceed on both, of course feel free to do so :-)

I can do both, this will let you work on something else ;)

-- 
Cyril Bonté



Re: Small patch for the appsession feature

2009-10-14 Thread Willy Tarreau
On Wed, Oct 14, 2009 at 10:36:49PM +0200, Cyril Bonté wrote:
 Le mercredi 14 octobre 2009 22:23:54, Willy Tarreau a écrit :
  Cyril, care to send an updated patch with the latest changes ?
 
 OK !
 
  Do it for either 1.3 or 1.4, I'll do the back/forward port. If you
  prefer to proceed on both, of course feel free to do so :-)
 
 I can do both, this will let you work on something else ;)

Nice, I appreciate it.

thanks,
Willy




Re: Small patch for the appsession feature

2009-10-14 Thread Cyril Bonté
Le mercredi 14 octobre 2009 22:39:39, Willy Tarreau a écrit :
   Do it for either 1.3 or 1.4, I'll do the back/forward port. If you
   prefer to proceed on both, of course feel free to do so :-)
  
  I can do both, this will let you work on something else ;)
 
 Nice, I appreciate it.

OK, here comes the 2 patch files for haproxy-1.3.21 and haproxy-1.4-dev4.
Please note 3 minor changes in those versions (proto_http.c / proto_http.h) :

1. void manage_client_side_appsession(struct session *t, char *buf);
becomes
void manage_client_side_appsession(struct session *t, const char *buf);

2. An unused variable in get_srv_from_appsession(...) has been removed.

3. After some more tests, I've applied a fix on request_count (used for debug) 
to have nearly the same behaviour in normal mode and request-learn mode.

-- 
Cyril Bonté
diff -Naur haproxy-1.4-dev4/include/proto/proto_http.h haproxy-1.4-dev4-appsession-final/include/proto/proto_http.h
--- haproxy-1.4-dev4/include/proto/proto_http.h	2009-10-12 06:40:53.0 +0200
+++ haproxy-1.4-dev4-appsession-final/include/proto/proto_http.h	2009-10-14 22:55:55.0 +0200
@@ -74,6 +74,7 @@
 int apply_filter_to_req_line(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_request(struct session *t, struct buffer *req, struct hdr_exp *exp);
 int apply_filters_to_response(struct session *t, struct buffer *rtr, struct hdr_exp *exp);
+void manage_client_side_appsession(struct session *t, const char *buf);
 void manage_client_side_cookies(struct session *t, struct buffer *req);
 void manage_server_side_cookies(struct session *t, struct buffer *rtr);
 void check_response_for_cacheability(struct session *t, struct buffer *rtr);
diff -Naur haproxy-1.4-dev4/include/types/proxy.h haproxy-1.4-dev4-appsession-final/include/types/proxy.h
--- haproxy-1.4-dev4/include/types/proxy.h	2009-10-12 06:40:53.0 +0200
+++ haproxy-1.4-dev4-appsession-final/include/types/proxy.h	2009-10-12 21:24:42.0 +0200
@@ -124,6 +124,9 @@
 #define PR_O2_INDEPSTR	0x1000	/* independant streams, don't update rex on write */
 #define PR_O2_SOCKSTAT	0x2000	/* collect  provide separate statistics for sockets */
 
+/* bits for proxy-appsession_options */
+#define PR_O_AS_REQL	0x0001  /* learn the session id from the request */
+
 struct error_snapshot {
 	struct timeval when;		/* date of this event, (tv_sec == 0) means never */
 	unsigned int len;		/* original length of the last invalid request/response */
@@ -177,6 +180,7 @@
 	char *appsession_name;			/* name of the cookie to look for */
 	int  appsession_name_len;		/* strlen(appsession_name), computed only once */
 	int  appsession_len;			/* length of the appsession cookie value to be used */
+	int  appsession_options;		/* options for appsession */
 	struct appsession_hash htbl_proxy;	/* Per Proxy hashtable */
 	char *capture_name;			/* beginning of the name of the cookie to capture */
 	int  capture_namelen;			/* length of the cookie name to match */
diff -Naur haproxy-1.4-dev4/include/types/session.h haproxy-1.4-dev4-appsession-final/include/types/session.h
--- haproxy-1.4-dev4/include/types/session.h	2009-10-12 06:40:53.0 +0200
+++ haproxy-1.4-dev4-appsession-final/include/types/session.h	2009-10-12 21:25:15.0 +0200
@@ -162,6 +162,7 @@
 	int conn_retries;			/* number of connect retries left */
 	int flags;/* some flags describing the session */
 	unsigned term_trace;			/* term trace: 4*8 bits indicating which part of the code closed */
+	char *sessid;			/* the session id, if found in the request or in the response */
 	struct buffer *req;			/* request buffer */
 	struct buffer *rep;			/* response buffer */
 	struct stream_interface si[2];  /* client and server stream interfaces */
diff -Naur haproxy-1.4-dev4/src/cfgparse.c haproxy-1.4-dev4-appsession-final/src/cfgparse.c
--- haproxy-1.4-dev4/src/cfgparse.c	2009-10-12 06:40:53.0 +0200
+++ haproxy-1.4-dev4-appsession-final/src/cfgparse.c	2009-10-12 21:26:10.0 +0200
@@ -1517,6 +1517,7 @@
 		}
 	}
 	else if (!strcmp(args[0], appsession)) {  /* cookie name */
+		int cur_arg;
 
 		if (warnifnotcap(curproxy, PR_CAP_BE, file, linenum, args[0], NULL))
 			err_code |= ERR_WARN;
@@ -1546,6 +1547,14 @@
 			err_code |= ERR_ALERT | ERR_ABORT;
 			goto out;
 		}
+
+		cur_arg = 6;
+		while (*(args[cur_arg])) {
+			if (!strcmp(args[cur_arg], request-learn)) {
+curproxy-appsession_options |= PR_O_AS_REQL;
+			}
+			cur_arg++;
+		}
 	} /* Url App Session */
 	else if (!strcmp(args[0], capture)) {
 		if (warnifnotcap(curproxy, PR_CAP_FE, file, linenum, args[0], NULL))
diff -Naur haproxy-1.4-dev4/src/client.c haproxy-1.4-dev4-appsession-final/src/client.c
--- haproxy-1.4-dev4/src/client.c	2009-10-12 06:40:53.0 +0200
+++ haproxy-1.4-dev4-appsession-final/src/client.c	2009-10-12 21:26:38.0 +0200
@@ -184,7 +184,7 @@
 		 */
 		s-be = s-fe = p;
 
-		s-req = s-rep = NULL; /* will be allocated later */
+		

Re: Small patch for the appsession feature

2009-10-13 Thread Cyril Bonté
Le lundi 12 octobre 2009 23:17:43, Aleksandar Lazic a écrit :
 Hi Cyril,
 
 good catch.
 
 +}
 +
 +/*
 -#if defined(DEBUG_HASH)
 -Alert(manage_client_side_cookies\n);
 -
 appsession_hash_dump((t-be-htbl_proxy));
 -#endif
 
 After a quick look I think it would be nice to dump  the sessions also
 in the client.

Yes, you're right, I missed it after several tests on different snapshots.
Here comes a second patch to reintroduce these debug lines :

diff -Naur haproxy-1.4-dev4-appsession/src/proto_http.c 
haproxy-1.4-dev4-appsession2/src/proto_http.c
--- haproxy-1.4-dev4-appsession/src/proto_http.c2009-10-12 
21:56:38.0 +0200
+++ haproxy-1.4-dev4-appsession2/src/proto_http.c   2009-10-13 
21:26:19.0 +0200
@@ -3862,6 +3862,10 @@
 
/* Cool... it's the right one */
manage_client_side_appsession(t, p3);
+#if defined(DEBUG_HASH)
+   Alert(manage_client_side_cookies\n);
+   
appsession_hash_dump((t-be-htbl_proxy));
+#endif
}/* end if ((t-proxy-appsession_name != NULL) 
... */
}
 
@@ -4450,6 +4454,10 @@
request_line += t-be-appsession_name_len + 1;

manage_client_side_appsession(t, request_line);
+#if defined(DEBUG_HASH)
+   Alert(get_srv_from_appsession\n);
+   appsession_hash_dump((t-be-htbl_proxy));
+#endif
 }

--
Cyril Bonté



Re: Small patch for the appsession feature

2009-10-13 Thread Aleksandar Lazic

On Die 13.10.2009 21:34, Cyril Bonté wrote:

Le lundi 12 octobre 2009 23:17:43, Aleksandar Lazic a écrit :

Yes, you're right, I missed it after several tests on different
snapshots.
Here comes a second patch to reintroduce these debug lines :


Thanks.