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

Jump to bottom

TCP listener stuck while processing TLS handshake #1766

Open
amoerie opened this issue Mar 22, 2024 · 4 comments · May be fixed by #1767
Open

TCP listener stuck while processing TLS handshake #1766

amoerie opened this issue Mar 22, 2024 · 4 comments · May be fixed by #1767
Assignees
@amoerie
Labels
enhancement

Comments

@amoerie
Copy link
Collaborator

amoerie commented Mar 22, 2024

Describe the bug
While the TCP listener (DesktopNetworkListener) is processing an SSL handshake, no further connections are accepted.
We are seeing situations in the wild where this SSL handshake is taking a long time, due to protocol mismatch, broken environments or buggy network software meddling with our bytes in transit.
More specifically, the method AuthenticateAsServer can hang for a long time.
See also dotnet/runtime#914, it looks like we're not the only ones running into this.

The good news is that we have 1 minute timeout (default, configurable via DefaultTlsAcceptor) that eventually makes us recover from the situation, but the bad news remains that we don't accept any extra TCP connections while this SSL handshake is frozen.

Today most of this is bugs or faulty setups, but I also imagine a bad actor with malevolent intentions that wants to screw up a DICOM listener somewhere that uses Fellow Oak DICOM. You could DDOS every Fellow Oak DICOM listener if you know how and are able to reach it.

To Reproduce
This is the hard part. I have some Wireshark captures that I still need to analyze, but I can reproduce the issue by manually sending the initial bytes of a TLS 1.2 handshake. Our server keeps waiting for more bytes to arrive (but they never do, in my little test), and I confirmed that spinning up a DicomClient then doesn't work until the bad handshake times out.

Expected behavior
I believe the SSL handshake should be offloaded to a background thread and should not block the main loop that accepts incoming TCP connections. Not only would this be a performance gain in the case of multiple connections, it would also solve this "bug".

Screenshots or test DICOM files
I'll write a unit test, this stuff is hard to reproduce.

Environment
Fellow Oak DICOM version: Latest
OS: Irrelevant
Platform: Irrelevant
@amoerie amoerie added the new label Mar 22, 2024
@amoerie amoerie linked a pull request Mar 22, 2024 that will close this issue
Open
5 tasks
@amoerie amoerie added enhancement and removed new labels Mar 22, 2024
@amoerie amoerie self-assigned this Mar 22, 2024
@aerik
Copy link

aerik commented Mar 28, 2024 •
edited

Ooooh, good find. I hadn't thought about that, but I've seen that too; the TLS connection routinely takes a couple seconds or more to finish the handshake. Seems like the only viable solution is to pull the TLS code out of the Networkstream constructor so that it can run in parallel to the listener continuing to listen...

[EDIT] Or - and this is maybe a little clunky - you could make some sort of lazy initializer happen inside AsStream... but if it failed, then we are potentially throwing in unexpected places

@amoerie
Copy link
Collaborator Author

amoerie commented Mar 29, 2024

@aerik I have a fix! #1767 ;-)

@aerik
Copy link

aerik commented Apr 14, 2024 •
edited

Hmm... You probably don't want to reference DesktopNetworkStream inside DicomServer...?

var networkStream = new DesktopNetworkStream(tcpClient, _tlsAcceptor, true);

What if you make an asynchronous method on the listener instead, so it will return the right kind of INetworkStream?

Maybe something like this

    var tcpClient = await listener.AcceptTcpClientAsync(noDelay, this.cancellationSource.Token).ConfigureAwait(false);
    if(tcpClient != null)
    {
        Task<INetworkStream> streamTask = listener.GetNetorkStream(tcpClient, this.certificateName);
        _ = streamTask.ContinueWith(task =>
        {
            if (!task.IsFaulted)
            {
                var scp = this.CreateScp(task.Result);
                if (this.Options != null)
                {
                    scp.Options = this.Options;
                }
                lock (_clientLocker)
                {
                    this.clients.Add(scp);
                }
            }
        });
    }

?

[EDIT] Just realized I don't need .ConfigureAwait(false) since it's not awaiting.

@amoerie
Copy link
Collaborator Author

amoerie commented May 2, 2024

Hmm... You probably don't want to reference DesktopNetworkStream inside DicomServer...?

Good call. There is already some "CreateNetworkStream" responsibility in INetworkManager so I'll add an overload there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@amoerie amoerie

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

TCP listener stuck while processing TLS handshake fo-dicom/fo-dicom
2 participants
@aerik @amoerie