make System.Security.Cryptography.X509Certificates compliant with interop guidelines - part 3 #61435

pedrobsaila · 2021-11-10T19:29:42Z

Fixes partially #51564

The issue is still under progress, more PRs are to come. The current one makes the following calls compliant with Interop guidelines :

Crypt32.CryptDecodeObjectPointer
Crypt32.CertCreateCertificateChainEngine
Crypt32.CertFreeCertificateChainEngine
Crypt32.CertGetCertificateChain
Crypt32.CryptHashPublicKeyInfo
Crypt32.CertSaveStore
Crypt32.CertFindCertificateInStore
Crypt32.CertVerifyTimeValidity
Crypt32.CertFindExtension
Crypt32.CertGetIntendedKeyUsage
Crypt32.CertGetValidUsages
Crypt32.CertControlStore
Crypt32.CertDeleteCertificateFromStore
Crypt32.CertFreeCertificateChain
Crypt32.CertGetIntendedKeyUsage
Crypt32.CertVerifyCertificateChainPolicy
Crypt32.CryptImportPublicKeyInfoEx2
Crypt32.CertVerifyCertificateChainPolicy
Crypt32.CryptAcquireCertificatePrivateKey
Advapi32.CryptAcquireContext

There are 5 DllImport methods inside System.Security.Cryptography.X509Certificates.Tests project, should I move them also ?

…erop guidelines - part 3

ghost · 2021-11-10T19:29:49Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #51564

This PR makes the following calls compliant with Interop gudielines :

Crypt32.CryptDecodeObjectPointer
Crypt32.CertCreateCertificateChainEngine
Crypt32.CertFreeCertificateChainEngine
Crypt32.CertGetCertificateChain
Crypt32.CryptHashPublicKeyInfo
Crypt32.CertSaveStore
Crypt32.CertFindCertificateInStore
Crypt32.CertVerifyTimeValidity
Crypt32.CertFindExtension
Crypt32.CertGetIntendedKeyUsage
Crypt32.CertGetValidUsages
Crypt32.CertControlStore
Crypt32.CertDeleteCertificateFromStore
Crypt32.CertFreeCertificateChain
Crypt32.CertGetIntendedKeyUsage
Crypt32.CertVerifyCertificateChainPolicy
Crypt32.CryptImportPublicKeyInfoEx2
Crypt32.CertVerifyCertificateChainPolicy
Crypt32.CryptAcquireCertificatePrivateKey
Advapi32.CryptAcquireContext

There's 5 Interop calls inside System.Security.Cryptography.X509Certificates.Tests project, should I move them also or it is OK for DllImport to be in test libraries?

Author:	pedrobsaila
Assignees:	-
Labels:	`area-System.Security`
Milestone:	-

bartonjs · 2021-11-15T22:24:24Z

