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

Unable to make https request when Oid lookup takes too long #21320

Merged
merged 8 commits into from Jul 4, 2017

Conversation

@primarilysoftware
Copy link
Contributor

commented Jun 21, 2017

This issue was discovered in .NET 4.6, but would also affect .NET core apps running on windows.

Here is a link to the feedback I created on the connect website for this issue:
https://connect.microsoft.com/VisualStudio/feedback/details/3136313

The solution is to use a different Oid constructor that prevents the potentially costly Oid friendly name lookup. What follows is a more detailed description of how issue we were seeing in our application.

I support a desktop application that calls a WCF service hosted on our servers over https. A small number of our clients with the desktop application installed (~20/10000) have been reporting issues connecting to the WCF service since installing KB4014511. The exception being thrown is:

System.IO.IOException: Unable to write data to the transport connection: An existing connection was forcibly closed by the remote host. ---> System.Net.Sockets.SocketException: An existing connection was forcibly closed by the remote host

The error is misleading though. After quite a lot of troubleshooting, we were able to pinpoint the source of the problem. By decompiling the System.dll, I saw that the following 2 lines were added to the System.Net.Security.SecureChannel class shipped with KB4014511:

private readonly Oid m_ServerAuthOid = new Oid("1.3.6.1.5.5.7.3.1");
private readonly Oid m_ClientAuthOid = new Oid("1.3.6.1.5.5.7.3.2");

The initialization of those Oid objects is triggering a system call to the CryptFindOIDInfo function to lookup the friendly name associated with the specified OID. When the machine is part of a domain, that call will perform the lookup against active directory (see the Remarks in the docs https://msdn.microsoft.com/en-us/library/windows/desktop/aa379938%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396).

For our clients that are having issues connecting to our WCF service, this lookup is taking up to 15 seconds. Because these Oid lookups are happening after WCF has opened the TCP connection to our server, but before the SSL/TLS handshake has started, we are seeing the above error. So, the TCP connection is opened, 2 OID lookups happen taking up to 30 seconds, the application then attempts to start the SSL/TLS handshake, but by the time the SSL/TLS handshake is attempted, the TCP connection has already been reset by the server (in our case the server resets the connection after 10 seconds).

Using alternative Oid constructor
This alternative contructor will avoid trigging a potentially costly
lookup for the friendly name associated with the OID.
private readonly Oid _serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1");
private readonly Oid _clientAuthOid = new Oid("1.3.6.1.5.5.7.3.2");
private readonly Oid _serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1", "Server Authentication");
private readonly Oid _clientAuthOid = new Oid("1.3.6.1.5.5.7.3.2", "Client Authentication");

This comment has been minimized.

Copy link
@bartonjs

bartonjs Jun 21, 2017

Member

These OIDs aren't in https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs#L110, which means that Windows localizes the string. So if the Oid.FriendlyName can ever be exposed you'll be enforcing English.

Passing null would be valid for most contexts, as long as it's not being shared out. (And if it's not being shared out, it could be static readonly...)

const OidGroup CRYPT_OID_DISABLE_SEARCH_DS_FLAG = (OidGroup)(unchecked((int)0x80000000));
const OidGroup LocalEku = CRYPT_OID_DISABLE_SEARCH_DS_FLAG | OidGroup.EnhancedKeyUsage);
_clientAuthInfo = Oid.FromOidValue("1.3.6.1.5.5.7.3.2", LocalEku);

Probably works.

cinjguy added some commits Jun 22, 2017

Alternative way to lookup OID
This method avoids the AD lookup, while also not making any localization
assumptions
fix typo
had the wrong OID for Server Auth
@primarilysoftware

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2017

I implemented the change suggested by @bartonjs.

I was able to verify that doing the lookup in this way worked as expected on one of the machines where we were seeing issues.

@davidsh davidsh removed their assignment Jun 22, 2017

@Priya91

This comment has been minimized.

Copy link
Member

commented Jun 23, 2017

@CIPop Please take a look, the oid values are being changed. Thanks!

UPDATE: The value change was fixed as typo :)

@bartonjs
Copy link
Member

left a comment

LGTM, but someone from networking should also agree to take it.

@davidsh

This comment has been minimized.

cinjguy added some commits Jun 27, 2017

Optional parameter on FromOidValue to disable AD search
New optional parameter on FromOidValue to disable lookup, update server
auth OID lookup in WinHttpCertificateHelper and SecureChannel to use
this new parameter when doing lookup
@@ -55,10 +57,13 @@ public static Oid FromFriendlyName(String friendlyName, OidGroup group)
return new Oid(oidValue, friendlyName, group);
}

public static Oid FromOidValue(String oidValue, OidGroup group)
public static Oid FromOidValue(String oidValue, OidGroup group, bool disableActiveDirectorySearch = false)

