Peering support for SslBump #380
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
... from tunnel.cc into Http::Tunneler (inspired by PeerConnector). The next step is to use the extracted Tunneler in FwdState. Passes basic tests but needs a lot of polishing.
Also, the flag should be computed in FwdState (when a peer is selected) rather than in (various places inside) http.cc.
Just like tunnel.cc, FwdState can now establish HTTP CONNECT tunnels through HTTP proxies. Unlike tunnel.cc, FwdState cannot yet forward virgin peer errors to clients (TODO). Both clients need polishing. HTTPS proxies are not yet supported primarily because Squid cannot do TLS-in-TLS with a single fde::ssl field (both SslBump and an HTTPS proxy connection would need one TLS connection field for themselves). We could support HTTPS proxies when not using SslBump, but such exceptional support would probably make the already mind boggling "do we need TLS here?" conditions even worse.
... because we do not have a ready-to-use mechanism to supply them to the PeerConnector and because TLS servers should not send them.
Investigate the FwdState::ConnectingTimeout method to avoid code duplication
It can be used to set read timeout based on objects lifetime. This function replaces the HttpTunneler::setReadTimeout and PeerConnector::setReadTimeout methods.
The connection object to be used with HttpTunneler is passed by the caller, it does not make sense to report it back to the caller.
TunnelStateData::tunnelEstablishmentDone No need to close the client connection. The tunnelServerClosed will close the client connection if required.
Remove the TunnelStateData::waitingForConnectRequest() method Remove the TunnelStateData::waitingForConnectResponse() method Remove the TunnelStateData::connectReqWriting member Remove the TunnelStateData::connectRespBuf member Make the waitingForConnectExchange() method a simple bool member and update it when the Tunneler starts and when the tunneler respond.
The calls.connector holds the AsyncCall callback passed to CommOpener object It is used to cancel the CommOpener when the FwdState is gone for a reason (eg because client closed the connection)
If the Http::Tunneler received a valid error response from peer forward it to the client instead of sending a squid generated error page. This patch add the ERR_RELAY_REMOTE err_type to describe ErrorState objects which are created over a peer HttpReply. Maybe the Http::Tunneler should wait to retrieve all of body of peer response. Currently waits and forwards only the HTTP headers.
…neler This is can be used to omit leading space if the label is not defined
The caller is responsible to set correct DelayID for the Http::Tunneler. Please see the comments in the new code. This patch also fixes the delay pools related code inside tunnel.cc, it was completely broken (it is broken for squid-5.x and squid-4.x releases). XXX: Probably we should take care of Delay pools inside Ssl::ServerBump where we are building a fake Store entry to store Bumping errors.
Similar to HttpStateData::processReplyHeader() but a liitle simpler
- Remove FwdState::calls::connector member - Remove the Comm::SetClientObjectReadTimeout and replace with Comm::MortalReadTimeout function with a commSetConnTimeout call - Restruct ErrorPage constructors - Other polishing fixes
Run formatter Other fixes
... and complications that make correct timeout canceling difficult. Passing invalid timeouts to others is essentially a bug, but lots of code does that, most bugs are usually harmless, and fixing all those bugs is quite difficult (because there is no API for the cleanup code to know whether the currently set timeout belongs to this job). Tunneler is not the best place to bring attention to this complex problem.
Also fixed ConnOpener() constructor API that was pretending to modify its second argument.
Also documented how to pick the right ErrorState ctor.
When the HttpTunneler receives a valid HTTP status line but the headers
are malformed:
- If the HTTP status code is "200 OK" return an squid error page
- If the Http status code is not "200 OK" return truncated the original peer
response.
…Entry ... and use storeEntry to get DelayId in FwdState, instead of recomputing one
- The request->flags.sslPeek flag cover all bumping modes except client-fiest
- The boolean needTlsToOrigin:
const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS;
is true on client-first IF not a peer is used.
- The request->flags.sslBumped is true on all bumping modes
- Inside Security::BlindPeerConnector requires some cases to separate cases
where the BlindPeerConnector is used to encrypt CachePeer connections OR
the connections to the origin server.
Also spellchecked and polished a few comments.
This is lost with the commit 3265fd7 where the ErrorPage constructors fixes. As a result squid sent back the status code read from server but reported "Internal Error: Missing Template ERR_RELAY_REMOTE" (produced while tried to build a new HttpReply object).
... which derived from CallDialer and Http::TunnelerAnswer, to avoid creating a dedicated class for each peer connector/tunneler. The classes FwdStatePeerAnswerDialer2 and TunnelStateData::MyAnswerDialer2 are not needed anymore and removed.
Jenkins build on centos-7 reported: "error: declaration of 'answer' shadows a member of 'this'"
yadij
requested changes
Mar 17, 2019
rousskov
added
the
S-waiting-for-author
Fix comments and debugs, remove empty lines, replace NULLs with nullptr, sort includes and other fixes
rousskov
added
the
S-waiting-for-reviewer
Make all HttpTunneler members private. Implement the HttpTunneler::setDelayId to set delayid parameters
This is the traditional squid behaviour.
Do not use pconnPool for Http requests bumped at step1 when a peer is used. Current implementation may cause squid to send HTTP requests to wrong HTTP servers. To support pconn pool for these cases we need to use as pconn keys a combination of HTTP destination (e.g., origin1:443 in our case), peer (e.g., peer1:3128 in our case), and probably some kind of "secured" or "needs TLS" setting.
Also fix build error caused because the Http::Tunneler::setDelayId implementation is not wrapped inside "#if USE_DELAY_POOLS/#endif"
It is not needed any more.
rousskov
removed
the
S-waiting-for-author
|
@yadij, it looks like Christos is done with his changes, and this PR is now squarely on your side. |
rousskov
added
the
S-waiting-for-author
Update fd_note () inside Http::Tunneler::writeRequest with details of what the FD is now being used for.
rousskov
removed
the
S-waiting-for-author
|
I believe I done with requested changes. |
yadij
added
M-cleared-for-merge
squid-anubis
pushed a commit
that referenced
this pull request
Mar 31, 2019
Support forwarding of bumped, reencrypted HTTPS requests through a cache_peer using a standard HTTP CONNECT tunnel. The new Http::Tunneler class establishes HTTP CONNECT tunnels through forward proxies. It is used by TunnelStateData and FwdState classes. Just like before these changes, when a cache_peer replies to CONNECT with an error response, only the HTTP response headers are forwarded to the client, and then the connection is closed. No support for triggering client authentication when a cache_peer configuration instructs the bumping Squid to relay authentication info contained in client CONNECT request. The bumping Squid still responds with HTTP 200 (Connection Established) to the client CONNECT request (to see TLS client handshake) _before_ selecting the cache_peer. HTTPS cache_peers are not yet supported primarily because Squid cannot do TLS-in-TLS with a single fde::ssl state; SslBump and the HTTPS proxy client/tunneling code would need a dedicated TLS connection each. Also fixed delay pools for tunneled traffic. This is a Measurement Factory project.
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support forwarding of bumped, reencrypted HTTPS requests through a
cache_peer using a standard HTTP CONNECT tunnel.
The new Http::Tunneler class establishes HTTP CONNECT tunnels through
forward proxies. It is used by TunnelStateData and FwdState classes.
Just like before these changes, when a cache_peer replies to CONNECT
with an error response, only the HTTP response headers are forwarded to
the client, and then the connection is closed.
No support for triggering client authentication when a cache_peer
configuration instructs the bumping Squid to relay authentication info
contained in client CONNECT request. The bumping Squid still responds
with HTTP 200 (Connection Established) to the client CONNECT request (to
see TLS client handshake) before selecting the cache_peer.
HTTPS cache_peers are not yet supported primarily because Squid cannot
do TLS-in-TLS with a single fde::ssl state; SslBump and the HTTPS proxy
client/tunneling code would need a dedicated TLS connection each.
Also fixed delay pools for tunneled traffic.
This is a Measurement Factory project.