Skip to content

NegotiateAuthentication: Implement additional API surface #71777

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 11, 2022
Merged

NegotiateAuthentication: Implement additional API surface #71777

merged 2 commits into from
Jul 11, 2022

Conversation

@filipnavara
Copy link
Member

@filipnavara filipnavara commented Jul 7, 2022 •
edited
Loading

  • Implemented NegotiateAuthentication.Wrap/Unwrap/UnwrapInPlace APIs
  • Updated unit tests
  • Migrated System.Net.Mail to use NegotiateAuthentication API
  • Implementation of extended protection policy and impersonation APIs (no uses yet)

Best reviewed commit by commit.

Fixes #70909

@ghost ghost added area-System.Net new-api-needs-documentation community-contribution Indicates that the PR has been added by a community member labels Jul 7, 2022
@ghost
Copy link

ghost commented Jul 7, 2022

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@filipnavara filipnavara requested a review from wfurt July 7, 2022 18:28
@ghost
Copy link

ghost commented Jul 7, 2022

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

Issue Details

Updated unit tests
Migrate System.Net.Mail to use NegotiateAuthentication API

Contributes to #70909

Author: filipnavara
Assignees: -
Labels:

area-System.Net, new-api-needs-documentation, community-contribution
Milestone: -

@filipnavara filipnavara changed the title Implement NegotiateAuthentication.Wrap/Unwrap/UnwrapInPlace APIs NegotiateAuthentication: Implement additional API surface Jul 7, 2022
@filipnavara filipnavara marked this pull request as ready for review July 7, 2022 20:11
@filipnavara filipnavara force-pushed the negotiate-wrap-api2 branch from b2ca130 to d6f8157 Compare July 7, 2022 20:47
@filipnavara
Implement NegotiateAuthentication.Wrap/Unwrap/UnwrapInPlace APIs
e3260f2 
Updated unit tests
Migrate System.Net.Mail to use NegotiateAuthentication API
@filipnavara
Implementation of extended protection policy and impersonation in Neg…
fed4d67 
…otiateAuthentication
@filipnavara filipnavara force-pushed the negotiate-wrap-api2 branch from d6f8157 to fed4d67 Compare July 11, 2022 07:42
@filipnavara
Copy link
Member Author

filipnavara commented Jul 11, 2022

Rebased to resolve conflicts.

wfurt
wfurt reviewed Jul 11, 2022
View reviewed changes
src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpNegotiateAuthenticationModule.cs
// is negotiated.
if (OperatingSystem.IsLinux())
{
protectionLevel = ProtectionLevel.EncryptAndSign;
Copy link
Member

@wfurt wfurt Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is there harm of doing it also on Windows/macOS?

Copy link
Member Author

@filipnavara filipnavara Jul 11, 2022

Choose a reason for hiding this comment

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

(FWIW: this is not a new check in this PR, it is just re-indented existing one)

In theory it would be fine on all systems but I don't have sufficient data to confirm. The negotiated protocol can end up being either Kerberos or NTLM. If it's Kerberos on Linux/macOS we get forced confidentiality anyway. For all other scenarios it could result in unilaterally bumped security requirements from the client side. If the server is Exchange then it would work. If the server is something running on an embedded device with Cyrus SASL backend I would not be so sure that everything would still work (notably the NTLM implementation in Cyrus is not even interoperable with Windows 11 in default configuration).

Copy link
Member Author

@filipnavara filipnavara Jul 11, 2022

Choose a reason for hiding this comment

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

Short version: It's mostly just being cautious.

Copy link
Member

@wfurt wfurt Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That is fine. I was nice to see less platform specific code through this PR and if we could avoid one more platform check I would be happy. But even with it it seems like great improvement.

wfurt
wfurt approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.
thanks for big cleanup in SMTP @filipnavara

@wfurt wfurt merged commit 26da83d into dotnet:main Jul 11, 2022
@filipnavara
Copy link
Member Author

filipnavara commented Jul 11, 2022

Thanks for review! I'm very happy that we managed to get the API in at the last minute :-)

@filipnavara filipnavara deleted the negotiate-wrap-api2 branch July 11, 2022 11:14
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2022
@bartonjs bartonjs added cryptographic-docs-impact needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurt wfurt wfurt approved these changes

Assignees

No one assigned

Labels

area-System.Net community-contribution Indicates that the PR has been added by a community member needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration new-api-needs-documentation

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add Wrap and Unwrap methods to NegotiateAuthentication API

4 participants

@filipnavara @wfurt @karelz @bartonjs