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

Add basic fake NTLM server to test NTAuthentication round-trip scenarios #65611

Merged
merged 12 commits into from
May 7, 2022

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Feb 19, 2022

Add basic low-level NTLM exchange test. This is prerequisite for #29270, #62264 and test for the fixed scenario in #54101. It can also serve as basis for future tests for specific issues.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2022
@ghost
Copy link

ghost commented Feb 19, 2022

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

Issue Details

Ignore, just want to see CI results across all the system configurations.

Author: filipnavara
Assignees: -
Labels:

area-System.Net.Security, community-contribution

Milestone: -

@filipnavara filipnavara marked this pull request as ready for review February 19, 2022 16:58
@filipnavara filipnavara marked this pull request as draft February 19, 2022 17:18
@filipnavara filipnavara reopened this Feb 19, 2022
@filipnavara filipnavara marked this pull request as ready for review February 20, 2022 06:49
@filipnavara filipnavara changed the title Add basic NTLM fake server to test NTAuthentication round-trip scenarios Add basic fake NTLM server to test NTAuthentication round-trip scenarios Feb 20, 2022
@wfurt
Copy link
Member

wfurt commented Feb 24, 2022

I'm not sure if we really should go to the weeds of understanding NTLM internal in the tests.
I would much rather expand "echo server" e.g. corefx-net-http11.azurewebsites.net to provide reference NTLM server and run functional tests agains it.

cc: @rzikm @pavelsavara

@filipnavara
Copy link
Member Author

I'm not sure if we really should go to the weeds of understanding NTLM internal in the tests.

I tried to avoid it for as long as I could but I snapped in the end. There were too many issues not covered by the tests and that could not even easily be covered by the tests.

I would much rather expand "echo server" e.g. corefx-net-http11.azurewebsites.net to provide reference NTLM server and run functional tests agains it.

If you are willing to host a test infrastructure I think that may provide a solid middle ground. However, the issue is not just HTTP. The SMTP GSSAPI authentication is currently broken on anything but Windows and there's no test coverage at all. NegotiateStream doesn't work on macOS and is not interoperable between systems (except maybe Linux and Windows if stars align, or if Kerberos is used).

We, at @emclient, have been hosting our own testing infrastructure for quite a while now and it's not easy. The Exchange servers bit rot constantly, the Windows Server 2019 machine on Azure needs to be restarted daily (seems to be a VM size issue and resource depletion).

I would be fine writing a simple echo SMTP server to cover the scenario above but I would need a commitment on hosting it and supporting it to be hammered by the .NET test infrastructure. Is that even something reasonable to ask?

@wfurt
Copy link
Member

wfurt commented Feb 24, 2022

I would be fine writing a simple echo SMTP server to cover the scenario above but I would need a commitment on hosting it and supporting it to be hammered by the .NET test infrastructure. Is that even something reasonable to ask?

probably. It would finally close #19436 after 5+ years of waiting ;(
Perhaps off-line chat with me and @karelz would be beneficial.

@filipnavara
Copy link
Member Author

Perhaps off-line chat with me and @karelz would be beneficial.

Definitely up for it. You know how to reach me.

@jstedfast
Copy link
Member

Would love to know the conclusion of these discussions and whether MailKit would be able to take advantage of this.

@wfurt
Copy link
Member

wfurt commented May 4, 2022

Sorry for the delay @filipnava. Is this something you still want to push forward? It seems like RC4 was part of #66879 so if we need it we should take it from there.
I think we can take it even if various OS bugs will make it fragile IMHO.

@filipnavara
Copy link
Member Author

Is this something you still want to push forward?

Yes. I will rebase it and update the RC4 reference.

@filipnavara
Copy link
Member Author

Yes. I will rebase it and update the RC4 reference.

Done. The remaining test failure is unrelated (#13757).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thanks @filipnavara

@wfurt wfurt merged commit 6e49909 into dotnet:main May 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 6, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants