Explicit usage of TLS 1.2 via ServicePointManager

[intuit]

Explicit usage of TLS 1.2 via ServicePointManager

[nimisha84]

https://stackoverflow.com/questions/3028486/how-to-use-ssl3-instead-of-tls-in-a-particular-httpwebrequest?rq=1- Creating separate appdomain is another option

[nimisha84]

@AndreySemykin - Please check the latest 5.7.1 build of the SDK and test it out. Thanks to Matt for all his help.

[nimisha84]

Point taken, Thanks for the feedback.
I will try to make a release soon to make this configurable but keep the default value as Tls12. This way you can override the SDK settings if your code is not making calls to other services? This way if you are using .Net 4.5 or higher, you can totally skip it to say false and then default TLS12 calls would be made.

Changing the underlying implementation to use the way you have suggested would be a really big change in the internal wiring of the SDK. Let me know what you think about having this configurable setting? Just wondering, can you not override the setting for using lower tls versions when making the other services-> rest api calls?

[StasPerekrestov]

I believe the main point of the issue is that setting ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12 property is a destructive operation. It sets a particular security protocol globally (per a app domain). It may cause communication issues that are nasty to debug.
To my mind, @AndreySemykin makes a good point. It's better to pass a required protocol version by passing it to HttpClientHandler API.

PS: In asp.net core 2.0+ it's better to use IHttpClientFactory/ConfigurePrimaryHttpMessageHandler
see for details aspnet/HttpClientFactory#71

[MatthewSteeples]

It might bet better to set it to the following

ServicePointManager.SecurityProtocol |= SecurityProtocolType.Tls12;

The above code will enable TLS1.2 without changing which other protocols are supported

(This code has been modified from ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls | SecurityProtocolType.Tls11 | SecurityProtocolType.Tls12;)

[AndreySemykin]

If we test QuickBooks URL via curl it gives the following response
that indicates that the intuit api service uses TLS 1.2
@nimisha84 please remove the ervicePointManager.SecurityProtocol = SecurityProtocolType.Tls12 as it is useless, because the appropriate security protocol will be selected by default.

Proxy replied 200 to CONNECT request

• CONNECT phase completed!
• ALPN, offering h2
• ALPN, offering http/1.1
• successfully set certificate verify locations:
• CAfile: C:\Program Files\curl-7.62.0-win64-mingw\bin\curl-ca-bundle.crt
  CApath: none
• TLSv1.3 (OUT), TLS handshake, Client hello (1):
• CONNECT phase completed!
• CONNECT phase completed!
• TLSv1.3 (IN), TLS handshake, Server hello (2):
• TLSv1.2 (IN), TLS handshake, Certificate (11):
• TLSv1.2 (IN), TLS handshake, Server key exchange (12):
• TLSv1.2 (IN), TLS handshake, Server finished (14):
• TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
• TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
• TLSv1.2 (OUT), TLS handshake, Finished (20):
• TLSv1.2 (IN), TLS handshake, Finished (20):
• SSL connection using TLSv1.2 / ECDHE-RSA-AES256-GCM-SHA384
• ALPN, server did not agree to a protocol
• Server certificate:
• subject: C=US; ST=California; L=San Diego; O=INTUIT INC.; OU=CTO-DEV-ICS Ops; CN=*.api.intuit.com
• start date: Aug 14 00:00:00 2018 GMT
• expire date: Aug 24 12:00:00 2019 GMT
• subjectAltName: host "quickbooks.api.intuit.com" matched cert's "*.api.intuit.com"
• issuer: C=US; O=DigiCert Inc; CN=DigiCert SHA2 Secure Server CA
• SSL certificate verify ok.

OPTIONS / HTTP/1.1
Host: quickbooks.api.intuit.com
User-Agent: curl/7.62.0

[MatthewSteeples]

@AndreySemykin

It's not useless - https://stackoverflow.com/questions/43872575/net-framework-4-6-1-not-defaulting-to-tls-1-2#comment85356739_49063207

If your hosting environment is ASP.NET (or Office) then TLS 1.2 is not enabled by default, and on other systems it could be changed by a registry key

[MatthewSteeples]

Please conside #55

This will enable TLS 1.2 if it is not already enabled, and will also leave other protocols as they were

[nimisha84]

@AndreySemykin @StasPerekrestov - I agree that we should not be defaulting to TLS1.2 for all app domains but to the the point as to why this change was done in the SDK is as follows-
Intuit made it mandatory that only apps using TLS1.2 will be able to talk to the QBO Api.
Our original SDK was on .Net Framework 4.0 and that does not support TLS1.2 by default.
Thus we did the SDK migration to .Net 4.6.1 which use TLS1.2 by default in Net 4.0.0 release.
We did not add the SecurityProtocol as TLS1.2 at that time as we thought using just .Net 4.6.1 would resolve the issue for devs.
But some devs said that they would want to continue to use previous SDK versions on .Net framework 4.0.0 or even .Net 4.5 Framework which may or may not require them to add Securityprotocol as TLS1.2. It totally depends on the max Framework version and in turn the TLS version the OS supports.
But, as the date for deprecation deadline for other TLS versions came in, we saw that even after using 4.5 or 4.6.1, some of the devs still had to add SecurityProtocol line as a fix.