This comment has been minimized.

Copy link
@davidsh

davidsh Jun 27, 2017

Member

This particular change (adding new parameter with a default value) changes the public API surface of System.Security.Cryptography.Encoding. We can't do that kind of change in a PR like this.

Please remove this change from the PR.

cinjguy added some commits Jun 27, 2017

Limit Server Auth OID lookup to local machine
Same change as what was made to SecureChannel class to avoid OID lookup
via AD
@davidsh

This comment has been minimized.

Copy link
Member

commented Jun 29, 2017

@primarilysoftware Thank you for your work on this PR and also for your analysis of the problem itself. We would like to get this PR completed as soon as possible. We are also investigating the same issues on .NET Framework that you reported via Connect portal.

After several discussion with the crypto API experts @bartonjs, we have concluded that it would be best to further simplify this PR. Instead of using the CRYPT_OID_DISABLE_SEARCH_DS_FLAG etc., please just use the two string parameter new Oid() constructor as follows:

private readonly Oid _serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1", "1.3.6.1.5.5.7.3.1");
private readonly Oid _clientAuthOid = new Oid("1.3.6.1.5.5.7.3.2", "1.3.6.1.5.5.7.3.2");

Using the dotted number notation for the friendly name should avoid any Oid lookups and also not require any localization with the name.

Can you please revise your PR? And if possible, please test out the changes in your environment as well to assist overall test coverage of this change. Thanks!

Use alternative OID contructor
Use OID as friendly name to avoid lookup as well as localization issues
@primarilysoftware

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

@davidsh I made the change you suggested. I will try and test this out on one of our client's machines where this issue was reported. That may take some time though.

@davidsh

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

@davidsh I made the change you suggested. I will try and test this out on one of our client's machines where this issue was reported. That may take some time though.

Thanks for making the changes to the PR. At this point, we are going to merge this PR. Please continue your testing on your client's machines.

@davidsh

davidsh approved these changes Jul 4, 2017

@davidsh davidsh merged commit 81f80a8 into dotnet:master Jul 4, 2017

11 checks passed

CROSS Check Build finished.
Details
Linux arm Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
Tizen armel Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
Windows x64 Debug Build Build finished.
Details
Windows x86 Release Build Build finished.
Details
@primarilysoftware

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

@davidsh I was able to test out the latest version of this fix on a couple client's machines today, and it did work as expected.

Any idea when this will included in a .NET release or patch?

@davidsh

This comment has been minimized.

Copy link
Member

commented Jul 6, 2017

Any idea when this will included in a .NET release or patch?

@primarilysoftware I assume by ".NET release or patch", you are referring to .NET Framework and not .NET Core? We are actively looking into this to determine the earliest possible time to issue such a servicing release.

Historically, .NET Framework releases servicing fixes approximately monthly. But there is a lead time to getting a fix into any release train. We will communicate schedule expectations via the Connect portal and not this PR. This PR is now merged into master branch and is considered closed.

Please follow the Connect portal bug you opened up about .NET Framework for status updates on servicing releases. Thanks.

cc: @DavidGoll @karelz

davidsh added a commit that referenced this pull request Jul 7, 2017

[release/2.0] Unable to make https request when Oid lookup takes too …
…long (#21320) (#21964)

* Unable to make https request when Oid lookup takes too long (#21320)

* Using alternative Oid constructor

This alternative contructor will avoid trigging a potentially costly
lookup for the friendly name associated with the OID.

* Alternative way to lookup OID

This method avoids the AD lookup, while also not making any localization
assumptions

* fix typo

had the wrong OID for Server Auth

* Optional parameter on FromOidValue to disable AD search

New optional parameter on FromOidValue to disable lookup, update server
auth OID lookup in WinHttpCertificateHelper and SecureChannel to use
this new parameter when doing lookup

* Revert "Optional parameter on FromOidValue to disable AD search"

This reverts commit 6d0fd6d.

* Limit Server Auth OID lookup to local machine

Same change as what was made to SecureChannel class to avoid OID lookup
via AD

* Use alternative OID contructor

Use OID as friendly name to avoid lookup as well as localization issues

* Change tabs to spaces

davidsh added a commit that referenced this pull request Jul 7, 2017

Merge pull request #21989 from davidsh/port_10_21964
[release/1.0] Unable to make https request when Oid lookup takes too long (#21320)

davidsh added a commit that referenced this pull request Jul 7, 2017

Merge pull request #21987 from davidsh/port_11_21964
[release/1.1] Unable to make https request when Oid lookup takes too long (#21320)

@primarilysoftware primarilysoftware deleted the primarilysoftware:sslissue branch Aug 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.