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.

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

Added missing *Async overrides to TlsStream #91750

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Sep 7, 2023

#82644 replaced custom IAsyncResult with call to ReadAsync on BufferedReadStream. But TlsStream doesn't have ReadAsync and the call went to the base class NetworkStream.ReadAsync effectively bypassing SslStream.Decrypt of the data.

I added Read/WriteAsync overrides to call into _sslStream as is expected.

Note that we do not have any coverage for SSL enabled SMTP. So in order to add tests for this, I'll need add support for SSL into LoopbackSmtpServer. @karelz this might take some time, so let me know, if you want to move forward with this fix with just local validation and do the test afterwards.

Fixes #91377

@ghost
Copy link

ghost commented Sep 7, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

#82644 replaced custom IAsyncResult with call to ReadAsync on BufferedReadStream. But TlsStream doesn't have ReadAsync and the call went to the base class NetworkStream.ReadAsync effectively bypassing SslStream.Decrypt of the data.

I added Read/WriteAsync overrides to call into _sslStream as is expected.

Note that we do not have any coverage for SSL enabled SMTP. So in order to add tests for this, I'll need add support for SSL into LoopbackSmtpServer. @karelz this might take some time, so let me know, if you want to move forward with this fix with just local validation and do the test afterwards.

Fixes #91377

Author: ManickaP
Assignees: -
Labels:

area-System.Net

Milestone: -

@ManickaP ManickaP requested a review from a team September 7, 2023 18:05
@stephentoub
Copy link
Member

Ouch, sorry about that. Thanks for tracking it down.

@ManickaP
Copy link
Member Author

ManickaP commented Sep 8, 2023

We agreed with @karelz that we're ok with manual validation for this PR and to file an issue for the tests (#91776).

@karelz karelz added this to the 9.0.0 milestone Sep 8, 2023
@karelz
Copy link
Member

karelz commented Sep 8, 2023

Agreed, let's push the fix into 8.0 with manual validation. And let's file a tracking issue to add the automated tests during 9.0 - we can then prioritize it appropriately against other work items we will have based on cost, value and risk we impose if we do not have the automation.

@ManickaP
Copy link
Member Author

ManickaP commented Sep 8, 2023

Build error is #91705

@stephentoub
Copy link
Member

/backport to release/8.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6122263567

@stephentoub stephentoub merged commit 5a6d5ef into dotnet:main Sep 8, 2023
103 of 105 checks passed
@ManickaP ManickaP deleted the mapichov/smtp-fix branch September 8, 2023 14:14
@wfurt
Copy link
Member

wfurt commented Sep 11, 2023

this looks like major test gap. We should look into it IMHO. I know @filipnavara had more insight to additional test gaps.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net 8 Preview sending email exception
4 participants