To verify that the client app is making TLS1.2 qnd not breaking while making QBO API calls, solving for individual OS issues was not an option. It was also not a good experience for any dev to add SecurityProtocol in each of the client apps. That is the reason we added this line in the later release of the SDK, so that client apps do not break and the only thing devs need to do is to just update their SDK version.

Look at this doc- https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls
Using override is never a good option whether we use SecurityProtocol or SslPrototcol.
Thus we will have to discuss our next options and see what we can do.

@MatthewSteeples Thanks for your suggestion and your pull request. Your help is apprecaited.
But, if I look at this thread-
https://stackoverflow.com/questions/28286086/default-securityprotocol-in-net-4-5
To turn on TLS 1.1 and 1.2 without affecting other protocols:

System.Net.ServicePointManager.SecurityProtocol |=
SecurityProtocolType.Tls11 | SecurityProtocolType.Tls12;

Wouldn't this enable other protocols as well and this might cause some of the client apps to break if their OS does not supports Tls12? We would always want the dotnet apps talking to our apis to use TLS1.2 only. I'm not sure if this code change would help then. What do you think? I am not a TLS expert and would therefore love to hear what other options we can explore.

[MatthewSteeples]

@nimisha84 The pull request I have provided will fix the issue. What it does is it enables TLS 1.2 as part of the protocol negotiation. It's different behaviour from the current implementation, because what you've got currently requires TLS 1.2 as part of the protocol negotiation.

The change I've made will still fall back to TLS 1.1 or 1.0 if that was what the application was already configured to do.

[nimisha84]

@MatthewSteeples - The concern here is if we don't default to TLS1.2 suppose in the newer version of SDK, then apps who have default support to TLS1.1 or 1.0, will break on making QBO api calls using the newer SDK as it supports TLS1.2 only.

[AndreySemykin]

@nimisha84 from the link you have provided https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls

"To ensure .NET Framework applications remain secure, the TLS version should not be hardcoded. .NET Framework applications should use the TLS version the operating system (OS) supports."

This means that like it's been mentioned directly specifying the security protocol is totally wrong and will ruin the Apps that use TLS 1.3. If the client OS does not supports TLS 1.2 it's not the responsibility of SDK, neither setting the protocol explicitly will help.

[MatthewSteeples]

@nimisha84 We're not setting the default though, we're setting the options. The OS will always try and negotiate the best security that it's allowed to. That means that when talking to Intuit it will try TLS 1.2 and it will work. If it's talking to an internal system that only goes up to TLS 1.1 then it will use that.

@AndreySemykin That's all well and good, but it needs to be specified somewhere in code, whether it's in the library or your own init code, because Microsoft haven't made it the default if you're running in ASP.NET (which is where most .NET web applications are running). What I'm suggesting doesn't hardcode a requirement, and won't break apps that make use of TLS 1.3 anywhere (Please note that my PR is not the same code as I posted earlier in this issue, it's a better version). It just enables TLS 1.2 as an option if it wasn't already set. Any other options you've chosen (1.3, 1.1 etc) will still be in exactly the same configuration that you had them in.

If the Client OS doesn't support TLS 1.2 then this is a moot point, as you won't be able to access QBO's API no matter what your config is set up as

[nimisha84]

Thanks @MatthewSteeples I will approve this PR then. It seems like a great solution then.
@AndreySemykin- @MatthewSteeples PR will hopefully not break any of the apps using QBO api and will also enable your app calls to lesser TLS versions.
The point I was trying to make earlier was that hardcoding either using SecurityProtocol or SSLProtocols isn't good and changing my code to use SSLProtocols isn't really a great option then. I was thinking if we could avoid the hardcoding altogether but it seems like there isn't a way.
I believe @MatthewSteeples PR is great midway which can unblock your other api calls too.

[AndreySemykin]

@nimisha84 @MatthewSteeples that PR will do nothing. Default Value of
https://docs.microsoft.com/en-us/dotnet/api/system.net.servicepointmanager.securityprotocol?view=netframework-4.7.2#System_Net_ServicePointManager_SecurityProtocol
Is SystemDefault which is 0, so |= TLS1.2 will give TLS1.2
https://docs.microsoft.com/en-us/dotnet/api/system.net.securityprotocoltype?view=netframework-4.7.2

The only correct solution as it has been already told is to remove this assignation.

[MatthewSteeples]

@nimisha84 Hold off on merging it while I verify what @AndreySemykin is saying please

[MatthewSteeples]

