Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not TLS close_notify when resetting a TCP connection #1944

Closed
wants to merge 8 commits into from
Closed

Do not TLS close_notify when resetting a TCP connection #1944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
79fb8d2
Reduce SO_LINGER code duplication
eduard-bagdasaryan Nov 4, 2024
ae5ce4b
Moved OnOff into a dedicated header
eduard-bagdasaryan Nov 4, 2024
b141627
Do not TLS close_notify when resetting a TCP connection
eduard-bagdasaryan Nov 4, 2024
2ef9964
Merged from master
eduard-bagdasaryan Nov 10, 2024
fd624e6
Set harshClosureRequested flag in commConfigureLinger()
eduard-bagdasaryan Nov 12, 2024
67b6f33
fixup: Restore earlier flag description
rousskov Nov 12, 2024
46d3f51
fixup: Clarified what the condition means
rousskov Nov 12, 2024
c983871
Merge branch 'master' into SQUID-1028-do-not-tls-close-notify-when-re…
eduard-bagdasaryan Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/comm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rousskov rousskov Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's leave this function as is.


if (setsockopt(fd, SOL_SOCKET, SO_LINGER, reinterpret_cast<char*>(&l), sizeof(l)) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

const auto xerrno = errno;
debugs(50, DBG_CRITICAL, "ERROR: Failed to set closure behavior (SO_LINGER) for FD " << fd << ": " << xstrerr(xerrno));
Expand Down Expand Up @@ -877,7 +879,7 @@ _comm_close(int fd, char const *file, int line)
// For simplicity sake, we remain in the caller's context while still
// allowing individual advanced callbacks to overwrite it.

if (F->ssl) {
if (F->ssl && !F->flags.harshClosureRequested) {
const auto startCall = asyncCall(5, 4, "commStartTlsClose",
callDialer(commStartTlsClose, fd));
Comment on lines +882 to 884
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rousskov rousskov Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ScheduleCallHere(startCall);
Expand Down
2 changes: 2 additions & 0 deletions src/fde.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ class fde
bool read_pending = false;
//bool write_pending; //XXX seems not to be used
bool transparent = false;
/// whether comm_reset_close() (or old_comm_reset_close()) has been called
bool harshClosureRequested = false;
} flags;

int64_t bytes_read = 0;
Expand Down