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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/Calamari.Common/CalamariFlavourProgram.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ protected virtual int Run(string[] args)
}
catch (Exception ex)
{
return ConsoleFormatter.PrintError(ConsoleLog.Instance, ex);
return ConsoleFormatter.PrintError(Log, ex);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
using System;
using System.Collections.Generic;
using Calamari.Integration.Certificates.WindowsNative;
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
using System.Linq;
using System.Runtime.InteropServices;
using System.Security.AccessControl;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using static Calamari.Integration.Certificates.WindowsNative.WindowsX509Native;
#endif

namespace Calamari.Integration.Certificates
{
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
public static class CryptoKeySecurityAccessRules
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{

internal static void AddPrivateKeyAccessRules(ICollection<PrivateKeyAccessRule> accessRules, SafeCertContextHandle certificate)
{
try
{
var keyProvInfo = certificate.GetCertificateProperty<KeyProviderInfo>(CertificateProperty.KeyProviderInfo);

// If it is a CNG key
if (keyProvInfo.dwProvType == 0)
{
SetCngPrivateKeySecurity(certificate, accessRules);
}
else
{
SetCspPrivateKeySecurity(certificate, accessRules);
}
}
catch (Exception ex)
{
throw new Exception("Could not set security on private-key", ex);
}
}

static void SetCngPrivateKeySecurity(SafeCertContextHandle certificate, ICollection<PrivateKeyAccessRule> accessRules)
{
using (var key = CertificatePal.GetCngPrivateKey(certificate))
{
var security = GetCngPrivateKeySecurity(certificate);

foreach (var cryptoKeyAccessRule in accessRules.Select(ToCryptoKeyAccessRule))
{
security.AddAccessRule(cryptoKeyAccessRule);
}

var securityDescriptorBytes = security.GetSecurityDescriptorBinaryForm();
var gcHandle = GCHandle.Alloc(securityDescriptorBytes, GCHandleType.Pinned);

var errorCode = NCryptSetProperty(key,
WindowsX509Native.NCryptProperties.SecurityDescriptor,
gcHandle.AddrOfPinnedObject(),
securityDescriptorBytes.Length,
(int)WindowsX509Native.NCryptFlags.Silent | (int)WindowsX509Native.SecurityDesciptorParts.DACL_SECURITY_INFORMATION);

gcHandle.Free();

if (errorCode != 0)
{
throw new CryptographicException(errorCode);
}
}
}

static void SetCspPrivateKeySecurity(SafeCertContextHandle certificate, ICollection<PrivateKeyAccessRule> accessRules)
{
using (var cspHandle = CertificatePal.GetCspPrivateKey(certificate))
{
var security = GetCspPrivateKeySecurity(certificate);

foreach (var cryptoKeyAccessRule in accessRules.Select(ToCryptoKeyAccessRule))
{
security.AddAccessRule(cryptoKeyAccessRule);
}

var securityDescriptorBytes = security.GetSecurityDescriptorBinaryForm();

if (!CryptSetProvParam(cspHandle,
WindowsX509Native.CspProperties.SecurityDescriptor,
securityDescriptorBytes,
WindowsX509Native.SecurityDesciptorParts.DACL_SECURITY_INFORMATION))
{
throw new CryptographicException(Marshal.GetLastWin32Error());
}
}
}

static CryptoKeyAccessRule ToCryptoKeyAccessRule(PrivateKeyAccessRule privateKeyAccessRule)
{
switch (privateKeyAccessRule.Access)
{
case PrivateKeyAccess.ReadOnly:
return new CryptoKeyAccessRule(privateKeyAccessRule.GetIdentityReference(), CryptoKeyRights.GenericRead, AccessControlType.Allow);

case PrivateKeyAccess.FullControl:
// We use 'GenericAll' here rather than 'FullControl' as 'FullControl' doesn't correctly set the access for CNG keys
return new CryptoKeyAccessRule(privateKeyAccessRule.GetIdentityReference(), CryptoKeyRights.GenericAll, AccessControlType.Allow);

default:
throw new ArgumentOutOfRangeException(nameof(privateKeyAccessRule.Access));
}
}

static CryptoKeySecurity GetCngPrivateKeySecurity(SafeCertContextHandle certificate)
{
using (var key = CertificatePal.GetCngPrivateKey(certificate))
{
var security = new CryptoKeySecurity();
security.SetSecurityDescriptorBinaryForm(CertificatePal.GetCngPrivateKeySecurity(key),
AccessControlSections.Access);
return security;
}
}

static CryptoKeySecurity GetCspPrivateKeySecurity(SafeCertContextHandle certificate)
{
using (var cspHandle = CertificatePal.GetCspPrivateKey(certificate))
{
var security = new CryptoKeySecurity();
security.SetSecurityDescriptorBinaryForm(CertificatePal.GetCspPrivateKeySecurity(cspHandle), AccessControlSections.Access);
return security;
}
}


public static CryptoKeySecurity GetPrivateKeySecurity(string thumbprint, StoreLocation storeLocation, string storeName)
{
var store = new X509Store(storeName, storeLocation);
store.Open(OpenFlags.ReadOnly);

var found = store.Certificates.Find(X509FindType.FindByThumbprint, thumbprint, false);
store.Close();

if (found.Count == 0)
throw new Exception(
$"Could not find certificate with thumbprint '{thumbprint}' in store Cert:\\{storeLocation}\\{storeName}");

var certificate = new SafeCertContextHandle(found[0].Handle, false);

if (!certificate.HasPrivateKey())
throw new Exception("Certificate does not have a private-key");

var keyProvInfo =
certificate.GetCertificateProperty<WindowsX509Native.KeyProviderInfo>(WindowsX509Native.CertificateProperty.KeyProviderInfo);

// If it is a CNG key
return keyProvInfo.dwProvType == 0
? GetCngPrivateKeySecurity(certificate)
: GetCspPrivateKeySecurity(certificate);
}
}
#else
public static class CryptoKeySecurityAccessRules
{
internal static void AddPrivateKeyAccessRules(ICollection<PrivateKeyAccessRule> accessRules, SafeCertContextHandle certificate)
{
throw new NotImplementedException("Not Yet Available For NetCore");
}
}
#endif
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


using System;
using System;
using System.Security.Principal;

namespace Calamari.Integration.Certificates
Expand All @@ -20,5 +18,4 @@ public static IdentityReference GetIdentityReference(this PrivateKeyAccessRule p
}
}
}
}
#endif
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
using System;
using System;
using System.Runtime.InteropServices;
using System.Security.Cryptography;
using System.Text;
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

@zentron zentron Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
int bufferSize = 0;
Expand Down Expand Up @@ -179,6 +180,7 @@ public static void DeleteCngKey(SafeNCryptKeyHandle key)
throw new CryptographicException(errorCode);
}

#endif
public static string GetSubjectName(SafeCertContextHandle certificate)
{
var flags = CertNameFlags.None;
Expand All @@ -194,5 +196,4 @@ public static string GetSubjectName(SafeCertContextHandle certificate)
return sb.ToString();
}
}
}
#endif
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
using System;
using System;
using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;

Expand Down Expand Up @@ -57,5 +56,4 @@ public IntPtr Disconnect()
return ptr;
}
}
}
#endif
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT

namespace Calamari.Integration.Certificates.WindowsNative
namespace Calamari.Integration.Certificates.WindowsNative
{
internal static class SafeCertContextHandleExtensions
{
Expand All @@ -25,4 +23,3 @@ public static T GetCertificateProperty<T>(this SafeCertContextHandle certificate
}
}
}
#endif
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
using System;
using System;
using Microsoft.Win32.SafeHandles;

namespace Calamari.Integration.Certificates.WindowsNative
Expand Down Expand Up @@ -28,5 +27,4 @@ protected override bool ReleaseHandle()

public static SafeCertStoreHandle InvalidHandle => new SafeCertStoreHandle(IntPtr.Zero);
}
}
#endif
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
using System;
using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Security.Cryptography;
Expand Down Expand Up @@ -63,4 +62,3 @@ protected override bool ReleaseHandle()
}
}
}
#endif
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#if WINDOWS_CERTIFICATE_STORE_SUPPORT
#nullable disable
#nullable disable
using System;
using System.Runtime.InteropServices;
using System.Text;
Expand Down Expand Up @@ -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)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[return: MarshalAs(UnmanagedType.Bool)]
internal static extern bool CryptAcquireCertificatePrivateKey(SafeCertContextHandle pCert,
AcquireCertificateKeyOptions dwFlags,
IntPtr pvReserved, // void *
[Out] out SafeNCryptKeyHandle phCryptProvOrNCryptKey,
[Out] out int dwKeySpec,
[Out, MarshalAs(UnmanagedType.Bool)] out bool pfCallerFreeProvOrNCryptKey);


[DllImport("Crypt32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
internal static extern
Expand All @@ -91,6 +83,7 @@ bool CertEnumSystemStoreCallBackProto(
[MarshalAs(UnmanagedType.LPWStr)] string storeName, uint dwFlagsNotUsed, IntPtr notUsed1,
IntPtr notUsed2, IntPtr notUsed3);

#if WINDOWS_CERTIFICATE_STORE_SUPPORT
[DllImport("Ncrypt.dll", SetLastError = true, ExactSpelling = true)]
internal static extern int NCryptGetProperty(SafeNCryptHandle hObject, [MarshalAs(UnmanagedType.LPWStr)] string szProperty, [Out, MarshalAs(UnmanagedType.LPArray)] byte[] pbOutput, int cbOutput, ref int pcbResult, int flags);

Expand All @@ -100,6 +93,16 @@ bool CertEnumSystemStoreCallBackProto(
[DllImport("Ncrypt.dll")]
internal static extern int NCryptDeleteKey(SafeNCryptKeyHandle hKey, int flags);

[DllImport("Crypt32.dll", SetLastError = true)]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern bool CryptAcquireCertificatePrivateKey(SafeCertContextHandle pCert,
AcquireCertificateKeyOptions dwFlags,
IntPtr pvReserved, // void *
[Out] out SafeNCryptKeyHandle phCryptProvOrNCryptKey,
[Out] out int dwKeySpec,
[Out, MarshalAs(UnmanagedType.Bool)] out bool pfCallerFreeProvOrNCryptKey);
#endif

[Flags]
internal enum CertStoreProviders
{
Expand Down Expand Up @@ -333,5 +336,4 @@ internal struct CRYPT_BIT_BLOB
public int cUnusedBits;
}
}
}
#endif
}
Loading