-
Notifications
You must be signed in to change notification settings - Fork 645
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
Comments
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. |
If interested in WADO related stuff, DicomCloud https://github.com/DICOMcloud/DICOMcloud has some nice implementation based on fo-dicom |
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:
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. |
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. 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? |
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.
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.
You could be right. I'm implementing a proof of concept and so far I'm able to limit the changes to DesktopNetworkStream. |
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:
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).
The text was updated successfully, but these errors were encountered: