Add allow-plain server ports attribute #9574

shinrich · 2023-03-31T17:33:30Z

Description on matching issue.

Includes autest and document update.

This closes #9573

maskit · 2023-04-04T15:46:54Z

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?

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.

maskit · 2023-04-04T16:07:52Z

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.


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.

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.

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.

Will add comments to the effect that allow-plain does not make sense for quic. Could also implement an incompatible check.

bneradt · 2023-04-08T22:36:05Z

[approve ci fedora]

ezelkow1 · 2023-05-19T17:04:12Z

[approve ci centos]

SolidWallOfCode · 2023-06-05T21:01:43Z

iocore/net/P_SSLNextProtocolAccept.h

@@ -33,7 +33,7 @@
 class SSLNextProtocolAccept : public SessionAccept
 {
 public:
-  SSLNextProtocolAccept(Continuation *, bool);
+  SSLNextProtocolAccept(Continuation *, bool, bool);


We might want to look at aliased boolean or enum values for clarity.

SolidWallOfCode · 2023-06-05T21:08:22Z

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.

shinrich · 2023-08-30T15:39:05Z

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.

shinrich · 2023-08-30T19:21:18Z

Added a warning if both allow-plain and quic are specified. Warnings seemed to be how the other checks went.

shinrich · 2023-09-11T19:40:20Z

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++;


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.

maskit

Bumping recursion counter inside of nests in the middle of a function seems to be error prone.

maskit · 2023-09-14T22:29:20Z

iocore/net/SSLNetVConnection.cc

+    if (ret == EVENT_RESTART) {
+      // VC migrated into a new object
+      // Just give up and go home. Events should trigger on the new vc
+      return;


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.

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

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.

maskit · 2023-09-15T22:20:16Z

iocore/net/SSLNextProtocolAccept.cc

-      netvc->do_io_close();
+      if (netvc != nullptr) {
+        netvc->do_io_close();
+      } else { // Try making a unix netvc


do_io_close() is a virtual function, so I think we can simply call vio->vc_server->do_io_close() without casting.

maskit · 2023-09-15T22:25:34Z

iocore/net/SSLNextProtocolAccept.cc

+    // 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);


send_plugin_event receives void *. If we don't check nullptr, we can simply pass vio->vc_serve without casting.

* asf/master: (30 commits) add conveinience function to lookup name->IntType* (apache#10474) cmake: bigobj subdir has executables, not plugins (apache#10481) cmake: compile jsonrpc_protocol with -fPIC (apache#10478) Make sure new metrics are always considered (apache#10445) Refactor and rename restart metrics (apache#10472) Fix the SNI and HOST parsing properly (apache#10480) slice/Data.h: CID 1508924: Uninitialized scalar field (apache#10470) CID 1508882: initialize pointer (apache#10469) CID-1512726: Mute coverity, use explicit check for iterator use past end (apache#10467) Fix Coverity issue in inliner plugin. (apache#10442) Set the proper variable when find_package fails to find the package (apache#10468) CID1508860: double lock confusion (apache#10443) Stop using functions that are unavailable on the latest quiche (apache#10447) Add allow-plain server ports attribute (apache#9574) [Fuzzing] move build.sh in trafficserver (apache#10466) Some sort of "fix" to mute coverity. (apache#10451) CID-1512733: Fix coverity issue (apache#10452) Fix cmake autooptions (apache#10456) Cmake presets (apache#10457) This fixes CID 1518257 (apache#10404) ...

shinrich requested review from maskit and SolidWallOfCode March 31, 2023 17:33

shinrich self-assigned this Mar 31, 2023

shinrich force-pushed the allow-plain branch from bfb6734 to 99e23d7 Compare March 31, 2023 19:27

bneradt added this to the 10.0.0 milestone Apr 3, 2023

bneradt added the TLS label Apr 3, 2023

maskit reviewed Apr 4, 2023

View reviewed changes

SolidWallOfCode reviewed Jun 5, 2023

View reviewed changes

shinrich force-pushed the allow-plain branch from 93e8ff8 to 21b6fc5 Compare August 30, 2023 17:37

shinrich commented Sep 11, 2023

View reviewed changes

bryancall requested a review from maskit September 11, 2023 22:31

shinrich force-pushed the allow-plain branch from a2fe5fe to 0652345 Compare September 14, 2023 21:28

maskit reviewed Sep 14, 2023

View reviewed changes

shinrich added 4 commits September 15, 2023 21:13

Add allow-plain server ports attribute

ac2e2df

Fix use after free issue

cca5a99

Back out the recursion tracking memory solution

467112f

Fix conflicting contants

3de3260

shinrich force-pushed the allow-plain branch from 72f0678 to 3de3260 Compare September 15, 2023 21:44

maskit reviewed Sep 15, 2023

View reviewed changes

Address Maskit's comments

1be584a

maskit approved these changes Sep 18, 2023

View reviewed changes

shinrich merged commit 17d918a into master Sep 19, 2023

shinrich deleted the allow-plain branch November 29, 2023 17:49

zwoop added the New Feature label Aug 9, 2024

maskit mentioned this pull request Aug 15, 2024

☂ ATS 10 Documentation #11677

Open

91 tasks

cmcfarlen mentioned this pull request Aug 15, 2024

Documentation: Update release notes #11699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allow-plain server ports attribute #9574

Add allow-plain server ports attribute #9574

shinrich commented Mar 31, 2023 •

edited

Loading

maskit commented Apr 4, 2023 •

edited

Loading

maskit Apr 4, 2023

SolidWallOfCode Jun 5, 2023 •

edited

Loading

shinrich Aug 30, 2023

shinrich Aug 30, 2023

bneradt commented Apr 8, 2023

ezelkow1 commented May 19, 2023

SolidWallOfCode Jun 5, 2023

SolidWallOfCode commented Jun 5, 2023

shinrich commented Aug 30, 2023

shinrich commented Aug 30, 2023

shinrich Sep 11, 2023

maskit left a comment

maskit Sep 14, 2023

shinrich Sep 15, 2023

shinrich Sep 15, 2023

maskit Sep 15, 2023

maskit Sep 15, 2023

Add allow-plain server ports attribute #9574

Add allow-plain server ports attribute #9574

Conversation

shinrich commented Mar 31, 2023 • edited Loading

maskit commented Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

SolidWallOfCode Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bneradt commented Apr 8, 2023

ezelkow1 commented May 19, 2023

Choose a reason for hiding this comment

SolidWallOfCode commented Jun 5, 2023

shinrich commented Aug 30, 2023

shinrich commented Aug 30, 2023

Choose a reason for hiding this comment

maskit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinrich commented Mar 31, 2023 •

edited

Loading

maskit commented Apr 4, 2023 •

edited

Loading

SolidWallOfCode Jun 5, 2023 •

edited

Loading