Remove undocumented certificate APIs by TravisEz13 · Pull Request #3818 · PowerShell/PowerShell

TravisEz13 · 2017-05-19T01:49:41Z

Addresses #3792

The code paths deleted where using the undocumented API on WIn8 and newer for the following purposes:

Faster filtering
Getting two properties (left empty on Win7 and below)
Logging using Certificate components when a cert is deleted or copied.

After the change, all the code uses public APIs. Filtering is done in PowerShell using existing code. I don't believe the logging is needed.

SteveL-MSFT

Is there sufficient test coverage?

SteveL-MSFT · 2017-05-19T01:56:51Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-            {
-                return new CertificateProviderCodeSigningDynamicParameters();
-            }
+            return new CertificateProviderCodeSigningDynamicParameters();


This will work on downlevel like Win7?

This is the code path that is always taken on Windows PowerShell on Win7. It should work.

SteveL-MSFT · 2017-05-19T01:56:56Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-
-                    Security.NativeMethods.LogCertDelete(fMachine, cert.Handle);
-                }
+                //TODO: log cert move


filed #3826, until this is resolved

SteveL-MSFT · 2017-05-19T01:56:59Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-                {
-                    Security.NativeMethods.LogCertDelete(fMachine, cert.Handle);
-                }
+                //TODO: Log Cert Delete


Open issue for TODO

I'm not sure we need these, but I'll open an issue or remove them based on my investigation.

filed #3826, until this is resolved

SteveL-MSFT · 2017-05-19T01:58:50Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-                            names.Add(new DnsNameRepresentation(
-                                Marshal.PtrToStringUni(Marshal.ReadIntPtr(papwszPunycodeNames, i * Marshal.SizeOf(papwszPunycodeNames))),
-                                Marshal.PtrToStringUni(Marshal.ReadIntPtr(papwszUnicodeNames, i * Marshal.SizeOf(papwszUnicodeNames)))));
+                            //TODO: detect and handle punyCode


SteveL-MSFT · 2017-05-19T02:21:50Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-                                    out papwszUnicodeNames);
-                        if (hr != Security.NativeConstants.S_OK)
+                        // Get the part after 'DNS Name='
+                        if(nameLine.Length > 9)


Any validation needed here? Prefer to have const DnsNamePrefixLength used than 9

Yes, I was planning on adding it with the support for punyCode. The only way I can see puny code being supported is a different record format.

SteveL-MSFT

My concerns are addressed, but would like @LeeHolmes to review before we merge

PaulHigin · 2017-05-22T16:34:54Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

@@ -480,22 +445,6 @@ public void Open(bool includeArchivedCerts)
        public IntPtr GetFirstCert(
            CertificateFilterInfo filter)


It looks like the "filter" argument is no longer needed here.