@AndreySemykin Good spot, I hadn't considered the "Default" mode so the change I was proposing would have solved the issue for ASP.NET but would have broken applications that were already configured correctly.

Just so you can see what issue this is trying to fix, this is the default value before any code runs if you're hosted in IIS:

That's why applying the OR with TLS 1.2 will fix that scenario.

@nimisha84 I've updated the PR so it leaves default as-is

[nimisha84]

I realize that the pull req uses Framework 4.7, System.Net dll and therefore this solution cannot check for Default TLS protocol in 4.6.1 which is the current framework version of the SDK.

[MatthewSteeples]

@nimisha84 Added #56 to cater for this

[AndreySemykin]

@nimisha84 @MatthewSteeples Let me explain why this is a wrong solution. If the application code is written correctly the SystemDefault 0 value is used and nothing happens here. If somewhy the developers specify TSL 1.1 or 1,0 which is probably a wrong solution because they are redundant then this library will break their code again and there is going to be much madness to your side. Yes, avoiding reasssignation will not make it work and requests will fail but what is the strange reason for making only old protocols supported? Also, whin the intuit will switch to TLS 1.3 which they do already support the code will be redundant again, and the duplication is also no good coding practice. Just remove this code, as it is said in docs, there is really no good reason for leaving it as is.

[MatthewSteeples]

@AndreySemykin Yes if they've set it to Default (0) then it will fine, that we agree on.

Please try running the code under ASP.NET and tell me what the value is set to. You will find that it is not set to 0. It's not that developers have chosen to specify it, it's what the framework does. If you are running this library under ASP.NET, you will have to have this code somewhere that enables TLS 1.2. If you don't, it won't work. We're not enabling old protocols with this change, and there is an incredibly slim chance of any code breaking (if you can give me a scenario, then I'm happy to test it). If the OS supports TLS 1.3 then TLS 1.3 will be used if the option is enabled. This code is not enforcing TLS 1.2, it's simply enabling it as an option.

Sorry for the use of bold text everywhere, but what you're describing is not what this code is doing. All of the scenarios you're worried about are catered for and are fine with this code.

[AndreySemykin]

@MatthewSteeples

[AndreySemykin]

@MatthewSteeples If OS does not supports it, you are enabling nothing. If the code of the applications has disabled it for some crazy reason, you are also doing no good, because the application is the top level and this library might want to change nothing. Also, if there is only TLS 1.3 enabled by the app your code is breaking the security policy.

[AndreySemykin]

@MatthewSteeples
Can you simply describe the scenario when your code going to help?
If I target .net 4.6 It would be

[StasPerekrestov]

Hey, sorry for the late response.
https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls

To ensure .NET Framework applications remain secure, the TLS version should not be hardcoded. .NET Framework applications should use the TLS version the operating system (OS) supports.

We recommend that you:
...

• Do not specify the TLS version. Configure your code to let the OS decide on the TLS version.
• Perform a thorough code audit to verify you're not specifying a TLS or SSL version.

For .NET Framework 4.6 - 4.6.2 and not WCF
Set the DontEnableSystemDefaultTlsVersions AppContext switch to false

A value of false for Switch.System.Net.DontEnableSchUseStrongCrypto causes your app to use strong cryptography. A value of false for DontEnableSchUseStrongCrypto uses more secure network protocols (TLS 1.2, TLS 1.1, and TLS 1.0) and blocks protocols that are not secure.

Please note: For applications targeting .NET Framework 4.7.1 and later versions, this value defaults to false. For applications targeting .NET Framework 4.7 and earlier, this value defaults to true.

However, I believe that the library shouldn't change TLS settings for a whole application. To my mind, the best solution for a library is to limit TLS settings to a particular HttpClientHandler instance.

Thanks.

[MatthewSteeples]

@AndreySemykin Thanks for those. That's really strange. The screenshot I sent earlier is what our web project targeting .net 4.7.2 (on Windows 10) returns when starting up. If I was seeing what you're seeing I'd definitely be coming up with the same arguments you are. I'm at a loss as to why ours returns a different value. The only thing I can think of is that our web project initially started off as a .NET 3.5 (as that's how long we've been going) and has organically migrated through the versions, so maybe an original setting has been inadvertently maintained somewhere. It's definitely not in the code though.

I'd be happy for the line to be removed entirely, with a note in the docs advising developers to check their settings if they're having issues, but as it stands it would fix things for developers who are in the same boat that we are (and would have no effect on people in your situation).

[MatthewSteeples]

@AndreySemykin You also have a valid point about TLS 1.3. As much as it's not currently supported by the .NET Framework, it would end up unnecessarily changing the security requirements of the application

[nimisha84]

Thanks everyone for your feedback. We have released the 5.8.0 version of the SDK without the defaulting to any protocol and are delegating that on the client apps to decide whether they need it or not in their apps. Let me know if you see any issues.