CertReq: Removed hardcoded references

[p0shkar]

Pull Request (PR) description

Removed hardcoded reference in Compare-CertificateIssuer and Get-TargetResource.
This should fix an issue with third party CA's/HSM's which does not necessarily format the CN in the same order as a Microsoft CA does. This issue caused the CA to issue a new cert each time DSC was run even though a valid certificate existed.

This Pull Request (PR) fixes the following issues

• Fixes CertReq: Multiple certificates issued when having 3rd party CA #207

Task list

• Added an entry under the Unreleased section of the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in the resource folder.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[msftclas]

All CLA requirements met.

[codecov-io]

Codecov Report

Merging #209 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           dev   #209    +/-   ##
===================================
+ Coverage   89%    89%   +<1%     
===================================
  Files        7      7            
  Lines      982    987     +5     
===================================
+ Hits       876    881     +5     
  Misses     106    106

[PlagueHO]

Great job @p0shkar - thank you for submitting this! Great fix. Just a minor idea for a refactoring.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @p0shkar)

---

DSCResources/MSFT_CertReq/MSFT_CertReq.psm1, line 211 at r1 (raw file):

        $returnValue = @{
            Subject             = ($Cert.Subject.split(',') | ForEach-Object {$_.TrimStart(' ')} | Where-Object {$_ -match 'CN='}).replace('CN=', '')

Is it possible to move this code into a separate function? E.g. Something like Get-CertificateCommonName

That way we can throw some unit tests around it to validate the new behavior. It'll also result in a bit slimmer code with less duplication.

[PlagueHO]

P.S. If you're signed up for #hacktoberfest and this PR will count towards it 😁

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @p0shkar)

[p0shkar]

P.S. If you're signed up for #hacktoberfest and this PR will count towards it 😁

@PlagueHO I wasn't, since it's my first ever PR, but am now :)

I will adjust the code as you suggest within a few days, good catch.

[p0shkar]

So... 2 hours later instead of a few days, apparently I couldn't keep away...

What would be a good name for the parameter of this function? I'm leaning towards "DistinguishedName" since it's the X500 DN which will be the input, but I'm open for suggestions.

Also, I could either have this new function strip the "CN=" from the result/return or keep it.
Compare-CertificateIssuer compares the the $Issuer with "CN=$CARootName".

return (($Issuer.split(',') | ForEach-Object {$_.TrimStart(' ')} | Where-Object {$_ -match 'CN='}) -eq "CN=$CARootName")

I don't really see any point in keeping it this way, but instead changing it to below:

return (($Issuer.split(',') | ForEach-Object {$_.TrimStart(' ')} | Where-Object {$_ -match 'CN='}) -eq $CARootName)

(Except the code above will be replaced by the function instead).

Please give me your thoughts on this.

[PlagueHO]

Hi @p0shkar - awesome timing for contributing! 😁

$DistinguishedName sounds like a good parameter name to me. I'm Ok with refactoring and simplifying the code, as long as the tests pass and everything still works 😁 I recall looking at this a couple of years back but found there was a couple of gotchas: I think there was a couple of places where the CN= used/compared so it'll be worth confirming that.

[p0shkar]

@PlagueHO I've pushed the refactored code for review.

However, when looking at uses of "CN=" in the code which remains, I noticed that below code is used in Test- and Set-TargetResource, but not in Get-TargetResource.

# If the Subject does not contain a full X500 path, construct just the CN
    if (($Subject.split('=').count) -eq 1)
    {
        $Subject = "CN=$Subject"
    }

And in Get-TargetResource the following code is used:

