Skip to content

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 17, 2021

Backport of #60988 to release/6.0

Fixes #60949

/cc @wfurt

Customer Impact

This impacts ability to send custom trust list in TLS handshake. This feature was requested by AAD Gateway and it is new in 6.0. By default, SslStream/Schannel sends extra synthetic entry in the CA list. Would be generally fine, but it breaks use case when AAD Gateway tries to send empty list. Since there is extra entry, TLS Handshake fails with .NET clients.

Testing

This was tested manually and customer (AAD Gateway) verified with 7.0 daily build.

Risk

Small. The change is scoped to disable the option only when the new API is in use.

cc: @avparuch

@ghost
Copy link

ghost commented Nov 17, 2021

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

Issue Details

Backport of #60988 to release/6.0

/cc @wfurt

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@wfurt wfurt added this to the 6.0.x milestone Nov 17, 2021
@wfurt wfurt added the Servicing-consider Issue for next servicing release review label Nov 17, 2021
@karelz karelz requested a review from scalablecory November 30, 2021 17:25
@danmoseley
Copy link
Member

@wfurt @karelz the issue would meet the bar for servicing, and we see that AAD verified, but tactics want to feel confident that this change won't in turn break anyone else. how confident do you feel about that?

@wfurt
Copy link
Member

wfurt commented Dec 2, 2021

The actual change is scoped to

if (certificateContext?.Trust?._sendTrustInHandshake == true)
    flags |= Interop.SspiCli.SCH_CREDENTIALS.Flags.SCH_CRED_NO_SYSTEM_MAPPER;

That is new API added in 6.0 and AAD is only one user at the moment AFAIK. Impact on existing users would be possible only if there is some new bug in the hashing that comes with this change. (to treat the context with trust as different (separate small bug))

@wfurt
Copy link
Member

wfurt commented Dec 2, 2021

One more note that when we implement this on Linux/macOS, there will not be any "NT Authority". So this also makes it more consistent across (future) platforms)
On Windows this allows to map client certificate to user but we don't have way how to expose this in SslStream or HttpClient.

@safern safern added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 2, 2021
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 3, 2021
@danmoseley
Copy link
Member

Approved offline by Steve

@karelz
Copy link
Member

karelz commented Dec 10, 2021

@safern just curious why is it marked as NO MERGE?

@safern
Copy link
Member

safern commented Dec 10, 2021

@safern just curious why is it marked as NO MERGE?

It is because the branch is closed, so we can't merge servicing fixes to the branch yet. We will be able to merge it next week when we open the branch.

@karelz
Copy link
Member

karelz commented Dec 15, 2021

Thanks @safern. I was just surprised that it is used for external reasons. I would naively expect that the label is property of the PR -- i.e. it is not approved yet, or is not ready. I would expect readiness of branch to be tracked externally as it is about timing. Also, using it this way means all servicing PRs should be labeled this way (which they are not), which would be probably not very useful - it is basically global boolean.
Anyway ... I can live with it ;), just trying to explain my confusion.

@safern
Copy link
Member

safern commented Dec 15, 2021

No worries. Thanks for explaining the confusion, I'll try to be consistent either way for next servicing release.

@safern safern removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 15, 2021
@safern safern merged commit fb56c4c into release/6.0 Dec 15, 2021
@safern safern deleted the backport/pr-60988-to-release/6.0 branch December 15, 2021 18:29
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants