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

Add tls13 support for win_get_url #573

Merged
merged 7 commits into from
Jan 14, 2024

Conversation

yacine-learning
Copy link
Contributor

@yacine-learning yacine-learning commented Dec 11, 2023

SUMMARY

Using the win_get_url module I was getting the following error: "The underlying connection was closed: Could not establish trust relationship for the SSL/TLS secure channel"

However, from the machine the download was working fine within a powershell terminal. After some troubleshooting, I noticed that the issue was with TLS1.3.

Adding an extra condition for TLS1.3 actually fixed the issue. Please let me know what you think.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_get_url

ADDITIONAL INFORMATION
Ansible Version
ansible [core 2.16.1]
  config file = /ansible/ansible.cfg
  configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.10/dist-packages/ansible
  ansible collection location = /root/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/local/bin/ansible
  python version = 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] (/usr/bin/python3)
  jinja version = 3.1.2
  libyaml = True
OS

Ansible controller: Ubuntu 20.04
Target Machine: Windows Server 2022

Major  Minor  Build  Revision
-----  -----  -----  --------
10     0      20348  0

Before change

FAILED! =>{"changed": false, "dest": "C:\\temp\\dotnet-runtime-wingeturl4.exe", "elapsed": 0.10937519999999999, "msg": "Error downloading 'https://download.visualstudio.microsoft.com/download/pr/2a7ae819-fbc4-4611-a1ba-f3b072d4ea25/32f3b931550f7b315d9827d564202eeb/dotnet-hosting-8.0.0-win.exe' to 'C:\\temp\\dotnet-runtime-wingeturl4.exe': The underlying connection was closed: An unexpected error occurred on a send.", "status_code": null, "url": "https://download.visualstudio.microsoft.com/download/pr/2a7ae819-fbc4-4611-a1ba-f3b072d4ea25/32f3b931550f7b315d9827d564202eeb/dotnet-hosting-8.0.0-win.exe"}

After change

: ok=3    changed=1    unreachable=0    failed=0    skipped=2    rescued=0    ignored=0

Using the win_get_url module I was getting the following error: 
"The underlying connection was closed: Could not establish trust relationship for the SSL/TLS secure channel"

However, from the machine the download was working fine with in a powershell terminal. After some troubleshooting, I noticed that the issue was with TLS1.3.

Adding an extra condition for TLS1.3 actually fixed the issue. Please let me know what you think.
@jborean93
Copy link
Collaborator

Hmm I was hoping that the initial value $security_protocols = [System.Net.ServicePointManager]::SecurityProtocol -bor [System.Net.SecurityProtocolType]::SystemDefault would at least enable whatever system set protocols are present. I know we added TLS 1.2 to the list because older dotnet versions didn't do so but the ones that ship with TLS 1.3 should have a new enough dotnet version this point is moot. I'll have a play around and see if I can replicate the problem.

The change itself is fine but we would a changelog fragment as documented under https://docs.ansible.com/ansible/latest/community/development_process.html#creating-changelog-fragments for this bugfix.

@jborean93
Copy link
Collaborator

Just tested myself and can confirm even with SystemDefault it's not using TLS 1.3. With this change TLS 1.3 is now used. I'm still unsure if there are any registry settings that could get it working without the flag but I don't see the harm in adding TLS 1.3 if the enum value exists. We will still need a changelog fragment, I would classify this as a bugfix.

@yacine-learning
Copy link
Contributor Author

yacine-learning commented Dec 19, 2023

Sorry I have been a bit busy lately.
I noticed that the issue seems to come from the fact that now, on newer Windows version: [System.Net.ServicePointManager]::SecurityProtocol gives a value of SystemDefault, then any -bor operation we do will override this value. In our case we end up with a value of "Tls11, Tls12" and therefore Tls13 is not included in the protocols.

Another way we could deal with this, would be to add a condition to check the .NET Framework version, since the issue only seems to appear with tls1.3 in Windows 11 and Server 2022, if less than 528449 (Windows 11 and Server 2022), we enable tls1.1 and 1.2 otherwise we can just skip this block and leave whatever the system has for default.

https://learn.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed

I will look at how to create a changelog fragment.

@yacine-learning
Copy link
Contributor Author

I have added the changelog fragment, let me know if this looks ok to you.

@jborean93
Copy link
Collaborator

