relayd empty OPTIONS query

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

relayd empty OPTIONS query

Rivo Nurges-2
>Synopsis:      one side of the http relay gets changed to tcp relay if first query of the persistent connection is empty OPTIONS
 >Category:      system
 >Environment:
         System      : OpenBSD 6.5
         Details     : OpenBSD 6.5-current (GENERIC.MP) #11: Mon May  6 17:40:38 MDT 2019
                          [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP

         Architecture: OpenBSD.amd64
         Machine     : amd64
 >Description:
        When first query of persistent http connection is OPTIONS without body one direction of the session gets changed to tcp relay. The session
still works but eg appending request headers stops happening.

 >How-To-Repeat:
relayd.conf:
log state changes
log connection

table <test> { 4.5.6.7 }

http protocol test_http {
   match request header set "X-Forwarded-For" value "$REMOTE_ADDR"
   match request url log
}

relay test {
   listen on 1.2.3.4 port 80
   protocol test_http
   forward to <test> port 80
}

test program:
#!/usr/bin/env ruby

require 'net/http/persistent'

uri = URI.parse('http://someserver')
http = Net::HTTP::Persistent.new
req = Net::HTTP::Options.new(uri.path)
req.body = "asdjhakdjahsdkjsad" if ARGV[1]
http.request(uri, req) if ARGV[0]
req = Net::HTTP::Get.new(uri.path)
http.request(uri, req)
http.request(uri, req)

Just 2 GET queries.
$ ./emptyoptions.rb
relayd log: relay test, session 9 (1 active), 0, 1.2.3.4 -> 5.6.7.8:80, done, [User-Agent: Ruby] [someserver/] GET; [User-Agent: Ruby]
[someserver/] GET;

OPTIONS with some body as the first query.
$ ./emptyoptions.rb 1 1
relayd log: relay test, session 10 (1 active), 0, 1.2.3.4 -> 5.6.7.8:80, done, [User-Agent: Ruby] [someserver/] OPTIONS; [User-Agent: Ruby]
[someserver/] GET; [User-Agent: Ruby] [someserver/] GET;

OPTIONS without body as the first query.
$ ./emptyoptions.rb 1
relayd log: relay test, session 11 (1 active), 0, 1.2.3.4 -> 5.6.7.8:80, done, [User-Agent: Ruby] [someserver/] OPTIONS;;;

         <code/input/activities to reproduce the problem (multiple lines)>
 >Fix:
         I'm working on a fix but so far I haven't found one which doesn't break some regress test. Avoiding following code path in
relay_http.c solves the problem.
                         /* Single-pass HTTP body */
                         if (cre->toread < 0) {
                                 cre->toread = TOREAD_UNLIMITED;
                                 bev->readcb = relay_read;
                         }

Reply | Threaded
Open this post in threaded view
|

Re: relayd empty OPTIONS query

Reyk Floeter-2
On Sun, May 12, 2019 at 09:03:20PM +0000, Rivo Nurges wrote:

> >Synopsis:      one side of the http relay gets changed to tcp relay if first query of the persistent connection is empty OPTIONS
>  >Category:      system
>  >Environment:
>          System      : OpenBSD 6.5
>          Details     : OpenBSD 6.5-current (GENERIC.MP) #11: Mon May  6 17:40:38 MDT 2019
>                           [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>
>          Architecture: OpenBSD.amd64
>          Machine     : amd64
>  >Description:
> When first query of persistent http connection is OPTIONS without body one direction of the session gets changed to tcp relay. The session
> still works but eg appending request headers stops happening.
>
>  >How-To-Repeat:

Thanks for the report!  I was able to reproduce it.

In the general case, relayd should add a Connection: close header to
prevent a persistent connection after switching to TOREAD_UNLIMITED.
It will cause the server to close the connection after handling the
request (or, at least, kindly asking it to do so) and subsequent
requests will open a new connection.

The attached diff fixes the test case for me.

Ideally, methods like OPTIONS with optional body would have a
case-by-case handling to work in a persistent way, but this can be
done later.  We'd have to identify such methods and switch to
TOREAD_HTTP_HEADER instead of TOREAD_UNLIMITED, but only if we're sure
that it will not include a body without Content-Length.  [...] closing
the connection after one request is a safer fix for now.

OK?

Reyk

Index: usr.sbin/relayd/relay_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
retrieving revision 1.75
diff -u -p -u -p -r1.75 relay_http.c
--- usr.sbin/relayd/relay_http.c 13 May 2019 09:54:07 -0000 1.75
+++ usr.sbin/relayd/relay_http.c 13 May 2019 10:17:44 -0000
@@ -522,6 +522,15 @@ relay_read_http(struct bufferevent *bev,
  bev->readcb = relay_read_httpchunks;
  }
 
+ /*
+ * Ask the server to close the connection after this request
+ * since we don't read any further request headers.
+ */
+ if (cre->toread == TOREAD_UNLIMITED)
+ if (kv_add(&desc->http_headers, "Connection",
+    "close", 0) == NULL)
+ goto fail;
+
  if (cre->dir == RELAY_DIR_REQUEST) {
  if (relay_writerequest_http(cre->dst, cre) == -1)
     goto fail;

Reply | Threaded
Open this post in threaded view
|

Re: relayd empty OPTIONS query

Rivo Nurges-2
Hi!

It fixes my simplified test case and the real application the test case was modeled after. Session gets closed after first OPTIONS and
subsequent requests go into next persistent session.

I can try to test different OPTIONS possibilities(with and without payload) and move OPTIONS handling out of the common path.

Rivo

On 2019-05-13 13:32, Reyk Floeter wrote:

> On Sun, May 12, 2019 at 09:03:20PM +0000, Rivo Nurges wrote:
>>> Synopsis:      one side of the http relay gets changed to tcp relay if first query of the persistent connection is empty OPTIONS
>>   >Category:      system
>>   >Environment:
>>           System      : OpenBSD 6.5
>>           Details     : OpenBSD 6.5-current (GENERIC.MP) #11: Mon May  6 17:40:38 MDT 2019
>>                            [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>>
>>           Architecture: OpenBSD.amd64
>>           Machine     : amd64
>>   >Description:
>> When first query of persistent http connection is OPTIONS without body one direction of the session gets changed to tcp relay. The session
>> still works but eg appending request headers stops happening.
>>
>>   >How-To-Repeat:
>
> Thanks for the report!  I was able to reproduce it.
>
> In the general case, relayd should add a Connection: close header to
> prevent a persistent connection after switching to TOREAD_UNLIMITED.
> It will cause the server to close the connection after handling the
> request (or, at least, kindly asking it to do so) and subsequent
> requests will open a new connection.
>
> The attached diff fixes the test case for me.
>
> Ideally, methods like OPTIONS with optional body would have a
> case-by-case handling to work in a persistent way, but this can be
> done later.  We'd have to identify such methods and switch to
> TOREAD_HTTP_HEADER instead of TOREAD_UNLIMITED, but only if we're sure
> that it will not include a body without Content-Length.  [...] closing
> the connection after one request is a safer fix for now.
>
> OK?
>
> Reyk
>
> Index: usr.sbin/relayd/relay_http.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
> retrieving revision 1.75
> diff -u -p -u -p -r1.75 relay_http.c
> --- usr.sbin/relayd/relay_http.c 13 May 2019 09:54:07 -0000 1.75
> +++ usr.sbin/relayd/relay_http.c 13 May 2019 10:17:44 -0000
> @@ -522,6 +522,15 @@ relay_read_http(struct bufferevent *bev,
>   bev->readcb = relay_read_httpchunks;
>   }
>  
> + /*
> + * Ask the server to close the connection after this request
> + * since we don't read any further request headers.
> + */
> + if (cre->toread == TOREAD_UNLIMITED)
> + if (kv_add(&desc->http_headers, "Connection",
> +    "close", 0) == NULL)
> + goto fail;
> +
>   if (cre->dir == RELAY_DIR_REQUEST) {
>   if (relay_writerequest_http(cre->dst, cre) == -1)
>      goto fail;
>
>