Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

malformed non-200 responses when caddy accepts http/2 #6

Closed
panva opened this issue Sep 6, 2018 · 6 comments · Fixed by #9
Closed

malformed non-200 responses when caddy accepts http/2 #6

panva opened this issue Sep 6, 2018 · 6 comments · Fixed by #9

Comments

@panva
Copy link

panva commented Sep 6, 2018

with the following caddy (v0.11.0) config

https://foo.example.com {
  tls {
    // ...
  }

  awslambda / {
    aws_access [redacted]
    aws_secret [redacted]
    single lambdaFn
  }
}

and the following function (taken from the README with minor modification)

exports.handler = (evt, ctx, cb) => {
  var html, reply;
  html = '<html><head><title>Caddy Echo</title></head>' +
         '<body><h1>Request:</h1>' +
         '<pre>' + JSON.stringify(evt, null, 2) +
         '</pre></body></html>';
  reply = {
      'type': 'HTTPJSON-REP',
      'meta': {
          'status': evt.meta.path === '/error' ? 400 : 200,
          'headers': {
              'Content-Type': [ 'text/html' ]
          }
      },
      body: html
  };
  cb(null, reply);
}

When hitting a 200 path the response is OK, but when you hit /error and a 400 status code is returned (the same applies to any 4xx and a 500) then the response is appended with the status code e.g.400 Bad Request like so

❯ http -v https://foo.example.com/error
GET /error HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: foo.example.com
User-Agent: HTTPie/0.9.9



HTTP/1.1 400 Bad Request
Content-Length: 512
Content-Type: text/html
Date: Thu, 06 Sep 2018 08:33:31 GMT
Server: Caddy

<html><head><title>Caddy Echo</title></head><body><h1>Request:</h1><pre>{
  "type": "HTTPJSON-REQ",
  "meta": {
    "method": "GET",
    "path": "/error",
    "query": "",
    "host": "foo.example.com",
    "proto": "HTTP/1.1",
    "headers": {
      "accept": [
        "*/*"
      ],
      "accept-encoding": [
        "gzip, deflate"
      ],
      "connection": [
        "keep-alive"
      ],
      "user-agent": [
        "HTTPie/0.9.9"
      ]
    }
  },
  "body": ""
}</pre></body></html>400 Bad Request

This is only happening when caddy accepts http/2 (but the request itself can be http/1.1 as shown above), but when http/2 is completely disabled with the following everything works fine.

  tls {
    alpn http/1.1
    // ...
  }

This appending happens during caddy responding, it's not coming from the lambda response envelope.
@panva panva changed the title malformed responses when sending non-200 status responses when caddy accepts http/2 malformed non-200 responses when caddy accepts http/2 Sep 6, 2018
@panva panva mentioned this issue Sep 6, 2018
Closed
@coopernurse
Copy link
Owner

I just tried to repro this today but didn't have any luck. Could you clarify what the Caddyfile should look like? Here's what I tried:

Caddyfile

https://localhost:5000 {

    tls self_signed

    awslambda / {
        aws_access redacted
        aws_secret redacted
        aws_region us-west-2
        # body of lambda pasted from your report above
        single caddy-bug-6
    }
}

Then I ran:

$ curl --http2 -k -v https://localhost:5000/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 5000 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* ignoring certificate verify locations due to disabled peer verification
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-ECDSA-AES256-GCM-SHA384
* ALPN, server accepted to use h2
* Server certificate:
*  subject: O=Caddy Self-Signed
*  start date: Sep 10 20:53:26 2018 GMT
*  expire date: Sep 17 20:53:26 2018 GMT
*  issuer: O=Caddy Self-Signed
*  SSL certificate verify result: unable to get local issuer certificate (20), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x558bf87ba040)
> GET / HTTP/2
> Host: localhost:5000
> User-Agent: curl/7.59.0
> Accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200 
< content-type: text/html
< server: Caddy
< content-length: 377
< date: Mon, 10 Sep 2018 20:53:50 GMT
< 
<html><head><title>Caddy Echo</title></head><body><h1>Request:</h1><pre>{
  "type": "HTTPJSON-REQ",
  "meta": {
    "method": "GET",
    "path": "/",
    "query": "",
    "host": "localhost:5000",
    "proto": "HTTP/2.0",
    "headers": {
      "accept": [
        "*/*"
      ],
      "user-agent": [
        "curl/7.59.0"
      ]
    }
  },
  "body": ""
* Connection #0 to host localhost left intact
}</pre></body></html>

@coopernurse
Copy link
Owner

coopernurse commented Sep 10, 2018 •
edited
Loading

sorry never mind - I see the issue only happens with the /error path. I was able to repro that.

I'll see if I can figure out where this extra string is coming from.

@coopernurse
Copy link
Owner

Here's the line that causes that extraneous status string to be written to the response:

https://github.com/mholt/caddy/blob/master/caddyhttp/httpserver/server.go#L385

I'm not sure what the patch should be though.

@coopernurse coopernurse mentioned this issue Sep 11, 2018
Merged
@coopernurse
Copy link
Owner

Ok, this may do the trick: #9

@coopernurse
Copy link
Owner

@panva I've merged this fix, please give it a try by pulling/building from master

@panva
Copy link
Author

panva commented Sep 20, 2018

I can confirm this is fixed! cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@panva @coopernurse