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

SslStream.AuthenticateAsServer round trips to domain controller to map client certificate to user when domain joined #78350

Closed
mconnew opened this issue Nov 14, 2022 · 11 comments · Fixed by #80886
Labels
Milestone

Comments

@mconnew
Copy link
Member

mconnew commented Nov 14, 2022

Description

This only applies to a domain joined Windows host authenticating a client certificate (it might do the same for the server cert on the client side, but that issue hasn't been reported to me by a WCF customer).

When using client certificates, the default flags used with SChannel result in an attempt to map the client provided X509 Certificate to a domain user. This requires a round trip call to the domain controller to do so. If the local domain controller goes down (eg for maintenance) and the work of the DC shifts to another site temporarily, there is a period of time where a server either can't contact a DC or there's a large delay while it fails over to another DC causing a pause in SSL connections being established.

To be able to retrieve any potentially mapped user from the certificate, the security context handle needs to be exposed from SslStream, and this isn't the case. There's no mechanism to even use a potentially mapped user certificate if that was the case.

The flag to set when calling AquireCredentialsHandle is SCH_CRED_NO_SYSTEM_MAPPER. I had a look at the latest source code and it looks like this flag is passed in some scenarios, and that's when you are wanting to send additional certificates to a remote host if the remote host doesn't have access to some intermediate certs. This is done by creating a SslCertificateTrust by passing sendTrustInHandshake = true to the Create method. This is passed via SslStreamCertificateContext on SslServerAuthenticationOptions.

Configuring SslStream to send the trust chain in the handshake shouldn't be required (and makes the handshake larger and slower) to set the SCH_CRED_NO_SYSTEM_MAPPER flag. This flag should always be set to avoid the roundtrip to a domain controller.

Configuration

Running on a Windows domain joined machine authenticating clients using SslStream

Regression?

No, this issue exists on .NET Framework and all .NET Core/.NET releases

Data

The reporting customer was seeing an additional 3ms delay in SSL handshakes when the service host was domain joined. This is going to be dependent on network conditions and server load as there's an additional round trip to the domain controller, so the specific numbers aren't super helpful.

When communication is disrupted to the domain controller, there's about a 60 second delay before new connections can begin to be established again. This delay is the host failing over to a different domain controller.

Analysis

See description.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 14, 2022
@ghost
Copy link

ghost commented Nov 14, 2022

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

Issue Details

Description

This only applies to a domain joined Windows host authenticating a client certificate (it might do the same for the server cert on the client side, but that issue hasn't been reported to me by a WCF customer).

When using client certificates, the default flags used with SChannel result in an attempt to map the client provided X509 Certificate to a domain user. This requires a round trip call to the domain controller to do so. If the local domain controller goes down (eg for maintenance) and the work of the DC shifts to another site temporarily, there is a period of time where a server either can't contact a DC or there's a large delay while it fails over to another DC causing a pause in SSL connections being established.

To be able to retrieve any potentially mapped user from the certificate, the security context handle needs to be exposed from SslStream, and this isn't the case. There's no mechanism to even use a potentially mapped user certificate if that was the case.

The flag to set when calling AquireCredentialsHandle is SCH_CRED_NO_SYSTEM_MAPPER. I had a look at the latest source code and it looks like this flag is passed in some scenarios, and that's when you are wanting to send additional certificates to a remote host if the remote host doesn't have access to some intermediate certs. This is done by creating a SslCertificateTrust by passing sendTrustInHandshake = true to the Create method. This is passed via SslStreamCertificateContext on SslServerAuthenticationOptions.

Configuring SslStream to send the trust chain in the handshake shouldn't be required (and makes the handshake larger and slower) to set the SCH_CRED_NO_SYSTEM_MAPPER flag. This flag should always be set to avoid the roundtrip to a domain controller.

Configuration

Running on a Windows domain joined machine authenticating clients using SslStream

Regression?

No, this issue exists on .NET Framework and all .NET Core/.NET releases

Data

The reporting customer was seeing an additional 3ms delay in SSL handshakes when the service host was domain joined. This is going to be dependent on network conditions and server load as there's an additional round trip to the domain controller, so the specific numbers aren't super helpful.

When communication is disrupted to the domain controller, there's about a 60 second delay before new connections can begin to be established again. This delay is the host failing over to a different domain controller.

Analysis

See description.

Author: mconnew
Assignees: -
Labels:

area-System.Net.Security, tenet-performance

Milestone: -

@wfurt
Copy link
Member

wfurt commented Nov 14, 2022

This was done in #60988 to explicitly support particular deployment. It was not clear if this could break somebody so the scope was limited to newly added Trust. The perf implication was not clear either.

@rzikm
Copy link
Member

rzikm commented Nov 15, 2022

Triage: looks like the fix could be simple, we should take a closer look in 8.0 and decide if it is the way we want to proceed (such change could break users).

@rzikm rzikm removed the untriaged New issue has not been triaged by the area owner label Nov 15, 2022
@rzikm rzikm added this to the 8.0.0 milestone Nov 15, 2022
@wfurt
Copy link
Member

wfurt commented Jan 18, 2023

Would this be something that can break Kestrel @davidfowl @Tratcher ?
While I see it useful for something IIS to get users identity I don't see how that could be obtained from SslStream e.g. it make be just useless baggage.

@Tratcher
Copy link
Member

Tratcher commented Jan 18, 2023

This could be a good optional feature to enable.

  • Disable by default, don't contact the DC. (Does this cause any difference in client cert validation errors if there's a mismatch?)
  • Enabled: Expose the mapped user on the SslStream. People moving from IIS have asked for this.

dotnet/aspnetcore#41285

@wfurt
Copy link
Member

wfurt commented Jan 18, 2023

OK. It seems like this will need some research and perhaps API changes. It seems like this would Windows specific feature. I don't know how this could be done in general way.

@mconnew
Copy link
Member Author

mconnew commented Jan 19, 2023

If there's currently no way to get the mapped user, this should be disabled. I see re-enabling it once an API exists to expose the mapped user as a separate issue. The first is a performance/reliability issue (had real customer have 1 minute service downtime due to domain controller failovers) and had measurable higher handshake latency. The second is a feature.
My concern is lack of schedule to implement the feature would result in the performance and reliability issue being delayed too.

@wfurt
Copy link
Member

wfurt commented Jan 19, 2023

I was actually thinking about the same @mconnew. I'm happy to do it if @Tratcher agrees. Disable the flag now and open separate issue for future.

@Tratcher
Copy link
Member

That's fine, so long as we can clarify if it causes any verification changes?

@mconnew
Copy link
Member Author

mconnew commented Jan 19, 2023

My understanding is that it makes zero difference to validating the remote certificate. The chain build is done as a separate task. All this flag does is send the certificate to the domain controller which checks if it's mapped to a domain user account, and if it is, Windows makes the security context for that user account obtainable via other api calls. This would ultimately enable you to retrieve a WindowsIdentity instance for that user account, and depending on app permissions, potentially impersonate as that user.

For the second task of an API surface for exposing the remote user, I think NegotiateStream is a good model. It has a RemoteIdentity property which returns the identity of the remote user. On Windows you get a WindowsIdentity object, and on Linux/Mac, you get a GenericIdentity object. The property is defined as an IIdentity so isn't Windows specific. Having this property null if the feature is disabled or the certificate didn't map seems like a reasonable approach.

@wfurt
Copy link
Member

wfurt commented Jan 19, 2023

I did little bit more digging, here is the doc: https://learn.microsoft.com/en-us/windows/win32/secauthn/mapping-certificates
One would need to call QuerySecurityContextToken to get the user's identity and/or impersonate that user. The PCtxtHandle is internal property of SslStream so current users should not have access to it.

I also check with Schannel team and they see it as legacy functionality they are trying to obsolete. And they recommend not to use it on busy server. I agree with both as TLS seems wrong layer to solve this. And it is not portable either. So at this point I'm not even sure it make sense to open feature request. It make sense IMHO to make this change now to have time to discover if I was wrong.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants