change prefetch buffer size from 1024 -> 2048 bytes #248
Conversation
elee1766
changed the title
There was a problem hiding this comment.
This fixes a weird case with Hybrid Kyber KEM where the ClientHello is 1.7 KB, and the 1.0 KB buffer size was apparently causing the TLS matcher to not match, and thus causing the TLS connection to fail.
Thanks for the excellent debugging work, @elee1766 !
|
This does not seam like the correct fix for failing tls matching. Matching should load more bytes if necessary. |
|
I've tried it out and built it with the hash before afa78d7: And then also with your branch: None of these two versions fixed the behavior I had with caddy l4 in freebsd. Though strangely even just 1 Millisecond sleep time exactly in this spot fixes it for me.
Lines 140 to 141 in ea27408 So, my issues might be unrelated and of a different nature. The gpt solution might have only given it some arbitrary delay too in that spot due to more processing. I don't know the implications, but if adding 1 or 2ms there fixes it for most of the userbase of the plugin I downstream then I might just patch it in myself, since it seems kinda specific to one operating system. |
yes, I think prefetching beyond the initial buffer is broken somehow in this case, but I don't exactly know why. this solution isn't correct, I agree. if the other bug is fixed we can consider reverting (although as you state, majority of client hellos will probably be this large, at least for serving browser users) |
That sounds like a race condition to me... not necessarily a "data race" in the strict sense of what the race detetor would find, but more like a logical race. Either that or something funky in the kernel that depends on time or order of things that aren't concurrent. But if something concurrent/parallel is going on, this is where I've seen Sleeps make a spooky difference. But anyway, we should probably try to find what is going on and fix it better 🤔 |
I think there are two different issues at play here. First the FreeBSD one. config.json{
"admin": {
"disabled": true
},
"logging": {
"logs": {
"default": {"level":"DEBUG", "encoder": {"format":"console"}}
}
},
"apps": {
"tls": {
"certificates": {
"automate": ["localhost"]
},
"automation": {
"policies": [{
"subjects": ["localhost"],
"issuers": [{
"module": "internal"
}]
}]
}
},
"layer4": {
"servers": {
"https": {
"listen": ["0.0.0.0:10443"],
"routes": [
{
"match": [
{"tls": {"sni": ["localhost"]}}
],
"handle": [
{"handler": "tls"}
]
},
{
"handle": [
{
"handler": "proxy",
"upstreams": [{"dial": ["127.0.0.1:10080"]}]
}
]
}
]
}
}
},
"http": {
"servers": {
"backend": {
"protocols": ["h1","h2","h2c"],
"listen": ["127.0.0.1:10080"],
"routes": [
{
"handle": [{
"handler": "static_response",
"status_code": "200",
"body": "Hello World\n",
"headers": {
"Content-Type": ["text/plain"]
}
}]
}
]
}
}
}
}
}With Firefox it works. It also works with Chrome on the lazy-prefetch branch. For the FreeBSD issue ignoring 0 length |
|
It also occurred to me that the config I used above is inherently unreliable, because it uses a route without matcher as second route. That means once the first route fails layer4 will always choose the second one regardless of the first route indicating it needs more data. It is a config that should not really be used. In general one should avoid routes without matchers if it's not the only route. Eagerly loading all data the client sent in |
|
@ydylla I've built with 4f012d4 and removed the empty route and my config started working without the sleep in Chrome and Edge. I have added the empty route because it was suggested here: Check the I want to know if thats still required or why it was suggested. Since I maintain a template and I have hardcoded it there due to this example. If I can remove it, I want to know if theres possible side effects if possible. |
The empty route that is suggested there should indeed not be necessary anymore. It was a workaround for #208 and fixed with afa78d7. I can not guarantee you there are no other side effects. The fallback implemented with afa78d7 has indeed other behavior. It is only called if all matchers came to a final conclusion. |
Which is apparently also not necessarily true. According to the docs the tls wrapper may be or is a noop:
So using a tls machter may interfere with http -> https redirects. I have never used listener wrappers myself 🤷🏻♂️ |
|
@ydylla Its a kinda specific but widely used usecase popularized by HA Proxy (I think). People want to route some domains on their precious IPv4:443 port (since they dont want or can not use the benefits of almost unlimited IPv6 sockets) without TLS termination, and some of them with TLS termination and reverse proxied. They also want to multiplex their SSH and RDP into 443 while still using the HTTP reverse proxy. Thats why the listener wrapper seems to be quite important here. But I could be wrong, actually never tried setting it up differently since I interpreted the configuration examples this way. If theres a logic error I'm quite interested to find it. |
|
There is no logic error. One can use the listener wrapper for this. My point was I am not the best person to ask for advice for it. I still use configs where layer4 controls the socket and only forward http to the caddy http app listening on a unix socket. Because I find it more logical this way and since my configs are older than the listener wrapper. |
No description provided.