PaulHigin · 2017-05-22T16:51:34Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

            }
            if (Valid)
            {
-                if (_filterHandle != null)


What do we lose by removing this down level cert filtering option?

probably just performance. Looking at the CmdLet we only supported filtering by codesigning certs which the tests pass after the change.

line 2636, is filtering.

PaulHigin · 2017-05-22T17:00:33Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

        {
-            if (DownLevelHelper.NativeFilteringSupported())
-            {
-                return new CertificateProviderDynamicParameters();


It looks like the internal CertificateProviderDynamicParameters class is no longer used. Has it been removed?

PaulHigin · 2017-05-22T17:31:46Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-                Collection<string> ekuCollection = System.Management.Automation.Internal.SecuritySupport.GetCertEKU(cert);
-
-                foreach (string oidString in ekuCollection)
+                if (extension.Oid.FriendlyName == "Enhanced Key Usage")


Would it be better to use the Oid.Value property ("1.3.6.1.5.5.7.3.4")? Maybe it makes no difference but I wonder if we should use case insensitive string compare?

Comparing the value would be better. I'll update the code.

Except that is the OID for a specific EKU, not specifying an EKU. The OID should be 2.5.29.37

PaulHigin · 2017-05-22T17:32:51Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

+                if (extension.Oid.FriendlyName == "Enhanced Key Usage")
                {
-                    if (!String.IsNullOrEmpty(oidString))
+                    X509EnhancedKeyUsageExtension ext = (X509EnhancedKeyUsageExtension)extension;


Is this cast guaranteed? Or should we be using "as" keyword cast and check for null?

PaulHigin · 2017-05-22T17:38:40Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

    {
        private List<DnsNameRepresentation> _dnsList = new List<DnsNameRepresentation>();
+        private System.Globalization.IdnMapping idnMapping = new System.Globalization.IdnMapping();
+        private static string dnsNamePrefix = "DNS Name=";


We normally use "private const string"

PaulHigin · 2017-05-22T17:48:43Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

+            // extract DNS name from subject distinguish name
+            // if it exists and does not contain a comma
+            // a comma, indicates it is not a DNS name
+            if(cert.Subject.Length > distinguishedNamePrefix.Length && 


I don't think the Length check is necessary. I would expect the StartsWith method to do that check.

PaulHigin · 2017-05-22T17:51:59Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

-                        }
-                        if (cPunycodeName != cUnicodeName)
+                        // Get the part after 'DNS Name='
+                        if(nameLine.Length > dnsNamePrefix.Length && nameLine.StartsWith(dnsNamePrefix, System.StringComparison.InvariantCulture))


Again, I don't think the length check is needed.

PaulHigin · 2017-05-22T17:53:46Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

+                            dnsName = new DnsNameRepresentation(name,unicodeName);
+
+                            // Only add the name if it is not the same as an existing name.
+                            if(!_dnsList.Contains(dnsName))


Should this be a case sensitive compare?

The old code gave all variations back in the results. So, no.

PaulHigin · 2017-05-22T18:06:32Z

test/powershell/Modules/Microsoft.PowerShell.Security/CertificateProvider.Tests.ps1

@@ -0,0 +1,138 @@
+Import-Module (Join-Path -Path $PSScriptRoot 'certificateCommon.psm1') -Force


Please put this test prep inside "if ($IsWindows) {}"

What is the concern here?

It seems like we can skip the prep work if not running on Windows.

The only imports functions, the actual prep work is done in the beforeall of the feature tests and is skipped on non-windows platforms.

Compare OIDs by value instead of name Remove unneeded length comparison when comparing string prefixes

…latforms

PaulHigin

LGTM

Remove undocumented certificate APIs

bbda8f3

msftclas added the cla-already-signed label May 19, 2017

lzybkr requested a review from LeeHolmes May 19, 2017 02:10

SteveL-MSFT requested changes May 19, 2017

View reviewed changes

SteveL-MSFT requested a review from PaulHigin May 19, 2017 19:26

Make the cert provider properly support punycode

0348ee2

TravisEz13 force-pushed the Remove_undocumented_APIs branch from ca9f38b to 0348ee2 Compare May 19, 2017 19:35

SteveL-MSFT reviewed May 19, 2017

View reviewed changes

TravisEz13 added 4 commits May 19, 2017 14:38

refactor certificate code into common file

f3f7995

Add tests for certificate provider

9d78f52

Rename test helper functions to user approved verbs

813b210

Removed import of helper module since it is auto-loaded.

3b06616

daxian-dbw self-assigned this May 19, 2017

PaulHigin suggested changes May 22, 2017

View reviewed changes

TravisEz13 added 4 commits May 22, 2017 12:40

Remove dead code

fd691dd

Compare OIDs by value instead of name Remove unneeded length comparison when comparing string prefixes

Add basic tests for DNSNameList and EKUList

907169a

Update test common code to throw if attempted to run an non-windows p…

3c7fdef

…latforms

Only run common code import and test case table setup on windows

20d85bb

PaulHigin approved these changes May 22, 2017

View reviewed changes

SteveL-MSFT approved these changes May 22, 2017

View reviewed changes

daxian-dbw merged commit 3496136 into PowerShell:master May 23, 2017

TravisEz13 deleted the Remove_undocumented_APIs branch May 23, 2017 17:50

TravisEz13 mentioned this pull request May 23, 2017

Certificate provider should be able to filter on DocumentEncryptionCert #3847

Closed

5 tasks

iSazonov mentioned this pull request Sep 25, 2019

Bring back Certificate provider parameters #10622

Merged

14 tasks

KalleOlaviNiemitalo mentioned this pull request Nov 29, 2024

X509Certificate2 DNSNameList is not language independent, don't work on non-english clients #24594

Closed

5 tasks

		@@ -480,22 +445,6 @@ public void Open(bool includeArchivedCerts)
		public IntPtr GetFirstCert(
		CertificateFilterInfo filter)

		@@ -0,0 +1,138 @@
		Import-Module (Join-Path -Path $PSScriptRoot 'certificateCommon.psm1') -Force

Conversation

TravisEz13 commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented May 19, 2017 •

edited

Loading

TravisEz13 May 19, 2017 •

edited

Loading