$cert = Get-Childitem -Path Cert:\LocalMachine\My |
        Where-Object -FilterScript {
            $_.Subject -eq "CN=$Subject" -and `
            (Compare-CertificateIssuer -Issuer $_.Issuer -CARootName $CARootName)
    }

So if the subject contains the full X500 path, in Get-TargetResource this will result in it trying to find a Certificate with Subject "CN=CN=SomeSubject,O=Or,C=Other,S=etc". I guess it should have the same check as Test- and Set- and replace "CN=$Subject" with just "$Subject".

Could you confirm that I'm reading the code correct, that this is a bug?

[PlagueHO]

Hi @p0shkar - that does indeed look like a bug. Ideally the same code should be used for detecting the Subject should be used. If we refactor then this should be fixed.

[p0shkar]

@PlagueHO the code has been updated, I also created #210 for tracking of the bug we discussed a few weeks ago.

[PlagueHO]

Amazing job @p0shkar - loving this. Are you able to add some unit tests to cover the new Get-CertificateCommonName function?

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @p0shkar)

---

DSCResources/MSFT_CertReq/MSFT_CertReq.psm1, line 1104 at r3 (raw file):

    )

    return ((Get-CertificateCommonName -DistinguishedName $Issuer) -eq "$CARootName")

You can remove the quotes around the $CARootName here.

---

DSCResources/MSFT_CertReq/MSFT_CertReq.psm1, line 1156 at r3 (raw file):

    )

    return ($DistinguishedName.split(',') | ForEach-Object {$_.TrimStart(' ')} | Where-Object {$_ -match 'CN='}).replace('CN=', '')

Could you break this line up a bit to shorten it? Also, can you use named parameters? e.g.

return ($DistinguishedName.split(',') |
    ForEach-Object -Process { $_.TrimStart(' ') } |
    Where-Object -FilterScript { $_ -match 'CN=' }).replace('CN=', '')

[p0shkar]

Amazing job @p0shkar - loving this. Are you able to add some unit tests to cover the new Get-CertificateCommonName function?

Thanks @PlagueHO !

I've only dabbled a little with Pester before, and that was a while ago. Any pointers for writing unit tests and what it should test would be appreciated and helpful. :)

    return ((Get-CertificateCommonName -DistinguishedName $Issuer) -eq "$CARootName")

You can remove the quotes around the $CARootName here.

Sure, I think I left them there since I pretty much copied Compare-CertificateIssuer and wanted to make as little code-impact as possible, but I see now that it's not the same since Compare-CertificateIssuer also has "CN=" before the variable name.

    return ($DistinguishedName.split(',') | ForEach-Object {$_.TrimStart(' ')} | Where-Object {$_ -match 'CN='}).replace('CN=', '')

Could you break this line up a bit to shorten it? Also, can you use named parameters? e.g.

return ($DistinguishedName.split(',') |
    ForEach-Object -Process { $_.TrimStart(' ') } |
    Where-Object -FilterScript { $_ -match 'CN=' }).replace('CN=', '')

Of course! Will fix that right away.

[p0shkar]

An unrelated question: does the DSC Style Guidelines translate into any of the predefined styles available in VSCode or can a custom set be found somewhere which do the correct formatting? It would be awesome to be able to use the auto formatter when writing/updating DSC modules!

[PlagueHO]

@p0shkar - definitely. This is something we'd like to do, but we needed to start by getting all the custom PSSA validation rules in place (so that VSCode can highlight style violations). Everything is located over here: https://github.com/PowerShell/DscResource.Tests and you can see the issues/PRs tracking that over there.

[p0shkar]

@PlagueHO I've added a couple of tests, I couldn't think of any more which would be appropriate so I hope this is enough or you could give me some pointers.

[PlagueHO]

Just one minor tweak - otherwise looking awesome.

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @p0shkar)

---

Tests/Unit/MSFT_CertReq.Tests.ps1, line 1853 at r5 (raw file):

        }

        Describe 'MSFT_CertReq\Get-CertificateCommonName' {

Can you add one last test in here for when CN= is at the end of the DN? E.g.

            Context 'When called with certificate distinguished name with multiple X500 paths and CN at the end' {
                It 'Should return a string value containing only exactly the Common Name' {
                    Get-CertificateCommonName `
                        -DistinguishedName 'E=xyz@contoso.com, OU=Organisation Unit, O=Organisation, L=Locality, S=State, C=country, CN=xyz.contoso.com' | Should -BeExactly "xyz.contoso.com"
                }
            }

[p0shkar]

@PlagueHO Of course, good idea! The whole idea of this PR is for that to work, so can't believe I didn't think of that.. The changes has been pushed.

Don't know if you noticed in commit 6ba9bd3, but when looking at the other tests I found that the "It"-name said "should return true" while the "should"-statement returned $false, so I fixed that even though it doesn't really have anything to do with this PR.

[PlagueHO]

I didn't notice that! Good catch! 😁 Sorry about the delay in getting this through and thank you for all your hard work!

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved