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.

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

Add handler to just close a connection without responding #3871

Closed
segevfiner opened this issue Nov 16, 2020 · 14 comments · Fixed by #3983
Closed

Add handler to just close a connection without responding #3871

segevfiner opened this issue Nov 16, 2020 · 14 comments · Fixed by #3983
Labels
feature ⚙️ New feature or request help wanted 🆘 Extra attention is needed
Milestone

Comments

@segevfiner
Copy link

segevfiner commented Nov 16, 2020

Nginx supports return 444 to just close the connection without responding. This is often used in a default server so that Nginx doesn't answer any request with an invalid Host header. It would be nice to have something similar in Caddy (Doesn't have to use a magic status code of course).

P.S. In the Go net/http package, you panic(http.ErrAbortHandler) to abort with no response AFAIK (Needs to be checked). But anything calling recover along the way also needs to be aware of that.

@mholt
Copy link
Member

mholt commented Nov 24, 2020

P.S. In the Go net/http package, you panic(http.ErrAbortHandler) to abort with no response AFAIK (Needs to be checked).

I didn't know that! Worth a shot. Will check into it when I get a chance. (Or someone can beat me to it.)

@mholt mholt added feature ⚙️ New feature or request help wanted 🆘 Extra attention is needed labels Nov 24, 2020
@SvenDowideit
Copy link
Contributor

I like the same syntax that @mholt suggested at https://caddy.community/t/how-to-drop-connection/11167

respond {
    close
}

@mholt
Copy link
Member

mholt commented Jan 20, 2021

@SvenDowideit The only problem with that is I assumed you didn't mind writing an HTTP response. It's weird to me that a directive named respond would actually not respond.

Maybe a special case like respond close could just close the connection immediately without writing a response, but it kind of betrays its namesake. So I dunno.

@SvenDowideit
Copy link
Contributor

fair :) could promote close to some form of first class

ie just close if there's no previous matcher, close /deadend* to do so on a path - or something like that.

@SvenDowideit
Copy link
Contributor

its probably just as weird as when you have no matchers, the status is still 200

{
    admin 0.0.0.0:2019
    email [email protected]
    debug true
}

(dns_api_gandi) {
    tls {
        issuer acme {
            dns gandi {env.GANDIV5_API_KEY}
        }
    	issuer internal
    }
}

*.loc.alho.st loc.alho.st {
    import dns_api_gandi
}
dow184@township-sl:~/src/caddy$ curl -vk https://nomatch.loc.alho.st
*   Trying 127.0.0.1:443...
* TCP_NODELAY set
* Connected to nomatch.loc.alho.st (127.0.0.1) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: /etc/ssl/certs
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256
* ALPN, server accepted to use h2
* Server certificate:
*  subject: [NONE]
*  start date: Jan 19 22:20:29 2021 GMT
*  expire date: Jan 20 10:20:29 2021 GMT
*  issuer: CN=Caddy Local Authority - ECC Intermediate
*  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 0x56240fa5edf0)
> GET / HTTP/2
> Host: nomatch.loc.alho.st
> user-agent: curl/7.68.0
> accept: */*
> 
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200 
< server: Caddy
< content-length: 0
< date: Wed, 20 Jan 2021 01:31:57 GMT
< 
* Connection #0 to host nomatch.loc.alho.st left intact

@mholt
Copy link
Member

mholt commented Jan 20, 2021

That's normal, it means your web server is working.

I'm OK with adding a new close directive I suppose, but I'm trying to decide if we need a new handler module to implement it, or if we can just use the static_response module. On the one hand, it feels overkill to make a whole new handler module that's essentially one or two lines of code, and on the other hand, it doesn't seem quite right to put it in static_response since it's not actually responding.

But maybe in the JSON it's OK if we fudge a little, where "static_response" can still do its job by PREVENTING a response from being written. Hm.

@SvenDowideit
Copy link
Contributor

ah, yup - that quandry i grok - syntax is forever :)

@mholt mholt added this to the 2.x milestone Jan 20, 2021
@mholt
Copy link
Member

mholt commented Jan 20, 2021

Just curious... what is the actual need for this? Like why not just respond with a 400 status and then close? Why the need to be forceful/ungraceful about it?

@SvenDowideit
Copy link
Contributor

SvenDowideit commented Jan 21, 2021 via email

@mholt
Copy link
Member

mholt commented Jan 21, 2021

I see...

I'll be honest, I'm really struggling to make this work, at least, as far as curl is leading me to believe. The only way I can get it to NOT say "connection left intact" is if I write an HTTP response with Connection: close in the header. Obviously, this is not what you want.

I cannot get that line of output to go away with panic(http.ErrAbortHandler) or even when I hijack the connection and close it manually.

This is the best I've been able to do:

$ curl -v "http://localhost:8888/"
*   Trying 127.0.0.1:8888...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8888 (#0)
> GET / HTTP/1.1
> Host: localhost:8888
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Empty reply from server
* Connection #0 to host localhost left intact
curl: (52) Empty reply from server

At least there was an "empty" reply, but I assume * Connection #0 to host localhost left intact means that the connection is not closed...

Maybe I just don't understand curl. If someone has more time right now and wants to write a simple Go client to inspect the behavior it in more detail, that could help us know if it's a curl thing or a Go thing.

we really don't want every spelling mistake, or random string in a hostname to be considered a "this site exists, but you may have gotten some detail wrong"

Isn't that the case, though? Like, that is the whole point of 400 status.

@SvenDowideit
Copy link
Contributor

SvenDowideit commented Jan 21, 2021

https://www.reddit.com/r/nginx/comments/jv9z7b/issues_with_ip_not_returning_444_but_instead/

this is an artifact of the fact caddy is actually listening, so it can find out what host the request is intended for - so is the right result.

https://www.unixteacher.org/blog/blocking-access-by-user-agent-in-nginx/

@mholt
Copy link
Member

mholt commented Jan 21, 2021

Wouldn't ya know... turns out it was an unlikely bug in curl!

Daniel Stenberg has kindly identified and fixed it already:

It's a bug. When it detects "empty reply" it will wrongly also say "left intact" when not true. I'll fix it!

curl/curl#6503

With that cleared up, I will push this feature soon. Seems that the panic is all we need.

@SvenDowideit
Copy link
Contributor

SvenDowideit commented Jan 21, 2021

v interesting!

its fun to see what can happen when we ask questions - this has been a very long standing curl thing that too many of us assumed was "intended" - really excellent!

mholt added a commit that referenced this issue Jan 28, 2021
… (#3983)

* caddyhttp: Implement handler abort; new 'abort' directive (close #3871)

* Move abort directive ordering; clean up redirects

Seems logical for the end-all of handlers to go at the... end.

The Connection header no longer needs to be set there, since Close is
true, and the static_response handler now does that.
@p1tz
Copy link

p1tz commented Jan 28, 2021

thank you :) that was the last thing keeping us from moving off nginx 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request help wanted 🆘 Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants