Remove as much of the WINDOWS_CERT compiler directives as possible #1352

zentron · 2024-09-26T12:20:15Z

Background

This PR essentially moves away a bunch of #if WINDOWS_CERTIFICATE_STORE_SUPPORT compiler directives to pinpoint the code that is currently (as of this PR) unable to be compiled for .netcore.

The follow up PR takes the next leap and attempts to provide capabilities for netcore to involve the few places that are not supported as discovered by these changes.

Whats Changed

What this turns out to be is essentially just the code that deals with assigning access to windows certificates. Although now then exposes in netcore classes that uses unmanaged code (windows dlls etc), these code paths should only ever exist on tasks running on windows machines (or they would have otherwise been failing in the past anyway, who's trying to run IIS steps on linux?)

How to review

Hopefully just a sanity check, nothing functionally should really change for users.

zentron · 2024-09-26T22:28:07Z

source/Calamari.Common/CalamariFlavourProgram.cs

@@ -81,7 +81,7 @@ protected virtual int Run(string[] args)
 }
 catch (Exception ex)
 {
- return ConsoleFormatter.PrintError(ConsoleLog.Instance, ex);
+ return ConsoleFormatter.PrintError(Log, ex);


This change allows us to capture the output properly when debugging tests

zentron · 2024-09-26T22:29:30Z

source/Calamari.Shared/Integration/Certificates/WindowsNative/CertificatePal.cs

@@ -115,6 +114,8 @@ public static byte[] GetCspPrivateKeySecurity(SafeCspHandle cspHandle)
 return buffer;
 }

+ // At the moment these models are not in the Calamari Solution. This is the next step.
+#if WINDOWS_CERTIFICATE_STORE_SUPPORT
 public static byte[] GetCngPrivateKeySecurity(SafeNCryptKeyHandle hObject)


The SafeNCryptKeyHandle structures are not currently defined in the Octopus solution like some of the others, and so this will need to be introduced as netcore does not have access to them from mscorlib like natframework does.

To keep the changes in multiple stages, this will be introduced in a followup PR.
As such we can generally still assume that for the sake of this PR, most of the cert capabilities are fundamentally still only available in netfx, we are just starting to expose and test some of the classes in netcore.

zentron · 2024-09-26T22:31:04Z

source/Calamari.Shared/Integration/Certificates/WindowsNative/WindowsX509Native.cs

@@ -69,14 +68,7 @@ internal static extern bool CryptAcquireCertificatePrivateKey(SafeCertContextHan
 [Out] out int dwKeySpec,
 [Out, MarshalAs(UnmanagedType.Bool)] out bool pfCallerFreeProvOrNCryptKey);

- [DllImport("Crypt32.dll", SetLastError = true)]


Shuffled down to put between the other unmanaged invocations that are currently only available in netfx for this PR

zentron · 2024-09-26T22:32:00Z

source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs

+ }
+
+ // This should only be used in tests
+ public WindowsX509CertificateStore(): this(ConsoleLog.Instance)


We want to pass through the ILog to help with tests looking for specific output, but other logs dont care and so this default constructor makes it a little easier for those scenarios.

zentron · 2024-09-26T22:32:41Z

source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs


 store.Close();
 }
 }

- public CryptoKeySecurity GetPrivateKeySecurity(string thumbprint, StoreLocation storeLocation, string storeName)


Extracted out into the CryptoKeySecurityAccessRules class so that we can more cleanly control the differences between netcore and netfx

zentron · 2024-09-26T22:34:39Z

source/Calamari.Tests/Fixtures/Certificates/ImportCertificateCommandFixture.cs

 using Calamari.Tests.Helpers;
 using Calamari.Tests.Helpers.Certificates;
 using NUnit.Framework;
 using Octostache;

 namespace Calamari.Tests.Fixtures.Certificates
 {
+ [Category(TestCategory.CompatibleOS.OnlyWindows)]


Previously this whole test wouldnt even exist in netcore, now netcore can run some of these tests so long as its on windows

zentron · 2024-09-26T22:35:12Z

source/Calamari.Tests/Fixtures/Certificates/ImportCertificateCommandFixture.cs

@@ -53,12 +54,15 @@ public void AddingCertToStore_AddsCert()
 cert.EnsureCertificateNotInStore(storeName, certificateStoreLocation);
 }

+#if WINDOWS_CERTIFICATE_STORE_SUPPORT


This test still relies on some of the capabilities not yet available to netcore (see above), it will be exposed again in a later PR

zentron · 2024-10-01T11:41:58Z

source/Calamari.Shared/Integration/Certificates/PrivateKeyAccessRuleExtensionMethods.cs

@@ -1,6 +1,4 @@
-#if WINDOWS_CERTIFICATE_STORE_SUPPORT


