Re: [squid-users] Squid modification to only read client SNI without bumping.

2021-06-08 Thread Alex Rousskov
On 6/8/21 7:36 AM, squ...@treenet.co.nz wrote:

> The way I think to approach it though is to start with the
> configuration parser.

That starting point does not compute for me. We do need to agree on how
to configure this feature, but parsing any resulting Squid configuration
ought to be very straightforward. Perhaps you have meant "TLS
ClientHello parser", but Squid already has that.


> A simple peek-splice/terminate TLS traffic flow
> should not need certificates setup by admin.

Squid already does not generate/use certificates for splicing or
terminating connections. In splice-or-terminate use cases, the
certificates come into play only when delivery _errors_. A feature to
prevent bumping for error delivery (and remove any configuration
requirements for CA certificate) should be welcomed IMO.

Please drop squid-users if responding to this email.

Alex.
___
squid-users mailing list
squid-users@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-users


Re: [squid-users] Squid modification to only read client SNI without bumping.

2021-06-08 Thread His Shadow
Could you direct me to those scripts? Also, am I understanding
correctly that in this mode:
acl blocklist dstdomain ...

ssl_bump peek all
ssl_bump splice blocklist
ssl_bump terminate all

I will only need certs to display an error page from squid via ssl,
but unblocked domains should be just fine?
I think it should be
ssl_bump splice !blocklist
Since blocklist is the list of domains that needs blocking, so we
don't need to splice them. Oh, and one more thing, wouldn't dstdomain
match something that was sent in the CONNECT request itself, instead
of the SNI in the client hello if it is present?

-- 
HisShadow
___
squid-users mailing list
squid-users@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-users


Re: [squid-users] Squid modification to only read client SNI without bumping.

2021-06-08 Thread squid3

On 2021-06-08 22:51, His Shadow wrote:

Greetings. I've been trying to make a patch for squid,


Code changes should be discussed on the squid-dev mailing list.

FWIW, we (Squid devs) have already discussed this functionality change 
and I have a TODO list entry (far down sadly) of supporting your 
use-case. The way I think to approach it though is to start with the 
configuration parser. A simple peek-splice/terminate TLS traffic flow 
should not need certificates setup by admin.


If you want to pickup that TODO item please contact squid-dev to plan 
out the actual best approach with the other dev working on Squid crypto 
code.


Patch submission should be done by submitting a github PR targeted at 
our repository 'master' branch.




so that it
could read client hello on connect requests and set the SNI without
using ssl_bump, as that requires generating certificates and is too
complicated for my needs.


Should not be too complicated. We have test scripts available that can 
generate fake cert and CA for the *_port config settings. Or snakeoil 
certs can be used.


Apart from the port settings what your patch does is just this:


 acl blocklist dstdomain ...

 ssl_bump peek all
 ssl_bump splice blocklist
 ssl_bump terminate all



Amos
___
squid-users mailing list
squid-users@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-users


[squid-users] Squid modification to only read client SNI without bumping.

2021-06-08 Thread His Shadow
Greetings. I've been trying to make a patch for squid, so that it
could read client hello on connect requests and set the SNI without
using ssl_bump, as that requires generating certificates and is too
complicated for my needs. Here's the patch I've come up with. It seems
to be working, but I'm getting a bunch of connections in CLOSE_WAIT
state after using it under load. I can't seem to reproduce it locally,
but I bet I don't know something, or did something wrong. Can anyone
code check this patch, please? Also, not sure if it's the correct
place to post this. The patch is applicable to the latest release in
4.x series - 4.15.

-- 
HisShadow
diff --git a/src/SquidConfig.h b/src/SquidConfig.h
index b696ffc..e5fbc2d 100644
--- a/src/SquidConfig.h
+++ b/src/SquidConfig.h
@@ -365,6 +365,7 @@ public:
 acl_access *sendHit;
 acl_access *storeMiss;
 acl_access *stats_collection;
+acl_access *banned_domains;
 #if SQUID_SNMP
 
 acl_access *snmp;