src/libraries/Common/src/Interop/Windows/Advapi32/Interop.CryptAcquireContext_IntPtr.cs

    {
        [GeneratedDllImport(Libraries.Advapi32, EntryPoint = "CryptAcquireContextW", CharSet = CharSet.Unicode, SetLastError = true)]
        [return: MarshalAs(UnmanagedType.Bool)]
-        public static unsafe partial bool CryptAcquireContext(out IntPtr psafeProvHandle, char* pszContainer, char* pszProvider, int dwProvType, Crypt32.CryptAcquireContextFlags dwFlags);
+        internal static unsafe partial bool CryptAcquireContext(out IntPtr psafeProvHandle,


Suggested change

internal static unsafe partial bool CryptAcquireContext(out IntPtr psafeProvHandle,

internal static unsafe partial bool CryptAcquireContext(

out IntPtr psafeProvHandle,

bartonjs · 2021-11-15T22:26:40Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CertGetCertificateChain.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, SetLastError = true)]
+        internal static unsafe partial bool CertGetCertificateChain(IntPtr hChainEngine,


Personally, I'd put all of the contents of CERT_CHAIN_PARA.cs into this file, since there's no need for it with any other function.

bartonjs · 2021-11-15T22:27:08Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CertGetCertificateChain.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, SetLastError = true)]
+        internal static unsafe partial bool CertGetCertificateChain(IntPtr hChainEngine,


Suggested change

internal static unsafe partial bool CertGetCertificateChain(IntPtr hChainEngine,

internal static unsafe partial bool CertGetCertificateChain(

IntPtr hChainEngine,

bartonjs · 2021-11-15T22:27:18Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CertGetIntendedKeyUsage.cs

+        // Note: It's somewhat unusual to use an API enum as a parameter type to a P/Invoke but in this case, X509KeyUsageFlags was intentionally designed as bit-wise
+        // identical to the wincrypt CERT_*_USAGE values.
+        [GeneratedDllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
+        internal static unsafe partial bool CertGetIntendedKeyUsage(CertEncodingType dwCertEncodingType,


Suggested change

internal static unsafe partial bool CertGetIntendedKeyUsage(CertEncodingType dwCertEncodingType,

internal static unsafe partial bool CertGetIntendedKeyUsage(

CertEncodingType dwCertEncodingType,

bartonjs · 2021-11-15T22:27:57Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CertFindCertificateInStore.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
+        internal static unsafe partial SafeCertContextHandle CertFindCertificateInStore(SafeCertStoreHandle hCertStore,


Suggested change

internal static unsafe partial SafeCertContextHandle CertFindCertificateInStore(SafeCertStoreHandle hCertStore,

internal static unsafe partial SafeCertContextHandle CertFindCertificateInStore(

SafeCertStoreHandle hCertStore,

bartonjs · 2021-11-15T22:28:38Z

...src/Interop/Windows/Crypt32/Interop.CryptAcquireCertificatePrivateKey_SafeNCryptKeyHandle.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
+        public static partial bool CryptAcquireCertificatePrivateKey(SafeCertContextHandle pCert,


Suggested change

public static partial bool CryptAcquireCertificatePrivateKey(SafeCertContextHandle pCert,

public static partial bool CryptAcquireCertificatePrivateKey(

SafeCertContextHandle pCert,

bartonjs · 2021-11-15T22:28:46Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptDecodeObjectPointer_IntPtr.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, EntryPoint = "CryptDecodeObject", CharSet = CharSet.Unicode, SetLastError = true)]
+        internal static unsafe partial bool CryptDecodeObjectPointer(CertEncodingType dwCertEncodingType,


Suggested change

internal static unsafe partial bool CryptDecodeObjectPointer(CertEncodingType dwCertEncodingType,

internal static unsafe partial bool CryptDecodeObjectPointer(

CertEncodingType dwCertEncodingType,

bartonjs · 2021-11-15T22:28:57Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptDecodeObjectPointer_string.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, EntryPoint = "CryptDecodeObject", CharSet = CharSet.Unicode, SetLastError = true)]
+        internal static unsafe partial bool CryptDecodeObjectPointer(CertEncodingType dwCertEncodingType,


Suggested change

internal static unsafe partial bool CryptDecodeObjectPointer(CertEncodingType dwCertEncodingType,

internal static unsafe partial bool CryptDecodeObjectPointer(

CertEncodingType dwCertEncodingType,

bartonjs · 2021-11-15T22:29:06Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptHashPublicKeyInfo.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
+        internal static partial bool CryptHashPublicKeyInfo(IntPtr hCryptProv,


Suggested change

internal static partial bool CryptHashPublicKeyInfo(IntPtr hCryptProv,

internal static partial bool CryptHashPublicKeyInfo(

IntPtr hCryptProv,

bartonjs · 2021-11-15T22:29:15Z

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptImportPublicKeyInfoEx2.cs

+    internal static partial class Crypt32
+    {
+        [GeneratedDllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
+        internal static unsafe partial bool CryptImportPublicKeyInfoEx2(CertEncodingType dwCertEncodingType,


Suggested change

internal static unsafe partial bool CryptImportPublicKeyInfoEx2(CertEncodingType dwCertEncodingType,

internal static unsafe partial bool CryptImportPublicKeyInfoEx2(

CertEncodingType dwCertEncodingType,

bartonjs · 2021-11-15T22:52:41Z

There are 5 DllImport methods inside System.Security.Cryptography.X509Certificates.Tests project, should I move them also ?

Search-fu is failing me today. What are they?

pedrobsaila · 2021-11-16T19:42:06Z

Search-fu is failing me today. What are they?

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/HostnameMatchTests.Unix.cs

Lines 192 to 193 in 57bfe47

    
           [DllImport(Interop.Libraries.CryptoNative, EntryPoint = "CryptoNative_CheckX509Hostname")] 
        
           private static extern int CheckX509Hostname(IntPtr x509, string hostname, int cchHostname);

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/InteropTests.cs

Lines 115 to 131 in 57bfe47

    
           [DllImport("crypt32.dll", CharSet = CharSet.Unicode, SetLastError = true)] 
        
           private static extern bool CryptQueryObject( 
        
               CertQueryObjectType dwObjectType, 
        
               [In] ref CRYPTOAPI_BLOB pvObject, 
        
               ExpectedContentTypeFlags dwExpectedContentTypeFlags, 
        
               ExpectedFormatTypeFlags dwExpectedFormatTypeFlags, 
        
               int dwFlags, // reserved - always pass 0 
        
               IntPtr pdwMsgAndCertEncodingType, 
        
               IntPtr pdwContentType, 
        
               IntPtr pdwFormatType, 
        
               IntPtr phCertStore, 
        
               IntPtr phMsg, 
        
               out IntPtr ppvContext 
        
               ); 
        
           [DllImport("crypt32.dll", CharSet = CharSet.Unicode, SetLastError = true)] 
        
           private static extern bool CertFreeCertificateContext(IntPtr pCertContext);

runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/X509StoreTests.cs

Lines 579 to 582 in 57bfe47

    
           [DllImport("libc")] 
        
           private static extern int chmod(string path, int mode); 
        
           [DllImport("libc")] 
        
           private static extern uint geteuid();

bartonjs · 2021-11-16T21:03:32Z

Hm. I'm guessing that the tests had their own P/Invokes because they were originally compiled as AnyOS.

Since they're split now (and the in-progress unified System.Security.Cryptography.Tests is already split) they should probably be fixed to use the existing interop-decls. (CheckX509Hostname being the hardest of the three, since the Interop decl uses a SafeHandle instead of IntPtr and is in a grab-bag file... so for that one it's probably just making a dedicated Interop.CheckX509Hostname for the IntPtr version and leaving the SafeHandle version where it is). -- But that should be a followup PR, no need to reset this one 😄

bartonjs · 2021-11-22T18:04:30Z

Thanks for doing this, @pedrobsaila!

شكرا & merci

make System.Security.Cryptography.X509Certificates compliant with int…

8d2c3e4

…erop guidelines - part 3

dotnet-issue-labeler bot added the area-System.Security label Nov 10, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 10, 2021

jeffhandley assigned bartonjs Nov 15, 2021

bartonjs reviewed Nov 15, 2021

View reviewed changes

fix interop parameters aligning in source code

bbfbd4d

bartonjs approved these changes Nov 20, 2021

View reviewed changes

bartonjs merged commit c334e55 into dotnet:main Nov 22, 2021

pedrobsaila deleted the 51564/part4 branch November 22, 2021 18:30

ghost locked as resolved and limited conversation to collaborators Dec 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make System.Security.Cryptography.X509Certificates compliant with interop guidelines - part 3 #61435

make System.Security.Cryptography.X509Certificates compliant with interop guidelines - part 3 #61435

pedrobsaila commented Nov 10, 2021 •

edited

Loading

ghost commented Nov 10, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs Nov 15, 2021

bartonjs commented Nov 15, 2021

pedrobsaila commented Nov 16, 2021

bartonjs commented Nov 16, 2021

bartonjs commented Nov 22, 2021

	internal static unsafe partial bool CryptAcquireContext(out IntPtr psafeProvHandle,
	internal static unsafe partial bool CryptAcquireContext(
	out IntPtr psafeProvHandle,

	internal static unsafe partial bool CertGetCertificateChain(IntPtr hChainEngine,
	internal static unsafe partial bool CertGetCertificateChain(
	IntPtr hChainEngine,

	internal static unsafe partial bool CertGetIntendedKeyUsage(CertEncodingType dwCertEncodingType,
	internal static unsafe partial bool CertGetIntendedKeyUsage(
	CertEncodingType dwCertEncodingType,

	internal static unsafe partial SafeCertContextHandle CertFindCertificateInStore(SafeCertStoreHandle hCertStore,
	internal static unsafe partial SafeCertContextHandle CertFindCertificateInStore(
	SafeCertStoreHandle hCertStore,

	public static partial bool CryptAcquireCertificatePrivateKey(SafeCertContextHandle pCert,
	public static partial bool CryptAcquireCertificatePrivateKey(
	SafeCertContextHandle pCert,

	internal static partial bool CryptHashPublicKeyInfo(IntPtr hCryptProv,
	internal static partial bool CryptHashPublicKeyInfo(
	IntPtr hCryptProv,

make System.Security.Cryptography.X509Certificates compliant with interop guidelines - part 3 #61435

make System.Security.Cryptography.X509Certificates compliant with interop guidelines - part 3 #61435

Conversation

pedrobsaila commented Nov 10, 2021 • edited Loading

ghost commented Nov 10, 2021

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

bartonjs commented Nov 15, 2021

pedrobsaila commented Nov 16, 2021

bartonjs commented Nov 16, 2021

bartonjs commented Nov 22, 2021

pedrobsaila commented Nov 10, 2021 •

edited

Loading