|
14 | 14 | bytes and a complete terminal chunk. The client never sees the two ambiguous |
15 | 15 | lengths, so there is no framing disagreement to exploit. |
16 | 16 |
|
17 | | -Note on the inconsistent flag: nxt_http_proxy_content_length also sets |
18 | | -r->inconsistent, but once the EOF-framed body ends with a clean upstream |
19 | | -close, nxt_h1p_peer_closed recomputes the flag from the (already reset) |
20 | | -framing state, so a keep-alive-disabling close is not independently |
21 | | -observable here. Both tests therefore drive `Connection: close` and assert |
22 | | -the defense that matters on the wire: zero conflicting Content-Length headers |
23 | | -reach the client and the relayed body framing is unambiguous. |
| 17 | +Note on the inconsistent flag: nxt_http_proxy_content_length sets |
| 18 | +r->inconsistent so the downstream keepalive is disabled. This was previously |
| 19 | +clobbered -- nxt_h1p_peer_closed reassigned the flag from the (already reset) |
| 20 | +framing state on the clean upstream close, so the connection stayed |
| 21 | +keep-alive. That is fixed: peer_closed now only *sets* a separate r->truncated |
| 22 | +(which drives the terminal-chunk omission) and never clears r->inconsistent, |
| 23 | +so a complete-but-ambiguous body disables keepalive while keeping its terminal |
| 24 | +chunk. test_proxy_dup_cl_keepalive_disabled below asserts that directly. The |
| 25 | +first two tests drive `Connection: close` and assert the primary defense on |
| 26 | +the wire: zero conflicting Content-Length headers reach the client and the |
| 27 | +relayed body framing is unambiguous. |
24 | 28 |
|
25 | 29 | Driven by the `dup-cl` mode of the Rust mock upstream (test/fake_upstream/): |
26 | 30 | it sends `Content-Length: 20` then `Content-Length: 6`, followed by a 20-byte |
|
55 | 59 | # Reserved fake_upstream ports for these cases (see test/fake_upstream/README.md). |
56 | 60 | UPSTREAM_DUP_CL_PORT = 7984 |
57 | 61 | UPSTREAM_DUP_CL_RAW_PORT = 7983 |
| 62 | +UPSTREAM_DUP_CL_KA_PORT = 7979 |
58 | 63 |
|
59 | 64 | FAKE_UPSTREAM_BIN = '/usr/local/bin/fake_upstream' |
60 | 65 |
|
@@ -197,3 +202,75 @@ def test_proxy_dup_cl_raw(skip_alert): |
197 | 202 | finally: |
198 | 203 | proc.terminate() |
199 | 204 | proc.wait() |
| 205 | + |
| 206 | + |
| 207 | +@_skipif_no_fake_upstream |
| 208 | +def test_proxy_dup_cl_keepalive_disabled(skip_alert): |
| 209 | + # Regression for the inconsistent-flag clobber: a duplicate upstream |
| 210 | + # Content-Length must disable downstream keepalive even for a keep-alive |
| 211 | + # client, AND the re-framed body must keep its terminal chunk (a complete |
| 212 | + # body is not falsely truncated). Before the fix, nxt_h1p_peer_closed |
| 213 | + # reassigned r->inconsistent to 0 on the clean upstream close, so the |
| 214 | + # connection stayed keep-alive; a naive fix that reused that one flag would |
| 215 | + # instead drop the terminal chunk. Both properties are asserted here. |
| 216 | + skip_alert(r'upstream sent duplicate Content-Length') |
| 217 | + |
| 218 | + proc = _run(UPSTREAM_DUP_CL_KA_PORT, 'dup-cl') |
| 219 | + try: |
| 220 | + _conf_proxy(UPSTREAM_DUP_CL_KA_PORT) |
| 221 | + |
| 222 | + # Explicit keep-alive request: the default client sends Connection: |
| 223 | + # close, which would mask whether the server disables keepalive. |
| 224 | + sock = client.get( |
| 225 | + port=8080, |
| 226 | + headers={'Host': 'localhost', 'Connection': 'keep-alive'}, |
| 227 | + no_recv=True, |
| 228 | + ) |
| 229 | + sock.settimeout(10) |
| 230 | + |
| 231 | + data = b'' |
| 232 | + closed = False |
| 233 | + try: |
| 234 | + while True: |
| 235 | + part = sock.recv(4096) |
| 236 | + if not part: |
| 237 | + closed = True |
| 238 | + break |
| 239 | + data += part |
| 240 | + # Once the full response is in, only a short grace period is |
| 241 | + # needed to observe the server-initiated close; shorten the |
| 242 | + # timeout so a *failing* run (keepalive left enabled) does not |
| 243 | + # block for the full 10s. |
| 244 | + if b'0\r\n\r\n' in data: |
| 245 | + sock.settimeout(1) |
| 246 | + except socket.timeout: |
| 247 | + closed = False |
| 248 | + finally: |
| 249 | + sock.close() |
| 250 | + |
| 251 | + assert data[:12] == b'HTTP/1.1 200', f'status line: {data[:40]!r}' |
| 252 | + |
| 253 | + sep = data.index(b'\r\n\r\n') |
| 254 | + head = data[:sep].lower() |
| 255 | + body = data[sep + 4:] |
| 256 | + |
| 257 | + # Keepalive is disabled despite the client's keep-alive request: the |
| 258 | + # server closes the connection after the response. No explicit |
| 259 | + # "Connection: close" header is emitted -- nxt_h1p_request_header_send() |
| 260 | + # picks the Connection header from h1p->keepalive before |
| 261 | + # nxt_h1p_request_close() applies "keepalive &= !inconsistent" -- so the |
| 262 | + # socket close is the observable signal here. |
| 263 | + assert closed, 'server must close the connection after a dup-CL response' |
| 264 | + |
| 265 | + # The complete body keeps its terminal chunk: the fix decouples the |
| 266 | + # keepalive-disable (r->inconsistent) from truncation (r->truncated), |
| 267 | + # so framing stays unambiguous rather than being falsely cut. |
| 268 | + assert head.count(b'content-length') == 0, ( |
| 269 | + f'conflicting Content-Length must not reach the client: {data[:sep]!r}' |
| 270 | + ) |
| 271 | + assert b'transfer-encoding: chunked' in head, f'not re-framed: {head!r}' |
| 272 | + assert body.endswith(b'0\r\n\r\n'), f'terminal chunk missing: {body!r}' |
| 273 | + assert _dechunk(body) == BODY.encode(), f'relayed body mismatch: {body!r}' |
| 274 | + finally: |
| 275 | + proc.terminate() |
| 276 | + proc.wait() |
0 commit comments