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

Force-allow TLS 1.2 on .NET 4.5 #2297

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

HebaruSan
Copy link
Member

Background

  • GitHub has sunsetted TLS 1/1.1 as of today. They gave us one year's warning, but apparently we missed it or didn't realize it applied to us (or assumed that .NET would handle it properly for us). This means the only way to access an HTTPS URL on GitHub is now via TLS 1.2.
  • .NET 4.5 can support TLS 1.2, but it is not enabled by default. .NET 4.6 enables it by default.
  • CKAN targets .NET 4.5. Even if you have a later version installed, the behavior of the .NET components will mimic .NET 4.5.

Problem

Released versions of CKAN can't access GitHub HTTPS URLs anymore. They cause TLS errors.

CKAN uses GitHub HTTPS URLs not just for mod downloads, but also for registry updates and application self-auto-updates. So you can't even refresh the registry or update CKAN itself.

Workaround

You can force .NET 4.5 to allow only TLS 1.2 by setting a registry key. This allows current versions of CKAN to work:

However, this affects all applications rather than just CKAN, which might cause problems if any of them need TLS 1/1.1 to operate. Caution is advised.

Until we release a new version, this is the only option for getting CKAN to work.

Considered but not done

One way to fix this would be to change TargetFrameworkVersion in our csproj files from v4.5 to v4.6. We probably don't want to do this because some fraction of people might still be running the old framework and would see errors. If you think this is the better solution, please say so below.

Changes

Now we add SecurityProtocolTypes.Tls12 to System.Net.ServicePointManager.SecurityProtocol at startup via an OR-equals |= operator. This enables TLS 1.2 while ensuring that no other supported protocols are disabled.

Credit to @TruePikachu for proposing this solution.

Fixes #2293.

@HebaruSan HebaruSan added Pull request Windows Issues specific for Windows Network Issues affecting internet connections of CKAN labels Feb 23, 2018
@HebaruSan
Copy link
Member Author

Hmm, we're compiling against some Mono versions that don't have SecurityProtocolTypes.Tls12. But how does that square with our TargetFrameworkVersion of v4.5, which should have it?

@HebaruSan HebaruSan force-pushed the fix/force-allow-tls12 branch from fc00de2 to 42c30cd Compare February 23, 2018 04:55
@politas
Copy link
Member

politas commented Feb 23, 2018

Sounds like we need to move to Mono 5.0 and above,We'll be screwing over Debian users again, since even the current Unstable branch (Sid) only has Mono 4.6. If this is the only way to get us working with GitHub again, though...

@politas politas merged commit 42c30cd into KSP-CKAN:master Feb 23, 2018
politas added a commit that referenced this pull request Feb 23, 2018
@sundhaug92
Copy link

Since this is likely to cause headache for users, should an emergency release be considered?

@politas
Copy link
Member

politas commented Feb 23, 2018

@sundhaug92
Copy link

ty @politas

@HebaruSan HebaruSan deleted the fix/force-allow-tls12 branch February 23, 2018 15:51
@XaeroDT XaeroDT mentioned this pull request Feb 24, 2018
@HebaruSan HebaruSan mentioned this pull request Mar 19, 2018
@Ruedii
Copy link

Ruedii commented Apr 1, 2018

This is causing serious problems on Ubuntu. You need to use some sort of case on this line so it only enables it on platforms that need it.

One option is to check for the existence of the function object node in question and only execute the routine if that node exists. I'm not quite sure how to do that in C#, but this is how I would approach the problem in a shell script, JavaScript/ECMA script or python script.

@HebaruSan
Copy link
Member Author

@Ruedii, the way to report problems is to create an issue, not post random vague comments on old closed pull requests.

@Ruedii
Copy link

Ruedii commented Apr 12, 2018

@HebaruSan Sorry, I replied on the thread it belongs in (your thread on the identical Arch Linux issue.)

Next time could you try to be a little more gentle with your correction. I know you almost missed my comment, and hence I understand, but I initially took offense until I realized you were trying to be constructive.

Also, upon further examination of the situation (and what platforms .Net 4.6 is available on) the better option is to remove this patch entirely and just require .Net 4.6 where TLS 1.2 is enabled by default.

This should hopefully have no adverse effects, and won't require any detection or force-enabling.

Hence I'd like to formally request this pull be undone and replaced with simply building for .Net 4.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Network Issues affecting internet connections of CKAN Windows Issues specific for Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-update and other GitHub connections not working
4 participants