]>
Commit | Line | Data |
---|---|---|
d1b7736a MT |
1 | ------------------------------------------------------------ |
2 | revno: 13225 | |
3 | revision-id: squid3@treenet.co.nz-20150709032133-qg1patn5zngt4o4h | |
4 | parent: squid3@treenet.co.nz-20150501100500-3utkhrao1yrd8ig6 | |
5 | author: Alex Rousskov <rousskov@measurement-factory.com> | |
6 | committer: Amos Jeffries <squid3@treenet.co.nz> | |
7 | branch nick: 3.4 | |
8 | timestamp: Wed 2015-07-08 20:21:33 -0700 | |
9 | message: | |
10 | Do not blindly forward cache peer CONNECT responses. | |
11 | ||
12 | Squid blindly forwards cache peer CONNECT responses to clients. This | |
13 | may break things if the peer responds with something like HTTP 403 | |
14 | (Forbidden) and keeps the connection with Squid open: | |
15 | - The client application issues a CONNECT request. | |
16 | - Squid forwards this request to a cache peer. | |
17 | - Cache peer correctly responds back with a "403 Forbidden". | |
18 | - Squid does not parse cache peer response and | |
19 | just forwards it as if it was a Squid response to the client. | |
20 | - The TCP connections are not closed. | |
21 | ||
22 | At this stage, Squid is unaware that the CONNECT request has failed. All | |
23 | subsequent requests on the user agent TCP connection are treated as | |
24 | tunnelled traffic. Squid is forwarding these requests to the peer on the | |
25 | TCP connection previously used for the 403-ed CONNECT request, without | |
26 | proper processing. The additional headers which should have been applied | |
27 | by Squid to these requests are not applied, and the requests are being | |
28 | forwarded to the cache peer even though the Squid configuration may | |
29 | state that these requests must go directly to the origin server. | |
30 | ||
31 | This fixes Squid to parse cache peer responses, and if an error response | |
32 | found, respond with "502 Bad Gateway" to the client and close the | |
33 | connections. | |
34 | ------------------------------------------------------------ | |
35 | # Bazaar merge directive format 2 (Bazaar 0.90) | |
36 | # revision_id: squid3@treenet.co.nz-20150709032133-qg1patn5zngt4o4h | |
37 | # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.4 | |
38 | # testament_sha1: 6cbce093f30c8a09173eb610eaa423c7c305ff23 | |
39 | # timestamp: 2015-07-09 03:40:35 +0000 | |
40 | # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.4 | |
41 | # base_revision_id: squid3@treenet.co.nz-20150501100500-\ | |
42 | # 3utkhrao1yrd8ig6 | |
43 | # | |
44 | # Begin patch | |
45 | === modified file 'src/tunnel.cc' | |
46 | --- src/tunnel.cc 2014-04-26 10:58:22 +0000 | |
47 | +++ src/tunnel.cc 2015-07-09 03:21:33 +0000 | |
48 | @@ -122,6 +122,10 @@ | |
49 | (request->flags.interceptTproxy || request->flags.intercepted)); | |
50 | } | |
51 | ||
52 | + /// Sends "502 Bad Gateway" error response to the client, | |
53 | + /// if it is waiting for Squid CONNECT response, closing connections. | |
54 | + void informUserOfPeerError(const char *errMsg); | |
55 | + | |
56 | class Connection | |
57 | { | |
58 | ||
59 | @@ -139,13 +143,14 @@ | |
60 | ||
61 | void error(int const xerrno); | |
62 | int debugLevelForError(int const xerrno) const; | |
63 | - /// handles a non-I/O error associated with this Connection | |
64 | - void logicError(const char *errMsg); | |
65 | void closeIfOpen(); | |
66 | void dataSent (size_t amount); | |
67 | + /// writes 'b' buffer, setting the 'writer' member to 'callback'. | |
68 | + void write(const char *b, int size, AsyncCall::Pointer &callback, FREE * free_func); | |
69 | int len; | |
70 | char *buf; | |
71 | int64_t *size_ptr; /* pointer to size in an ConnStateData for logging */ | |
72 | + AsyncCall::Pointer writer; ///< pending Comm::Write callback | |
73 | ||
74 | Comm::ConnectionPointer conn; ///< The currently connected connection. | |
75 | ||
76 | @@ -195,13 +200,14 @@ | |
77 | TunnelStateData *tunnelState = (TunnelStateData *)params.data; | |
78 | debugs(26, 3, HERE << tunnelState->server.conn); | |
79 | tunnelState->server.conn = NULL; | |
80 | + tunnelState->server.writer = NULL; | |
81 | ||
82 | if (tunnelState->noConnections()) { | |
83 | delete tunnelState; | |
84 | return; | |
85 | } | |
86 | ||
87 | - if (!tunnelState->server.len) { | |
88 | + if (!tunnelState->client.writer) { | |
89 | tunnelState->client.conn->close(); | |
90 | return; | |
91 | } | |
92 | @@ -213,13 +219,14 @@ | |
93 | TunnelStateData *tunnelState = (TunnelStateData *)params.data; | |
94 | debugs(26, 3, HERE << tunnelState->client.conn); | |
95 | tunnelState->client.conn = NULL; | |
96 | + tunnelState->client.writer = NULL; | |
97 | ||
98 | if (tunnelState->noConnections()) { | |
99 | delete tunnelState; | |
100 | return; | |
101 | } | |
102 | ||
103 | - if (!tunnelState->client.len) { | |
104 | + if (!tunnelState->server.writer) { | |
105 | tunnelState->server.conn->close(); | |
106 | return; | |
107 | } | |
108 | @@ -343,6 +350,23 @@ | |
109 | handleConnectResponse(len); | |
110 | } | |
111 | ||
112 | +void | |
113 | +TunnelStateData::informUserOfPeerError(const char *errMsg) | |
114 | +{ | |
115 | + server.len = 0; | |
116 | + if (!clientExpectsConnectResponse()) { | |
117 | + // closing the connection is the best we can do here | |
118 | + debugs(50, 3, server.conn << " closing on error: " << errMsg); | |
119 | + server.conn->close(); | |
120 | + return; | |
121 | + } | |
122 | + ErrorState *err = new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw()); | |
123 | + err->callback = tunnelErrorComplete; | |
124 | + err->callback_data = this; | |
125 | + *status_ptr = Http::scBadGateway; | |
126 | + errorSend(http->getConn()->clientConnection, err); | |
127 | +} | |
128 | + | |
129 | /* Read from client side and queue it for writing to the server */ | |
130 | void | |
131 | TunnelStateData::ReadConnectResponseDone(const Comm::ConnectionPointer &, char *buf, size_t len, comm_err_t errcode, int xerrno, void *data) | |
132 | @@ -374,7 +398,7 @@ | |
133 | const bool parsed = rep.parse(connectRespBuf, eof, &parseErr); | |
134 | if (!parsed) { | |
135 | if (parseErr > 0) { // unrecoverable parsing error | |
136 | - server.logicError("malformed CONNECT response from peer"); | |
137 | + informUserOfPeerError("malformed CONNECT response from peer"); | |
138 | return; | |
139 | } | |
140 | ||
141 | @@ -383,7 +407,7 @@ | |
142 | assert(!parseErr); | |
143 | ||
144 | if (!connectRespBuf->hasSpace()) { | |
145 | - server.logicError("huge CONNECT response from peer"); | |
146 | + informUserOfPeerError("huge CONNECT response from peer"); | |
147 | return; | |
148 | } | |
149 | ||
150 | @@ -397,7 +421,8 @@ | |
151 | ||
152 | // bail if we did not get an HTTP 200 (Connection Established) response | |
153 | if (rep.sline.status() != Http::scOkay) { | |
154 | - server.logicError("unsupported CONNECT response status code"); | |
155 | + // if we ever decide to reuse the peer connection, we must extract the error response first | |
156 | + informUserOfPeerError("unsupported CONNECT response status code"); | |
157 | return; | |
158 | } | |
159 | ||
160 | @@ -416,13 +441,6 @@ | |
161 | } | |
162 | ||
163 | void | |
164 | -TunnelStateData::Connection::logicError(const char *errMsg) | |
165 | -{ | |
166 | - debugs(50, 3, conn << " closing on error: " << errMsg); | |
167 | - conn->close(); | |
168 | -} | |
169 | - | |
170 | -void | |
171 | TunnelStateData::Connection::error(int const xerrno) | |
172 | { | |
173 | /* XXX fixme xstrerror and xerrno... */ | |
174 | @@ -517,7 +535,7 @@ | |
175 | debugs(26, 3, HERE << "Schedule Write"); | |
176 | AsyncCall::Pointer call = commCbCall(5,5, "TunnelBlindCopyWriteHandler", | |
177 | CommIoCbPtrFun(completion, this)); | |
178 | - Comm::Write(to.conn, from.buf, len, call, NULL); | |
179 | + to.write(from.buf, len, call, NULL); | |
180 | } | |
181 | ||
182 | /* Writes data from the client buffer to the server side */ | |
183 | @@ -526,6 +544,7 @@ | |
184 | { | |
185 | TunnelStateData *tunnelState = (TunnelStateData *)data; | |
186 | assert (cbdataReferenceValid (tunnelState)); | |
187 | + tunnelState->server.writer = NULL; | |
188 | ||
189 | tunnelState->writeServerDone(buf, len, flag, xerrno); | |
190 | } | |
191 | @@ -575,6 +594,7 @@ | |
192 | { | |
193 | TunnelStateData *tunnelState = (TunnelStateData *)data; | |
194 | assert (cbdataReferenceValid (tunnelState)); | |
195 | + tunnelState->client.writer = NULL; | |
196 | ||
197 | tunnelState->writeClientDone(buf, len, flag, xerrno); | |
198 | } | |
199 | @@ -592,7 +612,14 @@ | |
200 | } | |
201 | ||
202 | void | |
203 | -TunnelStateData::writeClientDone(char *buf, size_t len, comm_err_t flag, int xerrno) | |
204 | +TunnelStateData::Connection::write(const char *b, int size, AsyncCall::Pointer &callback, FREE * free_func) | |
205 | +{ | |
206 | + writer = callback; | |
207 | + Comm::Write(conn, b, size, callback, free_func); | |
208 | +} | |
209 | + | |
210 | +void | |
211 | +TunnelStateData::writeClientDone(char *, size_t len, comm_err_t flag, int xerrno) | |
212 | { | |
213 | debugs(26, 3, HERE << client.conn << ", " << len << " bytes written, flag=" << flag); | |
214 | ||
215 | @@ -712,6 +739,7 @@ | |
216 | { | |
217 | TunnelStateData *tunnelState = (TunnelStateData *)data; | |
218 | debugs(26, 3, HERE << conn << ", flag=" << flag); | |
219 | + tunnelState->client.writer = NULL; | |
220 | ||
221 | if (flag != COMM_OK) { | |
222 | *tunnelState->status_ptr = Http::scInternalServerError; | |
223 | @@ -728,6 +756,7 @@ | |
224 | { | |
225 | TunnelStateData *tunnelState = (TunnelStateData *)data; | |
226 | debugs(26, 3, conn << ", flag=" << flag); | |
227 | + tunnelState->server.writer = NULL; | |
228 | assert(tunnelState->waitingForConnectRequest()); | |
229 | ||
230 | if (flag != COMM_OK) { | |
231 | @@ -768,7 +797,7 @@ | |
232 | else { | |
233 | AsyncCall::Pointer call = commCbCall(5,5, "tunnelConnectedWriteDone", | |
234 | CommIoCbPtrFun(tunnelConnectedWriteDone, tunnelState)); | |
235 | - Comm::Write(tunnelState->client.conn, conn_established, strlen(conn_established), call, NULL); | |
236 | + tunnelState->client.write(conn_established, strlen(conn_established), call, NULL); | |
237 | } | |
238 | } | |
239 | ||
240 | @@ -955,29 +984,20 @@ | |
241 | debugs(11, 2, "Tunnel Server REQUEST: " << tunnelState->server.conn << ":\n----------\n" << | |
242 | Raw("tunnelRelayConnectRequest", mb.content(), mb.contentSize()) << "\n----------"); | |
243 | ||
244 | - if (tunnelState->clientExpectsConnectResponse()) { | |
245 | - // hack: blindly tunnel peer response (to our CONNECT request) to the client as ours. | |
246 | - AsyncCall::Pointer writeCall = commCbCall(5,5, "tunnelConnectedWriteDone", | |
247 | - CommIoCbPtrFun(tunnelConnectedWriteDone, tunnelState)); | |
248 | - Comm::Write(srv, &mb, writeCall); | |
249 | - } else { | |
250 | - // we have to eat the connect response from the peer (so that the client | |
251 | - // does not see it) and only then start shoveling data to the client | |
252 | - AsyncCall::Pointer writeCall = commCbCall(5,5, "tunnelConnectReqWriteDone", | |
253 | - CommIoCbPtrFun(tunnelConnectReqWriteDone, | |
254 | - tunnelState)); | |
255 | - Comm::Write(srv, &mb, writeCall); | |
256 | - tunnelState->connectReqWriting = true; | |
257 | - | |
258 | - tunnelState->connectRespBuf = new MemBuf; | |
259 | - // SQUID_TCP_SO_RCVBUF: we should not accumulate more than regular I/O buffer | |
260 | - // can hold since any CONNECT response leftovers have to fit into server.buf. | |
261 | - // 2*SQUID_TCP_SO_RCVBUF: HttpMsg::parse() zero-terminates, which uses space. | |
262 | - tunnelState->connectRespBuf->init(SQUID_TCP_SO_RCVBUF, 2*SQUID_TCP_SO_RCVBUF); | |
263 | - tunnelState->readConnectResponse(); | |
264 | - | |
265 | - assert(tunnelState->waitingForConnectExchange()); | |
266 | - } | |
267 | + AsyncCall::Pointer writeCall = commCbCall(5,5, "tunnelConnectReqWriteDone", | |
268 | + CommIoCbPtrFun(tunnelConnectReqWriteDone, tunnelState)); | |
269 | + | |
270 | + tunnelState->server.write(mb.buf, mb.size, writeCall, mb.freeFunc()); | |
271 | + tunnelState->connectReqWriting = true; | |
272 | + | |
273 | + tunnelState->connectRespBuf = new MemBuf; | |
274 | + // SQUID_TCP_SO_RCVBUF: we should not accumulate more than regular I/O buffer | |
275 | + // can hold since any CONNECT response leftovers have to fit into server.buf. | |
276 | + // 2*SQUID_TCP_SO_RCVBUF: HttpMsg::parse() zero-terminates, which uses space. | |
277 | + tunnelState->connectRespBuf->init(SQUID_TCP_SO_RCVBUF, 2*SQUID_TCP_SO_RCVBUF); | |
278 | + tunnelState->readConnectResponse(); | |
279 | + | |
280 | + assert(tunnelState->waitingForConnectExchange()); | |
281 | ||
282 | AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", | |
283 | CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); | |
284 |