Fix FTP/SSL data stream closure causing protocol violation in .NET 10#123234
Fix FTP/SSL data stream closure causing protocol violation in .NET 10#123234
Conversation
…tworkStream Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
|
Hey @copilot. Look at test suite here: https://github.com/dotnet/runtime/tree/main/src/libraries/Common/tests/System/Net/EnterpriseTests/setup there is already docker setup where you can add the sever and test for FTP over https. |
I'll add an FTP server with SSL support to the enterprise docker test suite and create tests for FTP/SSL operations. This will provide automated testing for the fix. |
- Created ftpserver container with ProFTPD and SSL/TLS support - Added ftpserver to docker-compose.yml for enterprise test environment - Created System.Net.Requests EnterpriseTests project - Added FTP/SSL upload and download tests to validate the fix - Tests specifically verify stream disposal doesn't cause protocol violations Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
|
Hey @copilot it seems like the test is failing to build and run "The name 'EnterpriseTestConfiguration' does not exist in the current context". And you can probably just add the package to existing container instead of creating new one. It should not conflict with HTTP. |
…heweb container - Added System.Net.Test.Common namespace import to fix EnterpriseTestConfiguration reference - Integrated ProFTPD into existing apacheweb container instead of separate ftpserver - Updated apacheweb Dockerfile to install ProFTPD with SSL support - Modified run.sh to start ProFTPD alongside Apache - Changed FTP test to use apacheweb.linux.contoso.com instead of separate ftpserver - Removed standalone ftpserver container configuration - Restored docker-compose.yml to original structure (no ftpserver service) Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
Fixed in commit d110002. Added the missing |
|
It looks like the new tests did not run. do we need to add it to some pipeline configuration @copilot ? |
- Added System.Net.Requests to path triggers in enterprise pipeline - Added build step for System.Net.Requests.Enterprise.Tests.csproj - Added test run step for System.Net.Requests enterprise tests - Tests will now run automatically in the enterprise test pipeline Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
Added to pipeline in commit 5aff1b4. The
The tests will now run automatically when PRs affect System.Net.Requests code. |
src/libraries/Common/tests/System/Net/EnterpriseTests/setup/apacheweb/Dockerfile
Show resolved
Hide resolved
- Added ServerCertificateValidationCallback in test constructor to accept self-signed certificates - This is required because the ProFTPD server uses a self-signed certificate in the test environment - Added System.Net.Security and System.Security.Cryptography.X509Certificates namespaces - Callback returns true for all certificates in the controlled test environment Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
You're absolutely right. Fixed in commit d5b7ff8. I mistakenly applied |
|
what is next step @copilot? The test is still failing. |
I need to see the current test failure output to diagnose further. The last reported error was "(500) Syntax error, command unrecognized" which I addressed by fixing the control stream SSL configuration. Could you provide:
Without seeing the actual failure, I can't determine if:
Alternatively, if you have access to the test environment, could you check if ProFTPD is running ( |
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
…com/wfurt/runtime into copilot/fix-ftp-ssl-upload-download
|
hello @copilot, can you revert change to the containers? Separate change was merged to main branch to address that. It would be great if you can rebase. Also there was one failing test for disposal. Can you write identical test for just plain FTP to see if that behavior is specific to TLS or if it is common? Perhaps convert the added tests to theory so it always covers both cases. |
- Reverted apacheweb/Dockerfile and apacheweb/run.sh to main branch state (container setup merged separately) - Converted FtpUpload tests from Fact to Theory with InlineData for both SSL and non-SSL cases - Converted FtpDownload tests from Fact to Theory with InlineData for both SSL and non-SSL cases - Consolidated 4 separate test methods into 2 Theory tests covering both SSL enabled/disabled scenarios - Each Theory runs twice: once with useSsl=false (baseline) and once with useSsl=true (regression test) - This allows comparing behavior between SSL and non-SSL to identify SSL-specific issues Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
Done in commit 4158564:
The tests now cover both scenarios in each test method, making it easier to see if the disposal issue is specific to TLS or affects plain FTP as well. |
|
it still shows merge conflict. Can you revert the changes to enterprise setup completely @copilot? |
- Reverted all enterprise setup container files to origin/main state - Added THIRD-PARTY-NOTICES file for mod_auth_ntlm_winbind from main - Container setup (ProFTPD, apache2.conf, Dockerfile, run.sh) now matches main branch - This resolves merge conflicts by using the container changes that were merged separately Co-authored-by: wfurt <14356188+wfurt@users.noreply.github.com>
Done in commit c545c2c. Reverted all enterprise setup files to origin/main state. The PR now only contains:
All container setup changes (ProFTPD, Dockerfile, run.sh, etc.) now match main branch, resolving the merge conflict. |
FTP/SSL Upload and Download Regression Fix for .NET 10
Problem
Root Cause
The FTP data stream disposal was not properly handling SSL/TLS shutdown before closing the underlying network connection, causing protocol violations when using explicit FTPS.
Solution
Modified
FtpDataStream.csto properly handle SSL stream closure:_stream != _originalStream), properly closes the SslStream to perform SSL shutdownChanges Made
Code Fix:
FtpDataStream.cs: ModifiedCloseExmethod to properly handle both SSL and non-SSL stream closure_stream != _originalStream(SSL case)Enterprise Test Infrastructure:
System.Net.Requests.Enterprise.TestsprojectFtpUpload_StreamDisposal(bool useSsl)- Tests upload stream disposal with SSL=false and SSL=trueFtpDownload_StreamDisposal(bool useSsl)- Tests download stream disposal with SSL=false and SSL=trueServicePointManager.ServerCertificateValidationCallbackto accept self-signed certificatesCI Pipeline Integration:
System.Net.Requeststo enterprise pipeline path triggers ineng/pipelines/libraries/enterprise/linux.ymlSystem.Net.Requests.Enterprise.TestsNote: Container setup (ProFTPD server, apache2 config, etc.) was merged to main branch separately and is not included in this PR.
Testing
Enterprise tests run automatically in CI against the ProFTPD server (configured in main branch), or can be run manually:
Fixes #123135
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.