8000 Support Proxy Protocol v1 and v2 · Issue #1853 · fo-dicom/fo-dicom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support Proxy Protocol v1 and v2 #1853

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.

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

Open
amoerie opened this issue Sep 26, 2024 · 5 comments
Open

Support Proxy Protocol v1 and v2 #1853

amoerie opened this issue Sep 26, 2024 · 5 comments

Comments

@amoerie
Copy link
Collaborator
amoerie commented Sep 26, 2024

What is, according to you, missing from fo-dicom? Please describe.
When DICOM services are hosted behind reverse proxies, the original client IP + client port is lost.
HAProxy and most other load balancers support the Proxy Protocol ( https://www.haproxy.org/download/3.1/doc/proxy-protocol.txt )

Describe the solution you'd like
Add support for parsing a Proxy Protocol header (v1 or v2) in DesktopNetworkStream.
This should be toggled on via a setting.
According to the spec, it is forbidden to automatically detect the presence of this header:

The receiver MUST be configured to only receive the protocol described in this
specification and MUST not try to guess whether the protocol header is present
or not. This means that the protocol explicitly prevents port sharing between
public and private access. Otherwise it would open a major security breach by
allowing untrusted parties to spoof their connection addresses. The receiver
SHOULD ensure proper access filtering so that only trusted proxies are allowed
to use this protocol.

Describe possible alternatives or existing workarounds you've considered
TCP spoofing is an option, but that's not always supported and could cause trouble.
HTTP supports the Forwarded header, but raw DICOM uses TCP. We'd need to switch to WADO to have this, and fo-dicom doesn't have anything WADO related (yet).

@amoerie
Copy link
Collaborator Author
amoerie commented Sep 26, 2024

There is a C# implementation to parse such a Proxy Protocol header, but unfortunately it only supports very recent .NET versions: https://github.com/nazar554/Scintillating.ProxyProtocol.Parser

It could be used as a solid foundation though, and the tests seem very useful to have.

@gofal
8000 Copy link
Contributor
gofal commented Oct 1, 2024

If interested in WADO related stuff, DicomCloud https://github.com/DICOMcloud/DICOMcloud has some nice implementation based on fo-dicom

@amoerie
Copy link
Collaborator Author
amoerie commented Oct 1, 2024

Thanks, I already saw that project before, but it looks like it's been abandoned, and this sentence means we could only use it for "inspiration" nowadays:

The code is written in C# .NET Framework 4.5.2

But yeah, switching to WADO would be quite the enterprise anyway, so I'm investigating whether we can add Proxy Protocol support to our TCP based implementation today.

Local tests are actually working quite nice, but I can't seem to get things running in our test environment yet. But there's a lot on my plate, so I'm doing this in bits and pieces when I can make the time.

@gofal
Copy link
Contributor
gofal commented Oct 1, 2024

The WADO-standard has not changed much. The project is 2 years old, but WADO is older. so the code was written with the latest WADO-standard definition. it is perfectly fine. After everything was implemented, there is no need to have unnecessary additional pushes every month.
You can safely take the code from the project - if you want then update it to latest .net version.

Switching to WADO would be the best way, but maybe take more changes.

This proxy protocol seems to be very specific and it is completely out of scope of DICOM, and therefore to be honest out of scope of fo-dicom. So is seems to be the perfect use-case for Dependency Injection, where you build your own NetworkStream implementation and there you can use the Scintillating.ProxyProtocol.Parser, and then via DI inject your own implementation if INetworkManager. Isn't it?

@amoerie
Copy link
Collaborator Author
amoerie commented Oct 2, 2024

The WADO-standard has not changed much. The project is 2 years old, but WADO is older. so the code was written with the latest WADO-standard definition. it is perfectly fine. After everything was implemented, there is no need to have unnecessary additional pushes every month. You can safely take the code from the project - if you want then update it to latest .net version.

I'm not saying anything's wrong with it, just that we are moving everything to .NET 8 in our company and that a library written against .NET Framework 4.5.2 would be a considerable effort to adapt. Updating such code bases to the latest .NET version is never easy or trivial unfortunately. So, for our purposes, we would probably be able to reuse parts of it in the shape of copy pasting code, but not in the shape of importing a nuget package.

Switching to WADO would be the best way, but maybe take more changes.

This is the long term plan in our company, yes, but as long as we integrate with other PACS software and modalities, raw DICOM tcp will remain necessary to support.

This proxy protocol seems to be very specific and it is completely out of scope of DICOM, and therefore to be honest out of scope of fo-dicom. So is seems to be the perfect use-case for Dependency Injection, where you build your own NetworkStream implementation and there you can use the Scintillating.ProxyProtocol.Parser, and then via DI inject your own implementation if INetworkManager. Isn't it?

You could be right. I'm implementing a proof of concept and so far I'm able to limit the changes to DesktopNetworkStream.
I might end up not making a pull request if I can keep them there.

5C0D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants
0