I've just come across something interesting. I initially tried your changes against a TLS 1.3 only host and it was failing, it would only work if I explicitly set the TLS 1.3 protocol on the ServicePointManager.SecurityProtocol flags. It turns out that the problem there was due to the WSMan PSRemoting hosting model that the psrp connection plugin I was using is run under. If I was to move to the winrm connection plugin then things work just fine with no explicit SecurityProtocol setting. Testing locally I was able to see the same thing with a failure only occurring when running through the PSRemoting layer

PS C:\Users\vagrant-domain> powershell.exe -File C:\temp\web-tls.ps1
{"tls": {"protocol": "TLSv1.3", "cipher": "TLS_AES_256_GCM_SHA384"}, "request_headers": {"Host": "192.168.56.1:41669", "Connection": "Keep-Alive"}}
PS C:\Users\vagrant-domain> Invoke-Command localhost -FilePath C:\temp\web-tls.ps1
Exception calling "GetResponse" with "0" argument(s): "The request was aborted: Could not create SSL/TLS secure
channel."
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : WebException
    + PSComputerName        : localhost

Turns out that due to how the PSRemoting WSMan host is configured it has SecurityProtocol set to Tls, Tls11, Tls12 rather than SystemDefault as the default. You can see this outside of Ansible with

PS C:\Users\vagrant-domain> invoke-command localhost { [System.Net.ServicePointManager]::SecurityProtocol }

PSComputerName RunspaceId                           Value
-------------- ----------                           -----
localhost      ba9a7483-7d6a-40da-a2dd-4a04efc9dffa Tls, Tls11, Tls12

Unfortunately in this scenario we can't just set the SecurityProtocol to SystemDefault as it also has the policy Switch.System.Net.DontEnableSystemDefaultTlsVersions set to $true which disables using SystemDefault.

$code = {
    [System.Net.ServicePointManager].GetField(
        's_disableSystemDefaultTlsVersions',
        [System.Reflection.BindingFlags]'NonPublic, Static').GetValue($null)
}

powershell.exe $code
# $false

Invoke-Command localhost $code
# $true

Which means for us to actually rely on the OS settings we also need to reconfigure this policy which requires reflection. I'll submit a suggestion to the code needed here which works locally for both the winrm and psrp connection plugins.

@agibson2
Copy link
Contributor

agibson2 commented Dec 21, 2023

From a security standpoint with the future in mind, I think best practice in general is not to set/force TLS version in the app. It seems like letting the OS/.net decide on what version of TLS to use is best so that system wide settings are applied. Newer versions of .net 4.7.2, etc use TLS 1.2 and 4.8+ can use TLS 1.3. Think of a major issue with TLS 1.3 or TLS 1.2 coming out tomorrow and businesses being required to disable one of them (like what has happened with earlier TLS versions). Using system wide settings would make it easier to deal with. I know I have had to set .net registry settings for some older TLS versions to get them to use TLS 1.2. I do get that isn't convenient though. To me it seems like the app shouldn't set anything by default but then have the ability to force it if the user needs. It will allow newer TLS versions to be used many years from now instead of needing newer TLS versions added as time go by.
https://learn.microsoft.com/en-us/dotnet/framework/network-programming/tls

@jborean93
Copy link
Collaborator

From a security standpoint with the future in mind, I think best practice in general is not to set/force TLS version in the app. It seems like letting the OS/.net decide on what version of TLS to use is best so that system wide settings are applied

That's what the suggested change does. If the Tls13 member is present in the enum we know we are running with .NET Framework 4.8+ which supports pretty much anything right now. Technically we could also include .NET Framework 4.7.2 but we would need to find a way to determine the runtime version that isn't checking a registry key.

The extra logic added is to ensure that the OS policies are used if the PSRemoting host is used with the psrp connection where OS policies are disabled in .NET. The reflection code will ensure the host policies can be used in newer .NET versions. The remaining code is for older .NET versions which still need to be explicitly opted in to use newer protocols like TLS 1.1 and TLS 1.2.

Co-authored-by: Jordan Borean <jborean93@gmail.com>
@yacine-learning
Copy link
Contributor Author

Sorry I have been away for a long time.
Indeed PSRemoting WSMan is set to Tls, Tls11, Tls12 so I don't see another way of doing that than what you did.

@jborean93
Copy link
Collaborator

Thanks for working on this @yacine-learning!

@jborean93 jborean93 merged commit 1a9e3ae into ansible-collections:main Jan 14, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants