tempesta icon indicating copy to clipboard operation
tempesta copied to clipboard

fix(1902): correct the skb len

Open kingluo opened this issue 1 year ago • 3 comments

Fix #1902

When we contruct frames for HTTP/2 response and reuse the same skbs from the backend (HTTP/1), we forget to adjust the skb size according to the trailer headers (if exist) size, then it comes with extra partial HTTP/1 response to the client. Then, the client will send a GOAWAY to temepesta due to wrong frame size, and tempesta sends back the TCP RESET.

kingluo avatar Apr 05 '24 10:04 kingluo

FYI, test H2 trailer headers in go:

// trailers.go
package main

import (
        "io"
        "log"
        "net/http"
)

func main() {
        mux := http.NewServeMux()
        mux.HandleFunc("/trailers", func(w http.ResponseWriter, req *http.Request) {
                w.Header().Set("Trailer", "foobar")
                w.Header().Set("Content-Type", "text/html")
                w.Header().Set("Server", "Deproxy Server")
                w.WriteHeader(http.StatusOK)
                io.WriteString(w, "GHIJKLMNOPQRSTUV")
                w.Header()["Trailer:foobar"] = []string{"foobar"}
        })
        log.Fatal(http.ListenAndServe(":8080", mux))
}
# run demo backend
go run trailers.go

# check trailer headers
curl --raw -k --http2 -v \
    --resolve "server.test.com:443:127.0.0.1" https://server.test.com/trailers
< HTTP/2 200
< content-type: text/html
< trailer: foobar
< date: Wed, 10 Apr 2024 10:12:52 GMT
< via: 2.0 tempesta_fw (Tempesta FW 0.8.0)
< content-length: 20
< server: Tempesta FW/0.8.0
<
* TLSv1.2 (IN), TLS header, Supplemental data (23):
< foobar: foobar
* Connection #0 to host server.test.com left intact
GHIJKLMNOPQRSTUVWXYZ

kingluo avatar Apr 10 '24 10:04 kingluo

@kingluo pelase don't forget to squash at least several commits (like printk removal) before the merge

krizhanovsky avatar Apr 26 '24 14:04 krizhanovsky

then we also must remove trailer: X-Token or we confuse a client that we have a trailer, but we actually don't. In this context the PR doesn't solve this since it doesn't remove the header.

Yes, I'll correct it.

kingluo avatar May 01 '24 16:05 kingluo