A bunch of the file changes are just this, removing the #if WINDOWS_CERTIFICATE_STORE_SUPPORT compiler directive, meaning it compiles into netcore, even if it shouldnt actually be getting invoked by anything (since the Calamari.netfx flavour is used for anything on Windows that would have the IIS/WindowsCert variables provided)

zentron · 2024-10-01T11:44:24Z

source/Calamari.Shared/Integration/Certificates/CryptoKeySecurityAccessRules.cs

+namespace Calamari.Integration.Certificates
+{
+#if WINDOWS_CERTIFICATE_STORE_SUPPORT
+ public static class CryptoKeySecurityAccessRules


This class is just the extraction of a bunch of CryptoKey logic that was moved out of WindowsX509CertificateStore that cannot currently compile in netcore. The functionality is essentially unchanged, with the addition of a compiler directive returning the "failing" implementation to be used by netcore (not actually used-used, but compiled for)

zentron · 2024-10-01T11:46:22Z

source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs

 return;
 }

 throw new CryptographicException(error);
 }

- Log.Info($"Imported certificate '{subjectName}' into store '{storeName}'");


The use of these static Log methods made testing using the in-process Calamari fail (as opposed to many tests which actually invoke Calamari.exe and make debugging impossible)

zentron · 2024-10-01T11:46:31Z

source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs

@@ -406,116 +389,6 @@ static IList<SafeCertContextHandle> GetCertificatesFromPfx(byte[] pfxBytes, stri
 }
 }

- static void AddPrivateKeyAccessRules(ICollection<PrivateKeyAccessRule> accessRules, SafeCertContextHandle certificate)


Moved out to CryptoKeySecurityAccessRules

zentron · 2024-10-01T11:48:13Z

source/Calamari.Tests/Fixtures/Certificates/ImportCertificateCommandFixture.cs

 CalamariResult Invoke(VariableDictionary variables)
 {
 using (var variablesFile = new TemporaryFile(Path.GetTempFileName()))
 {
 variables.Save(variablesFile.FilePath);
- return Invoke(Calamari()
+ return InvokeInProcess(Calamari()


This simply runs the test in the same process as the test harness, making debugging possible.
I'd like to move all Calamari tests to this format now that we have a bit more of a handle on the different runtimes that we are actually running

zentron · 2024-10-01T11:49:41Z

source/Calamari.Tests/Fixtures/Certificates/WindowsX509CertificateStoreFixture.cs

@@ -221,12 +227,22 @@ public void ImportExistingCertificateShouldNotOverwriteExistingPrivateKeyRights(
 Convert.FromBase64String(sampleCertificate.Base64Bytes()), sampleCertificate.Password,
 storeLocation, storeName, sampleCertificate.HasPrivateKey);

- var privateKeySecurity = new WindowsX509CertificateStore().GetPrivateKeySecurity(sampleCertificate.Thumbprint,
- storeLocation, storeName);
+ var privateKeySecurity = CryptoKeySecurityAccessRules.GetPrivateKeySecurity(sampleCertificate.Thumbprint,


Now that the CryptoKeySecurityAccessRule method is extracted, we can actually consolidate some of the test assertions that check for certificate access rules (See Below)

APErebus

LGTM

Remove as much of the WINDOWS_CERT compiler directives as possible

410707f

zentron commented Sep 26, 2024

View reviewed changes

zentron commented Oct 1, 2024

View reviewed changes

Move CryptoKeySecurityAccessRules to its own file

e6193a9

zentron commented Oct 1, 2024

View reviewed changes

zentron mentioned this pull request Oct 1, 2024

Support using NetCore for all Windows Cert functionality #1353

Merged

zentron marked this pull request as ready for review October 1, 2024 21:58

APErebus approved these changes Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove as much of the WINDOWS_CERT compiler directives as possible #1352

Remove as much of the WINDOWS_CERT compiler directives as possible #1352

zentron commented Sep 26, 2024 •

edited

Loading

zentron Sep 26, 2024

zentron Sep 26, 2024 •

edited

Loading

zentron Sep 26, 2024

zentron Sep 26, 2024

zentron Sep 26, 2024 •

edited

Loading

zentron Sep 26, 2024

zentron Sep 26, 2024

zentron Oct 1, 2024

zentron Oct 1, 2024

zentron Oct 1, 2024

zentron Oct 1, 2024

zentron Oct 1, 2024

zentron Oct 1, 2024

APErebus left a comment

Remove as much of the WINDOWS_CERT compiler directives as possible #1352

Are you sure you want to change the base?

Remove as much of the WINDOWS_CERT compiler directives as possible #1352

Conversation

zentron commented Sep 26, 2024 • edited Loading

Background

Whats Changed

How to review

Choose a reason for hiding this comment

zentron Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentron Sep 26, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APErebus left a comment

Choose a reason for hiding this comment

zentron commented Sep 26, 2024 •

edited

Loading

zentron Sep 26, 2024 •

edited

Loading

zentron Sep 26, 2024 •

edited

Loading