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/31 17:33:33 UTC
[GitHub] [trafficserver] shinrich opened a new pull request, #9574: Add allow-plain server ports attribute
shinrich opened a new pull request, #9574:
URL: https://github.com/apache/trafficserver/pull/9574
Description on matching issue.
This closes #9573
--
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] maskit commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1327837296
##########
iocore/net/SSLNextProtocolAccept.cc:
##########
@@ -93,25 +99,32 @@ struct SSLNextProtocolTrampoline : public Continuation {
return EVENT_ERROR;
}
- // Cancel the action, so later timeouts and errors don't try to
- // send the event to the Accept object. After this point, the accept
- // object does not care.
- netvc->set_action(nullptr);
-
- Continuation *endpoint_cont = netvc->endpoint();
- if (!endpoint_cont) {
- // Route to the default endpoint
- endpoint_cont = npnParent->endpoint;
- }
-
- if (endpoint_cont) {
- // disable read io, send events to endpoint
- netvc->do_io_read(endpoint_cont, 0, nullptr);
-
- send_plugin_event(endpoint_cont, NET_EVENT_ACCEPT, netvc);
+ // This wasn't really a TLS connection
+ // Trying to process it as a TCP connection
+ if (netvc == nullptr) {
+ UnixNetVConnection *plain_netvc = dynamic_cast<UnixNetVConnection *>(vio->vc_server);
Review Comment:
`send_plugin_event` receives `void *`. If we don't check nullptr, we can simply pass `vio->vc_serve` without casting.
--
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] SolidWallOfCode commented on pull request #9574: Add allow-plain server ports attribute
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#issuecomment-1577478241
I also want to riff of @maskit and say the consistency checking in processing the server ports should be extended to generate an error if both `quic` and `allow-plain` are enabled on the same server port.
--
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] ezelkow1 commented on pull request #9574: Add allow-plain server ports attribute
Posted by "ezelkow1 (via GitHub)" <gi...@apache.org>.
ezelkow1 commented on PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#issuecomment-1554943208
[approve ci centos]
--
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] SolidWallOfCode commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1218589925
##########
iocore/net/P_SSLNextProtocolAccept.h:
##########
@@ -33,7 +33,7 @@
class SSLNextProtocolAccept : public SessionAccept
{
public:
- SSLNextProtocolAccept(Continuation *, bool);
+ SSLNextProtocolAccept(Continuation *, bool, bool);
Review Comment:
We might want to look at aliased boolean or enum values for clarity.
--
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] bneradt commented on pull request #9574: Add allow-plain server ports attribute
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#issuecomment-1500990516
[approve ci fedora]
--
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] SolidWallOfCode commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1218589233
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -783,6 +786,9 @@ mptcp
Requires custom Linux kernel available at https://multipath-tcp.org.
+allow-plain
+ For TLS ports, will fall back to non-TLS processing if the TLS handshake fails.
Review Comment:
Fails for any reason, including say an expired certificate? Or does this require a protocol failure? Looking at the code it seems only a protocol error on the first read / packet will failover to plain text.
--
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] maskit commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1327835376
##########
iocore/net/SSLNextProtocolAccept.cc:
##########
@@ -77,14 +77,20 @@ struct SSLNextProtocolTrampoline : public Continuation {
vio = static_cast<VIO *>(edata);
netvc = dynamic_cast<SSLNetVConnection *>(vio->vc_server);
- ink_assert(netvc != nullptr);
switch (event) {
case VC_EVENT_EOS:
case VC_EVENT_ERROR:
case VC_EVENT_ACTIVE_TIMEOUT:
case VC_EVENT_INACTIVITY_TIMEOUT:
- netvc->do_io_close();
+ if (netvc != nullptr) {
+ netvc->do_io_close();
+ } else { // Try making a unix netvc
Review Comment:
`do_io_close()` is a virtual function, so I think we can simply call `vio->vc_server->do_io_close()` without casting.
--
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] maskit commented on pull request #9574: Add allow-plain server ports attribute
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#issuecomment-1496210511
Will all the data received is re-read by UnixNetVC? I'm wondering what happens if TLS handshake fails at the very late stage of it. Will SNI actions be triggered? If yes, will everything done on SNI actions be reset on the plain connection?
If all the data will be re-read by UnixNetVC, parsing the "HTTP request" probably fails because the fake TLS data is invalid as an HTTP request format.
--
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] maskit commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1326576975
##########
iocore/net/SSLNetVConnection.cc:
##########
@@ -603,11 +603,20 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread)
if (!getSSLHandShakeComplete()) {
int err = 0;
+ // May get into logic that will clean up the current VC
+ // Increment the recursion to delay do_io_close cleaup.
+ this->recursion++;
+
if (get_context() == NET_VCONNECTION_OUT) {
ret = sslStartHandShake(SSL_EVENT_CLIENT, err);
} else {
ret = sslStartHandShake(SSL_EVENT_SERVER, err);
}
+ if (ret == EVENT_RESTART) {
+ // VC migrated into a new object
+ // Just give up and go home. Events should trigger on the new vc
+ return;
Review Comment:
Don't we need to decrement the recursion counter here?
If no, it's difficult to understand. Want a comment why we don't need it.
If yes, it's fragile. I don't like to use `goto` but we may need it use it.
--
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 merged pull request #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich merged PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1327513124
##########
iocore/net/SSLNetVConnection.cc:
##########
@@ -603,11 +603,20 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread)
if (!getSSLHandShakeComplete()) {
int err = 0;
+ // May get into logic that will clean up the current VC
+ // Increment the recursion to delay do_io_close cleaup.
+ this->recursion++;
+
if (get_context() == NET_VCONNECTION_OUT) {
ret = sslStartHandShake(SSL_EVENT_CLIENT, err);
} else {
ret = sslStartHandShake(SSL_EVENT_SERVER, err);
}
+ if (ret == EVENT_RESTART) {
+ // VC migrated into a new object
+ // Just give up and go home. Events should trigger on the new vc
+ return;
Review Comment:
I tried the recursion counts then changed the return value. I think that just sending back a different return value will suffice. I'll back out the recursion could and verify that everything still works. And update the PR
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1321995046
##########
iocore/net/SSLNetVConnection.cc:
##########
@@ -587,6 +587,10 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread)
if (!getSSLHandShakeComplete()) {
int err = 0;
+ // May get into logic that will clean up the current VC
+ // Increment the recursion to delay do_io_close cleaup.
+ this->recursion++;
Review Comment:
Adding this recursion bump, other use after free ASAN error would trigger on the later if (this->handshakeReader) check because with addition of the _migrateSsl, the SSLNetVC object (this) may have been deleted.
By incrementing recursion and adding test_inline_close() on return, we ensure that the deletion of this does not occur until the end of the function.
--
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] maskit commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1157475995
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -783,6 +786,9 @@ mptcp
Requires custom Linux kernel available at https://multipath-tcp.org.
+allow-plain
+ For TLS ports, will fall back to non-TLS processing if the TLS handshake fails.
Review Comment:
We may want to mention that this is for TLS over TCP (or ports configured with `ssl`), although plain QUIC connection does not exist in the first place. We could also say this is incompatible with `quic` like some other descriptors say.
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1327515381
##########
iocore/net/SSLNetVConnection.cc:
##########
@@ -603,11 +603,20 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread)
if (!getSSLHandShakeComplete()) {
int err = 0;
+ // May get into logic that will clean up the current VC
+ // Increment the recursion to delay do_io_close cleaup.
+ this->recursion++;
+
if (get_context() == NET_VCONNECTION_OUT) {
ret = sslStartHandShake(SSL_EVENT_CLIENT, err);
} else {
ret = sslStartHandShake(SSL_EVENT_SERVER, err);
}
+ if (ret == EVENT_RESTART) {
+ // VC migrated into a new object
+ // Just give up and go home. Events should trigger on the new vc
+ return;
Review Comment:
And you are exactly right. Should be dropping the recursion count here too. Should use the scoped pattern to deal with the decrement if we really need to manipulate the recursion counter.
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1310470366
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -783,6 +786,9 @@ mptcp
Requires custom Linux kernel available at https://multipath-tcp.org.
+allow-plain
+ For TLS ports, will fall back to non-TLS processing if the TLS handshake fails.
Review Comment:
Will add comments to the effect that allow-plain does not make sense for quic. Could also implement an incompatible check.
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1310468321
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -783,6 +786,9 @@ mptcp
Requires custom Linux kernel available at https://multipath-tcp.org.
+allow-plain
+ For TLS ports, will fall back to non-TLS processing if the TLS handshake fails.
Review Comment:
Allow-plain only kicks in of the first packet fails to be recognized as a valid client-hello. Once packets are exchanged in the TLS path, it is no reasonable to try an reinterpret as HTTP.
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#issuecomment-1699415692
> Will all the data received be re-read by UnixNetVC? I'm wondering what happens if TLS handshake fails at the very late stage of it. Will SNI actions be triggered? If yes, will everything done on SNI actions be reset on the plain connection?
The allow-plain will only kick in if there is a TLS parse failure on the first packet. No TLS-oriented hooks should be triggered.
--
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] SolidWallOfCode commented on a diff in pull request #9574: Add allow-plain server ports attribute
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#discussion_r1218589233
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -783,6 +786,9 @@ mptcp
Requires custom Linux kernel available at https://multipath-tcp.org.
+allow-plain
+ For TLS ports, will fall back to non-TLS processing if the TLS handshake fails.
Review Comment:
Fails for any reason, including say an expired certificate? Or does this require a protocol failure?
--
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 #9574: Add allow-plain server ports attribute
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on PR #9574:
URL: https://github.com/apache/trafficserver/pull/9574#issuecomment-1699711640
Added a warning if both allow-plain and quic are specified. Warnings seemed to be how the other checks went.
--
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