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.

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

Implement part of SslCertificateTrust #55104

Merged
merged 16 commits into from
Jul 12, 2021
Merged

Implement part of SslCertificateTrust #55104

merged 16 commits into from
Jul 12, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 2, 2021

related to #54219
This adds approved API and uses provided certificate for managing trust.
I assume we don't need anything beyond chain.ChainPolicy.TrustMode, right @bartonjs?

This also adds support for sending Distinguished Names on Windows.
The caveat is that it depends on registry setting as well as very new Windows builds.
So testing is problematic and I did some manual preliminary test.

@wfurt wfurt requested review from bartonjs and a team July 2, 2021 22:11
@wfurt wfurt self-assigned this Jul 2, 2021
@ghost
Copy link

ghost commented Jul 2, 2021

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

Issue Details

related to #54219
This adds approved API and uses provided certificate for managing trust.
I assume we don't need anything beyond chain.ChainPolicy.TrustMode, right @bartonjs?

This also adds support for sending Distinguished Names on Windows.
The caveat is that it depends on registry setting as well as very new Windows builds.
So testing is problematic and I did some manual preliminary test.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security
Milestone: -

@dotnet-issue-labeler
Copy link

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.

@wfurt
Copy link
Member Author

wfurt commented Jul 5, 2021

feedback addressed, CI is clean.

geoffkizer
geoffkizer reviewed Jul 8, 2021
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

if (errorCode != Interop.SECURITY_STATUS.OK)
{
throw new Win32Exception((int)errorCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be throwing something other than Win32Exception here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. I look at the code around Acquire Credentials and it seems like that what we throw. I can check as we may perhaps wrap it.

@geoffkizer
Copy link
Contributor

Tests?

@wfurt
Copy link
Member Author

wfurt commented Jul 8, 2021

Tests?

As I mentioned tests are problematic on Windows as the handshake depends on registry setting and very recent Windows changes. I'll try to figure out something but for now I would like to defer it. More changes are still coming for #54219.

ManickaP
ManickaP approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM, small comments. Please do get another approval, I'm not an expert here, barely noob.

src/libraries/System.Net.Security/src/System.Net.Security.csproj Show resolved Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslCertificateTrust.cs Show resolved Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslCertificateTrust.cs Show resolved Hide resolved
@wfurt
Copy link
Member Author

wfurt commented Jul 12, 2021

mono build failures are independent.

@wfurt wfurt merged commit cd4df7d into dotnet:main Jul 12, 2021
@wfurt wfurt deleted the certStore branch July 12, 2021 19:57
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
This was referenced Jul 16, 2021
Closed
Closed
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@bartonjs bartonjs bartonjs left review comments

@geoffkizer geoffkizer geoffkizer left review comments

@ManickaP ManickaP ManickaP approved these changes

Assignees

@wfurt wfurt

Labels
area-System.Net.Security new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wfurt @geoffkizer @jkotas @bartonjs @ManickaP @karelz