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

Travis Plunk (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.

Steve Lee (SteveL-MSFT)

Is there sufficient test coverage?

Steve Lee (SteveL-MSFT) · 2017-05-19T01:56:51Z

-            {
-                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.

Steve Lee (SteveL-MSFT) · 2017-05-19T01:56:56Z

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


filed #3826, until this is resolved

Steve Lee (SteveL-MSFT) · 2017-05-19T01:56:59Z

-                {
-                    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

Steve Lee (SteveL-MSFT) · 2017-05-19T01:58:50Z

-                            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


Steve Lee (SteveL-MSFT) · 2017-05-19T02:21:50Z

-                                    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.

Steve Lee (SteveL-MSFT)

My concerns are addressed, but would like Lee Holmes (@LeeHolmes) to review before we merge

Paul Higinbotham (PaulHigin) · 2017-05-22T16:34:54Z

@@ -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.

Paul Higinbotham (PaulHigin) · 2017-05-22T16:51:34Z

            }
            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.

Paul Higinbotham (PaulHigin) · 2017-05-22T17:00:33Z

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


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

Paul Higinbotham (PaulHigin) · 2017-05-22T17:31:46Z

-                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

Paul Higinbotham (PaulHigin) · 2017-05-22T17:32:51Z

+                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?

Paul Higinbotham (PaulHigin) · 2017-05-22T17:38:40Z

    {
        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"

Paul Higinbotham (PaulHigin) · 2017-05-22T17:48:43Z

+            // 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.

Paul Higinbotham (PaulHigin) · 2017-05-22T17:51:59Z

-                        }
-                        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.

Paul Higinbotham (PaulHigin) · 2017-05-22T17:53:46Z

+                            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.

Paul Higinbotham (PaulHigin) · 2017-05-22T18:06:32Z

@@ -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

Paul Higinbotham (PaulHigin)

LGTM

The code paths deleted where using the undocumented Win32 APIs 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.

Remove undocumented certificate APIs

bbda8f3

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label May 19, 2017

Jason Shirk (lzybkr) requested a review from Lee Holmes (LeeHolmes) May 19, 2017 02:10

Steve Lee (SteveL-MSFT) requested changes May 19, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) requested a review from Paul Higinbotham (PaulHigin) May 19, 2017 19:26

Make the cert provider properly support punycode

0348ee2

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

Steve Lee (SteveL-MSFT) reviewed May 19, 2017

View reviewed changes

Travis Plunk (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

Dongbo Wang (daxian-dbw) self-assigned this May 19, 2017

Paul Higinbotham (PaulHigin) suggested changes May 22, 2017

View reviewed changes

Travis Plunk (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

Paul Higinbotham (PaulHigin) approved these changes May 22, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) approved these changes May 22, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit 3496136 into PowerShell:master May 23, 2017

Travis Plunk (TravisEz13) deleted the Remove_undocumented_APIs branch May 23, 2017 17:50

Travis Plunk (TravisEz13) mentioned this pull request May 23, 2017

Certificate provider should be able to filter on DocumentEncryptionCert #3847

Closed

5 tasks

Ilya (iSazonov) mentioned this pull request Sep 25, 2019

Bring back Certificate provider parameters #10622

Merged

14 tasks

Kalle Olavi Niemitalo (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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis Plunk (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!

Steve Lee (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

Travis Plunk (TravisEz13) commented May 19, 2017 •

edited

Loading

Travis Plunk (TravisEz13) May 19, 2017 •

edited

Loading