Do not TLS close_notify when resetting a TCP connection #1944

eduard-bagdasaryan · 2024-11-12T15:39:41Z

No description provided.

reducing code duplication.

Neither is perfect, but I think the earlier version of this documentation gave the reader more/better information about this flag intent/purpose. Here, SO_LINGER is just an implementation detail that `.h` reader should not really care/know about.

rousskov

This review annotates this PR without requesting changes.

rousskov · 2024-11-12T16:54:15Z

src/comm.cc

+    if (F->ssl && !F->flags.harshClosureRequested) {
        const auto startCall = asyncCall(5, 4, "commStartTlsClose",
                                         callDialer(commStartTlsClose, fd));


To me, it seems fairly obvious that if we are going to send TCP RST to the client, then we do not want to nicely close TLS session either. Our harsh closure reasons that apply to TCP layer ought to apply to TLS layer as well. For example, if Squid is sending the client an unchunked HTTP response without Content-Length header, and Squid has to abort that transaction, then we do not want successful TLS closure to trick that client into thinking that it has gotten the entire response body.

If others disagree with this "fairly obvious" assertion, then we should add an explanation to PR description. That explanation may be similar to the above paragraph (sans the "obvious" claim itself, of course).

While what you write seems reasonable, it is not clear that RST is the only result of this function being called. It can and is also called for the FIN cases where TLS should be completed cleanly.

RST is the only result of this function being called

FWIW, the function (commStartTlsClose()) is not called for RST cases (F->flags.harshClosureRequested is false) but called for other (i.e., FIN) cases, as you noted.

Amos: it is not clear that RST is the only result of this function being called. It can and is also called for the FIN cases where TLS should be completed cleanly.

If "this function" is commStartTlsClose():

In official code, before this PR: commStartTlsClose() was called in RST and FIN cases, and that was wrong.

In PR code, commStartTlsClose() is only called in FIN cases, as intended.

RST is not the "result of calling this function".

If "this function" is commConfigureLinger():

In official code, before this PR: commConfigureLinger() was called in both RST and FIN cases. That function call had no effect on commStartTlsClose() calls, and that was wrong.

In PR code: commConfigureLinger() is called in both RST and FIN cases. That function call bans future commStartTlsClose() calls in RST cases only, as intended.

RST is the "result of calling this function" with On parameter, followed by a close(2) system call.

It is, of course, possible that some code paths that end with commStartTlsClose() are missing a commConfigureLinger() call to RST the connection, but that possible problem is outside this PR scope. This PR does not change and should not change when/where commConfigureLinger() is called.

rousskov · 2024-11-12T17:05:03Z

src/comm.cc

@@ -783,6 +783,8 @@ commConfigureLinger(const int fd, const OnOff enabled)
    l.l_onoff = (enabled == OnOff::on ? 1 : 0);
    l.l_linger = 0; // how long to linger for, in seconds

+    fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true
+
    if (setsockopt(fd, SOL_SOCKET, SO_LINGER, reinterpret_cast<char*>(&l), sizeof(l)) < 0) {


It may be tempting to make new harshClosureRequested flag assignment conditional on this setsockopt() call success, but I do not think we should do that: The caller has requested harsh connection closing; setsockopt() success is pretty much irrelevant. Even if TCP layer closes nicely despite our attempt to close harshly, we still want to close TLS layer harshly...

rousskov · 2024-11-12T17:07:58Z

src/comm.cc

@@ -783,6 +783,8 @@ commConfigureLinger(const int fd, const OnOff enabled)
    l.l_onoff = (enabled == OnOff::on ? 1 : 0);
    l.l_linger = 0; // how long to linger for, in seconds

+    fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true


This is a low-level private function with callers already checking fd, so I think it may be OK to avoid checking fd value before accessing fd_table here. If you disagree, consider applying this suggestion:

Suggested change

fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true

if (isOpen(fd))

fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true

We would need also to protect setsockopt() by the same check, i.e., basically this function should immediately return if isOpen() is false. I would just assert(isOpen(fd)) instead.

We would need also to protect setsockopt() by the same check.

Not necessarily: Leaving that system call exposed to caller bugs may be better in this case because it will result in level-0 ERROR message, exposing the bug.

I would just assert(isOpen(fd)) instead.

I would too, but "The following [isOpen(fd) check] fails because ipc.c is doing calls to pipe() to create sockets" comment in _comm_close() worries me. That old comment may no longer reflect what is actually going on, of course, but it is a red flag.

All these complications/problems are outside this PR scope IMO (as detailed at the beginning of this thread). They are best resolved in a dedicated PR. That is why I did not recommend any changes in my review. Said that, if others insist on an if check or assertion, let's add them to make progress.

Ok, let's leave this function as is.

yadij · 2024-11-13T22:40:55Z

Please rebase for easier review. There are changes here from other PRs which were controversial.

…setting

rousskov

Amos: Please rebase for easier review. There are changes here from other PRs which were controversial.

FWIW, this PR diff did not (and does not) contain any changes from other PRs. It was already based on a recent master, and Eduard has now merged all latest/unrelated master changes into this PR branch, addressing your request.

rousskov · 2024-11-14T16:45:51Z

src/comm.cc

@@ -783,6 +783,8 @@ commConfigureLinger(const int fd, const OnOff enabled)
    l.l_onoff = (enabled == OnOff::on ? 1 : 0);
    l.l_linger = 0; // how long to linger for, in seconds

+    fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true


We would need also to protect setsockopt() by the same check.

Not necessarily: Leaving that system call exposed to caller bugs may be better in this case because it will result in level-0 ERROR message, exposing the bug.

I would just assert(isOpen(fd)) instead.

I would too, but "The following [isOpen(fd) check] fails because ipc.c is doing calls to pipe() to create sockets" comment in _comm_close() worries me. That old comment may no longer reflect what is actually going on, of course, but it is a red flag.

All these complications/problems are outside this PR scope IMO (as detailed at the beginning of this thread). They are best resolved in a dedicated PR. That is why I did not recommend any changes in my review. Said that, if others insist on an if check or assertion, let's add them to make progress.

rousskov · 2024-11-14T17:03:28Z

src/comm.cc

+    if (F->ssl && !F->flags.harshClosureRequested) {
        const auto startCall = asyncCall(5, 4, "commStartTlsClose",
                                         callDialer(commStartTlsClose, fd));


Amos: it is not clear that RST is the only result of this function being called. It can and is also called for the FIN cases where TLS should be completed cleanly.

If "this function" is commStartTlsClose():

In official code, before this PR: commStartTlsClose() was called in RST and FIN cases, and that was wrong.

In PR code, commStartTlsClose() is only called in FIN cases, as intended.

RST is not the "result of calling this function".

If "this function" is commConfigureLinger():

In official code, before this PR: commConfigureLinger() was called in both RST and FIN cases. That function call had no effect on commStartTlsClose() calls, and that was wrong.

In PR code: commConfigureLinger() is called in both RST and FIN cases. That function call bans future commStartTlsClose() calls in RST cases only, as intended.

RST is the "result of calling this function" with On parameter, followed by a close(2) system call.

It is, of course, possible that some code paths that end with commStartTlsClose() are missing a commConfigureLinger() call to RST the connection, but that possible problem is outside this PR scope. This PR does not change and should not change when/where commConfigureLinger() is called.

rousskov · 2024-11-19T15:42:40Z

AFAICT, all earlier concerns were addressed on or before November 14. If there are no objections by then, I plan to clear this PR for merging on November 25, 2024.

eduard-bagdasaryan and others added 7 commits November 4, 2024 16:21

Reduce SO_LINGER code duplication

79fb8d2

Moved OnOff into a dedicated header

ae5ce4b

Do not TLS close_notify when resetting a TCP connection

b141627

Merged from master

2ef9964

Set harshClosureRequested flag in commConfigureLinger()

fd624e6

reducing code duplication.

fixup: Restore earlier flag description

67b6f33

Neither is perfect, but I think the earlier version of this documentation gave the reader more/better information about this flag intent/purpose. Here, SO_LINGER is just an implementation detail that `.h` reader should not really care/know about.

fixup: Clarified what the condition means

46d3f51

rousskov approved these changes Nov 12, 2024

View reviewed changes

rousskov added the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label Nov 12, 2024

yadij added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Nov 13, 2024

Merge branch 'master' into SQUID-1028-do-not-tls-close-notify-when-re…

c983871

…setting

rousskov reviewed Nov 14, 2024

View reviewed changes

rousskov added S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-author author action is expected (and usually required) labels Nov 14, 2024

squid-anubis pushed a commit that referenced this pull request Nov 23, 2024

Do not TLS close_notify when resetting a TCP connection (#1944)

f157e3a

squid-anubis pushed a commit that referenced this pull request Nov 24, 2024

Do not TLS close_notify when resetting a TCP connection (#1944)

6bd7411

rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Nov 26, 2024

squid-anubis closed this Nov 26, 2024

squid-anubis removed the M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Nov 26, 2024

squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not TLS close_notify when resetting a TCP connection #1944

Do not TLS close_notify when resetting a TCP connection #1944

eduard-bagdasaryan commented Nov 12, 2024

rousskov left a comment

rousskov Nov 12, 2024

yadij Nov 13, 2024

eduard-bagdasaryan Nov 14, 2024

rousskov Nov 14, 2024 •

edited

Loading

rousskov Nov 12, 2024

rousskov Nov 12, 2024

eduard-bagdasaryan Nov 14, 2024

rousskov Nov 14, 2024 •

edited

Loading

eduard-bagdasaryan Nov 15, 2024

yadij commented Nov 13, 2024

rousskov left a comment

rousskov Nov 14, 2024 •

edited

Loading

rousskov Nov 14, 2024 •

edited

Loading

rousskov commented Nov 19, 2024

	fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true
	if (isOpen(fd))
	fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true

Do not TLS close_notify when resetting a TCP connection #1944

Do not TLS close_notify when resetting a TCP connection #1944

Conversation

eduard-bagdasaryan commented Nov 12, 2024

rousskov left a comment

Choose a reason for hiding this comment

rousskov Nov 12, 2024

Choose a reason for hiding this comment

yadij Nov 13, 2024

Choose a reason for hiding this comment

eduard-bagdasaryan Nov 14, 2024

Choose a reason for hiding this comment

rousskov Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

rousskov Nov 12, 2024

Choose a reason for hiding this comment

rousskov Nov 12, 2024

Choose a reason for hiding this comment

eduard-bagdasaryan Nov 14, 2024

Choose a reason for hiding this comment

rousskov Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

eduard-bagdasaryan Nov 15, 2024

Choose a reason for hiding this comment

yadij commented Nov 13, 2024

rousskov left a comment

Choose a reason for hiding this comment

rousskov Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

rousskov Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

rousskov commented Nov 19, 2024

rousskov Nov 14, 2024 •

edited

Loading

rousskov Nov 14, 2024 •

edited

Loading

rousskov Nov 14, 2024 •

edited

Loading

rousskov Nov 14, 2024 •

edited

Loading