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

xCertificateExport: Fixed bug causing pfx export with matchsource enabled to fail #117

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Projects
None yet
4 participants
@hackjammer
Copy link
Contributor

hackjammer commented Dec 13, 2017

Pull Request (PR) description
Fixed pfx import check when matchsource is enabled to use a securestring instead of attempting to use a credential object.
Also updated integration test to make this less likely to happen again (now just creating a plaintext password and a credential variable as used in the DSC resource itself).

This Pull Request (PR) fixes the following issues:
#116


This change is Reviewable

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 13, 2017

Codecov Report

Merging #117 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@        Coverage Diff         @@
##           dev   #117   +/-   ##
==================================
  Coverage   94%    94%           
==================================
  Files        5      5           
  Lines      517    517           
  Branches     2      2           
==================================
  Hits       488    488           
  Misses      27     27           
  Partials     2      2
@PlagueHO

This comment has been minimized.

Copy link
Collaborator

PlagueHO commented Dec 16, 2017

Great job @hackjammer

I'm a bit worried about this change though. All the documentation I can find as well as examining the System.Security.Cryptography.X509Certificates.X509Certificate2Collection objects on Windows Server 2016/Windows 10 and Windows Server 2012 R2 indicate that the Import method does not support SecureString - only string. The type you linked to in your second link is actually for a different object type.

So, with that in mind I'd suggest the following (and see all my other comments):

Could you replace the Context 'Export PFX' { ... } integration tests in the file MSFT_xCertificateExport.Integration.Test.ps1 with:

        Context 'Export PFX and then Export PFX again to ensure no errors' {
            #region DEFAULT TESTS
            It 'Should compile and apply the MOF without throwing' {
                {
                    $ConfigData = @{
                        AllNodes = @(
                            @{
                                NodeName                    = 'localhost'
                                Path                        = $script:pfxPath
                                FriendlyName                = $certFriendlyName
                                Subject                     = $certificateSubject
                                DNSName                     = $certificateDNSNames
                                Issuer                      = $certificateSubject
                                KeyUsage                    = $certificateKeyUsage
                                EnhancedKeyUsage            = $certificateEKU
                                MatchSource                 = $true
                                Type                        = 'PFX'
                                ChainOption                 = 'BuildChain'
                                Password                    = $pfxCredential
                                PsDscAllowPlainTextPassword = $true
                            }
                        )
                    }

                    & "$($script:DSCResourceName)_Config" `
                        -OutputPath $TestDrive `
                        -ConfigurationData $ConfigData

                    Start-DscConfiguration `
                        -Path $TestDrive `
                        -ComputerName localhost `
                        -Wait `
                        -Verbose `
                        -Force `
                        -ErrorAction Stop
                } | Should -Not -Throw
            }

            It 'Should be able to call Get-DscConfiguration without throwing' {
                { $script:currentPFX = Get-DscConfiguration -Verbose -ErrorAction Stop } | Should -Not -Throw
            }
            #endregion

            It 'Should have exported a PFX certificate' {
                $script:currentPFX.IsExported | Should -Be $true
            }

            It 'Should have set the resource and the thumbprint of the exported certificate should match' {
                $exportedCertificate = New-Object -TypeName 'System.Security.Cryptography.X509Certificates.X509Certificate2Collection'
                $exportedCertificate.Import($script:certificatePath,$pfxPassword,[System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::PersistKeySet)
                $exportedCertificate[0].Thumbprint | Should -Be $script:validCertificateThumbprint
            }

            # Apply the MOF a second time to ensure no errors occur
            It 'Should apply the MOF a second time without throwing' {
                {
                    Start-DscConfiguration `
                        -Path $TestDrive `
                        -ComputerName localhost `
                        -Wait `
                        -Verbose `
                        -Force `
                        -ErrorAction Stop
                } | Should -Not -Throw
            }

            It 'Should be able to call Get-DscConfiguration without throwing' {
                { $script:currentPFX = Get-DscConfiguration -Verbose -ErrorAction Stop } | Should -Not -Throw
            }
            #endregion

            It 'Should have exported a PFX certificate' {
                $script:currentPFX.IsExported | Should -Be $true
            }

            It 'Should have set the resource and the thumbprint of the exported certificate should match' {
                $exportedCertificate = New-Object -TypeName 'System.Security.Cryptography.X509Certificates.X509Certificate2Collection'
                $exportedCertificate.Import($script:certificatePath,$pfxPassword,[System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::PersistKeySet)
                $exportedCertificate[0].Thumbprint | Should -Be $script:validCertificateThumbprint
            }
        }

I actually started fixing this one before I saw you already had - so I'd updated the integration tests to catch this one (I like to practice test driven development wherever I can). So these are the tests I created that validate this.


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


CHANGELOG.md, line 6 at r1 (raw file):

- xCertificateExport:
  - Fixed bug causing pfx export with matchsource enabled to fail

Can you add link to issue as per other changes. E.g.
- see [issue #117](https://github.com/PowerShell/xCertificate/issues/117).


Modules/xCertificate/DSCResources/MSFT_xCertificateExport/MSFT_xCertificateExport.psm1, line 446 at r1 (raw file):

                elseif ($Type -eq 'PFX')
                {
                    $exportedCertificate.Import($Path, $Password.Password, [System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::PersistKeySet)

You'll need to use $Password.GetNetworkCredential().Password here. To validate this I actually updated the integration tests.


Tests/Integration/MSFT_xCertificateExport.Integration.Tests.ps1, line 151 at r1 (raw file):

            It 'Should have set the resource and the thumbprint of the exported certificate should match' {
                $exportedCertificate = New-Object -TypeName 'System.Security.Cryptography.X509Certificates.X509Certificate2Collection'
                $exportedCertificate.Import($script:certificatePath,$pfxCredential.Password,[System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::PersistKeySet)

This won't work because the Import method of System.Security.Cryptography.X509Certificates.X509Certificate2Collection does not support a password using secure string. So you'll need to use $pfxCredential.GetNetworkCredential().Password or revert these changes back to passing the PFX as a string.


Comments from Reviewable

@PlagueHO

This comment has been minimized.

Copy link
Collaborator

PlagueHO commented Dec 20, 2017

Hi @hackjammer - this one now has some merge conflicts in it due to the previously merged PR. Can you resolve?

@PlagueHO

This comment has been minimized.

Copy link
Collaborator

PlagueHO commented Feb 10, 2018

@hackjammer - are you still planning on working on this one?

@johlju

This comment has been minimized.

Copy link
Contributor

johlju commented May 3, 2018

Labeling this abandoned so that someone else can continue this work.

@johlju johlju added abandoned and removed needs review labels May 3, 2018

@PlagueHO

This comment has been minimized.

Copy link
Collaborator

PlagueHO commented Dec 1, 2018

I'm picking this one up now, so will close this and reopen a new PR.

@PlagueHO PlagueHO closed this Dec 1, 2018

@PlagueHO PlagueHO referenced this pull request Dec 1, 2018

Merged

Fix export of PFX with matchsource enabled - Fixes #116 #170

3 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment