You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "shinrich (via GitHub)" <gi...@apache.org> on 2023/03/22 22:22:50 UTC
[GitHub] [trafficserver] shinrich opened a new pull request, #9552: Add tr-allow-plain
shinrich opened a new pull request, #9552:
URL: https://github.com/apache/trafficserver/pull/9552
This is the implementation of what I suggested on the ats-dev list "[api] Add tr-try-plain port descriptor". With this code change if a port has tr-allow-plain and ssl, a connection that does not look like TLS will get converted to a UnixNetVConnection and try to process as non-TLS HTTP.
I have an implementation running over 9.1.3 using the tr-pass port descriptor. The conversion of the SSLNetVConnection to UnixNetVConnection works.
I need to write an autest for this. I was going to first write a transparent mode test, but I guess this feature strictly speaking does not need transparent mode although that is likely the case where this would be most likely used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich closed pull request #9552: Add tr-allow-plain
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich closed pull request #9552: Add tr-allow-plain
URL: https://github.com/apache/trafficserver/pull/9552
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a diff in pull request #9552: Add tr-allow-plain
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9552:
URL: https://github.com/apache/trafficserver/pull/9552#discussion_r1148137308
##########
proxy/http/HttpProxyServerMain.cc:
##########
@@ -227,7 +227,7 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor &acceptor, HttpProxyPort &port, unsigned
ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_1, create_h1_server_session});
if (port.isSSL()) {
- SSLNextProtocolAccept *ssl = new SSLNextProtocolAccept(probe, port.m_transparent_passthrough);
+ SSLNextProtocolAccept *ssl = new SSLNextProtocolAccept(probe, port.m_transparent_passthrough, port.m_transparent_allow_plain);
Review Comment:
@SolidWallOfCode should be able to talk to the memory ownership of the probe logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #9552: Add tr-allow-plain
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on PR #9552:
URL: https://github.com/apache/trafficserver/pull/9552#issuecomment-1485478317
Will reopen a new PR. I ended up building my local version which this attribute disconnected from the transparent feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9552: Add tr-allow-plain
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9552:
URL: https://github.com/apache/trafficserver/pull/9552#discussion_r1147040976
##########
proxy/http/HttpProxyServerMain.cc:
##########
@@ -227,7 +227,7 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor &acceptor, HttpProxyPort &port, unsigned
ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_1, create_h1_server_session});
if (port.isSSL()) {
- SSLNextProtocolAccept *ssl = new SSLNextProtocolAccept(probe, port.m_transparent_passthrough);
+ SSLNextProtocolAccept *ssl = new SSLNextProtocolAccept(probe, port.m_transparent_passthrough, port.m_transparent_allow_plain);
Review Comment:
So the new object takes ownership of `probe`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9552: Add tr-allow-plain
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9552:
URL: https://github.com/apache/trafficserver/pull/9552#discussion_r1147036449
##########
iocore/net/SSLNetVConnection.cc:
##########
@@ -2457,3 +2456,68 @@ SSLNetVConnection::_ssl_read_buffer(void *buf, int64_t nbytes, int64_t &nread)
return ssl_error;
}
+
+void
+SSLNetVConnection::_propagateHandShakeBuffer(UnixNetVConnection *target, EThread *t)
+{
+ // Take ownership of the handShake buffer
+ this->sslHandshakeStatus = SSL_HANDSHAKE_DONE;
+ NetState *s = &target->read;
+ s->vio.buffer.writer_for(this->handShakeBuffer);
+ s->vio.set_reader(this->handShakeHolder);
+ s->vio.vc_server = target;
+ s->vio.cont = this->read.vio.cont;
+ s->vio.mutex = this->read.vio.cont->mutex;
+ this->handShakeReader = nullptr;
+ this->handShakeHolder = nullptr;
+ this->handShakeBuffer = nullptr;
+
+ // Kick things again, so the data that was copied into the
+ // vio.read buffer gets processed
+ target->readSignalDone(VC_EVENT_READ_COMPLETE, get_NetHandler(t));
+}
+
+/*
+ * Replaces the current SSLNetVConnection with a UnixNetVConnection
+ * Propagates any data in the SSL handShakeBuffer to be processed
+ * by the UnixNetVConnection logic
+ */
+UnixNetVConnection *
Review Comment:
This does not seem to need a return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9552: Add tr-allow-plain
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9552:
URL: https://github.com/apache/trafficserver/pull/9552#discussion_r1147039220
##########
proxy/ProtocolProbeSessionAccept.cc:
##########
@@ -170,8 +170,14 @@ ProtocolProbeSessionAccept::mainEvent(int event, void *data)
ink_assert(data);
VIO *vio;
- NetVConnection *netvc = static_cast<NetVConnection *>(data);
- ProtocolProbeTrampoline *probe = new ProtocolProbeTrampoline(this, netvc->mutex, nullptr, nullptr);
+ NetVConnection *netvc = static_cast<NetVConnection *>(data);
+ ProtocolProbeTrampoline *probe;
+ UnixNetVConnection *unix_netvc = dynamic_cast<UnixNetVConnection *>(netvc);
+ if (unix_netvc != nullptr && unix_netvc->read.vio.get_writer() != nullptr) {
+ probe = new ProtocolProbeTrampoline(this, netvc->mutex, unix_netvc->read.vio.get_writer(), unix_netvc->read.vio.get_reader());
+ } else {
+ probe = new ProtocolProbeTrampoline(this, netvc->mutex, nullptr, nullptr);
+ }
Review Comment:
`probe` looks like it was leaked, even before your change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org