diff --git a/src/cf.data.pre b/src/cf.data.pre
index 4aef432..3250545 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -10157,4 +10157,13 @@ DOC_START
 		server_pconn_for_nonretriable allow SpeedIsWorthTheRisk
 DOC_END
 
+NAME: banned_domains
+TYPE: acl_access
+DEFAULT: none
+DEFAULT_DOC: Banned domains.
+LOC: Config.accessList.banned_domains
+DOC_START
+	Banned domains.
+DOC_END
+
 EOF
diff --git a/src/client_side.h b/src/client_side.h
index 9fe8463..a1b861e 100644
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -120,6 +120,8 @@ public:
  */
 void setAuth(const Auth::UserRequest::Pointer , const char *cause);
 #endif
+/// TLS client delivered SNI value. Empty string if none has been received.
+SBuf tlsClientSni_;
 
 Ip::Address log_addr;
 
@@ -413,8 +415,6 @@ private:
 unsigned short tlsConnectPort; ///< The TLS server port number as passed in the CONNECT request
 SBuf sslCommonName_; ///< CN name for SSL certificate generation
 
-/// TLS client delivered SNI value. Empty string if none has been received.
-SBuf tlsClientSni_;
 SBuf sslBumpCertKey; ///< Key to use to store/retrieve generated certificate
 
 /// HTTPS server cert. fetching state for bump-ssl-server-first
diff --git a/src/tunnel.cc b/src/tunnel.cc
index 217e947..6a015ca 100644
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -79,6 +79,7 @@ public:
 static void ReadServer(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag errcode, int xerrno, void *data);
 static void WriteClientDone(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data);
 static void WriteServerDone(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data);
+static void CloseConnections(const Comm::ConnectionPointer &, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data);
 
 /// Starts reading peer response to our CONNECT request.
 void readConnectResponse();
@@ -177,6 +178,10 @@ public:
 SBuf preReadServerData;
 time_t startTime; ///< object creation time, before any peer selection/connection attempts
 
+SBuf tlsData;
+size_t tlsBodySize, tlsAlreadyRead, tlsHeaderLeftToRead;
+bool tlsFirstByteChecked;
+
 void copyRead(Connection , IOCB *completion);
 
 /// continue to set up connection to a peer, going async for SSL peers
@@ -224,6 +229,7 @@ public:
 void readConnectResponseDone(char *buf, size_t len, Comm::Flag errcode, int xerrno);
 void copyClientBytes();
 void copyServerBytes();
+void copyAlert();
 };
 
 static const char *const conn_established = "HTTP/1.1 200 Connection established\r\n\r\n";
@@ -872,11 +878,13 @@ static void
 tunnelStartShoveling(TunnelStateData *tunnelState)
 {
 assert(!tunnelState->waitingForConnectExchange());
+if (!tunnelState->tlsData.isEmpty()) {
+tunnelState->tlsData.consume();
+}
 *tunnelState->status_ptr = Http::scOkay;
 if (tunnelState->logTag_ptr)
 *tunnelState->logTag_ptr = LOG_TCP_TUNNEL;
 if (cbdataReferenceValid(tunnelState)) {
-
 // Shovel any payload already pushed into reply buffer by the server response
 if (!tunnelState->server.len)
 tunnelState->copyServerBytes();
@@ -895,6 +903,248 @@ tunnelStartShoveling(TunnelStateData *tunnelState)
 }
 }
 
+static bool isSNICompatible(SBuf ) {
+if (((uint8_t)header[0] & 0x80) && header[2] == 1) {
+return false;
+}
+
+if (header[1] < 3) {
+return false;
+}
+return true;
+}
+
+void
+TunnelStateData::CloseConnections(const Comm::ConnectionPointer &, char *, size_t, Comm::Flag, int, void *data) {
+TunnelStateData *tunnelState = (TunnelStateData *)data;
+CbcPointer safetyLock(tunnelState);
+
+if (Comm::IsConnOpen(tunnelState->client.conn))
+tunnelState->client.conn->close();
+
+if (Comm::IsConnOpen(tunnelState->server.conn))
+tunnelState->server.conn->close();
+}
+
+//