net48 and net6 #422

veochen-octopus · 2022-10-04T01:26:01Z

Background

net452 is end of support and is preventing us from getting security patches both in the framework itself and 3rd party libraries.
We'll need to support Ubuntu 22.04 pretty soon so net6 is also required.

Results

Fixes [sc-12030]
Fixes [sc-27468]
Fixes #424

Before

<TargetFrameworks>net452;netcoreapp3.1</TargetFrameworks>

After

<TargetFrameworks>net48;net6.0</TargetFrameworks>

How to review this PR

Quality ✔️

Pre-requisites

I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
I have considered informing or consulting the right people, according to the ownership map.
I have considered appropriate testing for my change.

veochen-octopus · 2022-10-16T22:55:14Z

source/Octopus.Tentacle/Security/Certificates/CertificateEncoder.cs

@@ -186,7 +186,7 @@ static bool HasPrivateKey(X509Certificate2 certificate2)
        {
            try
            {
-                return certificate2.HasPrivateKey && certificate2.PrivateKey != null;
+                return certificate2.GetRSAPrivateKey() != null;


Changed due to the PrivateKey property supposedly not working in .net framework with EphemeralKeySet.

Keys loaded in this manner are almost always loaded via Windows CNG. Therefore, callers must access the private key by calling extension methods, such as cert.GetRSAPrivateKey(). The X509Certificate2.PrivateKey property does not function.

I'm assuming GetECDsaPrivateKey doesn't come into play here. There's only reference to RSA.

Also TODO: Add a comment that this may be reverted to .PrivateKey once .NET framework is dropped entirely. Will use this change to trigger the final build before merge.

Does this TODO need action?

Yeah that was confusing. Looking at it again, no action needed.

veochen-octopus · 2022-10-17T01:02:33Z

source/Octopus.Tentacle/Security/Certificates/CertificateEncoder.cs

@@ -103,7 +103,7 @@ static X509Certificate2 FromBase64String(string? thumbprint, string certificateS
            }
        }

-#if NET472_OR_GREATER || NETCOREAPP || NETSTANDARD
+#if NETCOREAPP || NETSTANDARD


This used to be bypassed as we were targeting net452, but breaks net48. With EphemeralKeySet the cert loses the private key once added to the key store. As per the doc

Since the keys are not persisted to disk, certificates loaded with this flag are not good candidates to add to an X509Store.

This essentially allows us to keep the same behaviour as net452

veochen-octopus · 2022-10-17T01:04:20Z

Github still configured to wait for a status from the "real" tentacle project in teamcity. Thinking once the PR is ready, we update the main build first to target net48, then get a green build and merge.

Here's a green build on the cloned project - https://build.octopushq.com/buildConfiguration/TeamFireAndMotion_OctopusTentacleNet48_SensibleDefaultsChainFullChain/5907122?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&expandPull+Request+Details=true

veochen-octopus · 2022-10-17T01:51:39Z

source/Octopus.Tentacle.Tests.Integration/Octopus.Tentacle.Tests.Integration.csproj

+        <PackageReference Include="FluentAssertions" Version="6.7.0" />
+        <PackageReference Include="Assent" Version="1.8.2" />
+        <PackageReference Include="NUnit" Version="3.13.3" />
+        <PackageReference Include="NUnit3TestAdapter" Version="4.2.1" />


Taking the opportunity to update some test packages. No non-test dependencies were changed.

tothegills · 2022-10-17T19:24:10Z

What kind of customer comms are we doing? Does that need to happen before this is merged?

veochen-octopus · 2022-10-17T20:20:43Z

What kind of customer comms are we doing? Does that need to happen before this is merged?

@tothegills I think @octokhor is handling the comms side of things.

Given the change of plan, I'll piggyback the net6 changes onto this PR as well so we end up with only 1 major/minor version bump. Moving PR back to draft for now.

veochen-octopus · 2022-10-20T01:44:18Z

source/Octopus.Tentacle.Tests/Configuration/Crypto/LinuxMachineKeyEncryptorFixture.cs

@@ -70,7 +70,7 @@ class InMemoryCryptoKeyNixSource : ICryptoKeyNixSource
            readonly byte[] iv;
            public InMemoryCryptoKeyNixSource()
            {
-                var d = new RijndaelManaged();
+                var d = Aes.Create();


RijndaelManaged obsolete. All references changed to Aes.Create

veochen-octopus · 2022-10-20T01:45:25Z

source/Octopus.Tentacle/Certificates/CertificateGenerator.cs

@@ -97,7 +97,7 @@ X509Certificate2 Generate(string fullName, bool exportable)
            using (var ms = new MemoryStream())
            {
                store.Save(ms, exportpw.ToCharArray(), random);
-                var platformSpecificX509KeyStorageFlags = PlatformDetection.IsRunningOnMac ? X509KeyStorageFlags.DefaultKeySet : X509KeyStorageFlags.EphemeralKeySet;
+                var platformSpecificX509KeyStorageFlags = PlatformDetection.IsRunningOnNix ? X509KeyStorageFlags.EphemeralKeySet : X509KeyStorageFlags.DefaultKeySet;


Don't think EphemeralKeySet works with Windows at all. See other change.

veochen-octopus · 2022-10-20T01:47:32Z

source/Octopus.Tentacle/Security/AesEncryption.cs

-                BlockSize = 128,
-                Key = key
-            };
+            var provider = Aes.Create();


Another obsolete. Updated to current standard.

veochen-octopus · 2022-10-20T01:51:26Z

source/Octopus.Tentacle/Security/Certificates/CertificateEncoder.cs


        static bool HasFlagEphemeralKeySet(X509KeyStorageFlags flags)
        {
-            return flags.HasFlag(X509KeyStorageFlags.EphemeralKeySet);
+            return !PlatformDetection.IsRunningOnWindows && flags.HasFlag(X509KeyStorageFlags.EphemeralKeySet);


This is based on the assumption that EphemeralKeySet doesn't work with Windows. See the issue mentioned in the comment - dotnet/runtime#23749. Halibut uses SslStream which in the current version of Windows is not compatible with EphemeralKeySet causing authentication to fail, on Windows tentacles only. All signs are suggesting this only works with Linux.

veochen-octopus · 2022-10-20T02:07:08Z

Not sure about the best way to update the teamcity build. There are quite a few changes to be made and it's hard to keep track of them. Thinking probably the easiest is to replace the old tentacle project with the new one and make sure everything has the correct names and ids, get a green build and merge.

veochen-octopus · 2022-10-20T04:02:30Z

docker/windows/Dockerfile

-
-WORKDIR /tmp
-RUN msiexec /i Octopus.Tentacle.%BUILD_NUMBER%-x64.msi /qn /l*v Tentacle-Installation.log
+COPY _artifacts/tentacle "${INSTALLATION_FOLDER}"


This is the only way I've found that works with COPY and Windows paths that contain spaces, i.e. with an ARG. Open to suggestions.

veochen-octopus · 2022-10-20T04:03:22Z

Latest green build - https://build.octopushq.com/buildConfiguration/TeamFireAndMotion_OctopusTentacleNet48_SensibleDefaultsChainFullChain/5962281?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&expandPull+Request+Details=true

tothegills · 2022-10-26T00:13:32Z

It'd be worth creating an Octopus Server branch, updating the Tentacle version and running the Octopus Server test suite.

tothegills

Great work, way to move this forward.
I think there are some installer pre-reqs that need updating.
I think Fixes should be a public issue in this repository.
It would be great to have a green Octopus Server build with these changes integrated.

tothegills · 2022-10-26T00:20:14Z

installer/Octopus.Tentacle.Installer/Product.wxs

-
+    <PropertyRef Id="NETFRAMEWORK45" />
+    <PropertyRef Id="NETFRAMEWORK40FULLINSTALLROOTDIR" />
+    <Condition Message="This application requires Microsoft .NET Framework 4.5 Runtime in order to run. Please install the .NET Framework and then run this installer again."><![CDATA[Installed OR NETFRAMEWORK45]]></Condition>


I don't think this message about .NET framework 4.5 is still true

🤔 I think this was auto generated. Let me see where it got this information from.

Can I please get some help on this? Don't want to dump too much time into learning Wix (can do that later). But from what I can see from the manual, there's no good replacement for NETFRAMEWORK45 for net48. This is wix v3. Not sure about v4.

Also found a snippet in their source code that has both similar and very different settings. A little confused...

tothegills · 2022-10-26T00:22:23Z

source/Octopus.Tentacle.Tests/Configuration/InMemoryKeyValueStoreFixture.cs

@@ -91,8 +91,9 @@ public void ComplexTypeGetsHandledCorrectly()
            var store = new InMemoryKeyValueStore(mapper);

            var settings = store.TryGet<TestConfig[]>("Test");
-            settings.value.Single().SettingA.Should().Be("some value", "strings should get parsed");
-            settings.value.Single().SomethingElse.Should().Be(12, "ints should get parsed");
+            settings.value.Should().NotBeNull();


The null check is an important assertion

tothegills · 2022-10-26T00:23:32Z

source/Octopus.Tentacle/Communications/OctopusServerChecker.cs

@@ -38,34 +38,40 @@ public string CheckServerCommunicationsIsOpen(Uri serverAddress, IWebProxy? prox

            Retry("Checking that server communications are open", () =>
            {
-#pragma warning disable DE0003
+                // TODO see if can deal with SYSLIB0014


Is this TODO still to do? What are the plans for doing it?

Removing this TODO. Fix would be to properly inject an HttpClient (or the factory) and go async from there. Treating it as out of scope for now, as it might not be a local change. But important to address later on. Ideally make tentacle more async overall.

tothegills · 2022-10-26T00:32:24Z

source/Octopus.Tentacle/Security/Certificates/CertificateEncoder.cs

@@ -186,7 +186,7 @@ static bool HasPrivateKey(X509Certificate2 certificate2)
        {
            try
            {
-                return certificate2.HasPrivateKey && certificate2.PrivateKey != null;
+                return certificate2.GetRSAPrivateKey() != null;


Does this TODO need action?

tothegills · 2022-10-26T00:34:08Z

source/Octopus.Tentacle/Versioning/AssemblyExtensions.cs

@@ -9,13 +9,13 @@ public static class AssemblyExtensions
        public static string GetFileVersion(this Assembly assembly)
        {
            var fileVersionInfo = FileVersionInfo.GetVersionInfo(assembly.FullLocalPath());
-            return fileVersionInfo.FileVersion;
+            return fileVersionInfo.FileVersion ?? "";


Is "" a good backup value?

Good question. We could return "0.0.0". It would be more friendly to the semantic version parser. But it would also hide the fact that there is no version. Thinking about the responsibility of this method, should it know what a good default is? Maybe we should simply return a string? and let the consumer decide what they want to use as the default. For example

// SemanticVersionInfo.cs SemanticVersion = SemanticVersion.Parse(assembly.GetInformationalVersion() ?? "0.0.0"); // OctopusTentacle.cs public static readonly string InformationalVersion = AssemblyExtensions.GetInformationalVersion(Assembly) ?? "Unknown";

veochen-octopus · 2022-10-27T05:36:06Z

Main changes

Updated AssemblyExtensions.GetInformationalVersion to return a string? and force the consumer to provide a default, i.e. assembly.GetInformationalVersion() ?? "0.0.0"
Updated Product.wxs to require net48. (Couldn't merge your PR Shane so I've copied your change)
Enabled Ubuntu 22.04 test and added libssl3 to the deb dependencies so that the test is passing
Renamed new Teamcity build to Octopus Tentacle 2022 and updated build ids.
- Need to make sure the build number is valid before switching to the new build

Green build on the server referencing this tentacle build

https://github.com/OctopusDeploy/OctopusDeploy/pull/14821

Remaining discussion points

Is this a major or minor version?
There was something else 🤔

Future work

Lots of obsolete warnings/errors from the upgrade. Most are suppressed for now. Need to properly address.
Related to ☝️ might want to make Tentacle more async.

tothegills · 2022-10-31T00:30:37Z

build/Build.Tests.cs

@@ -85,8 +85,7 @@ void RunLinuxPackageTestsFor(TestConfigurationOnLinuxDistribution testConfigurat
                new TestConfigurationOnLinuxDistribution(NetCore, "linux-x64", "debian:oldstable-slim", "deb"),
                new TestConfigurationOnLinuxDistribution(NetCore, "linux-x64", "debian:stable-slim", "deb"),
                new TestConfigurationOnLinuxDistribution(NetCore, "linux-x64", "linuxmintd/mint19.3-amd64", "deb"),
-                // new TestConfigurationOnLinuxDistribution(NetCore, "linux-x64", "ubuntu:latest", "deb"), // 22.04 doesn't support netcore, https://github.com/dotnet/core/issues/7038
-                // new TestConfigurationOnLinuxDistribution(NetCore, "linux-x64", "ubuntu:rolling", "deb"), // 22.04 doesn't support netcore, https://github.com/dotnet/core/issues/7038
+                new TestConfigurationOnLinuxDistribution(NetCore, "linux-x64", "ubuntu:jammy", "deb"), // 22.04


It's helpful to test against latest and rolling because they alert us to problems that are not caught by testing the explicit versions.

I've added these back and turned them on. I assume when this starts to fail, we'll disable them again until we're ready for the next upgrade?

tothegills

Looks good to me. Intending to merge this PR on November 21st.

This reverts commit 7219917.

shortcut-integration · 2022-11-21T03:14:38Z

This pull request has been linked to Shortcut Story #27468: Target .NET Core 6 in Tentacle.

shortcut-integration · 2022-11-21T03:15:35Z

This pull request has been linked to Shortcut Story #12030: Target .NET framework 4.8 in Tentacle.

This reverts commit cd4af33.

veochen-octopus added 2 commits October 3, 2022 15:33

all projects to net48

ea482bf

test deps

7440f40

veochen-octopus commented Oct 16, 2022

View reviewed changes

veochen-octopus commented Oct 17, 2022

View reviewed changes

veochen-octopus force-pushed the veo/net48 branch from 08ba578 to 5a7098f Compare October 17, 2022 01:55

veochen-octopus marked this pull request as ready for review October 17, 2022 02:22

veochen-octopus requested a review from a team as a code owner October 17, 2022 02:22

veochen-octopus marked this pull request as draft October 17, 2022 20:28

veochen-octopus changed the title ~~net48~~ net48 and net6 Oct 17, 2022

veochen-octopus force-pushed the veo/net48 branch 2 times, most recently from 2d1a73f to 1d1767c Compare October 19, 2022 05:52

veochen-octopus commented Oct 20, 2022

View reviewed changes

veochen-octopus marked this pull request as ready for review October 20, 2022 04:03

tothegills reviewed Oct 26, 2022

View reviewed changes

veochen-octopus requested a review from tothegills October 27, 2022 20:55

tothegills reviewed Oct 31, 2022

View reviewed changes

veochen-octopus requested a review from tothegills October 31, 2022 04:47

tothegills reviewed Oct 31, 2022

View reviewed changes

veochen-octopus added 23 commits November 21, 2022 11:32

again

df3831f

test windows without build

eff894a

test without build

09e949d

remove redundant build

beefbf2

does this work

afc5c57

does this also work?

5688640

what about this

75cfb27

have to do this

e2ecb78

use 6.0 runtime for linux image

4e7c4d4

missed some 4.5 references

7a8f2f7

manually update Product.wxs to net48

fd73add

old settings only updating the message

59777e4

see if this works

5002518

Revert "see if this works"

cc6d44b

This reverts commit 7219917.

remove todo that's too hard.

394600e

wix net48

6001caa

better informational version handling

cb7fb3d

turn on ubuntu 22.04

5368f8a

require libssl3 for ubuntu 22.04

3485bfd

test ubuntu rolling and latest

83bd63a

remove reference to sqlclient

c3d8bef

bump NUnit3TestAdapter

e0776f9

+semver: minor

533e51e

veochen-octopus force-pushed the veo/net48 branch from 9ded67d to 533e51e Compare November 20, 2022 22:33

N-lson approved these changes Nov 21, 2022

View reviewed changes

veochen-octopus merged commit cd4af33 into main Nov 21, 2022

veochen-octopus deleted the veo/net48 branch November 21, 2022 01:00

veochen-octopus added a commit that referenced this pull request Dec 13, 2022

Revert "net48 and net6 (#422) +semver: minor"

7a04629

This reverts commit cd4af33.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net48 and net6 #422

net48 and net6 #422

veochen-octopus commented Oct 4, 2022 •

edited

Loading

veochen-octopus Oct 16, 2022 •

edited

Loading

veochen-octopus Oct 17, 2022

tothegills Oct 26, 2022

veochen-octopus Oct 27, 2022

veochen-octopus Oct 17, 2022

veochen-octopus Oct 17, 2022

veochen-octopus commented Oct 17, 2022 •

edited

Loading

veochen-octopus Oct 17, 2022

tothegills commented Oct 17, 2022

veochen-octopus commented Oct 17, 2022 •

edited

Loading

veochen-octopus Oct 20, 2022

veochen-octopus Oct 20, 2022

veochen-octopus Oct 20, 2022

veochen-octopus Oct 20, 2022

veochen-octopus commented Oct 20, 2022

veochen-octopus Oct 20, 2022

veochen-octopus commented Oct 20, 2022

tothegills commented Oct 26, 2022

tothegills left a comment

tothegills Oct 26, 2022

veochen-octopus Oct 26, 2022

veochen-octopus Oct 26, 2022

tothegills Oct 26, 2022

tothegills Oct 26, 2022

veochen-octopus Oct 27, 2022

tothegills Oct 26, 2022

tothegills Oct 26, 2022

veochen-octopus Oct 26, 2022

veochen-octopus commented Oct 27, 2022 •

edited

Loading

tothegills Oct 31, 2022

veochen-octopus Oct 31, 2022

tothegills left a comment

shortcut-integration bot commented Nov 21, 2022

shortcut-integration bot commented Nov 21, 2022

net48 and net6 #422

net48 and net6 #422

Conversation

veochen-octopus commented Oct 4, 2022 • edited Loading

Background

Results

Before

After

How to review this PR

Pre-requisites

veochen-octopus Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veochen-octopus commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

tothegills commented Oct 17, 2022

veochen-octopus commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veochen-octopus commented Oct 20, 2022

Choose a reason for hiding this comment

veochen-octopus commented Oct 20, 2022

tothegills commented Oct 26, 2022

tothegills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veochen-octopus commented Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tothegills left a comment

Choose a reason for hiding this comment

shortcut-integration bot commented Nov 21, 2022

shortcut-integration bot commented Nov 21, 2022

veochen-octopus commented Oct 4, 2022 •

edited

Loading

veochen-octopus Oct 16, 2022 •

edited

Loading

veochen-octopus commented Oct 17, 2022 •

edited

Loading

veochen-octopus commented Oct 17, 2022 •

edited

Loading

veochen-octopus commented Oct 27, 2022 •

edited

Loading